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 d7fa94e52ca3c3f3e71c66096a5aaaf07c076047..07db3754407da781044f6605ff72629379cd27f5 100644 (file)
--- 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 (file)
index 15484ab..0000000
--- 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)
-  }
-}