From 77b96e8e40776970aa9ec6059c907cdea1f46e17 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Wed, 29 Mar 2023 20:51:14 +0200 Subject: [PATCH 1/2] Fix add_function_array() separation result may be a slot in op2. In that case SEPARATE_ARRAY() will change both result and the slot in op2. Looping over op2 and inserting the element results in both reference-less recursion which we don't allow, and increasing the refcount to 2, failing any further insertions into the array. Avoid this by copying result into a temporary zval and performing separation there instead. Fixes GH-10085 --- Zend/tests/gh10085.phpt | 9 +++++++++ Zend/zend_operators.c | 9 ++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 Zend/tests/gh10085.phpt diff --git a/Zend/tests/gh10085.phpt b/Zend/tests/gh10085.phpt new file mode 100644 index 0000000000000..8c00b3e863334 --- /dev/null +++ b/Zend/tests/gh10085.phpt @@ -0,0 +1,9 @@ +--TEST-- +GH-10085: Assertion in add_function_array() +--FILE-- + +--EXPECT-- diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index ef5c50c4336d2..895ee1855ae8c 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -969,12 +969,15 @@ static zend_never_inline void ZEND_FASTCALL add_function_array(zval *result, zva /* $a += $a */ return; } + zval tmp; if (result != op1) { - ZVAL_ARR(result, zend_array_dup(Z_ARR_P(op1))); + ZVAL_ARR(&tmp, zend_array_dup(Z_ARR_P(op1))); } else { - SEPARATE_ARRAY(result); + ZVAL_COPY_VALUE(&tmp, result); + SEPARATE_ARRAY(&tmp); } - zend_hash_merge(Z_ARRVAL_P(result), Z_ARRVAL_P(op2), zval_add_ref, 0); + zend_hash_merge(Z_ARRVAL_P(&tmp), Z_ARRVAL_P(op2), zval_add_ref, 0); + ZVAL_COPY_VALUE(result, &tmp); } /* }}} */ From 9eae5f0ec9afa4bc1bd8f352376682dfa7781998 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Wed, 29 Mar 2023 22:12:42 +0200 Subject: [PATCH 2/2] Fix another case without separation --- Zend/tests/{gh10085.phpt => gh10085_1.phpt} | 13 +++++++++++ Zend/tests/gh10085_2.phpt | 25 +++++++++++++++++++++ Zend/zend_operators.c | 12 +++++----- 3 files changed, 44 insertions(+), 6 deletions(-) rename Zend/tests/{gh10085.phpt => gh10085_1.phpt} (50%) create mode 100644 Zend/tests/gh10085_2.phpt diff --git a/Zend/tests/gh10085.phpt b/Zend/tests/gh10085_1.phpt similarity index 50% rename from Zend/tests/gh10085.phpt rename to Zend/tests/gh10085_1.phpt index 8c00b3e863334..cc11c96a09d32 100644 --- a/Zend/tests/gh10085.phpt +++ b/Zend/tests/gh10085_1.phpt @@ -5,5 +5,18 @@ GH-10085: Assertion in add_function_array() $i = [[], 0]; $ref = &$i; $i[0] += $ref; +var_dump($i); ?> --EXPECT-- +array(2) { + [0]=> + array(2) { + [0]=> + array(0) { + } + [1]=> + int(0) + } + [1]=> + int(0) +} diff --git a/Zend/tests/gh10085_2.phpt b/Zend/tests/gh10085_2.phpt new file mode 100644 index 0000000000000..7895999f2cd05 --- /dev/null +++ b/Zend/tests/gh10085_2.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-10085: Assertion in add_function_array() +--FILE-- + +--EXPECT-- +array(2) { + [0]=> + array(2) { + [0]=> + array(0) { + } + [1]=> + int(0) + } + [1]=> + int(0) +} diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index 895ee1855ae8c..ad206f8dfe462 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -970,13 +970,13 @@ static zend_never_inline void ZEND_FASTCALL add_function_array(zval *result, zva return; } zval tmp; - if (result != op1) { - ZVAL_ARR(&tmp, zend_array_dup(Z_ARR_P(op1))); - } else { - ZVAL_COPY_VALUE(&tmp, result); - SEPARATE_ARRAY(&tmp); - } + ZVAL_ARR(&tmp, zend_array_dup(Z_ARR_P(op1))); zend_hash_merge(Z_ARRVAL_P(&tmp), Z_ARRVAL_P(op2), zval_add_ref, 0); + if (result == op1) { + /* When result == op1 the caller is assuming that reused when rc == 1. + * Since we're not doing that (GH-10085) we need to release it. */ + zval_ptr_dtor(result); + } ZVAL_COPY_VALUE(result, &tmp); } /* }}} */