Do not copy value onto vm sandbox in strict mode#7908
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the c++ 
test/parallel/test-vm-strict-mode.js Outdated
There was a problem hiding this comment.
nit: use assert.strictEqual wherever it makes sense
Fishrock123 commented Jul 28, 2016
src/node_contextify.cc Outdated
There was a problem hiding this comment.
Minor nit: the readability of this expression would be better if the FromJust were to be done above as part of the definition of is_declared.
fhinkel commented Jul 28, 2016
Failed on Windows: I guess that's unrelated to the change? |
ofrobots commented Jul 28, 2016
Another attempt: https://ci.nodejs.org/job/node-test-pull-request/3448/ |
ofrobots commented Jul 28, 2016
LGTM, but it would be good to get @hashseed and @bnoordhuis to sign-off as well. The arm64 buildbot seems to have run infrastructure issues (it passed in the earlier build). I would consider the CI to be green. |
hashseed commented Jul 29, 2016
LGTM. As discussed, this hinges on getting the prediction right. However, since that part is specified in ECMA262, I think it won't change anytime soon. |
src/node_contextify.cc Outdated
There was a problem hiding this comment.
Style nit: && should go on previous line. I'd write it as:
bool set_property_will_throw = args.ShouldThrowOnError() && !is_declared && is_contextual_store;
bnoordhuis commented Jul 29, 2016
LGTM with some style nits. |
test/parallel/test-vm-strict-mode.js Outdated
There was a problem hiding this comment.
minor nit... would you mind adding a short comment here about what precisely this test is verifying? ... just to make it easier for other devs who are looking at this test in the future.
jasnell commented Jul 29, 2016
LGTM with a nit. |
fhinkelforce-pushed the i5344 branch 2 times, most recently from b560a65 to ba84d34Compare
In vm, the setter interceptor should not copy a value onto the sandbox, if setting it on the global object will fail. It will fail if we are in strict mode and set a value without declaring it. Fixesnodejs#5344
fhinkel commented Jul 30, 2016
Style nits addressed and added a comment explaining the tests. |
cjihrig commented Aug 1, 2016
LGTM. Another CI run after changes: https://ci.nodejs.org/job/node-test-pull-request/3481/ |
jasnell commented Aug 1, 2016
LGTM with the changes! Thank you! |
jasnell commented Aug 1, 2016
Build bot failure on Windows on the last CI run... trying again: https://ci.nodejs.org/job/node-test-pull-request/3486/ |
Trott commented Aug 2, 2016
More buildbot failures. Let's try again. CI: https://ci.nodejs.org/job/node-test-pull-request/3495/ |
fhinkel commented Aug 2, 2016
Green. Thanks! |
In vm, the setter interceptor should not copy a value onto the sandbox, if setting it on the global object will fail. It will fail if we are in strict mode and set a value without declaring it. Fixes: #5344 PR-URL: #7908 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
jasnell commented Aug 4, 2016
Landed in 588ee22! Thank you! |
ofrobots commented Aug 4, 2016
This might be good a good candidate backport to 6.x and possibly 4.x (if the issue is present there). |
In vm, the setter interceptor should not copy a value onto the sandbox, if setting it on the global object will fail. It will fail if we are in strict mode and set a value without declaring it. Fixes: #5344 PR-URL: #7908 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins commented Oct 10, 2016
@fhinkel it would appear that this problem exists on v4.x but Would you be able to backport? |
fhinkel commented Oct 11, 2016
Sounds like the API version is too old, I don't think we can easily backport :( |
MylesBorins commented Oct 11, 2016
@fhinkel do you think that this bug is bad enough to come up with another solution, or should we just let it be? |
fhinkel commented Oct 11, 2016
Let it be. That's fine. |
11 participants
Checklist
make -j4 test(UNIX)Affected core subsystem(s)
src
Description of change
In vm, the setter interceptor should not copy a value onto the sandbox, if setting on
the global object will fail because we are in strict mode and the variable is not declared.
Fixes#5344
/cc @bnoordhuis@hashseed@ofrobots