Skip to content

Conversation

@jhdxr
Copy link
Member

@jhdxr jhdxr commented Dec 19, 2018

9cf87aa brings zend_empty_array, whose gc.refcount is 2, when unserialize a string representing empty array ("a:0:{}"), and leads to HT_ASSERT_RC1 failure.

to be honest I don't think it's a good fix, since spl_array_get_hash_table is very common used in ArrayObject, and SEPARATE_ARRAY will check gc_refcount everytime, the overhead may defeat the benfit of using zend_empty_array. Another possible solution is we can do SEPARATE_ARRAY in ArrayObject::unserialize, which defeats the purpose of zend_empty_array as well.

@nikic
Copy link
Member

nikic commented Dec 19, 2018

As you said, it would be better to handle this during unserialize. I would use the following pattern:

zval_ptr_dtor(&intern->array);
ZVAL_COPY_VALUE(&intern->array, array);
ZVAL_NULL(array);
SEPARATE_ARRAY(&intern->array);

This will take ownership of the unserialized array, and prevent anyone else from acquiring a reference to it through malicious unserialize input using r/R.

@jhdxr
Copy link
Member Author

jhdxr commented Dec 19, 2018

@nikic why ZVAL_NULL(array); ? my understanding of zend_empty_array is everyone are sharing a immutable empty array.

@nikic
Copy link
Member

nikic commented Dec 19, 2018

@jhdxr Using ZVAL_COPY + SEPARATE_ARRAY would work, but would always lead to an array copy, because the refcount will always be > 1 after the COPY. Using COPY_VALUE + NULLing the original value transfers ownership to you, so if nobody else was using it, no copy will have to be performed.

@jhdxr
Copy link
Member Author

jhdxr commented Dec 19, 2018

@nikic my point is the original value in this case is zend_empty_array, which is shared and should be used by others when needed. ZVAL_NULL will modify the shared instance.

is it a good idea to do ZVAL_COPY for zend_empty_array, and ZVAL_COPY_VALUE+ZVAL_NULL for other cases?

@nikic
Copy link
Member

nikic commented Dec 19, 2018

@jhdxr ZVAL_NULL will not modify the array, it will only modify the place storing the array (a slot in the serialization backreference table).

@jhdxr
Copy link
Member Author

jhdxr commented Dec 19, 2018

@nikic got it. thanks!

@petk petk added the Bug label Dec 19, 2018
@cmb69
Copy link
Member

cmb69 commented Dec 21, 2018

Thanks! Applied as b15189f.

@cmb69 cmb69 closed this Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants