Skip to content

Commit

Permalink
Merge branch 'PHP-8.4'
Browse files Browse the repository at this point in the history
* PHP-8.4:
  Fix GH-16628: FPM logs are getting corrupted with this log statement
  Fix GH-16601: Memory leak in Reflection constructors
  • Loading branch information
nielsdos committed Nov 2, 2024
2 parents 5d7fe13 + bfd9e0c commit 64f2d11
Show file tree
Hide file tree
Showing 10 changed files with 217 additions and 13 deletions.
45 changes: 35 additions & 10 deletions ext/reflection/php_reflection.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
{
Expand All @@ -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;
Expand Down Expand Up @@ -2529,6 +2535,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;
Expand All @@ -2538,11 +2548,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 {
Expand Down Expand Up @@ -3974,10 +3988,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 {
Expand Down Expand Up @@ -5606,13 +5622,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);
Expand Down Expand Up @@ -6434,7 +6457,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;
Expand Down
20 changes: 20 additions & 0 deletions ext/reflection/tests/ReflectionExtension_double_construct.phpt
Original file line number Diff line number Diff line change
@@ -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"
}
21 changes: 21 additions & 0 deletions ext/reflection/tests/ReflectionObject_double_construct.phpt
Original file line number Diff line number Diff line change
@@ -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"
}
27 changes: 27 additions & 0 deletions ext/reflection/tests/ReflectionParameter_double_construct.phpt
Original file line number Diff line number Diff line change
@@ -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"
}
24 changes: 24 additions & 0 deletions ext/reflection/tests/ReflectionProperty_double_construct.phpt
Original file line number Diff line number Diff line change
@@ -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"
}
11 changes: 11 additions & 0 deletions ext/zend_test/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,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);
Expand Down
2 changes: 2 additions & 0 deletions ext/zend_test/test.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ function zend_test_set_fmode(bool $binary): 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 {
Expand Down
8 changes: 7 additions & 1 deletion ext/zend_test/test_arginfo.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 17 additions & 2 deletions sapi/fpm/fpm/zlog.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) /* {{{ */
Expand Down Expand Up @@ -189,6 +190,7 @@ static size_t zlog_buf_prefix(
}
}

/* Important: snprintf returns the number of bytes if the print were complete. */
return len;
}
/* }}} */
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
53 changes: 53 additions & 0 deletions sapi/fpm/tests/gh16628.phpt
Original file line number Diff line number Diff line change
@@ -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();
?>

0 comments on commit 64f2d11

Please sign in to comment.