Le dim. 12 juil. 2026 à 00:13, Nicolas Grekas <[email protected]> a écrit :
> 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. > Following an off-list review + suggestion by Arnaud, I added a hash of closures's body to the identifier. This hash serves as a staleness check and replaces the line number. It's stored at compile-time in the AST, and compared when unserializing. The site@rank next to the hash remains so that polyfill/ext can be written to help transitioning. > > > >> > 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 >
