From 396ac4df1415b868c493f56873b21ce42b09b35d Mon Sep 17 00:00:00 2001 From: Alexandre Daubois Date: Tue, 30 Sep 2025 10:58:20 +0200 Subject: [PATCH 1/2] Fix GH-19999: GC refcount assertion failure when destructor modifies variable during concat --- Zend/tests/gh19999.phpt | 16 ++++++++++++++++ Zend/zend_operators.c | 35 ++++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 Zend/tests/gh19999.phpt diff --git a/Zend/tests/gh19999.phpt b/Zend/tests/gh19999.phpt new file mode 100644 index 0000000000000..e42a907fd0ec9 --- /dev/null +++ b/Zend/tests/gh19999.phpt @@ -0,0 +1,16 @@ +--TEST-- +GH-19999 (GC refcount assertion failure when destructor modifies variable during concat) +--FILE-- + +--EXPECTF-- +Warning: Array to string conversion in %s on line %d diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index 8646fc25be260..1728938dab9b1 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -1954,9 +1954,21 @@ ZEND_API zend_result ZEND_FASTCALL shift_right_function(zval *result, zval *op1, ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval *op2) /* {{{ */ { zval *orig_op1 = op1; + zval orig_result = {0}; zend_string *op1_string, *op2_string; bool free_op1_string = false; bool free_op2_string = false; + bool result_is_orig = (result == orig_op1); + + /* Save the original result value and protect it from being freed during conversion. + * We add a reference because destructors can modify the zval in-place, and we need + * to ensure we can properly clean up the original value. */ + if (result_is_orig) { + ZVAL_COPY_VALUE(&orig_result, result); + if (Z_REFCOUNTED(orig_result)) { + Z_ADDREF(orig_result); + } + } do { if (EXPECTED(Z_TYPE_P(op1) == IS_STRING)) { @@ -1973,6 +1985,9 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval op1_string = zval_get_string_func(op1); if (UNEXPECTED(EG(exception))) { zend_string_release(op1_string); + if (result_is_orig && Z_REFCOUNTED(orig_result)) { + i_zval_ptr_dtor(&orig_result); + } if (orig_op1 != result) { ZVAL_UNDEF(result); } @@ -2008,6 +2023,9 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval if (UNEXPECTED(EG(exception))) { zend_string_release(op1_string); zend_string_release(op2_string); + if (result_is_orig && Z_REFCOUNTED(orig_result)) { + i_zval_ptr_dtor(&orig_result); + } if (orig_op1 != result) { ZVAL_UNDEF(result); } @@ -2020,9 +2038,6 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval has_op2_string:; if (UNEXPECTED(ZSTR_LEN(op1_string) == 0)) { if (EXPECTED(result != op2 || Z_TYPE_P(result) != IS_STRING)) { - if (result == orig_op1) { - i_zval_ptr_dtor(result); - } if (free_op2_string) { /* transfer ownership of op2_string */ ZVAL_STR(result, op2_string); @@ -2033,9 +2048,6 @@ has_op2_string:; } } else if (UNEXPECTED(ZSTR_LEN(op2_string) == 0)) { if (EXPECTED(result != op1 || Z_TYPE_P(result) != IS_STRING)) { - if (result == orig_op1) { - i_zval_ptr_dtor(result); - } if (free_op1_string) { /* transfer ownership of op1_string */ ZVAL_STR(result, op1_string); @@ -2054,6 +2066,9 @@ has_op2_string:; if (UNEXPECTED(op1_len > ZSTR_MAX_LEN - op2_len)) { if (free_op1_string) zend_string_release(op1_string); if (free_op2_string) zend_string_release(op2_string); + if (result_is_orig && Z_REFCOUNTED(orig_result)) { + i_zval_ptr_dtor(&orig_result); + } zend_throw_error(NULL, "String size overflow"); if (orig_op1 != result) { ZVAL_UNDEF(result); @@ -2065,7 +2080,9 @@ has_op2_string:; /* Destroy the old result first to drop the refcount, such that $x .= ...; may happen in-place. */ if (free_op1_string) { /* op1_string will be used as the result, so we should not free it */ - i_zval_ptr_dtor(result); + if (!result_is_orig) { + i_zval_ptr_dtor(result); + } /* Set it to NULL in case that the extension will throw an out-of-memory error. * Otherwise the shutdown sequence will try to free this again. */ ZVAL_NULL(result); @@ -2095,6 +2112,10 @@ has_op2_string:; ZSTR_VAL(result_str)[result_len] = '\0'; } + if (result_is_orig && Z_REFCOUNTED(orig_result)) { + i_zval_ptr_dtor(&orig_result); + } + if (free_op1_string) zend_string_release(op1_string); if (free_op2_string) zend_string_release(op2_string); From 72bf931b6cf5521f17639f305de6b3d24ffe23b2 Mon Sep 17 00:00:00 2001 From: Alexandre Daubois Date: Tue, 30 Sep 2025 15:54:21 +0200 Subject: [PATCH 2/2] Fix GH-19999: GC refcount assertion failure when destructor modifies variable during concat --- Zend/zend_operators.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index 1728938dab9b1..5d6b89868fa2f 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -2080,9 +2080,7 @@ has_op2_string:; /* Destroy the old result first to drop the refcount, such that $x .= ...; may happen in-place. */ if (free_op1_string) { /* op1_string will be used as the result, so we should not free it */ - if (!result_is_orig) { - i_zval_ptr_dtor(result); - } + i_zval_ptr_dtor(result); /* Set it to NULL in case that the extension will throw an out-of-memory error. * Otherwise the shutdown sequence will try to free this again. */ ZVAL_NULL(result);