Repository navigation
Conversation
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
I don't like the Fiber part. This could break IO hooks, as any IO there may suspend. Could we make the context fiber-local instead and keep the existing nesting behavior?
php_var_unserialize_init() joined the context of any parse in progress, so user code that runs mid-parse and calls unserialize() resolved back-references into the outer value: a destructor run by the garbage collector, an error handler for a parser warning, an allowed_classes __toString(), a session_decode() overwrite. Only the unserialize handler of a C: payload is meant to share it. object_custom() now publishes the context for the duration of that call, a nested init shares it only while serialize_lock is not held, as serialize() does, and a parse hides it for its own duration. The published context is kept per fiber, so a fiber switch inside the handler neither exposes it to another fiber nor loses it, and a bailout restores the previous one. A nested init under serialize_lock, as in a C: payload unserialized from __wakeup() or __unserialize(), now keeps the allowed_classes and max_depth of the payload. Fixes phpGH-24674
f151074 to
8fbf61a
Compare
|
Reworked: dropped |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks better but I think it needs also Fiber destroy observer otherwise it might leave the destroyed fiber in BG(unserialize).parked with an old fiber context which could be then reused by later Fiber.
php_var_unserialize_init() hands the context of a parse in progress to any caller, so user code that runs mid-parse and calls unserialize() (a GC destructor, an error handler, an allowed_classes __toString(), a session_decode() overwrite) resolves R: and r: into the outer value. The context is now shared only with the unserialize handler of a C: payload, only while serialize_lock is not held, as serialize() does, and only within the fiber that published it. This targets master because it narrows which nested unserialize() calls see back-references. #24433 fixed the failed __unserialize() destructor case from the report.
Fixes #24674