You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This script seems to result in a memory corruption since at least 7.4 according to https://3v4l.org/8SDGE
The problem seems to be that we discard an exception that was thrown by an other frame, but the frame will still try to handle it. Since EG(exception) is NULL, it will not take the right code path, and continue to execute after having freed live variables.
ZEND_ADD_ARRAY_UNPACK throws an Error("Keys must be of type int|string during array unpacking"). EG(exception) is set, and EG(current_execute_data)->opline is set to EG(exception_op)
The generator is closed, which resumes in the finally block
ZEND_DISCARD_EXCEPTION clears EG(exception), but the execute_data->opline of the parent frame remains EG(exception_op)
When resuming the parent frame, we are in an unexpected state: execute_data->opline is EG(exception_op), but EG(exception) is NULL
ZEND_HANDLE_EXCEPTION_SPEC_HANDLER cleanups live variables (including the array being built), and calls zend_leave_helper_SPEC
In zend_leave_helper_SPEC jumps to the next opline (ZEND_FETCH_DIM) because EG(exception) is NULL
@nielsdos your fix looks good. The case that still crashes is because EG(opline_before_exception) points to the generator op array, so in ZEND_HANDLE_EXCEPTION we call cleanup_unfinished_calls with an invalid op_num. We should probably save/restore EG(opline_before_exception) as well.
Thanks. That makes sense. I missed the opline_before_exception part so that's why I didn't understand what was going on with my extra testcase.
I'll update the fix and PR it when I have time.
Activity
ndossche commentedon Apr 7, 2023
Actually this seems to be a whole category of "yield"-related issues with finally's. This also doesn't work:
and if you replace
nullwith(new stdClass), orfalse, ortrue, ... for example, it also doesn't work.EDIT: progress....
php-src/Zend/zend_vm_def.h
Line 6236 in 7de83e2
ndossche commentedon Apr 8, 2023
I attempted a fix here: ndossche@6650263
This commit fixes both OP's issue and the issue I reported in my previous comment.
However, I found a variant of my previously reported issue which still crashes (may be different root cause?). It can be found here: ndossche@6650263#diff-e61ec0f8972fbb546701b53555d59a0bfe720abdf3040f6b87c23a8a7f94db40
I'm not sure what's going on in that case tbh
iluuu1994 commentedon Apr 13, 2023
Maybe @bwoebi or @arnaud-lb can help, they've touched this code most recently 🙂
arnaud-lb commentedon Apr 14, 2023
This script seems to result in a memory corruption since at least 7.4 according to https://3v4l.org/8SDGE
The problem seems to be that we discard an exception that was thrown by an other frame, but the frame will still try to handle it. Since EG(exception) is NULL, it will not take the right code path, and continue to execute after having freed live variables.
Error("Keys must be of type int|string during array unpacking"). EG(exception) is set, and EG(current_execute_data)->opline is set to EG(exception_op)@nielsdos your fix looks good. The case that still crashes is because EG(opline_before_exception) points to the generator op array, so in ZEND_HANDLE_EXCEPTION we call cleanup_unfinished_calls with an invalid op_num. We should probably save/restore EG(opline_before_exception) as well.
ndossche commentedon Apr 14, 2023
Thanks. That makes sense. I missed the opline_before_exception part so that's why I didn't understand what was going on with my extra testcase.
I'll update the fix and PR it when I have time.
Merge branch 'PHP-8.1' into PHP-8.2
Merge branch 'PHP-8.2'