Fix GH-22060 and GH-22122: pin object/closure in callback dispatch#22151
Fix GH-22060 and GH-22122: pin object/closure in callback dispatch#22151iliaal wants to merge 1 commit into
Conversation
Pin object and closure across zend_call_known_fcc and spl_perform_autoload so a callback that releases the borrowed FCC (autoloader self-unregister, SQLite3 setAuthorizer(null)) doesn't free $this mid-call. Initialize fcc.closure in ReflectionFunction::invoke/invokeArgs since the pin reads it. Fixes phpGH-22060 Fixes phpGH-22122
DanielEScherzer
left a comment
There was a problem hiding this comment.
okay for ext/reflection, don't know about the other parts
Girgias
left a comment
There was a problem hiding this comment.
I was trying to understand why we would need to add ref the fcc.object, and my understanding is that it represents the $this value, so I guess this is valid.
However, we are once again fixing bugs and affecting performance for idiosyncratic code. I would rather prefer a solution that bans unsetting such callables within the calls. Similar to a fix we recently did for an unserialising routine in SPL.
|
Pinning is two refcount ops against a full call-frame setup, so the per-call cost is negligible. Banning is the bigger change: the autoload loop is deliberately built to allow add/remove mid-iteration, and disabling the authorizer mid-call is a one-shot the sqlite3 test relies on. Turning either into a thrown error is a behavior change that can't go in a stable branch. If the stricter semantics are worth having, that's a master-only follow-up. |
GH-22060 + GH-22122 fix for PHP-8.4. Same UAF in two callback-dispatch sites: zend_call_known_fcc and spl_perform_autoload forward the borrowed object/closure into the call frame without addref. 8.4 and 8.5 both need the pair, since SPL autoload still uses zend_call_known_function direct. Master only needs the zend_API change because Zend/zend_autoload.c routes through zend_call_known_fcc.