Skip to content

Fix GH-17935: Add delayed separation in ArrayObject #20725

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

Open
alexandre-daubois wants to merge 1 commit into
base: master
Choose a base branch
from
+39 −61
Open

Fix GH-17935: Add delayed separation in ArrayObject #20725

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
1 change: 1 addition & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ PHP NEWS
- SPL:
. DirectoryIterator key can now work better with filesystem supporting larger
directory indexing. (David Carlier)
. Fixed bug GH-17935 (Add delayed separation in ArrayObject). (alexandre-daubois)

- Sqlite3:
. Fix NUL byte truncation in sqlite3 TEXT column handling. (ndossche)
Expand Down
4 changes: 4 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ PHP 8.6 UPGRADE NOTES
. Invalid values now throw in Phar::mungServer() instead of being silently
ignored.

- SPL:
. RecursiveArrayIterator modifications now follow standard CoW semantics and
child modifications no longer affect the parent array.

========================================
2. New Features
========================================
Expand Down
67 changes: 14 additions & 53 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 @@ -466,21 +462,12 @@ static zval *spl_array_read_dimension(zend_object *object, zval *offset, int typ
} /* }}} */

/*
* 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.
* Before modifying the internal array, we must ensure exclusive ownership
* to satisfy HT_ASSERT_RC1 in Zend hash operations.
*
* SEPARATE_ARRAY implements CoW: if the array is shared (RC > 1),
* it creates a duplicate via zend_array_dup() and updates intern->array.
*/
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 @@ -505,18 +492,15 @@ 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) {
if (Z_TYPE(intern->array) == IS_ARRAY) {
SEPARATE_ARRAY(&intern->array);
}
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 @@ -526,22 +510,20 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
return;
}

if (Z_TYPE(intern->array) == IS_ARRAY) {
SEPARATE_ARRAY(&intern->array);
}
ht = spl_array_get_hash_table(intern);
if (UNEXPECTED(ht == intern->sentinel_array)) {
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 +552,10 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
return;
}

if (Z_TYPE(intern->array) == IS_ARRAY) {
SEPARATE_ARRAY(&intern->array);
}
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);
Expand All @@ -597,10 +581,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 @@ -970,17 +950,7 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
if (Z_REFCOUNT_P(array) == 1) {
ZVAL_COPY(&intern->array, array);
} 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 {
php_error_docref(NULL, E_DEPRECATED,
Expand Down Expand Up @@ -1850,15 +1820,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
3 changes: 2 additions & 1 deletion ext/spl/tests/ArrayObject/gh10519.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ $example->bugySetMethod(5, 'in here');
var_dump(json_encode($example));
var_dump(json_encode($a));

// child iterator modifications do not propagate to the parent iterator when working with arrays
$b = [
'test' => [
'b' => [2 => '',3 => '',4 => ''],
Expand All @@ -66,5 +67,5 @@ var_dump(json_encode($b));
Deprecated: ArrayIterator::__construct(): Using an object as a backing array for ArrayIterator is deprecated, as it allows violating class constraints and invariants in %s on line %d
string(51) "{"test":{"a":{"2":"","3":"","4":"","5":"in here"}}}"
string(51) "{"test":{"a":{"2":"","3":"","4":"","5":"in here"}}}"
string(56) "{"test":{"b":{"2":"","3":"","4":"","5":"must be here"}}}"
string(37) "{"test":{"b":{"2":"","3":"","4":""}}}"
string(37) "{"test":{"b":{"2":"","3":"","4":""}}}"
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
2 changes: 1 addition & 1 deletion ext/spl/tests/bug42654_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ array(3) {
[0]=>
array(1) {
["key2"]=>
string(5) "alter"
string(4) "val2"
}
["key3"]=>
string(4) "val3"
Expand Down
11 changes: 11 additions & 0 deletions ext/spl/tests/gh17935.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
--TEST--
GH-17935 (ArrayObject __serialize() should not cause assertion failure on modification)
--FILE--
<?php
$o = new ArrayObject();
$s2 = $o->__serialize();
$o['a'] = 'b';
echo "Success\n";
?>
--EXPECT--
Success