Repository navigation
Fix GH-17935: Separate ArrayObject and ArrayIterator storage on write - #25432
Open
nicolas-grekas wants to merge 1 commit into
Open
nicolas-grekas wants to merge 1 commit into
nicolas-grekas wants to merge 1 commit into
Conversation
ArrayObject and ArrayIterator assumed they were the only owner of their storage, but __serialize(), __debugInfo(), var_export(), json_encode() and the sort methods hand it out or borrow it without copying it. Writing to the object then modified the array that was handed out, and failed the refcount assertion of zend_hash.c in debug builds. The storage is now separated before any write. When this happens while it is iterated, iterators move to the copy of their position that zend_array_dup() registers on the new table. When the storage is replaced, they keep their position and get registered on the new table. An error handler can now separate the storage by writing to the object, so the table is fetched after the code that can call one: get_hash_key() on read paths, and argument parsing in ArrayIterator::seek(). This also fixes use-after-frees when a handler calls exchangeArray() there, or while an element is being created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from #23945.
ArrayObject and ArrayIterator assume that they're the only owner of their storage. But
__serialize(),__debugInfo(),var_export(),json_encode()and the sort methods return or borrow this storage without a copy. A write to the object then also changes the array that was returned, and debug builds fail the refcount assertion of zend_hash.c. That's GH-17935.This PR separates the storage before any write. When that happens during an iteration, the iterator moves to the copy of its position that
zend_array_dup()registers on the new table. Object-backed storage isn't changed.An error handler can now separate the storage by writing to the object. So read paths fetch the table after the code that can call a handler. This also fixes use-after-frees when a handler calls
exchangeArray()during a read with a null offset, or while an element is created.This is the delayed separation discussed in #17942. It overlaps with #20725, which separates at three write sites, and it fixes the array-backed cases of #22921.
foreachover an ArrayObject needs about 3% more instructions (callgrind, 101 elements)./cc @ndossche @iluuu1994 @alexandre-daubois @iliaal