Fixed PHP SEGV when constructing messages from a destructor.
This also fixes the keep_descriptor_pool_after_request option, which
was not working properly.
diff --git a/kokoro/linux/php_all/build.sh b/kokoro/linux/php_all/build.sh
index 1ee79ad..cfcab00 100755
--- a/kokoro/linux/php_all/build.sh
+++ b/kokoro/linux/php_all/build.sh
@@ -15,6 +15,9 @@
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"
diff --git a/php/ext/google/protobuf/def.c b/php/ext/google/protobuf/def.c
index 88c201f..6662383 100644
--- a/php/ext/google/protobuf/def.c
+++ b/php/ext/google/protobuf/def.c
@@ -731,44 +731,25 @@
}
/**
- * 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));
+ DescriptorPool *intern = emalloc(sizeof(DescriptorPool));
+ zend_object_std_init(&intern->std, DescriptorPool_class_entry);
+ intern->std.handlers = &DescriptorPool_object_handlers;
+ intern->symtab = symtab;
- if (symtab) {
- DescriptorPool *intern = GetPool(zv);
- upb_symtab_free(intern->symtab);
- 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 @@
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;
diff --git a/php/ext/google/protobuf/def.h b/php/ext/google/protobuf/def.h
index e705642..ed944ab 100644
--- a/php/ext/google/protobuf/def.h
+++ b/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.
diff --git a/php/ext/google/protobuf/protobuf.c b/php/ext/google/protobuf/protobuf.c
index 888b434..625008f 100644
--- a/php/ext/google/protobuf/protobuf.c
+++ b/php/ext/google/protobuf/protobuf.c
@@ -66,7 +66,7 @@
// 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 @@
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 @@
// 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_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_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 @@
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()
diff --git a/php/tests/memory_leak_test.php b/php/tests/memory_leak_test.php
index 1f2d095..ddb8491 100644
--- a/php/tests/memory_leak_test.php
+++ b/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;
diff --git a/php/tests/memory_leak_test.sh b/php/tests/memory_leak_test.sh
new file mode 100755
index 0000000..0b66957
--- /dev/null
+++ b/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
diff --git a/php/tests/multirequest.sh b/php/tests/multirequest.sh
index ec4a1ae..24db507 100755
--- a/php/tests/multirequest.sh
+++ b/php/tests/multirequest.sh
@@ -5,28 +5,58 @@
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 &
+run_test() {
+ echo
+ echo "Running multirequest test, args: $@"
-sleep 1
+ RUN_UNDER=""
+ EXTRA_ARGS=""
+ ARGS="-d xdebug.profiler_enable=0 -d display_errors=on -dextension=../ext/google/protobuf/modules/protobuf.so"
-wget http://localhost:$PORT/multirequest.result -O multirequest.result
-wget http://localhost:$PORT/multirequest.result -O multirequest.result
+ 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
-pushd ../ext/google/protobuf
-phpize --clean
-popd
+ export ZEND_DONT_UNLOAD_MODULES=1
+ export USE_ZEND_ALLOC=0
+ rm nohup.out
+ nohup $RUN_UNDER php $ARGS $EXTRA_ARGS -S localhost:$PORT multirequest.php >nohup.out 2>&1 &
+ PID=$!
-PID=`ps | grep "php" | awk '{print $1}'`
-echo $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
-if [[ -z "$PID" ]]
-then
- echo "Failed"
- exit 1
-else
- kill $PID
- echo "Succeeded"
-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
diff --git a/php/tests/valgrind.supp b/php/tests/valgrind.supp
index e83b0a3..8d31466 100644
--- a/php/tests/valgrind.supp
+++ b/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
+}
