Fix more memory leak for php c extension (#4211)

* Fix more memory leak for php c extension

* Fix memory leak for php5.5

php/ext/google/protobuf/encode_decode.c

@@ -484,11 +484,11 @@ static void map_slot_init(void* memory, upb_fieldtype_t type, zval* cache) {
       // Store zval** in memory in order to be consistent with the layout of
       // singular fields.
       zval** holder = ALLOC(zval*);
+      *(zval***)memory = holder;
       zval* tmp;
       MAKE_STD_ZVAL(tmp);
       PHP_PROTO_ZVAL_STRINGL(tmp, "", 0, 1);
       *holder = tmp;
-      *(zval***)memory = holder;
 #else
       *(zval**)memory = cache;
       PHP_PROTO_ZVAL_STRINGL(*(zval**)memory, "", 0, 1);
@@ -521,7 +521,7 @@ static void map_slot_uninit(void* memory, upb_fieldtype_t type) {
     case UPB_TYPE_BYTES: {
 #if PHP_MAJOR_VERSION < 7
       zval** holder = *(zval***)memory;
-      php_proto_zval_ptr_dtor(*holder);
+      zval_ptr_dtor(holder);
       FREE(holder);
 #else
       php_proto_zval_ptr_dtor(*(zval**)memory);
@@ -1621,6 +1621,7 @@ static void discard_unknown_fields(MessageHeader* msg) {
   stringsink* unknown = DEREF(message_data(msg), 0, stringsink*);
   if (unknown != NULL) {
     stringsink_uninit(unknown);
+    FREE(unknown);
     DEREF(message_data(msg), 0, stringsink*) = NULL;
   }
 

php/ext/google/protobuf/map.c

@@ -293,13 +293,46 @@ static bool map_field_read_dimension(zval *object, zval *key, int type,
   }
 }
 
+static bool map_index_unset(Map *intern, const char* keyval, int length) {
+  upb_value old_value;
+  if (upb_strtable_remove2(&intern->table, keyval, length, &old_value)) {
+    switch (intern->value_type) {
+      case UPB_TYPE_MESSAGE: {
+#if PHP_MAJOR_VERSION < 7
+        zval_ptr_dtor(upb_value_memory(&old_value));
+#else
+        zend_object* object = *(zend_object**)upb_value_memory(&old_value);
+        if(--GC_REFCOUNT(object) == 0) {
+          zend_objects_store_del(object);
+        }
+#endif
+        break;
+      }
+      case UPB_TYPE_STRING:
+      case UPB_TYPE_BYTES: {
+#if PHP_MAJOR_VERSION < 7
+        zval_ptr_dtor(upb_value_memory(&old_value));
+#else
+        zend_string* object = *(zend_string**)upb_value_memory(&old_value);
+        zend_string_release(object);
+#endif
+        break;
+      }
+      default:
+        break;
+    }
+  }
+}
+
 bool map_index_set(Map *intern, const char* keyval, int length, upb_value v) {
   // Replace any existing value by issuing a 'remove' operation first.
-  upb_strtable_remove2(&intern->table, keyval, length, NULL);
+  map_index_unset(intern, keyval, length);
+
   if (!upb_strtable_insert2(&intern->table, keyval, length, v)) {
     zend_error(E_USER_ERROR, "Could not insert into table");
     return false;
   }
+
   return true;
 }
 
@@ -326,12 +359,7 @@ static void map_field_write_dimension(zval *object, zval *key,
   v.ctype = UPB_CTYPE_UINT64;
 #endif
 
-  // Replace any existing value by issuing a 'remove' operation first.
-  upb_strtable_remove2(&intern->table, keyval, length, NULL);
-  if (!upb_strtable_insert2(&intern->table, keyval, length, v)) {
-    zend_error(E_USER_ERROR, "Could not insert into table");
-    return;
-  }
+  map_index_set(intern, keyval, length, v);
 }
 
 static bool map_field_unset_dimension(zval *object, zval *key TSRMLS_DC) {
@@ -348,7 +376,7 @@ static bool map_field_unset_dimension(zval *object, zval *key TSRMLS_DC) {
   v.ctype = UPB_CTYPE_UINT64;
 #endif
 
-  upb_strtable_remove2(&intern->table, keyval, length, &v);
+  map_index_unset(intern, keyval, length);
 
   return true;
 }

php/ext/google/protobuf/message.c

@@ -292,7 +292,9 @@ PHP_METHOD(Message, clear) {
   Descriptor* desc = msg->descriptor;
   zend_class_entry* ce = desc->klass;
 
+  zend_object_std_dtor(&msg->std TSRMLS_CC);
   object_properties_init(&msg->std, ce);
+
   layout_init(desc->layout, message_data(msg), &msg->std TSRMLS_CC);
 }
 
@@ -918,6 +920,7 @@ PHP_METHOD(Any, unpack) {
   PHP_PROTO_FAKE_SCOPE_BEGIN(any_type);
   zval* type_url_php = php_proto_message_read_property(
       getThis(), &type_url_member PHP_PROTO_TSRMLS_CC);
+  zval_dtor(&type_url_member);
   PHP_PROTO_FAKE_SCOPE_END;
 
   // Get fully-qualified name from type url.
@@ -953,6 +956,7 @@ PHP_METHOD(Any, unpack) {
   PHP_PROTO_FAKE_SCOPE_RESTART(any_type);
   zval* value = php_proto_message_read_property(
       getThis(), &value_member PHP_PROTO_TSRMLS_CC);
+  zval_dtor(&value_member);
   PHP_PROTO_FAKE_SCOPE_END;
 
   merge_from_string(Z_STRVAL_P(value), Z_STRLEN_P(value), desc, msg);
@@ -981,6 +985,8 @@ PHP_METHOD(Any, pack) {
   PHP_PROTO_FAKE_SCOPE_BEGIN(any_type);
   message_handlers->write_property(getThis(), &member, &data,
                                    NULL PHP_PROTO_TSRMLS_CC);
+  zval_dtor(&data);
+  zval_dtor(&member);
   PHP_PROTO_FAKE_SCOPE_END;
 
   // Set type url.
@@ -998,6 +1004,8 @@ PHP_METHOD(Any, pack) {
   PHP_PROTO_FAKE_SCOPE_RESTART(any_type);
   message_handlers->write_property(getThis(), &member, &type_url_php,
                                    NULL PHP_PROTO_TSRMLS_CC);
+  zval_dtor(&type_url_php);
+  zval_dtor(&member);
   PHP_PROTO_FAKE_SCOPE_END;
   FREE(type_url);
 }
@@ -1030,6 +1038,7 @@ PHP_METHOD(Any, is) {
   PHP_PROTO_FAKE_SCOPE_BEGIN(any_type);
   zval* value =
       php_proto_message_read_property(getThis(), &member PHP_PROTO_TSRMLS_CC);
+  zval_dtor(&member);
   PHP_PROTO_FAKE_SCOPE_END;
 
   // Compare two type url.

php/tests/gdb_test.sh

@@ -1,10 +1,18 @@
 #!/bin/bash
 
+VERSION=$1
+
+export PATH=/usr/local/php-$VERSION/bin:$PATH
+export C_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$C_INCLUDE_PATH
+export CPLUS_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$CPLUS_INCLUDE_PATH
+
+php -i | grep "Configuration"
+
 # gdb --args php -dextension=../ext/google/protobuf/modules/protobuf.so `which
 # phpunit` --bootstrap autoload.php tmp_test.php
 #
-gdb --args php -dextension=../ext/google/protobuf/modules/protobuf.so `which phpunit` --bootstrap autoload.php encode_decode_test.php
+# gdb --args php -dextension=../ext/google/protobuf/modules/protobuf.so `which phpunit` --bootstrap autoload.php encode_decode_test.php
 #
-# gdb --args php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
+gdb --args php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
 #
 # USE_ZEND_ALLOC=0 valgrind --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php

php/tests/memory_leak_test.php

@@ -50,6 +50,13 @@ $to->mergeFromString($data);
 
 TestUtil::assertTestMessage($to);
 
+$from = new TestMessage();
+TestUtil::setTestMessage2($from);
+
+$data = $from->serializeToString();
+
+$to->mergeFromString($data);
+
 // TODO(teboring): This causes following tests fail in php7.
 # $from->setRecursive($from);
 
@@ -104,7 +111,7 @@ assert(1 === $n->getOneofMessage()->getA());
 
 $m = new TestMessage();
 $m->mergeFromString(hex2bin('F80601'));
-assert('F80601', bin2hex($m->serializeToString()));
+assert('f80601' === bin2hex($m->serializeToString()));
 
 // Test create repeated field via array.
 $str_arr = array("abc");
@@ -142,13 +149,32 @@ $from = new \Google\Protobuf\Value();
 $from->setNumberValue(1);
 assert(1, $from->getNumberValue());
 
+// Test discard unknown in message.
+$m = new TestMessage();
+$from = hex2bin('F80601');
+$m->mergeFromString($from);
+$m->discardUnknownFields();
+$to = $m->serializeToString();
+assert("" === bin2hex($to));
+
+// Test clear
+$m = new TestMessage();
+TestUtil::setTestMessage($m);
+$m->clear();
+
+// Test unset map element
+$m = new TestMessage();
+$map = $m->getMapStringString();
+$map[1] = 1;
+unset($map[1]);
+
 // Test descriptor
 $pool = \Google\Protobuf\DescriptorPool::getGeneratedPool();
 $desc = $pool->getDescriptorByClassName("\Foo\TestMessage");
 $field = $desc->getField(1);
 
-# $from = new TestMessage();
-# $to = new TestMessage();
-# TestUtil::setTestMessage($from);
-# $to->mergeFrom($from);
-# TestUtil::assertTestMessage($to);
+$from = new TestMessage();
+$to = new TestMessage();
+TestUtil::setTestMessage($from);
+$to->mergeFrom($from);
+TestUtil::assertTestMessage($to);

php/tests/test.sh

@@ -1,5 +1,11 @@
 #!/bin/bash
 
+VERSION=$1
+
+export PATH=/usr/local/php-$VERSION/bin:$PATH
+export C_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$C_INCLUDE_PATH
+export CPLUS_INCLUDE_PATH=/usr/local/php-$VERSION/include/php/main:/usr/local/php-$VERSION/include/php:$CPLUS_INCLUDE_PATH
+
 # Compile c extension
 pushd ../ext/google/protobuf/
 make clean || true
@@ -15,7 +21,7 @@ do
   echo "****************************"
   echo "* $t"
   echo "****************************"
-  php -dextension=../ext/google/protobuf/modules/protobuf.so `which phpunit` --bootstrap autoload.php $t
+  # php -dextension=../ext/google/protobuf/modules/protobuf.so `which phpunit` --bootstrap autoload.php $t
   echo ""
 done
 
@@ -25,3 +31,15 @@ done
 export ZEND_DONT_UNLOAD_MODULES=1
 export USE_ZEND_ALLOC=0
 valgrind --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
+
+# TODO(teboring): Only for debug (phpunit has memory leak which blocks this beging used by
+# regresssion test.)
+
+# for t in "${tests[@]}"
+# do
+#   echo "****************************"
+#   echo "* $t (memory leak)"
+#   echo "****************************"
+#   valgrind --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so `which phpunit` --bootstrap autoload.php $t
+#   echo ""
+# done

tests.sh

@@ -420,7 +420,7 @@ build_php5.5_c() {
   use_php 5.5
   wget https://phar.phpunit.de/phpunit-4.8.0.phar -O /usr/bin/phpunit
   pushd php/tests
-  /bin/bash ./test.sh
+  /bin/bash ./test.sh 5.5
   popd
   # TODO(teboring): Add it back
   # pushd conformance
@@ -431,7 +431,7 @@ build_php5.5_c() {
 build_php5.5_zts_c() {
   use_php_zts 5.5
   wget https://phar.phpunit.de/phpunit-4.8.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 5.5-zts && cd ../..
   # TODO(teboring): Add it back
   # pushd conformance
   # make test_php_zts_c
@@ -454,7 +454,7 @@ build_php5.6() {
 build_php5.6_c() {
   use_php 5.6
   wget https://phar.phpunit.de/phpunit-5.7.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 5.6 && cd ../..
   # TODO(teboring): Add it back
   # pushd conformance
   # make test_php_c
@@ -464,7 +464,7 @@ build_php5.6_c() {
 build_php5.6_zts_c() {
   use_php_zts 5.6
   wget https://phar.phpunit.de/phpunit-5.7.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 5.6-zts && cd ../..
   # TODO(teboring): Add it back
   # pushd conformance
   # make test_php_zts_c
@@ -512,7 +512,7 @@ build_php7.0() {
 build_php7.0_c() {
   use_php 7.0
   wget https://phar.phpunit.de/phpunit-5.6.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 7.0 && cd ../..
   # TODO(teboring): Add it back
   # pushd conformance
   # make test_php_c
@@ -522,7 +522,7 @@ build_php7.0_c() {
 build_php7.0_zts_c() {
   use_php_zts 7.0
   wget https://phar.phpunit.de/phpunit-5.6.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 7.0-zts && cd ../..
   # TODO(teboring): Add it back.
   # pushd conformance
   # make test_php_zts_c
@@ -576,7 +576,7 @@ build_php7.1() {
 build_php7.1_c() {
   use_php 7.1
   wget https://phar.phpunit.de/phpunit-5.6.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 7.1 && cd ../..
   pushd conformance
   # make test_php_c
   popd
@@ -585,7 +585,7 @@ build_php7.1_c() {
 build_php7.1_zts_c() {
   use_php_zts 7.1
   wget https://phar.phpunit.de/phpunit-5.6.0.phar -O /usr/bin/phpunit
-  cd php/tests && /bin/bash ./test.sh && cd ../..
+  cd php/tests && /bin/bash ./test.sh 7.1-zts && cd ../..
   pushd conformance
   # make test_php_c
   popd