Fix GH-10519: Array Data Address Reference Issue #10749
Conversation
NathanFreeman
changed the title
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
This looks like it fixes the bug. But I do not understand this magic to be honest, so could you answer my couple of questions?
| } 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.
Why is the comment removed? The duplication still happens.
There was a problem hiding this comment.
Sorry, I deleted it carelessly.
ext/spl/spl_array.c
Outdated
| 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.
Move the definition to where it's first use please.
ext/spl/spl_array.c
Outdated
| 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; | ||
| } /* }}} */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ext/spl/spl_array.c
Outdated
| 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.
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?
There was a problem hiding this comment.
These things happened when creating this object:
- Setting
new_intern->is_child = trueindicating that this child object was created byRecursiveArrayIterator::getChildren()method. - After copying the array by
zend_array_dup, I need to share the newarraybetween the parent and child object. - So, I need this
Bucketof parent object. In this way, I can directly modify this valueintern->bucket->val = intern->array, so that changes between child object and parent object can affect each other.
There was a problem hiding this comment.
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.
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Okay with the explanations now and the comments, I get what's going on!
Thanks for tackling this!
|
Thank you. |
3 participants
No description provided.