-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
[Merged by Bors] - Correct pop_on_return behaviour #1853
[Merged by Bors] - Correct pop_on_return behaviour #1853
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1853 +/- ##
=======================================
Coverage 56.29% 56.30%
=======================================
Files 201 201
Lines 17936 17937 +1
=======================================
+ Hits 10097 10099 +2
+ Misses 7839 7838 -1
Continue to review full report at Codecov.
|
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.
Feels weird to not check anything after executing the JS code. We could at least check whether an exception was thrown, in the first case the result of exec
should be Err
and in the second case Ok
, right? Just asserting that they got the right variants feels better than doing nothing else.
Sorry for the late response on this; busy Monday. :) Do the changes provided suffice? |
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.
Looks good!
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.
bors r+
This PR changes the following: - Updates the value of `pop_on_return` after a catch as to prevent VM stack corruption - Adds two test cases which demonstrate the issue and demonstrate that it has been fixed I am unsure if it is possible to abuse the patch provided; one would need to catch from within an array initialisation without calling into another frame (e.g., with a lambda), which I don't think is possible.
Pull request successfully merged into main. Build succeeded: |
This PR changes the following:
pop_on_return
after a catch as to prevent VM stack corruptionI am unsure if it is possible to abuse the patch provided; one would need to catch from within an array initialisation without calling into another frame (e.g., with a lambda), which I don't think is possible.