Repository navigation
Fix deepclone_to_array() on objects released during the walk - #58
Merged
Merged
Conversation
The pool of deepclone_to_array() is keyed by object handle but didn't keep the objects alive: the objects that __serialize(), __sleep() or Serializable::serialize() create and release got their handle reused by the next ones, which were then merged into the first, eg the dates of two DatePeriod. Pool entries now hold a reference to their object until the payload is built. They no longer store the class name, which keeps them at 48 bytes. Serializable::serialize() also sees the objects met before as back-references now, like with serialize(), as they aren't reference counted once anymore. __serialize() is called whatever its visibility, like serialize() does, and references that nothing else holds are exported as values, like serialize() and the polyfill do, instead of taking a reference id.
nicolas-grekas
force-pushed
the
fuzz-to-array
branch
from
September 29, 2026 18:32
05b07e2 to
290f606
Compare
nicolas-grekas
added a commit
to symfony/polyfill
that referenced
this pull request
Sep 29, 2026
…s-grekas) This PR was merged into the 1.x branch. Discussion ---------- [DeepClone] Fix issues found by differential fuzzing | Q | A | ------------- | --- | Branch? | 1.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Found by fuzzing the polyfill against the extension, which gets its side in symfony/php-ext-deepclone#58, #59, #60 and #61. - Named closures are created like `Closure::fromCallable()` and the extension do, with the same errors, over methods that `__call()` or `__callStatic()` handle too, instead of throwing `ReflectionException` or PHP errors. - `__serialize()`, `__sleep()`, `__unserialize()` and `__wakeup()` are called whatever their visibility, like `serialize()` and `unserialize()` do. - `deepclone_from_array()` rejects the malformed payloads the extension rejects, without warnings before: properties scoped to a class that isn't a parent of their object or that doesn't declare them, property names that start with a NUL byte, `null` resolve entries, references to themselves. - On PHP 8.5+, closures declared on the same line in distinct sites of a method, eg an attribute and a default value, resolved to the first one: they're refused, like same-line closures in one site already are. - Class names are cut at their first NUL byte in messages, like PHP does for anonymous classes. The checks run per scope and property, not per value: +0.35% instructions on `deepclone_from_array()` of 3300 objects, `deepclone_to_array()` unchanged. Commits ------- 9e22e62 [DeepClone] Fix issues found by differential fuzzing
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.
Found by the differential fuzzing of the polyfill against the extension.
The pool of
deepclone_to_array()is keyed by object handle, but didn't keep the objects alive: the ones that__serialize(),__sleep()orSerializable::serialize()create and release got their handle reused by the next ones, which were merged into the first:Two
DatePeriodin one graph also gave a payload that couldn't be decoded. Pool entries now hold a reference to their object until the payload is built. They don't store the class name anymore, which keeps them at 48 bytes: -0.4% to -0.6% instructions on graphs of 3000 to 5000 objects. As a side effect,Serializable::serialize()sees the objects met before as back-references, like withserialize().__serialize()is also called whatever its visibility, likeserialize()does, and references that nothing else holds, eg afterunset()of the other side, are exported as values, likeserialize()and the polyfill do, instead of taking a reference id.