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.
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.
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.
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.
Best regards
Tim Düsterhus
