Fix add_function_array() separation

[iluuu1994]

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

[iluuu1994]

This doesn't solve the problem when no separation happens 🙁

$tmp = [0];
unset($tmp[0]);
$i = [$tmp, 0];
unset($tmp);
$ref = &$i;
$i[0] += $ref;
var_dump($i);

I don't know how to fix that without forcing a duplication...

[iluuu1994]

@dstogov Let me know if you have a better idea. I didn't see a way to prevent this without defensively copying the array.

[dstogov]

If Z_RECOUNT_P(op1) == 1 the duplication is completely useless.

[iluuu1994]

Unfortunately not, since the array might be behind a reference with RCN, we just don't know from this point.

[dstogov]

Unfortunately the fix introduces an unnecessary array duplication for a usual case $a1 += $a2;. I didn't find any better fix, so I approve this one.

I only propose to rearrange it a bit and ad more clear comments.

diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c
index 4263ae2a3b..45f02d9b97 100644
--- a/Zend/zend_operators.c
+++ b/Zend/zend_operators.c
@@ -1039,16 +1039,23 @@ static ZEND_COLD zend_never_inline void ZEND_FASTCALL zend_binop_error(const cha
 
 static zend_never_inline void ZEND_FASTCALL add_function_array(zval *result, zval *op1, zval *op2) /* {{{ */
 {
-	if (result == op1 && Z_ARR_P(op1) == Z_ARR_P(op2)) {
-		/* $a += $a */
-		return;
-	}
 	if (result != op1) {
 		ZVAL_ARR(result, zend_array_dup(Z_ARR_P(op1)));
+		zend_hash_merge(Z_ARRVAL_P(result), Z_ARRVAL_P(op2), zval_add_ref, 0);
+	} else if (Z_ARR_P(op1) == Z_ARR_P(op2)) {
+		/* $a += $a */
 	} else {
-		SEPARATE_ARRAY(result);
+		/* We have to duplicate op1 (even with refcount == 1), because it may be an element of op2,
+		 * and therefore its reference counter may be increced by zend_hash_merge().
+		 * that leads to the following assertion (see GH-10085 and Zend/tests/gh10085*.phpt)
+		 */
+		zval tmp;
+
+		ZVAL_ARR(&tmp, zend_array_dup(Z_ARR_P(op1)));
+		zend_hash_merge(Z_ARRVAL(tmp), Z_ARRVAL_P(op2), zval_add_ref, 0);
+		zval_ptr_dtor(result);
+		ZVAL_COPY_VALUE(result, &tmp);
 	}
-	zend_hash_merge(Z_ARRVAL_P(result), Z_ARRVAL_P(op2), zval_add_ref, 0);
 }
 /* }}} */
 

[iluuu1994]

Thank you for the review @dstogov!

[iluuu1994]

This change can cause issues when using += inside of foreach loops that depend on the array staying the same when no modifications are made. See #11171. I had to revert it in the meantime.

[dstogov]

#11171 occurs because the original HashTable is destroyed together with the corresponding iterator. So we continue the next iteration from start of the array. In general, we may try to update iterator (to transfer ownership to the copy of the HashTable), but I think this is wrong way.

[iluuu1994]

@dstogov I'll have a look at this again, but it's better to attempt only to fix this on master to avoid further issues.