Skip to content

[DeepClone] Support closures declared in constant expressions#630

Merged
nicolas-grekas merged 1 commit into
1.xfrom
deepclone-constexpr-closures
Jun 10, 2026
Merged

[DeepClone] Support closures declared in constant expressions#630
nicolas-grekas merged 1 commit into
1.xfrom
deepclone-constexpr-closures

Conversation

@nicolas-grekas

@nicolas-grekas nicolas-grekas commented Jun 10, 2026

Copy link
Copy Markdown
Member
Q A
Branch? 1.x
Bug fix? no
New feature? yes
Deprecations? no
Issues symfony/symfony#63228
License MIT

PHP 8.5 allows anonymous closures in constant expressions: attribute arguments, class constants, property and parameter defaults, property hooks. Frameworks now meet them when reading attributes (e.g. #[Assert\When(static function () { ... })]) and can no longer cache the resulting metadata, since closures refuse serialize() and deepclone_to_array() alike.

Const-expr closures are compile-time checked to be static and capture-free, so they are fully described by their declaration site. deepclone_to_array() now encodes them under the new mask marker 1 as [class, site, attrIndex|null, closureIndex, startLine], where site is "" (the class), "NAME" (constant or enum case), "$name" (property), "name()" (method), "name()#N" (parameter) or "$name::get()" / "$name::set()#N" (property hooks). deepclone_from_array() re-evaluates the addressed constant expression through reflection, picks the Nth closure of a depth-first walk (arrays in order, objects through their array-cast properties) and verifies the declaration line still matches, so payloads that outlived a code change fail loudly (stale payload) instead of resolving to a moved closure.

  • Identification. The extension matches literals by op_array identity; userland cannot, so the polyfill identifies them by closure name, file, line span and signature over a per-class index of every const-expr site. The closure name encodes nesting and method context, and a token-level count of closure literals per source line guards the remaining aliasing case: a closure created at runtime never silently resolves to a const-expr literal, declaration sites the polyfill cannot tell apart are refused. The same literal reached through several surfaces (an attribute argument referencing a class constant, a trait alias, a promoted default applied by a constructor evaluated elsewhere) resolves to the first site that exposes it; promoted properties are addressed through their constructor parameter, the canonical surface.
  • Source-less deployments. The aliasing guard needs to read the declaring file for method- and hook-scoped closures. When the file is not there (deployments shipping only precompiled opcodes), the refusal says so explicitly and points to the extension, which matches literals by op_array identity and needs no sources.
  • Security. The payload carries no code, only names and indices; the resolved code is whatever the loaded class declares. allowed_classes gates both directions: Closure must be allowed before any constant expression is evaluated on the to_array side, and the payload-named class must be allow-listed before it is autoloaded on the from_array side, so unserialization cannot drive autoloading or constructor execution for classes outside the allow-list.

Payloads are byte-identical and interchangeable with the extension's, covered by tests that run against both implementations (the few cases where op_array identity outperforms the userland heuristic branch per implementation). Closures created at runtime keep throwing NotInstantiableException.

Companion extension implementation: symfony/php-ext-deepclone#21

@nicolas-grekas nicolas-grekas force-pushed the deepclone-constexpr-closures branch from d87cc33 to a29cc58 Compare June 10, 2026 18:08
nicolas-grekas added a commit that referenced this pull request Jun 10, 2026
… instances on PHP 8.2 (nicolas-grekas)

This PR was merged into the 1.x branch.

Discussion
----------

[DeepClone] Fix round-tripping short-lived Random\Randomizer instances on PHP 8.2

| Q             | A
| ------------- | ---
| Branch?       | 1.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

This is what has been segfaulting the PHP 8.2 CI jobs since the Randomizer round-trip tests landed in #629 (and what #630 inherits).

Minimal reproducer, no polyfill involved, on PHP 8.2.31:

```php
$state = (new Random\Randomizer(new Random\Engine\Mt19937(7)))->__serialize();
serialize($state); // SIGSEGV
```

Before PHP 8.3, `Random\Randomizer::__serialize()` returns the object's raw property table with only the table itself addref'd. Its `engine` entry is an `IS_INDIRECT` slot pointing into the object, so once the Randomizer is released the retained state dangles. php-src fixed this for 8.3 by duplicating the table (php/php-src@c5fa7696e64), but 8.2 was already security-only and the fix was never backported; 8.2.31 still crashes.

`deepclone_to_array()` retains exactly such a state in its payload, so round-tripping a Randomizer that does not outlive the payload segfaulted on 8.2. There is no copy-based userland fix: plain array reads copy `IS_INDIRECT` slots verbatim instead of resolving them (`gettype()` reports `unknown type`), which is also how the contamination is detected. The serializer is the one userland-reachable code path that resolves these slots, so when a `__serialize()` result is contaminated (checked while the source object is still alive, and only on PHP < 8.3), it is passed through a `serialize()`/`unserialize()` round trip to materialize real values.

No new tests: the #629 Randomizer tests are the regression tests; they segfault on PHP 8.2 before this change and pass after (suite verified green on 8.1, 8.2, 8.3 and 8.5). The nested engine becomes a fresh copy rather than a shared instance on the affected versions, matching the already-documented identity behavior of nested `__serialize()` states.

Companion extension fix (the with-extensions 8.2 CI legs run the tagged ext via PIE and stay red until it ships in a release): symfony/php-ext-deepclone#23

Perf note: `Random\Randomizer` is special-cased instead of scanned for, after auditing PHP 8.2 core for the pattern: of the internal classes whose `__serialize()` retains the un-duplicated property table (Randomizer, `HashContext`, the Random engines), only Randomizer declares a property, and only declared properties create `IS_INDIRECT` slots (GMP uses the legacy serialize handler, consumed while the object is alive; the date classes already duplicate). Measured on 8.2: no overhead for any other class; Randomizer itself pays one serialize round trip (~190 us for a Mt19937 state), the price of not crashing. On PHP 8.3+ the version guard makes the branch dead code. The trade-off versus scanning every internal state: a hypothetical third-party extension class combining the same pattern with declared properties would keep crashing, as it does today.

Companion extension fix (the with-extensions 8.2 CI legs run the tagged ext via PIE, fixed in v0.7.2): symfony/php-ext-deepclone#23

Commits
-------

ae1778e [DeepClone] Fix round-tripping short-lived Random\Randomizer instances on PHP 8.2
@nicolas-grekas nicolas-grekas force-pushed the deepclone-constexpr-closures branch 2 times, most recently from abcd87d to c34ea1b Compare June 10, 2026 20:05
PHP 8.5 allows anonymous closures in constant expressions: attribute
arguments, class constants, property and parameter defaults, property
hooks. They are compile-time checked to be static and capture-free, so
they are fully described by their declaration site. deepclone_to_array()
now encodes them under the new mask marker 1 as [class, site,
attrIndex|null, closureIndex, startLine], where site is "" (the class),
"NAME" (constant or enum case), "$name" (property), "name()" (method),
"name()#N" (parameter) or "$name::get()" / "$name::set()#N" (property
hooks). deepclone_from_array() re-evaluates the addressed constant
expression through reflection, picks the Nth closure of a depth-first
walk (arrays in order, objects through their array-cast properties) and
verifies the declaration line still matches, so payloads that outlived
a code change fail loudly ("stale payload") instead of resolving to a
moved closure.

Unlike the extension, which matches literals by op_array identity, the
polyfill identifies them by closure name, file, line span and signature
over a per-class index of every const-expr site. The closure name
encodes nesting and method context, and a token-level count of closure
literals per source line guards the remaining aliasing case, so a
closure created at runtime never silently resolves to a const-expr
literal: declaration sites the polyfill cannot tell apart are refused.
When that count requires reading a file that is not there (deployments
shipping only precompiled opcodes), the refusal says so and points to
the extension, which needs no sources. The same literal reached through
several surfaces (an attribute argument referencing a class constant, a
trait alias, a promoted default applied by a constructor) resolves to
the first site that exposes it; promoted properties are addressed
through their constructor parameter.

The payload carries no code, only names and indices. allowed_classes
gates both directions: "Closure" must be allowed before any constant
expression is evaluated on the to_array side, and the payload-named
class must be allow-listed before it is autoloaded on the from_array
side. Closures created at runtime keep throwing
NotInstantiableException.
@nicolas-grekas nicolas-grekas force-pushed the deepclone-constexpr-closures branch from c34ea1b to e08201b Compare June 10, 2026 20:07
@nicolas-grekas nicolas-grekas merged commit eb0d984 into 1.x Jun 10, 2026
40 checks passed
@nicolas-grekas nicolas-grekas deleted the deepclone-constexpr-closures branch June 10, 2026 20:19
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