Initial value in generated code cannot be used by c extension. (#3367)

In the generated code of previous versions, each php field is given an
initial value. In c extension, it was assumed that the field order in
the generated code is consistent with upb fields order, so that the
correct initial value can be bound to the correct upb field. However,
this may not be true. The order of fields in generated code is decided
by proto compiler, while the order of upb fields is decided by the hash
function used in c extension.
This PR fixes the issue by reset the initial value at runtime.

5 files changed, 35 insertions, 2 deletions

diff --git a/php/ext/google/protobuf/protobuf.h b/php/ext/google/protobuf/protobuf.h
index 684a9c50..eecae136 100644
--- a/php/ext/google/protobuf/protobuf.h
+++ b/php/ext/google/protobuf/protobuf.h
@@ -640,7 +640,7 @@ bool native_slot_set(upb_fieldtype_t type, const zend_class_entry* klass,
 bool native_slot_set_by_array(upb_fieldtype_t type,
                               const zend_class_entry* klass, void* memory,
                               zval* value TSRMLS_DC);
-void native_slot_init(upb_fieldtype_t type, void* memory, void* cache);
+void native_slot_init(upb_fieldtype_t type, void* memory, CACHED_VALUE* cache);
 // For each property, in order to avoid conversion between the zval object and
 // the actual data type during parsing/serialization, the containing message
 // object use the custom memory layout to store the actual data type for each
diff --git a/php/ext/google/protobuf/storage.c b/php/ext/google/protobuf/storage.c
index 29a3db86..c0e4da3e 100644
--- a/php/ext/google/protobuf/storage.c
+++ b/php/ext/google/protobuf/storage.c
@@ -210,7 +210,7 @@ bool native_slot_set_by_array(upb_fieldtype_t type,
   return true;
 }
 
-void native_slot_init(upb_fieldtype_t type, void* memory, void* cache) {
+void native_slot_init(upb_fieldtype_t type, void* memory, CACHED_VALUE* cache) {
   zval* tmp = NULL;
   switch (type) {
     case UPB_TYPE_FLOAT:
@@ -224,6 +224,9 @@ void native_slot_init(upb_fieldtype_t type, void* memory, void* cache) {
       break;
     case UPB_TYPE_STRING:
     case UPB_TYPE_BYTES:
+      DEREF(memory, CACHED_VALUE*) = cache;
+      ZVAL_EMPTY_STRING(CACHED_PTR_TO_ZVAL_PTR(cache));
+      break;
     case UPB_TYPE_MESSAGE:
       DEREF(memory, CACHED_VALUE*) = cache;
       break;
@@ -621,6 +624,21 @@ void layout_init(MessageLayout* layout, void* storage,
     int cache_index = slot_property_cache(layout, storage, field);
     CACHED_VALUE* property_ptr = &properties_table[cache_index];
 
+    // Clean up initial value by generated code. In the generated code of
+    // previous versions, each php field is given an initial value. However, the
+    // order to initialize these fields may not be consistent with the order of
+    // upb fields.
+    if (Z_TYPE_P(CACHED_PTR_TO_ZVAL_PTR(property_ptr)) == IS_STRING) {
+#if PHP_MAJOR_VERSION < 7
+      if (!IS_INTERNED(Z_STRVAL_PP(property_ptr))) {
+        FREE(Z_STRVAL_PP(property_ptr));
+      }
+#else
+      zend_string_release(Z_STR_P(property_ptr));
+#endif
+    }
+    ZVAL_NULL(CACHED_PTR_TO_ZVAL_PTR(property_ptr));
+
     if (upb_fielddef_containingoneof(field)) {
       memset(memory, 0, NATIVE_SLOT_MAX_SIZE);
       *oneof_case = ONEOF_CASE_NONE;
diff --git a/php/tests/encode_decode_test.php b/php/tests/encode_decode_test.php
index 2ea360b2..b43dffb4 100644
--- a/php/tests/encode_decode_test.php
+++ b/php/tests/encode_decode_test.php
@@ -9,6 +9,7 @@ use Foo\TestEnum;
 use Foo\TestMessage;
 use Foo\TestMessage_Sub;
 use Foo\TestPackedMessage;
+use Foo\TestRandomFieldOrder;
 use Foo\TestUnpackedMessage;
 
 class EncodeDecodeTest extends TestBase
@@ -235,6 +236,13 @@ class EncodeDecodeTest extends TestBase
         $this->assertEquals(-1, $m->getOptionalInt32());
     }
 
+    public function testRandomFieldOrder()
+    {
+        $m = new TestRandomFieldOrder();
+        $data = $m->serializeToString();
+        $this->assertSame("", $data);
+    }
+
     /**
      * @expectedException Exception
      */
diff --git a/php/tests/memory_leak_test.php b/php/tests/memory_leak_test.php
index 6572fdd0..faa1833d 100644
--- a/php/tests/memory_leak_test.php
+++ b/php/tests/memory_leak_test.php
@@ -20,6 +20,7 @@ require_once('generated/Foo/TestMessage_NestedEnum.php');
 require_once('generated/Foo/TestMessage_Sub.php');
 require_once('generated/Foo/TestPackedMessage.php');
 require_once('generated/Foo/TestPhpDoc.php');
+require_once('generated/Foo/TestRandomFieldOrder.php');
 require_once('generated/Foo/TestUnpackedMessage.php');
 require_once('generated/GPBMetadata/Proto/Test.php');
 require_once('generated/GPBMetadata/Proto/TestEmptyPhpNamespace.php');
diff --git a/php/tests/proto/test.proto b/php/tests/proto/test.proto
index dada8b48..d81f66f5 100644
--- a/php/tests/proto/test.proto
+++ b/php/tests/proto/test.proto
@@ -181,3 +181,9 @@ message TestIncludeNamespaceMessage {
   TestNamespace namespace_message = 1;
   TestEmptyNamespace empty_namespace_message = 2;
 }
+
+// This will cause upb fields not ordered by the order in the generated code.
+message TestRandomFieldOrder {
+  int64 tag13 = 150;
+  string tag14 = 160;
+}