Skip to content

Revert 49b2ff5dbb94b76b265fd5909881997e1d95c6b3 #21532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Girgias wants to merge 1 commit into from
+27 −96
Closed

Revert 49b2ff5dbb94b76b265fd5909881997e1d95c6b3 #21532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 2 additions & 57 deletions ext/spl/spl_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ typedef struct _spl_array_object {
uint32_t ht_iter;
int ar_flags;
unsigned char nApplyCount;
bool is_child;
Bucket *bucket;
zend_function *fptr_offset_get;
zend_function *fptr_offset_set;
zend_function *fptr_offset_has;
Expand Down Expand Up @@ -166,8 +164,6 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o
object_properties_init(&intern->std, class_type);

intern->ar_flags = 0;
intern->is_child = false;
intern->bucket = NULL;
intern->ce_get_iterator = spl_ce_ArrayIterator;
if (orig) {
spl_array_object *other = spl_array_from_obj(orig);
Expand Down Expand Up @@ -464,22 +460,6 @@ static zval *spl_array_read_dimension(zend_object *object, zval *offset, int typ
return spl_array_read_dimension_ex(1, object, offset, type, rv);
} /* }}} */

/*
* The assertion(HT_ASSERT_RC1(ht)) failed because the refcount was increased manually when intern->is_child is true.
* We have to set the refcount to 1 to make assertion success and restore the refcount to the original value after
* modifying the array when intern->is_child is true.
*/
static uint32_t spl_array_set_refcount(bool is_child, HashTable *ht, uint32_t refcount) /* {{{ */
{
uint32_t old_refcount = 0;
if (is_child) {
old_refcount = GC_REFCOUNT(ht);
GC_SET_REFCOUNT(ht, refcount);
}

return old_refcount;
} /* }}} */

static void spl_array_write_dimension_ex(int check_inherited, zend_object *object, zval *offset, zval *value) /* {{{ */
{
spl_array_object *intern = spl_array_from_obj(object);
Expand All @@ -503,19 +483,12 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
}

Z_TRY_ADDREF_P(value);

uint32_t refcount = 0;
if (!offset || Z_TYPE_P(offset) == IS_NULL) {
ht = spl_array_get_hash_table(intern);
if (UNEXPECTED(ht == intern->sentinel_array)) {
return;
}
refcount = spl_array_set_refcount(intern->is_child, ht, 1);
zend_hash_next_index_insert(ht, value);

if (refcount) {
spl_array_set_refcount(intern->is_child, ht, refcount);
}
return;
}

Expand All @@ -530,17 +503,13 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
spl_hash_key_release(&key);
return;
}
refcount = spl_array_set_refcount(intern->is_child, ht, 1);

if (key.key) {
zend_hash_update_ind(ht, key.key, value);
spl_hash_key_release(&key);
} else {
zend_hash_index_update(ht, key.h, value);
}

if (refcount) {
spl_array_set_refcount(intern->is_child, ht, refcount);
}
} /* }}} */

static void spl_array_write_dimension(zend_object *object, zval *offset, zval *value) /* {{{ */
Expand Down Expand Up @@ -570,8 +539,6 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
}

ht = spl_array_get_hash_table(intern);
uint32_t refcount = spl_array_set_refcount(intern->is_child, ht, 1);

if (key.key) {
zval *data = zend_hash_find(ht, key.key);
if (data) {
Expand All @@ -596,10 +563,6 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
} else {
zend_hash_index_del(ht, key.h);
}

if (refcount) {
spl_array_set_refcount(intern->is_child, ht, refcount);
}
} /* }}} */

static void spl_array_unset_dimension(zend_object *object, zval *offset) /* {{{ */
Expand Down Expand Up @@ -973,15 +936,6 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
} else {
//??? TODO: try to avoid array duplication
ZVAL_ARR(&intern->array, zend_array_dup(Z_ARR_P(array)));

if (intern->is_child) {
Z_TRY_DELREF(intern->bucket->val);
/*
* replace bucket->val with copied array, so the changes between
* parent and child object can affect each other.
*/
ZVAL_COPY(&intern->bucket->val, &intern->array);
}
}
} else {
if (Z_OBJ_HT_P(array) == &spl_handler_ArrayObject) {
Expand Down Expand Up @@ -1244,7 +1198,7 @@ static void spl_array_method(INTERNAL_FUNCTION_PARAMETERS, char *fname, size_t f
*ht_ptr = Z_ARRVAL_P(ht_zv);
ZVAL_NULL(ht_zv);
zval_ptr_dtor(&params[0]);
zend_string_free(Z_STR(function_name));
zval_ptr_dtor(&function_name);
}
} /* }}} */

Expand Down Expand Up @@ -1854,15 +1808,6 @@ PHP_METHOD(RecursiveArrayIterator, hasChildren)
static void spl_instantiate_child_arg(zend_class_entry *pce, zval *retval, zval *arg1, zval *arg2) /* {{{ */
{
object_init_ex(retval, pce);
spl_array_object *new_intern = Z_SPLARRAY_P(retval);
/*
* set new_intern->is_child is true to indicate that the object was created by
* RecursiveArrayIterator::getChildren() method.
*/
new_intern->is_child = true;

/* find the bucket of parent object. */
new_intern->bucket = (Bucket *)((char *)(arg1) - XtOffsetOf(Bucket, val));;
zend_call_known_instance_method_with_2_params(pce->constructor, Z_OBJ_P(retval), NULL, arg1, arg2);
}
/* }}} */
Expand Down
12 changes: 6 additions & 6 deletions ext/spl/tests/bug42654.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@ object(RecursiveArrayIterator)#%d (1) {
[2]=>
array(2) {
[2]=>
string(5) "alter"
string(4) "val2"
[3]=>
array(1) {
[3]=>
string(5) "alter"
string(4) "val3"
}
}
[4]=>
Expand All @@ -129,11 +129,11 @@ object(RecursiveArrayIterator)#%d (1) {
[2]=>
array(2) {
[2]=>
string(5) "alter"
string(4) "val2"
[3]=>
array(1) {
[3]=>
string(5) "alter"
string(4) "val3"
}
}
[4]=>
Expand All @@ -146,11 +146,11 @@ array(3) {
[2]=>
array(2) {
[2]=>
string(5) "alter"
string(4) "val2"
[3]=>
array(1) {
[3]=>
string(5) "alter"
string(4) "val3"
}
}
[4]=>
Expand Down
33 changes: 0 additions & 33 deletions ext/spl/tests/bug42654_2.phpt
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions ext/spl/tests/gh10519.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
--TEST--
Bug GH-10519 (Array Data Address Reference Issue)
--XFAIL--
The fix for this was bad
--FILE--
<?php
interface DataInterface extends JsonSerializable, RecursiveIterator, Iterator
Expand Down
17 changes: 17 additions & 0 deletions ext/spl/tests/gh21499.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
GH-21499: RecursiveArrayIterator getChildren UAF after parent free
--FILE--
<?php
$it = new RecursiveArrayIterator([[1]]);
$child = $it->getChildren();
unset($it);
$child->__construct([2, 3]);
var_dump($child->getArrayCopy());
?>
--EXPECT--
array(2) {
[0]=>
int(2)
[1]=>
int(3)
}
Loading