Fix cycle dependency for repeated field not collected by gc (#3399)

php/ext/google/protobuf/array.c

@@ -142,12 +142,7 @@ static inline void php_proto_array_string_release(zval *value) {
   efree(ptr);
 }
 static inline void php_proto_array_object_release(zval *value) {
-  void* ptr = Z_PTR_P(value);
-  zend_object* object = *(zend_object**)ptr;
-  if(--GC_REFCOUNT(object) == 0) {
-    zend_objects_store_del(object);
-  }
-  efree(ptr);
+  zval_ptr_dtor(value);
 }
 static void php_proto_array_default_release(zval* value) {
   void* ptr = Z_PTR_P(value);
@@ -210,7 +205,11 @@ static void repeated_field_write_dimension(zval *object, zval *offset,
     }
   }
 
-  php_proto_zend_hash_index_update(ht, index, memory, size, NULL);
+  if (intern->type == UPB_TYPE_MESSAGE) {
+    php_proto_zend_hash_index_update_zval(ht, index, *(zval**)memory);
+  } else {
+    php_proto_zend_hash_index_update_mem(ht, index, memory, size, NULL);
+  }
 }
 
 #if PHP_MAJOR_VERSION < 7
@@ -233,9 +232,18 @@ void *repeated_field_index_native(RepeatedField *intern, int index TSRMLS_DC) {
   HashTable *ht = PHP_PROTO_HASH_OF(intern->array);
   void *value;
 
-  if (php_proto_zend_hash_index_find(ht, index, (void **)&value) == FAILURE) {
-    zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
-    return NULL;
+  if (intern->type == UPB_TYPE_MESSAGE) {
+    if (php_proto_zend_hash_index_find_zval(ht, index, (void **)&value) ==
+        FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
+      return NULL;
+    }
+  } else {
+    if (php_proto_zend_hash_index_find_mem(ht, index, (void **)&value) ==
+        FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
+      return NULL;
+    }
   }
 
   return value;
@@ -244,7 +252,11 @@ void *repeated_field_index_native(RepeatedField *intern, int index TSRMLS_DC) {
 void repeated_field_push_native(RepeatedField *intern, void *value) {
   HashTable *ht = PHP_PROTO_HASH_OF(intern->array);
   int size = native_slot_size(intern->type);
-  php_proto_zend_hash_next_index_insert(ht, (void **)value, size, NULL);
+  if (intern->type == UPB_TYPE_MESSAGE) {
+    php_proto_zend_hash_next_index_insert_zval(ht, value);
+  } else {
+    php_proto_zend_hash_next_index_insert_mem(ht, (void **)value, size, NULL);
+  }
 }
 
 void repeated_field_create_with_field(
@@ -367,11 +379,19 @@ PHP_METHOD(RepeatedField, offsetGet) {
   RepeatedField *intern = UNBOX(RepeatedField, getThis());
   HashTable *table = PHP_PROTO_HASH_OF(intern->array);
 
-  if (php_proto_zend_hash_index_find(table, index, (void **)&memory) == FAILURE) {
-    zend_error(E_USER_ERROR, "Element at %ld doesn't exist.\n", index);
-    return;
+  if (intern->type == UPB_TYPE_MESSAGE) {
+    if (php_proto_zend_hash_index_find_zval(table, index, (void **)&memory) ==
+        FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %ld doesn't exist.\n", index);
+      return;
+    }
+  } else {
+    if (php_proto_zend_hash_index_find_mem(table, index, (void **)&memory) ==
+        FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %ld doesn't exist.\n", index);
+      return;
+    }
   }
-
   native_slot_get_by_array(intern->type, memory,
                            ZVAL_PTR_TO_CACHED_PTR(return_value) TSRMLS_CC);
 }
@@ -491,10 +511,18 @@ PHP_METHOD(RepeatedFieldIter, current) {
 
   HashTable *table = PHP_PROTO_HASH_OF(repeated_field->array);
 
-  if (php_proto_zend_hash_index_find(table, intern->position, (void **)&memory) ==
-      FAILURE) {
-    zend_error(E_USER_ERROR, "Element at %ld doesn't exist.\n", index);
-    return;
+  if (repeated_field->type == UPB_TYPE_MESSAGE) {
+    if (php_proto_zend_hash_index_find_zval(table, intern->position,
+                                            (void **)&memory) == FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
+      return;
+    }
+  } else {
+    if (php_proto_zend_hash_index_find_mem(table, intern->position,
+                                           (void **)&memory) == FAILURE) {
+      zend_error(E_USER_ERROR, "Element at %d doesn't exist.\n", index);
+      return;
+    }
   }
   native_slot_get_by_array(repeated_field->type, memory,
                            ZVAL_PTR_TO_CACHED_PTR(return_value) TSRMLS_CC);

php/ext/google/protobuf/encode_decode.c

@@ -354,7 +354,7 @@ static size_t zendstringdata_handler(void* closure, const void* hd,
 
   HashTable *ht = PHP_PROTO_HASH_OF(intern->array);
   int index = zend_hash_num_elements(ht) - 1;
-  php_proto_zend_hash_index_update(
+  php_proto_zend_hash_index_update_mem(
       ht, index, memory, sizeof(zend_string*), NULL);
 
   return len;
@@ -1401,7 +1401,7 @@ static void putarray(zval* array, const upb_fielddef* f, upb_sink* sink,
         MessageHeader *submsg = UNBOX(MessageHeader, *(zval**)memory);
 #else
         MessageHeader *submsg =
-            (MessageHeader*)((char*)(*(zend_object**)memory) -
+            (MessageHeader*)((char*)(Z_OBJ_P((zval*)memory)) -
                 XtOffsetOf(MessageHeader, std));
 #endif
         putrawsubmsg(submsg, f, &subsink, depth TSRMLS_CC);

php/ext/google/protobuf/map.c

@@ -285,7 +285,7 @@ static bool map_field_read_dimension(zval *object, zval *key, int type,
 
   if (upb_strtable_lookup2(&intern->table, keyval, length, &v)) {
     void* mem = upb_value_memory(&v);
-    native_slot_get_by_array(intern->value_type, mem, retval TSRMLS_CC);
+    native_slot_get_by_map_value(intern->value_type, mem, retval TSRMLS_CC);
     return true;
   } else {
     zend_error(E_USER_ERROR, "Given key doesn't exist.");
@@ -318,7 +318,7 @@ static void map_field_write_dimension(zval *object, zval *key,
 
   mem = upb_value_memory(&v);
   memset(mem, 0, native_slot_size(intern->value_type));
-  if (!native_slot_set_by_array(intern->value_type, intern->msg_ce, mem,
+  if (!native_slot_set_by_map(intern->value_type, intern->msg_ce, mem,
                                 value TSRMLS_CC)) {
     return;
   }
@@ -535,8 +535,8 @@ PHP_METHOD(MapFieldIter, current) {
   upb_value value = map_iter_value(intern, &value_length);
 
   void* mem = upb_value_memory(&value);
-  native_slot_get_by_array(map_field->value_type, mem,
-                           ZVAL_PTR_TO_CACHED_PTR(return_value) TSRMLS_CC);
+  native_slot_get_by_map_value(map_field->value_type, mem,
+                               ZVAL_PTR_TO_CACHED_PTR(return_value) TSRMLS_CC);
 }
 
 PHP_METHOD(MapFieldIter, key) {

php/ext/google/protobuf/protobuf.c

@@ -55,13 +55,13 @@ static void add_to_table(HashTable* t, const void* def, void* value) {
   uint nIndex = (ulong)def & t->nTableMask;
 
   zval* pDest = NULL;
-  php_proto_zend_hash_index_update(t, (zend_ulong)def, &value, sizeof(zval*),
-                                   (void**)&pDest);
+  php_proto_zend_hash_index_update_mem(t, (zend_ulong)def, &value,
+                                       sizeof(zval*), (void**)&pDest);
 }
 
 static void* get_from_table(const HashTable* t, const void* def) {
   void** value;
-  if (php_proto_zend_hash_index_find(t, (zend_ulong)def, (void**)&value) ==
+  if (php_proto_zend_hash_index_find_mem(t, (zend_ulong)def, (void**)&value) ==
       FAILURE) {
     zend_error(E_ERROR, "PHP object not found for given definition.\n");
     return NULL;
@@ -71,13 +71,13 @@ static void* get_from_table(const HashTable* t, const void* def) {
 
 static bool exist_in_table(const HashTable* t, const void* def) {
   void** value;
-  return (php_proto_zend_hash_index_find(t, (zend_ulong)def, (void**)&value) ==
-          SUCCESS);
+  return (php_proto_zend_hash_index_find_mem(t, (zend_ulong)def,
+                                             (void**)&value) == SUCCESS);
 }
 
 static void add_to_list(HashTable* t, void* value) {
   zval* pDest = NULL;
-  php_proto_zend_hash_next_index_insert(t, &value, sizeof(void*),
+  php_proto_zend_hash_next_index_insert_mem(t, &value, sizeof(void*),
                                         (void**)&pDest);
 }
 

php/ext/google/protobuf/protobuf.h

@@ -74,13 +74,22 @@
 
 #define PHP_PROTO_HASH_OF(array) Z_ARRVAL_P(array)
 
-#define php_proto_zend_hash_index_update(ht, h, pData, nDataSize, pDest) \
+#define php_proto_zend_hash_index_update_zval(ht, h, pData) \
+  zend_hash_index_update(ht, h, &(pData), sizeof(void*), NULL)
+
+#define php_proto_zend_hash_index_update_mem(ht, h, pData, nDataSize, pDest) \
   zend_hash_index_update(ht, h, pData, nDataSize, pDest)
 
-#define php_proto_zend_hash_index_find(ht, h, pDest) \
+#define php_proto_zend_hash_index_find_zval(ht, h, pDest) \
+  zend_hash_index_find(ht, h, pDest)
+
+#define php_proto_zend_hash_index_find_mem(ht, h, pDest) \
   zend_hash_index_find(ht, h, pDest)
 
-#define php_proto_zend_hash_next_index_insert(ht, pData, nDataSize, pDest) \
+#define php_proto_zend_hash_next_index_insert_zval(ht, pData) \
+  zend_hash_next_index_insert(ht, pData, sizeof(void*), NULL)
+
+#define php_proto_zend_hash_next_index_insert_mem(ht, pData, nDataSize, pDest) \
   zend_hash_next_index_insert(ht, pData, nDataSize, pDest)
 
 #define php_proto_zend_hash_get_current_data_ex(ht, pDest, pos) \
@@ -217,7 +226,14 @@
 
 #define PHP_PROTO_HASH_OF(array) Z_ARRVAL_P(&array)
 
-static inline int php_proto_zend_hash_index_update(HashTable* ht, ulong h,
+static inline int php_proto_zend_hash_index_update_zval(HashTable* ht, ulong h,
+                                                        zval* pData) {
+  void* result = NULL;
+  result = zend_hash_index_update(ht, h, pData);
+  return result != NULL ? SUCCESS : FAILURE;
+}
+
+static inline int php_proto_zend_hash_index_update_mem(HashTable* ht, ulong h,
                                                    void* pData, uint nDataSize,
                                                    void** pDest) {
   void* result = NULL;
@@ -226,18 +242,33 @@ static inline int php_proto_zend_hash_index_update(HashTable* ht, ulong h,
   return result != NULL ? SUCCESS : FAILURE;
 }
 
-static inline int php_proto_zend_hash_index_find(const HashTable* ht, ulong h,
-                                                 void** pDest) {
+static inline int php_proto_zend_hash_index_find_zval(const HashTable* ht,
+                                                      ulong h, void** pDest) {
+  zval* result = zend_hash_index_find(ht, h);
+  if (pDest != NULL) *pDest = result;
+  return result != NULL ? SUCCESS : FAILURE;
+}
+
+static inline int php_proto_zend_hash_index_find_mem(const HashTable* ht,
+                                                     ulong h, void** pDest) {
   void* result = NULL;
   result = zend_hash_index_find_ptr(ht, h);
   if (pDest != NULL) *pDest = result;
   return result != NULL ? SUCCESS : FAILURE;
 }
 
-static inline int php_proto_zend_hash_next_index_insert(HashTable* ht,
-                                                        void* pData,
-                                                        uint nDataSize,
-                                                        void** pDest) {
+static inline int php_proto_zend_hash_next_index_insert_zval(HashTable* ht,
+                                                             void* pData) {
+  zval tmp;
+  ZVAL_OBJ(&tmp, *(zend_object**)pData);
+  zval* result = zend_hash_next_index_insert(ht, &tmp);
+  return result != NULL ? SUCCESS : FAILURE;
+}
+
+static inline int php_proto_zend_hash_next_index_insert_mem(HashTable* ht,
+                                                            void* pData,
+                                                            uint nDataSize,
+                                                            void** pDest) {
   void* result = NULL;
   result = zend_hash_next_index_insert_mem(ht, pData, nDataSize);
   if (pDest != NULL) *pDest = result;
@@ -640,6 +671,8 @@ 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);
+bool native_slot_set_by_map(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, 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
@@ -654,6 +687,10 @@ void native_slot_get(upb_fieldtype_t type, const void* memory,
 // So we need to make a special method to handle that.
 void native_slot_get_by_array(upb_fieldtype_t type, const void* memory,
                      CACHED_VALUE* cache TSRMLS_DC);
+void native_slot_get_by_map_key(upb_fieldtype_t type, const void* memory,
+                                int length, CACHED_VALUE* cache TSRMLS_DC);
+void native_slot_get_by_map_value(upb_fieldtype_t type, const void* memory,
+                                  CACHED_VALUE* cache TSRMLS_DC);
 void native_slot_get_default(upb_fieldtype_t type,
                              CACHED_VALUE* cache TSRMLS_DC);
 

php/ext/google/protobuf/storage.c

@@ -198,6 +198,57 @@ bool native_slot_set_by_array(upb_fieldtype_t type,
         DEREF(memory, zval*) = value;
         Z_ADDREF_P(value);
       }
+#else
+      DEREF(memory, zval*) = value;
+      ++GC_REFCOUNT(Z_OBJ_P(value));
+#endif
+      break;
+    }
+    default:
+      return native_slot_set(type, klass, memory, value TSRMLS_CC);
+  }
+  return true;
+}
+
+bool native_slot_set_by_map(upb_fieldtype_t type, const zend_class_entry* klass,
+                            void* memory, zval* value TSRMLS_DC) {
+  switch (type) {
+    case UPB_TYPE_STRING:
+    case UPB_TYPE_BYTES: {
+      if (!protobuf_convert_to_string(value)) {
+        return false;
+      }
+      if (type == UPB_TYPE_STRING &&
+          !is_structurally_valid_utf8(Z_STRVAL_P(value), Z_STRLEN_P(value))) {
+        zend_error(E_USER_ERROR, "Given string is not UTF8 encoded.");
+        return false;
+      }
+
+      // Handles repeated/map string field. Memory provided by
+      // RepeatedField/Map is not initialized.
+#if PHP_MAJOR_VERSION < 7
+      MAKE_STD_ZVAL(DEREF(memory, zval*));
+      PHP_PROTO_ZVAL_STRINGL(DEREF(memory, zval*), Z_STRVAL_P(value),
+                             Z_STRLEN_P(value), 1);
+#else
+      *(zend_string**)memory = zend_string_dup(Z_STR_P(value), 0);
+#endif
+      break;
+    }
+    case UPB_TYPE_MESSAGE: {
+      if (Z_TYPE_P(value) != IS_OBJECT) {
+        zend_error(E_USER_ERROR, "Given value is not message.");
+        return false;
+      }
+      if (Z_TYPE_P(value) == IS_OBJECT && klass != Z_OBJCE_P(value)) {
+        zend_error(E_USER_ERROR, "Given message does not have correct class.");
+        return false;
+      }
+#if PHP_MAJOR_VERSION < 7
+      if (EXPECTED(DEREF(memory, zval*) != value)) {
+        DEREF(memory, zval*) = value;
+        Z_ADDREF_P(value);
+      }
 #else
       DEREF(memory, zend_object*) = Z_OBJ_P(value);
       ++GC_REFCOUNT(Z_OBJ_P(value));
@@ -348,8 +399,7 @@ void native_slot_get_by_array(upb_fieldtype_t type, const void* memory,
         ZVAL_ZVAL(CACHED_PTR_TO_ZVAL_PTR(cache), value, 1, 0);
       }
 #else
-      ++GC_REFCOUNT(*(zend_object**)memory);
-      ZVAL_OBJ(cache, *(zend_object**)memory);
+      ZVAL_COPY(CACHED_PTR_TO_ZVAL_PTR(cache), memory);
 #endif
       return;
     }
@@ -371,6 +421,26 @@ void native_slot_get_by_map_key(upb_fieldtype_t type, const void* memory,
   }
 }
 
+void native_slot_get_by_map_value(upb_fieldtype_t type, const void* memory,
+                              CACHED_VALUE* cache TSRMLS_DC) {
+  switch (type) {
+    case UPB_TYPE_MESSAGE: {
+#if PHP_MAJOR_VERSION < 7
+      zval* value = CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory);
+      if (EXPECTED(CACHED_PTR_TO_ZVAL_PTR(cache) != value)) {
+        ZVAL_ZVAL(CACHED_PTR_TO_ZVAL_PTR(cache), value, 1, 0);
+      }
+#else
+      ++GC_REFCOUNT(*(zend_object**)memory);
+      ZVAL_OBJ(cache, *(zend_object**)memory);
+#endif
+      return;
+    }
+    default:
+      native_slot_get_by_array(type, memory, cache TSRMLS_CC);
+  }
+}
+
 void native_slot_get_default(upb_fieldtype_t type,
                              CACHED_VALUE* cache TSRMLS_DC) {
   switch (type) {
@@ -952,8 +1022,18 @@ void layout_merge(MessageLayout* layout, MessageHeader* from,
           void* to_memory =
               ALLOC_N(char, native_slot_size(upb_fielddef_type(field)));
           memset(to_memory, 0, native_slot_size(upb_fielddef_type(field)));
-          php_proto_zend_hash_index_find(PHP_PROTO_HASH_OF(from_array->array),
-                                         j, (void**)&from_memory);
+
+          if (to_array->type == UPB_TYPE_MESSAGE) {
+            php_proto_zend_hash_index_find_zval(
+                PHP_PROTO_HASH_OF(from_array->array), j, (void**)&from_memory);
+#if PHP_MAJOR_VERSION >= 7
+            from_memory = &Z_OBJ_P((zval*)from_memory);
+#endif
+          } else {
+            php_proto_zend_hash_index_find_mem(
+                PHP_PROTO_HASH_OF(from_array->array), j, (void**)&from_memory);
+          }
+
           native_slot_merge_by_array(field, from_memory,
                                      to_memory PHP_PROTO_TSRMLS_CC);
           repeated_field_push_native(to_array, to_memory);

php/tests/array_test.php

@@ -471,6 +471,18 @@ class RepeatedFieldTest extends PHPUnit_Framework_TestCase
         $sub_m->setA(2);
         $arr[0] = $sub_m;
         $this->assertSame(2, $arr[0]->getA());
+
+        // Test foreach.
+        $arr = new RepeatedField(GPBType::MESSAGE, TestMessage_Sub::class);
+        for ($i = 0; $i < 3; $i++) {
+          $arr[] = new TestMessage_Sub();
+          $arr[$i]->setA($i);
+        }
+        $i = 0;
+        foreach ($arr as $val) {
+          $this->assertSame($i++, $val->getA());
+        }
+        $this->assertSame(3, $i);
     }
 
     #########################################################
@@ -521,23 +533,22 @@ class RepeatedFieldTest extends PHPUnit_Framework_TestCase
     # Test memory leak
     #########################################################
 
-    // COMMENTED OUT BY @bshaffer
-    // @see https://github.com/google/protobuf/pull/3344#issuecomment-315162761
-    // public function testCycleLeak()
-    // {
-    //     $arr = new RepeatedField(GPBType::MESSAGE, TestMessage::class);
-    //     $arr[] = new TestMessage;
-    //     $arr[0]->SetRepeatedRecursive($arr);
-
-    //     // Clean up memory before test.
-    //     gc_collect_cycles();
-    //     $start = memory_get_usage();
-    //     unset($arr);
-
-    //     // Explicitly trigger garbage collection.
-    //     gc_collect_cycles();
-
-    //     $end = memory_get_usage();
-    //     $this->assertLessThan($start, $end);
-    // }
+    public function testCycleLeak()
+    {
+        gc_collect_cycles();
+        $arr = new RepeatedField(GPBType::MESSAGE, TestMessage::class);
+        $arr[] = new TestMessage;
+        $arr[0]->SetRepeatedRecursive($arr);
+
+        // Clean up memory before test.
+        gc_collect_cycles();
+        $start = memory_get_usage();
+        unset($arr);
+
+        // Explicitly trigger garbage collection.
+        gc_collect_cycles();
+
+        $end = memory_get_usage();
+        $this->assertLessThan($start, $end);
+    }
 }

php/tests/map_field_test.php

@@ -417,6 +417,29 @@ class MapFieldTest extends PHPUnit_Framework_TestCase {
         $this->assertSame(1, $arr[0]->getA());
 
         $this->assertEquals(1, count($arr));
+
+        // Test foreach.
+        $arr = new MapField(GPBType::INT32,
+            GPBType::MESSAGE, TestMessage_Sub::class);
+        for ($i = 0; $i < 3; $i++) {
+          $arr[$i] = new TestMessage_Sub();;
+          $arr[$i]->setA($i);
+        }
+        $i = 0;
+        $key_test = [];
+        $value_test = [];
+        foreach ($arr as $key => $val) {
+          $key_test[] = $key;
+          $value_test[] = $val->getA();
+          $i++;
+        }
+        $this->assertTrue(isset($key_test['0']));
+        $this->assertTrue(isset($key_test['1']));
+        $this->assertTrue(isset($key_test['2']));
+        $this->assertTrue(isset($value_test['0']));
+        $this->assertTrue(isset($value_test['1']));
+        $this->assertTrue(isset($value_test['2']));
+        $this->assertSame(3, $i);
     }
 
     #########################################################