Merge branch 'PHP-8.3' into PHP-8.4

* PHP-8.3: Fix GH-16628: FPM logs are getting corrupted with this log statement Fix GH-16601: Memory leak in Reflection constructors
2026-03-24 00:02:20 +01:00 · 2024-11-02 19:38:54 +01:00
parent 9e2367f29f 16cda10650
commit bfd9e0cca3
11 changed files with 224 additions and 13 deletions
--- a/7
+++ b/7
@@ -18,6 +18,10 @@ PHP                                                                        NEWS
  . Fixed bug GH-16593 (Assertion failure in DOM->replaceChild). (nielsdos)
  . Fixed bug GH-16595 (Another UAF in DOM -> cloneNode). (nielsdos)

+- FPM:
+  . Fixed bug GH-16628 (FPM logs are getting corrupted with this log
+    statement). (nielsdos)
+
 - GD:
  . Fixed bug GH-16559 (UBSan abort in ext/gd/libgd/gd_interpolation.c:1007).
    (nielsdos)
@@ -37,6 +41,9 @@ PHP                                                                        NEWS
 - PDO_ODBC:
  . Fixed bug GH-16450 (PDO_ODBC can inject garbage into field values). (cmb)

+- Reflection:
+  . Fixed bug GH-16601 (Memory leak in Reflection constructors). (nielsdos)
+
 - SPL:
  . Fixed bug GH-16588 (UAF in Observer->serialize). (nielsdos)
  . Fix GH-16477 (Segmentation fault when calling __debugInfo() after failed
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -225,18 +225,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:
 		{
@@ -251,9 +259,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;
@@ -2585,6 +2591,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;
@@ -2594,11 +2604,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 {
@@ -4129,10 +4143,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 {
@@ -5819,13 +5835,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);
@@ -6670,7 +6693,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;
--- a/ext/reflection/tests/ReflectionExtension_double_construct.phpt
+++ b/ext/reflection/tests/ReflectionExtension_double_construct.phpt
@@ -0,0 +1,20 @@
+--TEST--
+ReflectionExtension double construct call
+--FILE--
+<?php
+
+$r = new ReflectionExtension('standard');
+var_dump($r);
+$r->__construct('standard');
+var_dump($r);
+
+?>
+--EXPECT--
+object(ReflectionExtension)#1 (1) {
+  ["name"]=>
+  string(8) "standard"
+}
+object(ReflectionExtension)#1 (1) {
+  ["name"]=>
+  string(8) "standard"
+}
--- a/ext/reflection/tests/ReflectionObject_double_construct.phpt
+++ b/ext/reflection/tests/ReflectionObject_double_construct.phpt
@@ -0,0 +1,21 @@
+--TEST--
+ReflectionObject double construct call
+--FILE--
+<?php
+
+$obj = new stdClass;
+$r = new ReflectionObject($obj);
+var_dump($r);
+$r->__construct($obj);
+var_dump($r);
+
+?>
+--EXPECT--
+object(ReflectionObject)#2 (1) {
+  ["name"]=>
+  string(8) "stdClass"
+}
+object(ReflectionObject)#2 (1) {
+  ["name"]=>
+  string(8) "stdClass"
+}
--- a/ext/reflection/tests/ReflectionParameter_double_construct.phpt
+++ b/ext/reflection/tests/ReflectionParameter_double_construct.phpt
@@ -0,0 +1,27 @@
+--TEST--
+ReflectionParameter double construct call
+--FILE--
+<?php
+
+$closure = function (int $x): void {};
+$r = new ReflectionParameter($closure, 'x');
+var_dump($r);
+$r->__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"
+}
--- a/ext/reflection/tests/ReflectionProperty_double_construct.phpt
+++ b/ext/reflection/tests/ReflectionProperty_double_construct.phpt
@@ -0,0 +1,24 @@
+--TEST--
+ReflectionProperty double construct call
+--FILE--
+<?php
+
+$r = new ReflectionProperty(Exception::class, 'message');
+var_dump($r);
+$r->__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"
+}
--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -830,6 +830,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);
--- a/ext/zend_test/test.stub.php
+++ b/ext/zend_test/test.stub.php
@@ -307,6 +307,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 {
--- 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: 5ba6aceffff4e81330182eb84014928630fc9cdd */
+ * Stub hash: a7b5de3e4868f9a4aef78da6b98bbce882e129a9 */

 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0)
 ZEND_END_ARG_INFO()
@@ -165,6 +165,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
@@ -298,6 +302,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);
@@ -402,6 +407,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)
 #if (PHP_VERSION_ID >= 80400)
 	ZEND_RAW_FENTRY(ZEND_NS_NAME("ZendTestNS2", "namespaced_func"), zif_ZendTestNS2_namespaced_func, arginfo_ZendTestNS2_namespaced_func, 0, NULL, NULL)
 #else
--- a/sapi/fpm/fpm/zlog.c
+++ b/sapi/fpm/fpm/zlog.c
@@ -153,6 +153,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) /* {{{ */
@@ -189,6 +190,7 @@ static size_t zlog_buf_prefix(
 		}
 	}

+	/* Important: snprintf returns the number of bytes if the print were complete. */
 	return len;
 }
 /* }}} */
@@ -411,6 +413,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;
@@ -425,6 +428,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;
 	}
@@ -681,6 +685,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);
@@ -692,8 +707,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);
--- a/sapi/fpm/tests/gh16628.phpt
+++ 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--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+log_level = debug
+[unconfined]
+listen = {{ADDR}}
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+catch_workers_output = yes
+decorate_workers_output = no
+EOT;
+
+$code = <<<'EOT'
+<?php
+for ($i = 1; $i < 100; $i++) {
+	zend_test_log_err_debug(str_repeat("a", $i));
+}
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->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--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>