diff --git a/Makefile.am b/Makefile.am index 604932cef0..cd209f3cdc 100644 --- a/Makefile.am +++ b/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 \ diff --git a/kokoro/linux/php_all/build.sh b/kokoro/linux/php_all/build.sh index 1ee79adf81..cfcab00ecd 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/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" diff --git a/php/ext/google/protobuf/def.c b/php/ext/google/protobuf/def.c index 88c201f04e..666238302b 100644 --- a/php/ext/google/protobuf/def.c +++ b/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; diff --git a/php/ext/google/protobuf/def.h b/php/ext/google/protobuf/def.h index e70564260e..ed944abb31 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 888b434b7b..625008f5b0 100644 --- a/php/ext/google/protobuf/protobuf.c +++ b/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() diff --git a/php/tests/memory_leak_test.php b/php/tests/memory_leak_test.php index 1f2d0950e9..ddb8491c00 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 0000000000..0b669575aa --- /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 ec4a1ae51e..65bfcfdc65 100755 --- a/php/tests/multirequest.sh +++ b/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 diff --git a/php/tests/valgrind.supp b/php/tests/valgrind.supp index e83b0a3dfa..8d31466959 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 +}