Segfault when using ReflectionFiber #11121

Activity

1. danog

   added

   Bug

   Status: Needs Triage

   on Apr 23, 2023
2. iluuu1994 commented on Apr 26, 2023

   

   iluuu1994

   on Apr 26, 2023

   Member

   More actions

   Thank you! I can reproduce this. Interestingly, I also get errors in emalloc with ASAN. When disabling ZMM I can also reproduce the issue you've mentioned above. Unfortunately, the issue is hard to understand without a condensed example.
3. danog commented on Apr 26, 2023

   

   danog

   on Apr 26, 2023

   ContributorAuthor

   More actions

   Unfortunately, the issue is hard to understand without a condensed example.

   Precisely my problem as well, was hoping someone here could make more sense of it :)
4. ndossche commented on Apr 28, 2023

   

   ndossche

   on Apr 28, 2023

   Member

   More actions

   I tried to debug this for a bit now. Look like fiber->execute_data->func and fiber->execute_data->opline are pointing to garbage or uninitialised values when ZEND_METHOD(ReflectionFiber, getTrace) executes. Didn't really find a root cause for this yet.
5. bwoebi commented on May 1, 2023

   

   bwoebi

   on May 1, 2023

   Last edited by bwoebi

   Member

   More actions

   The root cause is the fiber->execute_data not being updated when a fiber is switched from due to resumption of another fiber.

   Essentially, it's pointing to memory in the middle of its VM stack.

   This non-crashing example, but having the wrong output illustrates the issue:
   Expected would be the top frame of the $f fiber to be Fiber->start(), but it's the position of the last suspension.

   root@bobtop-ubuntu:~/php-src-8.2# php -r 'function f() { Fiber::suspend(); } function g() { (new Fiber(function() { global $f; var_dump((new ReflectionFiber($f))->getTrace()); }))->start(); } $f = new Fiber(function() { f(); g(); }); $f->start(); $f->resume();'
   array(3) {
     [0]=>
     array(6) {
       ["file"]=>
       string(17) "Command line code"
       ["line"]=>
       int(1)
       ["function"]=>
       string(7) "suspend"
       ["class"]=>
       string(5) "Fiber"
       ["type"]=>
       string(2) "::"
       ["args"]=>
       array(0) {
       }
     }
     [1]=>
     array(4) {
       ["file"]=>
       string(17) "Command line code"
       ["line"]=>
       int(1)
       ["function"]=>
       string(1) "g"
       ["args"]=>
       array(0) {
       }
     }
     [2]=>
     array(2) {
       ["function"]=>
       string(9) "{closure}"
       ["args"]=>
       array(0) {
       }
     }
   }
   

   A crashing example (by stack spraying with that max() call):

   php -r 'function f() { Fiber::suspend(); } function g() { (new Fiber(function() { global $f; var_dump((new ReflectionFiber($f))->getTrace()); }))->start(); } $f = new Fiber(function() { f(); max(...[1,2,3,4,5,6,7,8,9,10,11,12]); g(); }); $f->start(); $f->resume();'
   

   However, I'm not sure what the proper fix would be:

   • Fiber::resume() has control of the fiber, but technically another extension could call zend_fiber_resume(), bypassing the method handler.
   • zend_fiber_switch_context() could check whether EG(active_fiber)->kind == zend_ce_fiber and conditionally set fiber->execute_data.
   • Moving the zend_fiber_vm_state to the fiber context instead of the stack and exposing it in the header and just accessing the data from there. (Not possible on non-master branch).

   I'd prefer the latter for master.

   For the current branches, we probably have to just special case zend_ce_fiber in zend_fiber_switch_context:

   diff --git a/Zend/zend_fibers.c b/Zend/zend_fibers.c
   index 53f78417f5..6f55455190 100644
   --- a/Zend/zend_fibers.c
   +++ b/Zend/zend_fibers.c
   @@ -395,6 +395,10 @@ ZEND_API void zend_fiber_switch_context(zend_fiber_transfer *transfer)
   
           zend_observer_fiber_switch_notify(from, to);
   
   +       if (from->kind == zend_ce_fiber) {
   +               zend_fiber_from_context(from)->execute_data = EG(current_execute_data);
   +       }
   +
           zend_fiber_capture_vm_state(&state);
   
           to->status = ZEND_FIBER_STATUS_RUNNING;

   Maybe someone else has a better idea?
6. bwoebi

   added

   Status: Verified

   and removed

   Status: Needs Triage

   on May 1, 2023
7. Girgias

   added

   Category: Fibers

   on May 5, 2023
8. ndossche commented on May 27, 2023

   

   ndossche

   on May 27, 2023

   Last edited by ndossche

   Member

   More actions

   Imo the plan for stable branches, and the plan (the last one, which is also your preference) for master sound good.
9. danog commented on Jul 31, 2023

   

   danog

   on Jul 31, 2023

   ContributorAuthor

   More actions

   Ping, could anyone merge the suggested fix? :)
10. danog

    added a commit that references this issue on Oct 9, 2023

    Fix phpGH-11121: ReflectionFiber segfault

    This issue will close once commit d4ff572 is merged into the 'master' branch.

    d4ff572
11. ndossche

    added 3 commits that reference this issue on Oct 11, 2023

    Fix GH-11121: ReflectionFiber segfault

    ...

    This issue will close once commit 71f1451 is merged into the 'master' branch.

    71f1451

    Merge branch 'PHP-8.1' into PHP-8.2

    ...

    This issue will close once commit d8cd0f4 is merged into the 'master' branch.

    d8cd0f4

    Merge branch 'PHP-8.2' into PHP-8.3

    ...

    This issue will close once commit 124c812 is merged into the 'master' branch.

    124c812
12. ndossche

    closed this as completedin 71f1451on Oct 11, 2023
13. ndossche

    added a commit that references this issue on Oct 11, 2023

    Merge branch 'PHP-8.3'

    ...

    This issue will close once commit 0ba24a5 is merged into the 'master' branch.

    0ba24a5