Thanks Tim, replying to both your messages in one go:
Le mer. 8 juil. 2026 à 21:28, Tim Düsterhus <[email protected]> a écrit : > Hi > > I have given the RFC another read before reading your email and the > replies I'm giving below might possibly reflect that. > > On 2026-07-04 10:47, Nicolas Grekas wrote: > >> As to the RFC itself: I think it might be useful to split this into > >> two > >> RFCs, similarly to how Volker and I split the initial support for > >> Closures in const-expr into support for Closures and support for first > >> class callables. > >> > > > > The 8.5 split worked because closures and FCCs were separable features. > > Here it's one mechanism, the FCC half alone or the anonymous-closure > > one > > alone would be just missing its other half. > > I'd keep it as one RFC. > > I made that suggestion, because I believe that building a robust > solution for first class callables is much easier than for “full > Closures”. > > While reading the RFC initially and now the updated version, I got the > feeling that it was “overfitted” to solve the specific use case and > deployment scenario that you consider a “best practice”, which I feel > results in “weird” behavior when one leaves that happy path. The updated > version is already better, particularly around first class callables, > but I can't say that I'm *happy* with it. > > >> I don't think it is a problem to make `unserialize()` a Closure > >> factory, > >> because the created Closure is “inert”. Contrary to arbitrary object > >> unserialization (which will immediately call the deserialization hooks > >> and then later __destruct()), the Closure will not actually do > >> anything > >> unless it is called. > >> > >> If folks are able to unserialize arbitrary payloads - which is > >> documented to be unsafe - they already have capabilities that are much > >> more powerful than “creating Closures”. > >> > > > > Creation is inert, but the point of these payloads is to be called. > > Once > > it's called, the only thing that matters is which callables a payload > > can > > name. > > Yes, but this requires application code to already be set up to try to > perform a function call on unserialized data, which is not a typical > situation. The typical “unserialization to RCE” vector looks something > like this: > > $sessionData = unserialize($_COOKIE['session']); > > where the cookie contains a serialized payload. This looks totally > reasonable unless you know that `unserialize()` will already execute > arbitrary code by itself. For Closures in the payload to be exploitable > you need extra code that is much less likely to be written “by accident” > in situations where untrusted data ends up in unserialize. > > And then the attacker also needs to control the inputs to the Closure in > question for this to actually be exploitable. Even if a Closure created > from unserialized data is called, the code performing the call expects a > function matching a specific signature, that is unlikely to be matched > by “exploitable” functionality. Calling `system()` without any arguments > will just fail. And the same is true if `system()` ends up in a place > where the caller passes an object as the first parameter. > > > So I'd keep the declared-set boundary as required defense-in-depth / > > hardening. > > That said, I think I'd be okay with keeping this defense-in-depth for > now, but I think I would then want a serialization format that allows > for a clean “forward compatibility” in case we want to relax this later. > Specifically I think we should be careful with “reserving” this many > top-level keys with specific names in the serialization payload. As an > example, the `class` key makes adding support for the future-scope > “free-function attributes” unnecessarily complicated, which is part of > what I meant by “overfitted to the use-case” above. > Happy we converged on this defense-in-depth aspect, I wouldn't be comfortable with an unbounded allowance for closures. An idea that I have not given too much thought would be making the > serialization payload a “tagged union” with something like > `[get_mangled_object_vars($this), ["const-expr", ['Order', > 'billingAddress@0']]]`. This could then be extended to a > `[get_mangled_object_vars($this), ["fcc", ["strrev"]]]` or similar. > Perhaps seek inspiration from the serialization format for ext/uri or > ext/random, which have explicitly been designed to be able to be > extended in a backwards compatible fashion and to avoid naming clashes > between actual userland properties and internal state. > Thanks for the idea, I made it to the RFC + implementation. > > Agreed: first-class callables now serialize with the function name as > > identifier, no ordinal involved. The id is `member@callable`, e.g. > > `$billingAddress@Order::isStrict` for `#[When(self::isStrict(...))]` on > > that property, `$p@strlen` for a plain function. The member prefix > > keeps > > resolution local to one reflection element instead of scanning the > > whole > > class on every cache read (fat classes would pay otherwise), and it > > gives > > both closure forms the same staleness rule: a reference is valid while > > its > > member and its declaration survive. Adding, removing or reordering > > anything > > else in the class changes nothing; renaming the target method fails. > > I wonder if using the function name would also work for regular > Closures. I've updated the names for PHP 8.4 to include the name of the > declaring scope and line number, which is exactly the information the > RFC is already using as a guard as of now. > > That said: I understand that accurately identifying “full Closures” is > complicated, but I also feel that the line number guard for proper > Closures is making serialization payloads fragile across deployments. > Adding a single “use” import at the top of the file would break all > unserialization - which again feels like the overfitting. I don't have a > good suggestion here, which is why I suggested to solve the first class > callable and “full Closure” cases separately. > I tweaked the line number to make it relative to the class, so that adding a `use` doesn't invalidate payloads. What we're building here is an addressing scheme and it's part of the consensus we need to have in this RFC. The one proposed here is quite efficient to implement and effective to achieve the goal. Also, no need for something as robust as a named identifier, it's really fine that these payloads stay valid only for the code revision they were generated with. > > One catch: the idiomatic form references a private helper of the same > > class, #[When(self::isStrict(...))], and > > Closure::fromCallable('C::priv') > > from global scope throws "cannot access private method". So resolution > > doesn't resolve the name directly: it checks if the named member > > declares > > that exact reference, then evaluates the declaration in class scope, > > name > > as address, declaration as guard. > > Yes, unserialization ignores or “trusts” visibility. That's just how it > works. > > >> As for the var_export() in the future scope: I think adding support > >> for > >> first class callables to `var_export()` would be a change that can > >> just > >> be done without an RFC. It might be a good first step that might > >> already > >> be helpful to your use case? > >> > > > > Not really helpful for the main use cases I gathered, which require > > full > > compat with serialize semantics, but yeah, I agree this can be dealt > > with > > on its own. > > Specifically with regard to `var_export()`, I'm also concerned about the > ReflectionFunction::getConstExprId(), > ReflectionFunction::getConstExprClass(), and Closure::fromConstExpr() > additions, which very much feel like “exposing an implementation > detail”. These functions will be part of the public API and > documentation, which means that users will come across them. I don't > currently see how we can meaningfully document them, since they serve > such a narrow use case based on very specific assumptions. see below Le mer. 8 juil. 2026 à 21:39, Tim Düsterhus <[email protected]> a écrit : > On 2026-07-04 11:02, Nicolas Grekas wrote: > > I'd also be fine with a limited version of this RFC that'd remove the > > serialize-related part and that'd keep only the proposed > > Reflection-based > > API. This is the very core where engine support is needed. The > > serialize > > part would make attributes work seamlessly with backends that use > > serialize(), but my use cases build on the deepclone/VarExporter > > extension/components, and those need only reflection. > > > > In case that can help bring a broader consensus. > > As indicated in my previous email, I'm also concerned about the > Reflection-based API and don't consider it good language design. Given > that the Reflection-based API needs Reflection and the specific > constraint of “the serialized payload is only valid until the next > deployment”, I also don't see how it would enable anything that you > can't already do. > > - For public first class callables, just generate a first class > callable. > - For private and protected ones generate `(new ReflectionMethod($class, > $method))->getClosure()`. > - For full Closures generate the the appropriate Reflection chain > accessing the right Closure, e.g. `(new ReflectionProperty($class, > $method))->getAttributes()[$attrNo]->getArguments()[$argumentNo]`. > What's missing from your analysis is the "provenance" metadata that reflection currently gives no access to. I even had to add a flag in the C struct to persist this. Nothing today distinguishes a closure that a class declared in a constant expression from one built at runtime, yet that is precisely the question a cache layer must answer before it may reference a closure at all. The userland deepclone extension and its pure-PHP polyfill are the existence proof: both need this and I had to instrument ''ReflectionAttribute'' by overloading the class in the ext, and the polyfill needs complex and fragile heuristic machinery. The RFC would make these go away. The proposed new API would effectively do the same, and then add the > extra safety checks that you, from what I understand, wouldn't need for > this use case > RFC updated. Cheers, Nicolas
