-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix crash during GC of suspended generator delegate #15275
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
Fix crash during GC of suspended generator delegate #15275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is your code.
You know it better then me.
I don't object.
This is sort-of annoying, as the opline now won't point to the line with the yield (from) in stack traces in zend_generator_throw_exception thrown by generator->values destructors (if I understood it correctly?). I think it would be actually beneficial to not replace the line numbers by %d in these tests. |
I think the line number will be correct, luckily, most of the time:
Agreed, I've updated the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's fine then. But yeah, I very much prefer the other patch, which can land only in master though, so please replace it by that one when merging to master.
* PHP-8.2: [ci skip] NEWS for phpGH-15275 Fix crash during GC of suspended generator delegate (php#15275)
* PHP-8.3: [ci skip] NEWS for phpGH-15275 [ci skip] NEWS for phpGH-15275 Fix crash during GC of suspended generator delegate (php#15275)
We assume that non-running generators have their
->execute_data->opline
pointing to the opline next to theyield
oryield from
that stopped it (e.g. here, here, and here).However, when fetching the next delegated value for a
yield from $nonGenerator
, the opline points to theyield from
op due to temporary adjustments inzend_generator_get_next_delegated_value()
.This causes crashes when cleaning up unfinished calls or assertion failures during GC.
Here I change
zend_generator_get_next_delegated_value()
to not adjust the opline, so that the expectation is always true.zend_generator_throw_exception()
also adjusts the opline, which causes similar issues ifzval_ptr_dtor(&generator->values)
triggers the GC. I update it to cancel the adjustment before destroying generator->values.Alternatively, I think it may be simpler to stop incrementing opline in the yield and yield from op handlers (and to leave this task to zend_generator_resume()), so that we don't have to patch opline in other places. However this targets 8.2, so I didn't explore this idea.
While testing I've also found that calling
->throw()
on a running generator, or a child of a running generator, may crash. In [1], thegetIterator()
generator is getting closed during->throw()
, but its execute_data is accessed after that. In [2] we breakzend_generator_throw_exception()
's assumption of being called on a non-running generator. Should we forbid calling->throw()
whenzend_generator_get_current()
is running?