[EdmondDantes]

Fix use-after-free when a Fiber callable resolves to the creating method's $this

Problem

Inside a method of a class, [A::class, 'm'] and "A::m" resolve to a call of m() on the current $this. Fiber::__construct() stores that object in fci.object and fci_cache.object but takes no reference to it. If the object dies before Fiber::start(), the fiber calls the method on freed memory.

<?php
class A {
    public $name = "a1";
    public function m() { echo "m on {$this->name}\n"; }
    public function make() { return new Fiber([A::class, 'm']); }
    public function __destruct() { echo "released\n"; }
}

$fiber = (new A)->make();   // nothing references the A after this line
$fiber->start();

PHP 8.3.6 and PHP-8.4 print released before m on a1: the destructor has run and the object is freed when m() executes. Valgrind reports invalid reads. A new object allocated between make() and start() can take the freed slot.

Cause

Fiber::__construct() (Zend/zend_fibers.c) keeps a reference only to the callable zval:

	fiber->fci = fci;
	fiber->fci_cache = fcc;

	// Keep a reference to closures or callable objects while the fiber is running.
	Z_TRY_ADDREF(fiber->fci.function_name);

For [A::class, 'm'] the zval holds a class name and a method name, not the object. The object comes from the calling frame's $this, so nothing keeps it alive.

On PHP-8.4 a fiber suspended inside m() hits the same bug: if the last reference to the object goes while the fiber is suspended, m() resumes on freed memory.

Fix

• Fiber::__construct() adds a reference to fci.object whenever it is set, for every kind of callable, so the release and the GC report need no condition.
• zend_fiber_release_callable() releases the callable zval and that object together, after the call returns or when an unstarted fiber is freed.
• zend_fiber_object_gc() reports the object, so a fiber stored in a property of the same object is a collectable cycle.

Not covered: a callable that resolves to __call is resolved again at start(), against the frame that calls start(). That is a separate issue.

Tests

Zend/tests/fibers/fiber-callable-object-outlives-caller.phpt covers five cases:

• the fiber starts after its creator's last reference went;
• the string form "A::m";
• a fiber that is never started, released with its object;
• a fiber suspended while its object has no other reference;
• a cycle through the fiber, which gc_collect_cycles() collects.

Verified on a --disable-all --enable-debug build of PHP-8.4:

• without the fix, the test fails and valgrind reports invalid reads;
• with the fix, all 90 tests in Zend/tests/fibers pass and the new test is clean under valgrind (run-tests.php -m).

The same change on master passes the 111 tests in Zend/tests/fibers.

[Girgias]

This looks correct, could just just extract the logic into some functions? As it would make rebasing #23994 easier.

[EdmondDantes]

This looks correct, could just just extract the logic into some functions? As it would make rebasing #23994 easier.

Done. Thank you

[Girgias]

Seems there is a conflict, mind rebasing?

[EdmondDantes]

Seems there is a conflict, mind rebasing?

@Girgias
I don't see a conflict with PHP-8.4: the branch merges into it cleanly, and GitHub reports it as mergeable. The only conflict I get is in NEWS when merging up into PHP-8.5 and master. Is that the one you mean, or something else?

[Girgias]

Seems there is a conflict, mind rebasing?

@Girgias I don't see a conflict with PHP-8.4: the branch merges into it cleanly, and GitHub reports it as mergeable. The only conflict I get is in NEWS when merging up into PHP-8.5 and master. Is that the one you mean, or something else?

GitHub was showing it as having a conflict and thus non-mergeable.... so I guess it was just drunk.