Skip to content

Commit

Permalink
Revert "Fixed bug #75961 (Strange references behavior)"
Browse files Browse the repository at this point in the history
This reverts commit 94e9d0a.

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.
  • Loading branch information
nikic committed Mar 5, 2018
1 parent 27a603e commit fd5bd37
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 37 deletions.
30 changes: 11 additions & 19 deletions ext/standard/array.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);

Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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;
}
Expand Down
18 changes: 0 additions & 18 deletions ext/standard/tests/array/bug75961.phpt

This file was deleted.

0 comments on commit fd5bd37

Please sign in to comment.