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..5d6b89868fa2f 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); @@ -2095,6 +2110,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);