Fix GH-22060 and GH-22122: pin $this in zend_call_function

[iliaal]

Both reports are the same use-after-free: the callback ran with fci_cache->object as $this without holding a reference, so a callback that released the last reference to its own receiver (autoloader self-unregistering, SQLite3 authorizer calling setAuthorizer(null)) freed it mid-call. Pinning the receiver across the call in zend_call_function() covers both reports and every other site that dispatches through it. gh20352's expected output shifts with it: the output handler's destructor now fires after __invoke() returns rather than mid-deactivation.

[DanielEScherzer]

okay for ext/reflection, don't know about the other parts

[Girgias]

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.

[iliaal]

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.

[iluuu1994]

However, we are once again fixing bugs and affecting performance for idiosyncratic code.

Such things will come up more and more with the raise of AI.

A while ago when fixing a similar issue, @TimWolla asked me if zend_call_function() should just take care of at least making sure $this survives. I think that's reasonable, and something that could be changed on master. In that case, we should remove immediate add/delref around zend_call_function() calls throughout the codebase.

This should target master anyway (we agreed to not fixing such bugs on stable branches that are unlikely to cause issues in the real world). So maybe let's just go with the above instead.

[iliaal]

The central pin doesn't work out. Output handlers dispatch straight through zend_call_function(), so pinning the receiver there defers the handler's destructor out of php_output_deactivate() and regresses gh20352 (it leaks when the destructor re-enters ob_start()). I moved the pin into zend_call_known_fcc() instead. The autoloader and the sqlite3/pdo_sqlite authorizers all route through it, so both reports stay covered, and output handlers (which call it directly) are untouched.

[iluuu1994]

GH-20352 is a fuzzer finding, so it doesn't really matter whether its behavior changes. The behavior actually seems correct now, the output handler won't be freed until we exit its __invoke() call. I tried to run your commit (502eb4b) and don't see a leak for this test, only a change in output. Destructor timing is also easy to mess up via cycles. Are there any other problematic cases?

[iliaal]

Checked the other zend_call_function callers as well (spl, sqlite3, pdo_sqlite, dom, reflection, pcre, phar, mbstring) and nothing else changes, so the central pin covers it.
The leak is actually wrong, something from output bit, that got cleaned out, so not relevant, hence no test, it is a non-issue.

[TimWolla]

@iliaal Does adjusting zend_call_function() work or not work after all? The PR still adjusts zend_call_known_fcc(), but your last comment refers to zend_call_function().

[TimWolla]

LGTM now, but please get someone to double-check.

[iliaal]

It works, moved the pin to zend_call_function(). The gh20352 output-handler timing was the only concern and iluuu confirmed it's correct (fuzzer finding, no leak); updated that test's expected output to match.

[iluuu1994]

Looks correct, but would be nice if tests ran, especially for the valgrind benchmark.

[iliaal]

@iluuu1994 tests run, silly NEWS conflict blocked them before :/