Skip to content

Fix deepclone_to_array() on objects released during the walk - #58

Merged
nicolas-grekas merged 1 commit into
mainfrom
fuzz-to-array
Sep 29, 2026
Merged

nicolas-grekas merged 1 commit into
mainfrom
fuzz-to-array

Conversation

@nicolas-grekas

Copy link
Copy Markdown
Member

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() or Serializable::serialize() create and release got their handle reused by the next ones, which were merged into the first:

class Fresh {
    function __construct(public $n) {}
    function __serialize(): array { return ['o' => (object) ['n' => $this->n]]; }
    function __unserialize(array $d): void { $this->n = $d['o']->n; }
}
deepclone_from_array(deepclone_to_array([new Fresh(1), new Fresh(2)])); // both got n=1

Two DatePeriod in 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 with serialize().

__serialize() is also called whatever its visibility, like serialize() does, and references that nothing else holds, eg after unset() of the other side, are exported as values, like serialize() and the polyfill do, instead of taking a reference id.

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
nicolas-grekas merged commit ef84150 into main Sep 29, 2026
24 checks passed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant