diff options
author | Nikita Popov <nikita.ppv@gmail.com> | 2018-03-05 15:28:58 +0100 |
---|---|---|
committer | Nikita Popov <nikita.ppv@gmail.com> | 2018-03-05 15:32:21 +0100 |
commit | fd5bd37ab129595d51cc05199437c8af3388b3b9 (patch) | |
tree | 0aa6b9a79b0297079ebe4348679d58af6118740a | |
parent | 27a603e8116b9efb36f55bb5c9327dc23183ab7c (diff) | |
download | php-git-fd5bd37ab129595d51cc05199437c8af3388b3b9.tar.gz |
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.
-rw-r--r-- | ext/standard/array.c | 30 | ||||
-rw-r--r-- | ext/standard/tests/array/bug75961.phpt | 18 |
2 files changed, 11 insertions, 37 deletions
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) - } -} |