-
Notifications
You must be signed in to change notification settings - Fork 79
test(ses): cover the allowNextEvalToBeUnsafe failure after stack overflow #3009
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
base: master
Are you sure you want to change the base?
Conversation
14b1571 to
a3aa921
Compare
a3aa921 to
c95461a
Compare
mhofman
left a comment
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'm surprised you managed to build a test reproducing this. I'll try to look how we actually end up in this case.
| depth += 1; | ||
| evaluate(`1+1`); | ||
| // didn't fail yet? keep digging. | ||
| return stackBuildUp(); |
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 would recommend avoiding tail recursion here as that is an explicitly allowed optional optimization.
| t.throws(() => evaluate('1+1'), { | ||
| message: /a handler did not reset allowNextEvalToBeUnsafe \(a RangeError\)/, | ||
| }); |
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.
Interesting, I thought we couldn't actually trigger this case. We may have broken the stack overflow protection at some point.
| const allowEvalProperties = freeze({ | ||
| eval: { | ||
| value: props => { | ||
| const { eval: evalProp } = props; | ||
| delete evalScope.eval; | ||
| defineProperties(evalScope, { eval: evalProp }); | ||
| }, |
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.
Took me a good 10 minutes of staring at this to figure out why it works.
I suggest we add a lengthy comment to explain it 😅
Something like this maybe:
| const allowEvalProperties = freeze({ | |
| eval: { | |
| value: props => { | |
| const { eval: evalProp } = props; | |
| delete evalScope.eval; | |
| defineProperties(evalScope, { eval: evalProp }); | |
| }, | |
| // This conforms to the shape of evalScope properties, but instead of evaluating what arrives as first | |
| // argument, it accepts a new shape to replace eval with. | |
| // As a result, when `apply(evaluate, globalObject, [oneTimeEvalProperties]);` is called below, | |
| // the same exact stack depth is reached and the same optimizations are applied to it before the | |
| // FERAL_EVAL is put in place, making it impossible for a stack overflow error to occur in the process | |
| // of getting eval for the main call to the evaluator if it didn't occur in revealing eval. | |
| const allowEvalProperties = freeze({ | |
| eval: { | |
| value: props => { | |
| const { eval: evalProp } = props; | |
| delete evalScope.eval; | |
| defineProperties(evalScope, { eval: evalProp }); | |
| }, |
Description
Just a test covering an edge-case that's rare but known but often hard to recall.