-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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; | ||
} /* }}} */ | ||
|
||
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; | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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) /* {{{ */ | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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) { | ||
|
@@ -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) /* {{{ */ | ||
|
@@ -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))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the comment removed? The duplication still happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These things happened when creating this object:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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) | ||
{ | ||
|
@@ -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); | ||
} | ||
/* }}} */ | ||
|
||
|
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" | ||
} |
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":""}}}" |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I increased the
refcount
+ 1 manually here and the assertionHT_ASSERT_RC1(ht)
will failed.So I have to set the
refcount
to 1 whenintern->is_child
is true to make assertion success and restore therefcount
to the original value after modifying thearray
.