Skip to content

Fix GH-10519: Array Data Address Reference Issue #10749

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.

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
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,7 @@ PHP NEWS
non-existing key). (Nikita)
. Fixed bug #80724 (FilesystemIterator::FOLLOW_SYMLINKS remove KEY_AS_FILE
from bitmask). (Cameron Porter)
. Fixed bug GH-10519 (Array Data Address Reference Issue). (Nathan Freeman)

- Standard:
. Fixed bug #81441 (gethostbyaddr('::1') returns ip instead of name after
Expand Down
51 changes: 48 additions & 3 deletions ext/spl/spl_array.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "php_spl.h"
#include "spl_array_arginfo.h"
#include "spl_functions.h"
#include "spl_engine.h"
#include "spl_iterators.h"
#include "spl_array.h"
#include "spl_exceptions.h"
Expand Down Expand Up @@ -63,6 +62,8 @@ 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 @@ -167,6 +168,8 @@ 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 @@ -477,11 +480,23 @@ static zval *spl_array_read_dimension(zend_object *object, zval *offset, int typ
return spl_array_read_dimension_ex(1, object, offset, type, rv);
} /* }}} */

static uint32_t spl_array_set_refcount(spl_array_object *intern, HashTable *ht, uint32_t refcount) /* {{{ */
{
uint32_t old_refcount = 0;
if (intern->is_child) {
old_refcount = GC_REFCOUNT(ht);
GC_SET_REFCOUNT(ht, refcount);
}

return old_refcount;
} /* }}} */
Copy link
Member

@Girgias Girgias Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really confused by what's the point of this function is, staring at the usage.

Is this to detect if an SPLArray is a child and is refcounted?

Copy link
Contributor Author

@NathanFreeman NathanFreeman Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZVAL_ARR(&intern->array, zend_array_dup(Z_ARR_P(array)));
if (intern->is_child) {
    Z_TRY_DELREF_P(&intern->bucket->val);
	intern->bucket->val = intern->array;
	Z_TRY_ADDREF_P(&intern->array);
}

Because I increased the refcount + 1 manually here and the assertion HT_ASSERT_RC1(ht) will failed.

static zend_always_inline zval *_zend_hash_index_add_or_update_i(HashTable *ht, zend_ulong h, zval *pData, uint32_t flag)
{
    ........
	IS_CONSISTENT(ht);
	HT_ASSERT_RC1(ht);
    ........

#define HT_ASSERT_RC1(ht) HT_ASSERT(ht, GC_REFCOUNT(ht) == 1)

So I have to set the refcount to 1 when intern->is_child is true to make assertion success and restore the refcount to the original value after modifying the array.


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);
HashTable *ht;
spl_hash_key key;
uint32_t refcount = 0;

if (check_inherited && intern->fptr_offset_set) {
zval tmp;
Expand All @@ -502,7 +517,12 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
Z_TRY_ADDREF_P(value);
if (!offset || Z_TYPE_P(offset) == IS_NULL) {
ht = spl_array_get_hash_table(intern);
refcount = spl_array_set_refcount(intern, ht, 1);
zend_hash_next_index_insert(ht, value);

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

Expand All @@ -513,12 +533,17 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
}

ht = spl_array_get_hash_table(intern);
refcount = spl_array_set_refcount(intern, 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, ht, refcount);
}
} /* }}} */

static void spl_array_write_dimension(zend_object *object, zval *offset, zval *value) /* {{{ */
Expand All @@ -531,6 +556,7 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
HashTable *ht;
spl_array_object *intern = spl_array_from_obj(object);
spl_hash_key key;
uint32_t refcount = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the definition to where it's first use please.


if (check_inherited && intern->fptr_offset_del) {
zend_call_method_with_1_params(object, object->ce, &intern->fptr_offset_del, "offsetUnset", NULL, offset);
Expand All @@ -548,6 +574,7 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
}

ht = spl_array_get_hash_table(intern);
refcount = spl_array_set_refcount(intern, ht, 1);
if (key.key) {
zval *data = zend_hash_find(ht, key.key);
if (data) {
Expand All @@ -570,6 +597,10 @@ 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, ht, refcount);
}
} /* }}} */

static void spl_array_unset_dimension(zend_object *object, zval *offset) /* {{{ */
Expand Down Expand Up @@ -1056,8 +1087,12 @@ 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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the comment removed? The duplication still happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I deleted it carelessly.

if (intern->is_child) {
Z_TRY_DELREF_P(&intern->bucket->val);
intern->bucket->val = intern->array;
Z_TRY_ADDREF_P(&intern->array);
}
}
} else {
if (Z_OBJ_HT_P(array) == &spl_handler_ArrayObject || Z_OBJ_HT_P(array) == &spl_handler_ArrayIterator) {
Expand Down Expand Up @@ -1544,6 +1579,16 @@ 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);
new_intern->is_child = true;
new_intern->bucket = (Bucket *)((char*)(arg1) - XtOffsetOf(Bucket, val));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs a comment, because I have no idea what the hell is happening here.

You are doing some address arithmetic but I don't really get why you are using a Bucket and not just a zval for the spl_array_object struct, as it seems you're just using the zval part anyway?

Copy link
Contributor Author

@NathanFreeman NathanFreeman Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things happened when creating this object:

  1. Setting new_intern->is_child = true indicating that this child object was created by RecursiveArrayIterator::getChildren() method.
  2. After copying the array by zend_array_dup, I need to share the new array between the parent and child object.
  3. So, I need this Bucket of parent object. In this way, I can directly modify this value intern->bucket->val = intern->array, so that changes between child object and parent object can affect each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_intern->bucket = (Bucket *)((char*)(arg1) - XtOffsetOf(Bucket, val));

So I can get the bucket of the parent object by address arithmetic and replace bucket->val with copy array.

zend_call_known_instance_method_with_2_params(pce->constructor, Z_OBJ_P(retval), NULL, arg1, arg2);
}
/* }}} */

/* {{{ Create a sub iterator for the current element (same class as $this) */
PHP_METHOD(RecursiveArrayIterator, getChildren)
{
Expand Down Expand Up @@ -1574,7 +1619,7 @@ PHP_METHOD(RecursiveArrayIterator, getChildren)
}

ZVAL_LONG(&flags, intern->ar_flags);
spl_instantiate_arg_ex2(Z_OBJCE_P(ZEND_THIS), return_value, entry, &flags);
spl_instantiate_child_arg(Z_OBJCE_P(ZEND_THIS), return_value, entry, &flags);
}
/* }}} */

Expand Down
12 changes: 6 additions & 6 deletions ext/spl/tests/bug42654.phpt
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(4) "val2"
string(5) "alter"
[3]=>
array(1) {
[3]=>
string(4) "val3"
string(5) "alter"
}
}
[4]=>
Expand All @@ -129,11 +129,11 @@ object(RecursiveArrayIterator)#%d (1) {
[2]=>
array(2) {
[2]=>
string(4) "val2"
string(5) "alter"
[3]=>
array(1) {
[3]=>
string(4) "val3"
string(5) "alter"
}
}
[4]=>
Expand All @@ -146,11 +146,11 @@ array(3) {
[2]=>
array(2) {
[2]=>
string(4) "val2"
string(5) "alter"
[3]=>
array(1) {
[3]=>
string(4) "val3"
string(5) "alter"
}
}
[4]=>
Expand Down
33 changes: 33 additions & 0 deletions ext/spl/tests/bug42654_2.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
Bug #42654 (RecursiveIteratorIterator modifies only part of leaves)
--FILE--
<?php
$data = array ('key1' => 'val1', array('key2' => 'val2'), 'key3' => 'val3');

$iterator = new RecursiveIteratorIterator(new RecursiveArrayIterator($data));
foreach($iterator as $foo) {
$key = $iterator->key();
switch($key) {
case 'key1': // first level
case 'key2': // recursive level
echo "update $key".PHP_EOL;
$iterator->offsetSet($key, 'alter');
}
}
$copy = $iterator->getArrayCopy();
var_dump($copy);
?>
--EXPECT--
update key1
update key2
array(3) {
["key1"]=>
string(5) "alter"
[0]=>
array(1) {
["key2"]=>
string(5) "alter"
}
["key3"]=>
string(4) "val3"
}
69 changes: 69 additions & 0 deletions ext/spl/tests/gh10519.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
--TEST--
Bug GH-10519 (Array Data Address Reference Issue)
--FILE--
<?php
interface DataInterface extends JsonSerializable, RecursiveIterator, Iterator
{
public static function init(Traversable $data): DataInterface;
}

class A extends RecursiveArrayIterator implements DataInterface
{
public function __construct($data)
{
parent::__construct($data);
}

public static function init($data): DataInterface
{
return new static($data);
}

public function getCols(string $colname): array
{
return array_column($this->getArrayCopy(), $colname);
}

public function bugySetMethod($key, $value)
{
$data = &$this;

while ($data->hasChildren()) {
$data = $data->getChildren();
}
$data->offsetSet($key, $value);
}

public function jsonSerialize(): mixed
{
return $this;
}
}

$a = [
'test' => [
'a' => (object) [2 => '',3 => '',4 => ''],
]
];

$example = A::init($a);
$example->bugySetMethod(5, 'in here');
var_dump(json_encode($example));
var_dump(json_encode($a));

$b = [
'test' => [
'b' => [2 => '',3 => '',4 => ''],
]
];
$example = A::init($b);

$example->bugySetMethod(5, 'must be here');
var_dump(json_encode($example));
var_dump(json_encode($b));
?>
--EXPECT--
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":""}}}"