Fix the bug in php c extension that setting one field can change another field's value. (#3455)

* Fix the bug in php c extension that setting one field can change another
field's value.

The reason is that previously, in c extension, it was assumed that the
order that fields were declared in php is the same as the order of
fields in upb. This is not true. Now, for every field in upb, we will
look up the actual property that is corresponding to the upb field.

* Cleanup pull request

* Fix indentation

* Port to php5

* Port with php7.1

* Port to zts

php/ext/google/protobuf/encode_decode.c

@@ -716,7 +716,7 @@ static void *oneofbytes_handler(void *closure,
   DEREF(message_data(msg), oneofdata->case_ofs, uint32_t) =
       oneofdata->oneof_case_num;
   DEREF(message_data(msg), oneofdata->ofs, CACHED_VALUE*) =
-      &(msg->std.properties_table)[oneofdata->property_ofs];
+      OBJ_PROP(&msg->std, oneofdata->property_ofs);
 
    return empty_php_string(DEREF(
        message_data(msg), oneofdata->ofs, CACHED_VALUE*));
@@ -747,7 +747,7 @@ static void* oneofsubmsg_handler(void* closure, const void* hd) {
 
     // Create new message.
     DEREF(message_data(msg), oneofdata->ofs, CACHED_VALUE*) =
-        &(msg->std.properties_table)[oneofdata->property_ofs];
+        OBJ_PROP(&msg->std, oneofdata->property_ofs);
     ZVAL_OBJ(CACHED_PTR_TO_ZVAL_PTR(
         DEREF(message_data(msg), oneofdata->ofs, CACHED_VALUE*)),
         subklass->create_object(subklass TSRMLS_CC));

php/ext/google/protobuf/message.c

@@ -172,7 +172,7 @@ static zval* message_get_property(zval* object, zval* member, int type,
       zend_get_property_info(Z_OBJCE_P(object), member, true TSRMLS_CC);
   return layout_get(
       self->descriptor->layout, message_data(self), field,
-      &Z_OBJ_P(object)->properties_table[property_info->offset] TSRMLS_CC);
+      OBJ_PROP(Z_OBJ_P(object), property_info->offset) TSRMLS_CC);
 #else
   property_info =
       zend_get_property_info(Z_OBJCE_P(object), Z_STR_P(member), true);
@@ -222,7 +222,7 @@ void custom_data_init(const zend_class_entry* ce,
   // case a collection happens during object creation in layout_init().
   intern->descriptor = desc;
   layout_init(desc->layout, message_data(intern),
-              intern->std.properties_table PHP_PROTO_TSRMLS_CC);
+              &intern->std PHP_PROTO_TSRMLS_CC);
 }
 
 void build_class_from_descriptor(
@@ -265,8 +265,7 @@ PHP_METHOD(Message, clear) {
   zend_class_entry* ce = desc->klass;
 
   object_properties_init(&msg->std, ce);
-  layout_init(desc->layout, message_data(msg),
-              msg->std.properties_table TSRMLS_CC);
+  layout_init(desc->layout, message_data(msg), &msg->std TSRMLS_CC);
 }
 
 PHP_METHOD(Message, mergeFrom) {
@@ -301,7 +300,8 @@ PHP_METHOD(Message, readOneof) {
 
   int property_cache_index =
       msg->descriptor->layout->fields[upb_fielddef_index(field)].cache_index;
-  zval* property_ptr = OBJ_PROP(Z_OBJ_P(getThis()), property_cache_index);
+  zval* property_ptr = CACHED_PTR_TO_ZVAL_PTR(
+      OBJ_PROP(Z_OBJ_P(getThis()), property_cache_index));
 
   // Unlike singular fields, oneof fields share cached property. So we cannot
   // let lay_get modify the cached property. Instead, we pass in the return

php/ext/google/protobuf/protobuf.h

@@ -151,7 +151,7 @@
 
 #define PHP_PROTO_GLOBAL_UNINITIALIZED_ZVAL EG(uninitialized_zval_ptr)
 
-#define OBJ_PROP(PROPERTIES, OFFSET) (PROPERTIES)->properties_table[OFFSET]
+#define OBJ_PROP(OBJECT, OFFSET) &((OBJECT)->properties_table[OFFSET])
 
 #define php_proto_zval_ptr_dtor(zval_ptr) \
   zval_ptr_dtor(&(zval_ptr))
@@ -644,7 +644,7 @@ PHP_PROTO_WRAP_OBJECT_END
 
 MessageLayout* create_layout(const upb_msgdef* msgdef);
 void layout_init(MessageLayout* layout, void* storage,
-                 CACHED_VALUE* properties_table PHP_PROTO_TSRMLS_DC);
+                 zend_object* object PHP_PROTO_TSRMLS_DC);
 zval* layout_get(MessageLayout* layout, const void* storage,
                  const upb_fielddef* field, CACHED_VALUE* cache TSRMLS_DC);
 void layout_set(MessageLayout* layout, MessageHeader* header,

php/ext/google/protobuf/storage.c

@@ -275,9 +275,6 @@ void native_slot_init(upb_fieldtype_t type, void* memory, CACHED_VALUE* 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;
@@ -586,6 +583,8 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
   upb_msg_oneof_iter oit;
   size_t off = 0;
   int i = 0;
+  TSRMLS_FETCH();
+  Descriptor* desc = UNBOX_HASHTABLE_VALUE(Descriptor, get_def_obj(msgdef));
 
   layout->fields = ALLOC_N(MessageField, nfields);
 
@@ -612,7 +611,37 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
     layout->fields[upb_fielddef_index(field)].offset = off;
     layout->fields[upb_fielddef_index(field)].case_offset =
         MESSAGE_FIELD_NO_CASE;
-    layout->fields[upb_fielddef_index(field)].cache_index = i++;
+
+    const char* fieldname = upb_fielddef_name(field);
+
+#if PHP_MAJOR_VERSION < 7 || (PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 0)
+    zend_class_entry* old_scope = EG(scope);
+    EG(scope) = desc->klass;
+#else
+    zend_class_entry* old_scope = EG(fake_scope);
+    EG(fake_scope) = desc->klass;
+#endif
+
+#if PHP_MAJOR_VERSION < 7
+    zval member;
+    ZVAL_STRINGL(&member, fieldname, strlen(fieldname), 0);
+    zend_property_info* property_info =
+        zend_get_property_info(desc->klass, &member, true TSRMLS_CC);
+#else
+    zend_string* member = zend_string_init(fieldname, strlen(fieldname), 1);
+    zend_property_info* property_info =
+        zend_get_property_info(desc->klass, member, true);
+    zend_string_release(member);
+#endif
+
+#if PHP_MAJOR_VERSION < 7 || (PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 0)
+    EG(scope) = old_scope;
+#else
+    EG(fake_scope) = old_scope;
+#endif
+
+    layout->fields[upb_fielddef_index(field)].cache_index =
+        property_info->offset;
     off += field_size;
   }
 
@@ -640,11 +669,40 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
     // Align the offset .
     off = align_up_to( off, field_size);
     // Assign all fields in the oneof this same offset.
+    const char* oneofname = upb_oneofdef_name(oneof);
     for (upb_oneof_begin(&fit, oneof); !upb_oneof_done(&fit);
          upb_oneof_next(&fit)) {
       const upb_fielddef* field = upb_oneof_iter_field(&fit);
       layout->fields[upb_fielddef_index(field)].offset = off;
-      layout->fields[upb_fielddef_index(field)].cache_index = i;
+
+#if PHP_MAJOR_VERSION < 7 || (PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 0)
+      zend_class_entry* old_scope = EG(scope);
+      EG(scope) = desc->klass;
+#else
+      zend_class_entry* old_scope = EG(fake_scope);
+      EG(fake_scope) = desc->klass;
+#endif
+
+#if PHP_MAJOR_VERSION < 7
+      zval member;
+      ZVAL_STRINGL(&member, oneofname, strlen(oneofname), 0);
+      zend_property_info* property_info =
+          zend_get_property_info(desc->klass, &member, true TSRMLS_CC);
+#else
+      zend_string* member = zend_string_init(oneofname, strlen(oneofname), 1);
+      zend_property_info* property_info =
+          zend_get_property_info(desc->klass, member, true);
+      zend_string_release(member);
+#endif
+
+#if PHP_MAJOR_VERSION < 7 || (PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 0)
+      EG(scope) = old_scope;
+#else
+      EG(fake_scope) = old_scope;
+#endif
+
+      layout->fields[upb_fielddef_index(field)].cache_index =
+          property_info->offset;
     }
     i++;
     off += field_size;
@@ -683,7 +741,7 @@ void free_layout(MessageLayout* layout) {
 }
 
 void layout_init(MessageLayout* layout, void* storage,
-                 CACHED_VALUE* properties_table PHP_PROTO_TSRMLS_DC) {
+                 zend_object* object PHP_PROTO_TSRMLS_DC) {
   int i;
   upb_msg_field_iter it;
   for (upb_msg_field_begin(&it, layout->msgdef), i = 0; !upb_msg_field_done(&it);
@@ -692,22 +750,7 @@ void layout_init(MessageLayout* layout, void* storage,
     void* memory = slot_memory(layout, storage, field);
     uint32_t* oneof_case = slot_oneof_case(layout, storage, field);
     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));
+    CACHED_VALUE* property_ptr = OBJ_PROP(object, cache_index);
 
     if (upb_fielddef_containingoneof(field)) {
       memset(memory, 0, NATIVE_SLOT_MAX_SIZE);
@@ -797,7 +840,7 @@ void layout_set(MessageLayout* layout, MessageHeader* header,
             header->descriptor->layout->fields[upb_fielddef_index(field)]
                 .cache_index;
         DEREF(memory, CACHED_VALUE*) =
-            &(header->std.properties_table)[property_cache_index];
+            OBJ_PROP(&header->std, property_cache_index);
         memory = DEREF(memory, CACHED_VALUE*);
         break;
       }
@@ -964,7 +1007,7 @@ void layout_merge(MessageLayout* layout, MessageHeader* from,
           int property_cache_index =
               layout->fields[upb_fielddef_index(field)].cache_index;
           DEREF(to_memory, CACHED_VALUE*) =
-              &(to->std.properties_table)[property_cache_index];
+              OBJ_PROP(&to->std, property_cache_index);
           break;
         }
         default:

php/tests/generated_class_test.php

@@ -13,6 +13,7 @@ use Foo\TestIncludeNamespaceMessage;
 use Foo\TestIncludePrefixMessage;
 use Foo\TestMessage;
 use Foo\TestMessage_Sub;
+use Foo\TestReverseFieldOrder;
 use Php\Test\TestNamespace;
 
 class GeneratedClassTest extends TestBase
@@ -702,4 +703,16 @@ class GeneratedClassTest extends TestBase
         $this->assertSame(1, $m->getOptionalInt32());
         $this->assertSame(2, $m->getOptionalUInt32());
     }
+
+    #########################################################
+    # Test Reverse Field Order.
+    #########################################################
+
+    public function testReverseFieldOrder()
+    {
+        $m = new TestReverseFieldOrder();
+        $m->setB("abc");
+        $this->assertSame("abc", $m->getB());
+        $this->assertNotSame("abc", $m->getA());
+    }
 }

php/tests/memory_leak_test.php

@@ -21,6 +21,7 @@ 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/TestReverseFieldOrder.php');
 require_once('generated/Foo/TestUnpackedMessage.php');
 require_once('generated/GPBMetadata/Proto/Test.php');
 require_once('generated/GPBMetadata/Proto/TestEmptyPhpNamespace.php');

php/tests/proto/test.proto

@@ -187,3 +187,8 @@ message TestRandomFieldOrder {
   int64 tag13 = 150;
   string tag14 = 160;
 }
+
+message TestReverseFieldOrder {
+  repeated int32 a = 2;
+  string b = 1;
+}