From: Nikita Popov <nikita.ppv@gmail.com>
Date: Mon, 5 Mar 2018 14:28:58 +0000 (+0100)
Subject: Revert "Fixed bug #75961 (Strange references behavior)"
X-Git-Tag: php-7.2.4RC1~31^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=fd5bd37ab129595d51cc05199437c8af3388b3b9;p=php

Revert "Fixed bug #75961 (Strange references behavior)"

This reverts commit 94e9d0a2ae76bad712495d820d3962e401085fef.

This code needs to be mindful about modifications to the array
happening during callback execution. It was written in a way that
only accessed the reference, which is guaranteed not to move. The
changed implementation instead accesses the array slot, leading to
use-after-free.

Run ext/standard/tests/array/bug61967.phpt under valgrind to see
the issue.
---

diff --git a/ext/standard/array.c b/ext/standard/array.c
index d7fa94e52c..07db375440 100644
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -1380,7 +1380,6 @@ static int php_array_walk(zval *array, zval *userdata, int recursive) /* {{{ */
 
 	/* Iterate through hash */
 	do {
-		zend_bool was_ref;
 		/* Retrieve value */
 		zv = zend_hash_get_current_data_ex(target_hash, &pos);
 		if (zv == NULL) {
@@ -1396,8 +1395,6 @@ static int php_array_walk(zval *array, zval *userdata, int recursive) /* {{{ */
 			}
 		}
 
-		was_ref = Z_ISREF_P(zv);
-
 		/* Ensure the value is a reference. Otherwise the location of the value may be freed. */
 		ZVAL_MAKE_REF(zv);
 
@@ -1415,16 +1412,14 @@ static int php_array_walk(zval *array, zval *userdata, int recursive) /* {{{ */
 			HashTable *thash;
 			zend_fcall_info orig_array_walk_fci;
 			zend_fcall_info_cache orig_array_walk_fci_cache;
-			zval rv;
+			zval ref;
+			ZVAL_COPY_VALUE(&ref, zv);
 
-			SEPARATE_ARRAY(Z_REFVAL_P(zv));
-			ZVAL_COPY_VALUE(&rv, Z_REFVAL_P(zv));
-			thash = Z_ARRVAL(rv);
+			ZVAL_DEREF(zv);
+			SEPARATE_ARRAY(zv);
+			thash = Z_ARRVAL_P(zv);
 			if (thash->u.v.nApplyCount > 1) {
 				php_error_docref(NULL, E_WARNING, "recursion detected");
-				if (!was_ref) {
-					ZVAL_UNREF(zv);
-				}
 				result = FAILURE;
 				break;
 			}
@@ -1433,15 +1428,15 @@ static int php_array_walk(zval *array, zval *userdata, int recursive) /* {{{ */
 			orig_array_walk_fci = BG(array_walk_fci);
 			orig_array_walk_fci_cache = BG(array_walk_fci_cache);
 
-			Z_ADDREF(rv);
+			Z_ADDREF(ref);
 			thash->u.v.nApplyCount++;
-			result = php_array_walk(&rv, userdata, recursive);
-			if (Z_TYPE_P(Z_REFVAL_P(zv)) == IS_ARRAY && thash == Z_ARRVAL_P(Z_REFVAL_P(zv))) {
+			result = php_array_walk(zv, userdata, recursive);
+			if (Z_TYPE_P(Z_REFVAL(ref)) == IS_ARRAY && thash == Z_ARRVAL_P(Z_REFVAL(ref))) {
 				/* If the hashtable changed in the meantime, we'll "leak" this apply count
 				 * increment -- our reference to thash is no longer valid. */
 				thash->u.v.nApplyCount--;
 			}
-			zval_ptr_dtor(&rv);
+			zval_ptr_dtor(&ref);
 
 			/* restore the fcall info and cache */
 			BG(array_walk_fci) = orig_array_walk_fci;
@@ -1457,15 +1452,12 @@ static int php_array_walk(zval *array, zval *userdata, int recursive) /* {{{ */
 
 			zval_ptr_dtor(&args[0]);
 		}
+
 		if (Z_TYPE(args[1]) != IS_UNDEF) {
 			zval_ptr_dtor(&args[1]);
 			ZVAL_UNDEF(&args[1]);
 		}
-		if (!was_ref && Z_ISREF_P(zv)) {
-			if (Z_REFCOUNT_P(zv) == 1) {
-				ZVAL_UNREF(zv);
-			}
-		}
+
 		if (result == FAILURE) {
 			break;
 		}
diff --git a/ext/standard/tests/array/bug75961.phpt b/ext/standard/tests/array/bug75961.phpt
deleted file mode 100644
index 15484ab16f..0000000000
--- a/ext/standard/tests/array/bug75961.phpt
+++ /dev/null
@@ -1,18 +0,0 @@
---TEST--
-Bug #75961 (Strange references behavior)
---FILE--
-<?php
-$arr = [[1]];
-
-array_walk($arr, function(){});
-array_map('array_shift', $arr);
-var_dump($arr);
-?>
---EXPECT--
-array(1) {
-  [0]=>
-  array(1) {
-    [0]=>
-    int(1)
-  }
-}