Skip to content

Fix GH-10437: Segfault on suspending a dead fiber #10438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

MaxSem
Copy link
Contributor

@MaxSem MaxSem commented Jan 24, 2023

No description provided.

set_time_limit(1);

register_shutdown_function(function () {
Fiber::getCurrent()->suspend();
Copy link
Member

@kelunik kelunik Jan 24, 2023

Choose a reason for hiding this comment

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

I can't find my previous comment anymore, so it might have gone lost... found it...

IMO, Fiber::getCurrent() should return null here, as I expect the stack to be unwound and shutdown functions being called from main then.

/cc @trowski

Copy link
Member

Choose a reason for hiding this comment

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

Yes, Fiber::getCurrent() should be returning null. Fix in #10443.

@trowski
Copy link
Member

trowski commented Jan 25, 2023

Throwing an exception is inappropriate, as this is not a user error, but a PHP bug.

@trowski trowski closed this Jan 25, 2023
@MaxSem
Copy link
Contributor Author

MaxSem commented Jan 25, 2023

Here's another way to reproduce the same issue that doesn't involve current():

<?php

set_time_limit(1);

register_shutdown_function(function () {
    global $fiber;
    $fiber->suspend();
});

$fiber = new Fiber(function () {
    while (1);
});
$fiber->start();

@trowski
Copy link
Member

trowski commented Jan 25, 2023

Fiber::suspend() is a static method. Its behavior depends on the context in which it is called. Using a proxy object to invoke it like an instance method doesn't change behavior.

Your second example using global is also fixed by #10443.

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.

4 participants