Merge pull request #8969 from haberman/php-destructor2

Fixed PHP SEGV when constructing messages from a destructor.

Makefile.am

@@ -967,6 +967,7 @@ php_EXTRA_DIST=                                                       \
   php/tests/GeneratedServiceTest.php                                  \
   php/tests/MapFieldTest.php                                          \
   php/tests/memory_leak_test.php                                      \
+  php/tests/memory_leak_test.sh                                       \
   php/tests/multirequest.php                                          \
   php/tests/multirequest.sh                                           \
   php/tests/PhpImplementationTest.php                                 \

kokoro/linux/php_all/build.sh

@@ -15,6 +15,9 @@ docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/ph
 docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.4.18-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
 docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
 
+# Run specialized memory leak & multirequest tests.
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test_c && tests/multirequest.sh && tests/memory_leak_test.sh"
+
 # Most of our tests use a debug build of PHP, but we do one build against an opt
 # php just in case that surfaces anything unexpected.
 docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"

php/ext/google/protobuf/def.c

@@ -730,45 +730,26 @@ static DescriptorPool *GetPool(const zval* this_ptr) {
   return (DescriptorPool*)Z_OBJ_P(this_ptr);
 }
 
-/**
- * Object handler to create an DescriptorPool.
- */
-static zend_object* DescriptorPool_create(zend_class_entry *class_type) {
-  DescriptorPool *intern = emalloc(sizeof(DescriptorPool));
-  zend_object_std_init(&intern->std, class_type);
-  intern->std.handlers = &DescriptorPool_object_handlers;
-  intern->symtab = upb_symtab_new();
-  // Skip object_properties_init(), we don't allow derived classes.
-  return &intern->std;
-}
-
 /**
  * Object handler to free an DescriptorPool.
  */
 static void DescriptorPool_destructor(zend_object* obj) {
   DescriptorPool* intern = (DescriptorPool*)obj;
-  if (intern->symtab) {
-    upb_symtab_free(intern->symtab);
-  }
-  intern->symtab = NULL;
+
+  // We can't free our underlying symtab here, because user code may create
+  // messages from destructors that will refer to it. The symtab will be freed
+  // by our RSHUTDOWN() handler in protobuf.c
+
   zend_object_std_dtor(&intern->std);
 }
 
 void DescriptorPool_CreateWithSymbolTable(zval *zv, upb_symtab *symtab) {
-  ZVAL_OBJ(zv, DescriptorPool_create(DescriptorPool_class_entry));
-
-  if (symtab) {
-    DescriptorPool *intern = GetPool(zv);
-    upb_symtab_free(intern->symtab);
-    intern->symtab = symtab;
-  }
-}
+  DescriptorPool *intern = emalloc(sizeof(DescriptorPool));
+  zend_object_std_init(&intern->std, DescriptorPool_class_entry);
+  intern->std.handlers = &DescriptorPool_object_handlers;
+  intern->symtab = symtab;
 
-upb_symtab *DescriptorPool_Steal(zval *zv) {
-  DescriptorPool *intern = GetPool(zv);
-  upb_symtab *ret = intern->symtab;
-  intern->symtab = NULL;
-  return ret;
+  ZVAL_OBJ(zv, &intern->std);
 }
 
 upb_symtab *DescriptorPool_GetSymbolTable() {
@@ -1120,7 +1101,7 @@ void Def_ModuleInit() {
                    DescriptorPool_methods);
   DescriptorPool_class_entry = zend_register_internal_class(&tmp_ce);
   DescriptorPool_class_entry->ce_flags |= ZEND_ACC_FINAL;
-  DescriptorPool_class_entry->create_object = DescriptorPool_create;
+  DescriptorPool_class_entry->create_object = CreateHandler_ReturnNull;
   h = &DescriptorPool_object_handlers;
   memcpy(h, &std_object_handlers, sizeof(zend_object_handlers));
   h->dtor_obj = DescriptorPool_destructor;

php/ext/google/protobuf/def.h

@@ -38,15 +38,10 @@
 // Initializes the Def module, which defines all of the descriptor classes.
 void Def_ModuleInit();
 
-// Creates a new DescriptorPool to wrap the given symtab. The DescriptorPool
-// takes ownership of the given symtab. If symtab is NULL, the DescriptorPool
-// will create an empty symtab instead.
+// Creates a new DescriptorPool to wrap the given symtab, which must not be
+// NULL.
 void DescriptorPool_CreateWithSymbolTable(zval *zv, upb_symtab *symtab);
 
-// Given a zval representing a DescriptorPool, steals and returns its symtab,
-// which is now owned by the caller.
-upb_symtab *DescriptorPool_Steal(zval *zv);
-
 upb_symtab *DescriptorPool_GetSymbolTable();
 
 // Returns true if the global descriptor pool already has the given filename.

php/ext/google/protobuf/protobuf.c

@@ -66,7 +66,7 @@ ZEND_BEGIN_MODULE_GLOBALS(protobuf)
   // to rebuild it from scratch. When keep_descriptor_pool_after_request==true,
   // we steal the upb_symtab from the global DescriptorPool object just before
   // destroying it.
-  upb_symtab *saved_symtab;
+  upb_symtab *global_symtab;
 
   // Object cache (see interface in protobuf.h).
   HashTable object_cache;
@@ -82,6 +82,13 @@ ZEND_BEGIN_MODULE_GLOBALS(protobuf)
   HashTable descriptors;
 ZEND_END_MODULE_GLOBALS(protobuf)
 
+void free_protobuf_globals(zend_protobuf_globals *globals) {
+  zend_hash_destroy(&globals->name_msg_cache);
+  zend_hash_destroy(&globals->name_enum_cache);
+  upb_symtab_free(globals->global_symtab);
+  globals->global_symtab = NULL;
+}
+
 ZEND_DECLARE_MODULE_GLOBALS(protobuf)
 
 const zval *get_generated_pool() {
@@ -146,14 +153,14 @@ const zval *get_generated_pool() {
 //     discouraged by the documentation: https://serverfault.com/a/231660
 
 static PHP_GSHUTDOWN_FUNCTION(protobuf) {
-  if (protobuf_globals->saved_symtab) {
-    upb_symtab_free(protobuf_globals->saved_symtab);
+  if (protobuf_globals->global_symtab) {
+    free_protobuf_globals(protobuf_globals);
   }
 }
 
 static PHP_GINIT_FUNCTION(protobuf) {
   ZVAL_NULL(&protobuf_globals->generated_pool);
-  protobuf_globals->saved_symtab = NULL;
+  protobuf_globals->global_symtab = NULL;
 }
 
 /**
@@ -164,12 +171,15 @@ static PHP_GINIT_FUNCTION(protobuf) {
 static PHP_RINIT_FUNCTION(protobuf) {
   // Create the global generated pool.
   // Reuse the symtab (if any) left to us by the last request.
-  upb_symtab *symtab = PROTOBUF_G(saved_symtab);
+  upb_symtab *symtab = PROTOBUF_G(global_symtab);
+  if (!symtab) {
+    PROTOBUF_G(global_symtab) = symtab = upb_symtab_new();
+    zend_hash_init(&PROTOBUF_G(name_msg_cache), 64, NULL, NULL, 0);
+    zend_hash_init(&PROTOBUF_G(name_enum_cache), 64, NULL, NULL, 0);
+  }
   DescriptorPool_CreateWithSymbolTable(&PROTOBUF_G(generated_pool), symtab);
 
   zend_hash_init(&PROTOBUF_G(object_cache), 64, NULL, NULL, 0);
-  zend_hash_init(&PROTOBUF_G(name_msg_cache), 64, NULL, NULL, 0);
-  zend_hash_init(&PROTOBUF_G(name_enum_cache), 64, NULL, NULL, 0);
   zend_hash_init(&PROTOBUF_G(descriptors), 64, NULL, ZVAL_PTR_DTOR, 0);
 
   return SUCCESS;
@@ -182,15 +192,12 @@ static PHP_RINIT_FUNCTION(protobuf) {
  */
 static PHP_RSHUTDOWN_FUNCTION(protobuf) {
   // Preserve the symtab if requested.
-  if (PROTOBUF_G(keep_descriptor_pool_after_request)) {
-    zval *zv = &PROTOBUF_G(generated_pool);
-    PROTOBUF_G(saved_symtab) = DescriptorPool_Steal(zv);
+  if (!PROTOBUF_G(keep_descriptor_pool_after_request)) {
+    free_protobuf_globals(ZEND_MODULE_GLOBALS_BULK(protobuf));
   }
 
   zval_dtor(&PROTOBUF_G(generated_pool));
   zend_hash_destroy(&PROTOBUF_G(object_cache));
-  zend_hash_destroy(&PROTOBUF_G(name_msg_cache));
-  zend_hash_destroy(&PROTOBUF_G(name_enum_cache));
   zend_hash_destroy(&PROTOBUF_G(descriptors));
 
   return SUCCESS;
@@ -296,7 +303,7 @@ static const zend_module_dep protobuf_deps[] = {
 
 PHP_INI_BEGIN()
 STD_PHP_INI_ENTRY("protobuf.keep_descriptor_pool_after_request", "0",
-                  PHP_INI_SYSTEM, OnUpdateBool,
+                  PHP_INI_ALL, OnUpdateBool,
                   keep_descriptor_pool_after_request, zend_protobuf_globals,
                   protobuf_globals)
 PHP_INI_END()

php/tests/memory_leak_test.php

@@ -2,9 +2,22 @@
 
 # phpunit has memory leak by itself. Thus, it cannot be used to test memory leak.
 
+class HasDestructor
+{
+  function __construct() {
+    $this->foo = $this;
+  }
+
+  function __destruct() {
+    new Foo\TestMessage();
+  }
+}
+
 require_once('../vendor/autoload.php');
 require_once('test_util.php');
 
+$has_destructor = new HasDestructor();
+
 use Google\Protobuf\Internal\RepeatedField;
 use Google\Protobuf\Internal\GPBType;
 use Foo\TestAny;

php/tests/memory_leak_test.sh

@@ -0,0 +1,40 @@
+#!/bin/bash
+
+cd $(dirname $0)
+
+set -ex
+
+PORT=12345
+TIMEOUT=10
+
+./compile_extension.sh
+
+run_test() {
+  echo
+  echo "Running memory leak test, args: $@"
+
+  EXTRA_ARGS=""
+  ARGS="-d xdebug.profiler_enable=0 -d display_errors=on -dextension=../ext/google/protobuf/modules/protobuf.so"
+
+  for i in "$@"; do
+    case $i in
+      --keep_descriptors)
+        EXTRA_ARGS=-dprotobuf.keep_descriptor_pool_after_request=1
+        shift
+        ;;
+    esac
+  done
+
+  export ZEND_DONT_UNLOAD_MODULES=1
+  export USE_ZEND_ALLOC=0
+
+  if valgrind --error-exitcode=1 --leak-check=full --show-leak-kinds=all --errors-for-leak-kinds=all --suppressions=valgrind.supp --num-callers=100 php $ARGS $EXTRA_ARGS memory_leak_test.php; then
+    echo "Memory leak test SUCCEEDED"
+  else
+    echo "Memory leak test FAILED"
+    exit 1
+  fi
+}
+
+run_test
+run_test --keep_descriptors

php/tests/multirequest.sh

@@ -5,28 +5,58 @@ cd $(dirname $0)
 set -e
 
 PORT=12345
+TIMEOUT=10
 
 ./compile_extension.sh
 
-nohup php -d protobuf.keep_descriptor_pool_after_request=1 -dextension=../ext/google/protobuf/modules/protobuf.so -S localhost:$PORT multirequest.php 2>&1 &
-
-sleep 1
-
-wget http://localhost:$PORT/multirequest.result -O multirequest.result
-wget http://localhost:$PORT/multirequest.result -O multirequest.result
-
-pushd ../ext/google/protobuf
-phpize --clean
-popd
-
-PID=`ps | grep "php" | awk '{print $1}'`
-echo $PID
-
-if [[ -z "$PID" ]]
-then
-  echo "Failed"
-  exit 1
-else
-  kill $PID
-  echo "Succeeded"
-fi
+run_test() {
+  echo
+  echo "Running multirequest test, args: $@"
+
+  RUN_UNDER=""
+  EXTRA_ARGS=""
+  ARGS="-d xdebug.profiler_enable=0 -d display_errors=on -dextension=../ext/google/protobuf/modules/protobuf.so"
+
+  for i in "$@"; do
+    case $i in
+      --valgrind)
+        RUN_UNDER="valgrind --error-exitcode=1"
+        shift
+        ;;
+      --keep_descriptors)
+        EXTRA_ARGS=-dprotobuf.keep_descriptor_pool_after_request=1
+        shift
+        ;;
+    esac
+  done
+
+  export ZEND_DONT_UNLOAD_MODULES=1
+  export USE_ZEND_ALLOC=0
+  rm -f nohup.out
+  nohup $RUN_UNDER php $ARGS $EXTRA_ARGS -S localhost:$PORT multirequest.php >nohup.out 2>&1 &
+  PID=$!
+
+  if ! timeout $TIMEOUT bash -c "until echo > /dev/tcp/localhost/$PORT; do sleep 0.1; done" > /dev/null 2>&1; then
+    echo "Server failed to come up after $TIMEOUT seconds"
+    cat nohup.out
+    exit 1
+  fi
+
+  seq 2 | xargs -I{} wget -nv http://localhost:$PORT/multirequest.result -O multirequest{}.result
+  REQUESTS_SUCCEEDED=$?
+
+
+  if kill $PID > /dev/null 2>&1 && [[ $REQUESTS_SUCCEEDED == "0" ]]; then
+    wait
+    echo "Multirequest test SUCCEEDED"
+  else
+    echo "Multirequest test FAILED"
+    cat nohup.out
+    exit 1
+  fi
+}
+
+run_test
+run_test --keep_descriptors
+run_test --valgrind
+run_test --valgrind --keep_descriptors

php/tests/valgrind.supp

@@ -10,3 +10,24 @@
    obj:/usr/bin/php7.3
    fun:__scandir64_tail
 }
+
+{
+   PHP_ModuleLoadingLeaks
+   Memcheck:Leak
+   ...
+   fun:php_module_startup
+}
+
+{
+   PHP_ModuleLoadingLeaks
+   Memcheck:Leak
+   ...
+   fun:php_module_startup
+}
+
+{
+   PHP_ModuleLoadingLeaks2
+   Memcheck:Leak
+   ...
+   fun:php_load_shlib
+}