From f0f666ba3f989167f69ffdbcd5826e78c96ab668 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 1 Nov 2024 19:39:39 +0100 Subject: [PATCH 1/2] Fix GH-16601: Memory leak in Reflection constructors Additionally fixes wrong behaviour in ReflectionParameter when you first have a construction that uses an object and the subsequent doesn't. Closes GH-16672. --- NEWS | 3 ++ ext/reflection/php_reflection.c | 45 ++++++++++++++----- .../ReflectionExtension_double_construct.phpt | 20 +++++++++ .../ReflectionObject_double_construct.phpt | 21 +++++++++ .../ReflectionParameter_double_construct.phpt | 27 +++++++++++ .../ReflectionProperty_double_construct.phpt | 24 ++++++++++ 6 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 ext/reflection/tests/ReflectionExtension_double_construct.phpt create mode 100644 ext/reflection/tests/ReflectionObject_double_construct.phpt create mode 100644 ext/reflection/tests/ReflectionParameter_double_construct.phpt create mode 100644 ext/reflection/tests/ReflectionProperty_double_construct.phpt diff --git a/NEWS b/NEWS index 7beb1d5711e..b7e985a7406 100644 --- a/NEWS +++ b/NEWS @@ -91,6 +91,9 @@ PHP NEWS - PHPDBG: . Fixed bug GH-16174 (Empty string is an invalid expression for ev). (cmb) +- Reflection: + . Fixed bug GH-16601 (Memory leak in Reflection constructors). (nielsdos) + - Session: . Fixed bug GH-16385 (Unexpected null returned by session_set_cookie_params). (nielsdos) diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 3faf8ed7868..af57d1812e4 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -217,18 +217,26 @@ static void _free_function(zend_function *fptr) /* {{{ */ } /* }}} */ +static void reflection_free_property_reference(property_reference *reference) +{ + zend_string_release_ex(reference->unmangled_name, 0); + efree(reference); +} + +static void reflection_free_parameter_reference(parameter_reference *reference) +{ + _free_function(reference->fptr); + efree(reference); +} + static void reflection_free_objects_storage(zend_object *object) /* {{{ */ { reflection_object *intern = reflection_object_from_obj(object); - parameter_reference *reference; - property_reference *prop_reference; if (intern->ptr) { switch (intern->ref_type) { case REF_TYPE_PARAMETER: - reference = (parameter_reference*)intern->ptr; - _free_function(reference->fptr); - efree(intern->ptr); + reflection_free_parameter_reference(intern->ptr); break; case REF_TYPE_TYPE: { @@ -243,9 +251,7 @@ static void reflection_free_objects_storage(zend_object *object) /* {{{ */ _free_function(intern->ptr); break; case REF_TYPE_PROPERTY: - prop_reference = (property_reference*)intern->ptr; - zend_string_release_ex(prop_reference->unmangled_name, 0); - efree(intern->ptr); + reflection_free_property_reference(intern->ptr); break; case REF_TYPE_ATTRIBUTE: { attribute_reference *attr_ref = intern->ptr; @@ -2546,6 +2552,10 @@ ZEND_METHOD(ReflectionParameter, __construct) } } + if (intern->ptr) { + reflection_free_parameter_reference(intern->ptr); + } + ref = (parameter_reference*) emalloc(sizeof(parameter_reference)); ref->arg_info = &arg_info[position]; ref->offset = (uint32_t)position; @@ -2555,11 +2565,15 @@ ZEND_METHOD(ReflectionParameter, __construct) intern->ptr = ref; intern->ref_type = REF_TYPE_PARAMETER; intern->ce = ce; + zval_ptr_dtor(&intern->obj); if (reference && is_closure) { ZVAL_COPY_VALUE(&intern->obj, reference); + } else { + ZVAL_UNDEF(&intern->obj); } prop_name = reflection_prop_name(object); + zval_ptr_dtor(prop_name); if (has_internal_arg_info(fptr)) { ZVAL_STRING(prop_name, ((zend_internal_arg_info*)arg_info)[position].name); } else { @@ -4015,10 +4029,12 @@ static void reflection_class_object_ctor(INTERNAL_FUNCTION_PARAMETERS, int is_ob object = ZEND_THIS; intern = Z_REFLECTION_P(object); + /* Note: class entry name is interned, no need to destroy them */ if (arg_obj) { ZVAL_STR_COPY(reflection_prop_name(object), arg_obj->ce->name); intern->ptr = arg_obj->ce; if (is_object) { + zval_ptr_dtor(&intern->obj); ZVAL_OBJ_COPY(&intern->obj, arg_obj); } } else { @@ -5510,13 +5526,20 @@ ZEND_METHOD(ReflectionProperty, __construct) } } - ZVAL_STR_COPY(reflection_prop_name(object), name); + zval *prop_name = reflection_prop_name(object); + zval_ptr_dtor(prop_name); + ZVAL_STR_COPY(prop_name, name); + /* Note: class name are always interned, no need to destroy them */ if (dynam_prop == 0) { ZVAL_STR_COPY(reflection_prop_class(object), property_info->ce->name); } else { ZVAL_STR_COPY(reflection_prop_class(object), ce->name); } + if (intern->ptr) { + reflection_free_property_reference(intern->ptr); + } + reference = (property_reference*) emalloc(sizeof(property_reference)); reference->prop = dynam_prop ? NULL : property_info; reference->unmangled_name = zend_string_copy(name); @@ -5949,7 +5972,9 @@ ZEND_METHOD(ReflectionExtension, __construct) RETURN_THROWS(); } free_alloca(lcname, use_heap); - ZVAL_STRING(reflection_prop_name(object), module->name); + zval *prop_name = reflection_prop_name(object); + zval_ptr_dtor(prop_name); + ZVAL_STRING(prop_name, module->name); intern->ptr = module; intern->ref_type = REF_TYPE_OTHER; intern->ce = NULL; diff --git a/ext/reflection/tests/ReflectionExtension_double_construct.phpt b/ext/reflection/tests/ReflectionExtension_double_construct.phpt new file mode 100644 index 00000000000..7bff11c4b17 --- /dev/null +++ b/ext/reflection/tests/ReflectionExtension_double_construct.phpt @@ -0,0 +1,20 @@ +--TEST-- +ReflectionExtension double construct call +--FILE-- +__construct('standard'); +var_dump($r); + +?> +--EXPECT-- +object(ReflectionExtension)#1 (1) { + ["name"]=> + string(8) "standard" +} +object(ReflectionExtension)#1 (1) { + ["name"]=> + string(8) "standard" +} diff --git a/ext/reflection/tests/ReflectionObject_double_construct.phpt b/ext/reflection/tests/ReflectionObject_double_construct.phpt new file mode 100644 index 00000000000..536b145243b --- /dev/null +++ b/ext/reflection/tests/ReflectionObject_double_construct.phpt @@ -0,0 +1,21 @@ +--TEST-- +ReflectionObject double construct call +--FILE-- +__construct($obj); +var_dump($r); + +?> +--EXPECT-- +object(ReflectionObject)#2 (1) { + ["name"]=> + string(8) "stdClass" +} +object(ReflectionObject)#2 (1) { + ["name"]=> + string(8) "stdClass" +} diff --git a/ext/reflection/tests/ReflectionParameter_double_construct.phpt b/ext/reflection/tests/ReflectionParameter_double_construct.phpt new file mode 100644 index 00000000000..cc69e6b10e3 --- /dev/null +++ b/ext/reflection/tests/ReflectionParameter_double_construct.phpt @@ -0,0 +1,27 @@ +--TEST-- +ReflectionParameter double construct call +--FILE-- +__construct($closure, 'x'); +var_dump($r); +$r->__construct('ord', 'character'); +var_dump($r); + +?> +--EXPECT-- +object(ReflectionParameter)#2 (1) { + ["name"]=> + string(1) "x" +} +object(ReflectionParameter)#2 (1) { + ["name"]=> + string(1) "x" +} +object(ReflectionParameter)#2 (1) { + ["name"]=> + string(9) "character" +} diff --git a/ext/reflection/tests/ReflectionProperty_double_construct.phpt b/ext/reflection/tests/ReflectionProperty_double_construct.phpt new file mode 100644 index 00000000000..086f2f781a5 --- /dev/null +++ b/ext/reflection/tests/ReflectionProperty_double_construct.phpt @@ -0,0 +1,24 @@ +--TEST-- +ReflectionProperty double construct call +--FILE-- +__construct(Exception::class, 'message'); +var_dump($r); + +?> +--EXPECT-- +object(ReflectionProperty)#1 (2) { + ["name"]=> + string(7) "message" + ["class"]=> + string(9) "Exception" +} +object(ReflectionProperty)#1 (2) { + ["name"]=> + string(7) "message" + ["class"]=> + string(9) "Exception" +} From e643129bbb40ff6fbfd8a0729d001e64fb8bd8c0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 2 Nov 2024 12:59:52 +0100 Subject: [PATCH 2/2] Fix GH-16628: FPM logs are getting corrupted with this log statement zlog_buf_prefix() can return a larger length than what actually was written due to its use of snprintf(). The code in zlog_stream_prefix_ex() does not take this into account, other callers do. What ends up happening then is that stream->length is set to the length as if snprintf() was able to write all bytes, causing stream->length to become larger than stream->buf.size, causing a segfault. In case the buffer was too small we try with a larger buffer up to a limit of zlog_limit. This makes sure that the stream length will remain bounded by the buffer size. This also adds assertions to make the programmer intent clear and catch this more easily in debug builds. Closes GH-16680. --- NEWS | 4 +++ ext/zend_test/test.c | 11 ++++++++ ext/zend_test/test.stub.php | 2 ++ ext/zend_test/test_arginfo.h | 8 +++++- sapi/fpm/fpm/zlog.c | 19 +++++++++++-- sapi/fpm/tests/gh16628.phpt | 53 ++++++++++++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 sapi/fpm/tests/gh16628.phpt diff --git a/NEWS b/NEWS index b7e985a7406..6eec978b3de 100644 --- a/NEWS +++ b/NEWS @@ -54,6 +54,10 @@ PHP NEWS - Filter: . Fixed bug GH-16523 (FILTER_FLAG_HOSTNAME accepts ending hyphen). (cmb) +- FPM: + . Fixed bug GH-16628 (FPM logs are getting corrupted with this log + statement). (nielsdos) + - GD: . Fixed bug GH-16334 (imageaffine overflow on matrix elements). (David Carlier) diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c index 55e21a6c40a..100ef25800f 100644 --- a/ext/zend_test/test.c +++ b/ext/zend_test/test.c @@ -639,6 +639,17 @@ static ZEND_FUNCTION(zend_test_is_zend_ptr) RETURN_BOOL(is_zend_ptr((void*)addr)); } +static ZEND_FUNCTION(zend_test_log_err_debug) +{ + zend_string *str; + + ZEND_PARSE_PARAMETERS_START(1, 1) + Z_PARAM_STR(str); + ZEND_PARSE_PARAMETERS_END(); + + php_log_err_with_severity(ZSTR_VAL(str), LOG_DEBUG); +} + static zend_object *zend_test_class_new(zend_class_entry *class_type) { zend_object *obj = zend_objects_new(class_type); diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index 8a605bdcd32..547e82d60ca 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -196,6 +196,8 @@ function zend_test_override_libxml_global_state(): void {} function zend_test_cast_fread($stream): void {} function zend_test_is_zend_ptr(int $addr): bool {} + + function zend_test_log_err_debug(string $str): void {} } namespace ZendTestNS { diff --git a/ext/zend_test/test_arginfo.h b/ext/zend_test/test_arginfo.h index 9c0735a1d6f..bd86f090ff1 100644 --- a/ext/zend_test/test_arginfo.h +++ b/ext/zend_test/test_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 5d861e05edfd57c385167b11b8b1ea977ed130a2 */ + * Stub hash: 1c6384894b21ff02bbc3d6088440de6616878a7d */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0) ZEND_END_ARG_INFO() @@ -126,6 +126,10 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_is_zend_ptr, 0, 1, _IS ZEND_ARG_TYPE_INFO(0, addr, IS_LONG, 0) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_log_err_debug, 0, 1, IS_VOID, 0) + ZEND_ARG_TYPE_INFO(0, str, IS_STRING, 0) +ZEND_END_ARG_INFO() + #define arginfo_ZendTestNS2_namespaced_func arginfo_zend_test_is_pcre_bundled #define arginfo_ZendTestNS2_namespaced_deprecated_func arginfo_zend_test_void_return @@ -237,6 +241,7 @@ static ZEND_FUNCTION(zend_test_set_fmode); #endif static ZEND_FUNCTION(zend_test_cast_fread); static ZEND_FUNCTION(zend_test_is_zend_ptr); +static ZEND_FUNCTION(zend_test_log_err_debug); static ZEND_FUNCTION(ZendTestNS2_namespaced_func); static ZEND_FUNCTION(ZendTestNS2_namespaced_deprecated_func); static ZEND_FUNCTION(ZendTestNS2_ZendSubNS_namespaced_func); @@ -302,6 +307,7 @@ static const zend_function_entry ext_functions[] = { #endif ZEND_FE(zend_test_cast_fread, arginfo_zend_test_cast_fread) ZEND_FE(zend_test_is_zend_ptr, arginfo_zend_test_is_zend_ptr) + ZEND_FE(zend_test_log_err_debug, arginfo_zend_test_log_err_debug) ZEND_NS_FALIAS("ZendTestNS2", namespaced_func, ZendTestNS2_namespaced_func, arginfo_ZendTestNS2_namespaced_func) ZEND_NS_DEP_FALIAS("ZendTestNS2", namespaced_deprecated_func, ZendTestNS2_namespaced_deprecated_func, arginfo_ZendTestNS2_namespaced_deprecated_func) ZEND_NS_FALIAS("ZendTestNS2", namespaced_aliased_func, zend_test_void_return, arginfo_ZendTestNS2_namespaced_aliased_func) diff --git a/sapi/fpm/fpm/zlog.c b/sapi/fpm/fpm/zlog.c index e5e49f8385d..232f0cb1b2b 100644 --- a/sapi/fpm/fpm/zlog.c +++ b/sapi/fpm/fpm/zlog.c @@ -150,6 +150,7 @@ static inline void zlog_external( } /* }}} */ +/* Returns the length if the print were complete, this can be larger than buf_size. */ static size_t zlog_buf_prefix( const char *function, int line, int flags, char *buf, size_t buf_size, int use_syslog) /* {{{ */ @@ -186,6 +187,7 @@ static size_t zlog_buf_prefix( } } + /* Important: snprintf returns the number of bytes if the print were complete. */ return len; } /* }}} */ @@ -408,6 +410,7 @@ static inline ssize_t zlog_stream_unbuffered_write( static inline ssize_t zlog_stream_buf_copy_cstr( struct zlog_stream *stream, const char *str, size_t str_len) /* {{{ */ { + ZEND_ASSERT(stream->len <= stream->buf.size); if (stream->buf.size - stream->len <= str_len && !zlog_stream_buf_alloc_ex(stream, str_len + stream->len)) { return -1; @@ -422,6 +425,7 @@ static inline ssize_t zlog_stream_buf_copy_cstr( static inline ssize_t zlog_stream_buf_copy_char(struct zlog_stream *stream, char c) /* {{{ */ { + ZEND_ASSERT(stream->len <= stream->buf.size); if (stream->buf.size - stream->len < 1 && !zlog_stream_buf_alloc_ex(stream, 1)) { return -1; } @@ -678,6 +682,17 @@ ssize_t zlog_stream_prefix_ex(struct zlog_stream *stream, const char *function, len = zlog_buf_prefix( function, line, stream->flags, stream->buf.data, stream->buf.size, stream->use_syslog); + if (!EXPECTED(len + 1 <= stream->buf.size)) { + /* If the buffer was not large enough, try with a larger buffer. + * Note that this may still truncate if the zlog_limit is reached. */ + len = MIN(len + 1, zlog_limit); + if (!zlog_stream_buf_alloc_ex(stream, len)) { + return -1; + } + zlog_buf_prefix( + function, line, stream->flags, + stream->buf.data, stream->buf.size, stream->use_syslog); + } stream->len = stream->prefix_len = len; if (stream->msg_prefix != NULL) { zlog_stream_buf_copy_cstr(stream, stream->msg_prefix, stream->msg_prefix_len); @@ -689,8 +704,8 @@ ssize_t zlog_stream_prefix_ex(struct zlog_stream *stream, const char *function, } else { char sbuf[1024]; ssize_t written; - len = zlog_buf_prefix(function, line, stream->flags, sbuf, 1024, stream->use_syslog); - written = zlog_stream_direct_write(stream, sbuf, len); + len = zlog_buf_prefix(function, line, stream->flags, sbuf, sizeof(sbuf), stream->use_syslog); + written = zlog_stream_direct_write(stream, sbuf, MIN(len, sizeof(sbuf))); if (stream->msg_prefix != NULL) { written += zlog_stream_direct_write( stream, stream->msg_prefix, stream->msg_prefix_len); diff --git a/sapi/fpm/tests/gh16628.phpt b/sapi/fpm/tests/gh16628.phpt new file mode 100644 index 00000000000..e2df0c8cb84 --- /dev/null +++ b/sapi/fpm/tests/gh16628.phpt @@ -0,0 +1,53 @@ +--TEST-- +GH-16628 (FPM logs are getting corrupted with this log statement) +--EXTENSIONS-- +zend_test +--SKIPIF-- + +--FILE-- +start(); +$tester->expectLogStartNotices(); +$tester->request()->expectEmptyBody(); +for ($i = 1; $i < 100; $i++) { + $tester->expectLogNotice("%sPHP message: " . str_repeat("a", $i)); +} +$tester->terminate(); +$tester->expectLogTerminatingNotices(); +$tester->close(); + +?> +Done +--EXPECT-- +Done +--CLEAN-- +