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.

[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 :/

[iluuu1994]

I'll benchmark locally, the diff is a bit bigger than I expected.

[iliaal]

Benchmarked it (callgrind, release build). The pin is a fixed ~15 instructions per zend_call_function call with $this bound, no GC-root churn. That's ~4% on a tight loop over a trivial method callback (array_map([$o,'m'], $arr) where m just returns), but under 1% once the callback does any real work, and below the wall-clock noise floor. call_user_func doesn't route through zend_call_function, so it's unaffected.

I also tried the cheaper version: drop the explicit add/release and set ZEND_CALL_RELEASE_THIS so the leave helper handles it. ~40% cheaper per call, but it doesn't hold up. The pure form leaks bound closures, since the leave helper treats RELEASE_THIS and CLOSURE as mutually exclusive. Carving closures out fixes the leak, but releasing $this from the leave helper instead of after the call shifts destructor timing: 41 gc/destructor/leak tests fail and bug71818 OOMs. The explicit release point, after fake_scope is restored and the call has unwound, is load-bearing, so I kept it.