Skip to content

Fiber: hold the object a callable resolved to in the creating frame - #24134

Open
EdmondDantes wants to merge 3 commits into
php:PHP-8.4from
true-async:fiber-callable-object-8.4
Open

EdmondDantes wants to merge 3 commits into
php:PHP-8.4from
true-async:fiber-callable-object-8.4

Conversation

@EdmondDantes

Copy link
Copy Markdown

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.

Inside a method of A, new Fiber([A::class, 'm']) or new Fiber("A::m")
resolves the callable to the method's $this. Fiber::__construct() kept
that object in fci.object and fci_cache.object without a reference, so
once the A died before start(), the fiber called A::m() on freed memory.

The fiber now holds fci.object for every callable, releases it with the
callable (after the call, or when the fiber is freed unstarted) and
reports it to the GC, so a fiber stored in a property of that object is
a collectable cycle.

A __call trampoline is not covered: it is resolved again at start(),
against the frame that calls start().

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Zend/zend_fibers.c Outdated
Comment thread Zend/zend_fibers.c Outdated
@EdmondDantes

Copy link
Copy Markdown
Author

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

Done. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants