Do not copy value onto vm sandbox in strict mode #7908

fhinkel · 2016-07-28T11:28:58Z

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

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

addaleax · 2016-07-28T11:46:43Z

test/parallel/test-vm-strict-mode.js

+const ctx = vm.createContext();
+
+vm.runInContext('w = 1;', ctx);
+assert.equal(1, ctx.w);


nit: use assert.strictEqual wherever it makes sense

Fishrock123 · 2016-07-28T13:22:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3447/

ofrobots · 2016-07-28T13:48:58Z

src/node_contextify.cc

+    bool is_contextual_store = ctx->global_proxy() != args.This();
+
+    bool set_property_will_throw = args.ShouldThrowOnError()
+        && !is_declared.FromJust() && is_contextual_store;


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 · 2016-07-28T14:19:39Z

Failed on Windows:

Resetting working tree
 > git reset --hard # timeout=10
 > git clean -fdx # timeout=10
FATAL: Command "git clean -fdx" returned status code 1:

I guess that's unrelated to the change?

ofrobots · 2016-07-28T16:52:23Z

Another attempt: https://ci.nodejs.org/job/node-test-pull-request/3448/

ofrobots · 2016-07-28T17:32:08Z

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 · 2016-07-29T05:15:15Z

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.

bnoordhuis · 2016-07-29T08:40:14Z

src/node_contextify.cc

+    bool is_contextual_store = ctx->global_proxy() != args.This();
+
+    bool set_property_will_throw = args.ShouldThrowOnError()
+        && !is_declared && is_contextual_store;


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 · 2016-07-29T08:41:43Z

LGTM with some style nits.

jasnell · 2016-07-29T15:59:58Z

test/parallel/test-vm-strict-mode.js

+const vm = require('vm');
+const ctx = vm.createContext();
+
+vm.runInContext('w = 1;', ctx);


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 · 2016-07-29T16:00:10Z

LGTM with a nit.

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 nodejs#5344

fhinkel · 2016-07-30T06:26:31Z

Style nits addressed and added a comment explaining the tests.

cjihrig · 2016-08-01T14:29:08Z

LGTM. Another CI run after changes: https://ci.nodejs.org/job/node-test-pull-request/3481/

jasnell · 2016-08-01T15:18:27Z

LGTM with the changes! Thank you!

jasnell · 2016-08-01T17:48:43Z

Build bot failure on Windows on the last CI run... trying again: https://ci.nodejs.org/job/node-test-pull-request/3486/

Trott · 2016-08-02T05:55:23Z

More buildbot failures. Let's try again. CI: https://ci.nodejs.org/job/node-test-pull-request/3495/

fhinkel · 2016-08-02T06:43:14Z

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 · 2016-08-04T19:44:50Z

Landed in 588ee22! Thank you!

ofrobots · 2016-08-04T19:49:56Z

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 · 2016-10-10T22:29:55Z

@fhinkel it would appear that this problem exists on v4.x but ShouldThrowOnError() does not exist in the version of V8 in v4.x

Would you be able to backport?

fhinkel · 2016-10-11T08:39:27Z

Sounds like the API version is too old, I don't think we can easily backport :(

MylesBorins · 2016-10-11T08:54:27Z

@fhinkel do you think that this bug is bad enough to come up with another solution, or should we just let it be?

fhinkel · 2016-10-11T17:04:22Z

Let it be. That's fine.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 28, 2016

addaleax added the vm Issues and PRs related to the vm subsystem. label Jul 28, 2016

addaleax reviewed Jul 28, 2016
View reviewed changes

fhinkel force-pushed the i5344 branch from 734b1b7 to b8d9556 Compare July 28, 2016 12:50

ofrobots reviewed Jul 28, 2016
View reviewed changes

fhinkel force-pushed the i5344 branch from b8d9556 to c745d5f Compare July 28, 2016 14:33

bnoordhuis reviewed Jul 29, 2016
View reviewed changes

jasnell reviewed Jul 29, 2016
View reviewed changes

fhinkel force-pushed the i5344 branch 2 times, most recently from b560a65 to ba84d34 Compare July 30, 2016 06:09

src: do not copy on failing setProperty()

a349ca8

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 nodejs#5344

fhinkel force-pushed the i5344 branch from ba84d34 to a349ca8 Compare July 30, 2016 06:16

jasnell closed this Aug 4, 2016

jasnell added the lts-watch-v4.x label Aug 4, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Oct 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not copy value onto vm sandbox in strict mode #7908

Do not copy value onto vm sandbox in strict mode #7908

fhinkel commented Jul 28, 2016

addaleax Jul 28, 2016

Fishrock123 commented Jul 28, 2016

ofrobots Jul 28, 2016

fhinkel commented Jul 28, 2016

ofrobots commented Jul 28, 2016

ofrobots commented Jul 28, 2016

hashseed commented Jul 29, 2016

bnoordhuis Jul 29, 2016

bnoordhuis commented Jul 29, 2016

jasnell Jul 29, 2016

jasnell commented Jul 29, 2016

fhinkel commented Jul 30, 2016

cjihrig commented Aug 1, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 1, 2016

Trott commented Aug 2, 2016

fhinkel commented Aug 2, 2016

jasnell commented Aug 4, 2016

ofrobots commented Aug 4, 2016

MylesBorins commented Oct 10, 2016

fhinkel commented Oct 11, 2016

MylesBorins commented Oct 11, 2016

fhinkel commented Oct 11, 2016

Do not copy value onto vm sandbox in strict mode #7908

Do not copy value onto vm sandbox in strict mode #7908

Conversation

fhinkel commented Jul 28, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax Jul 28, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jul 28, 2016

ofrobots Jul 28, 2016

Choose a reason for hiding this comment

fhinkel commented Jul 28, 2016

ofrobots commented Jul 28, 2016

ofrobots commented Jul 28, 2016

hashseed commented Jul 29, 2016

bnoordhuis Jul 29, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jul 29, 2016

jasnell Jul 29, 2016

Choose a reason for hiding this comment

jasnell commented Jul 29, 2016

fhinkel commented Jul 30, 2016

cjihrig commented Aug 1, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 1, 2016

Trott commented Aug 2, 2016

fhinkel commented Aug 2, 2016

jasnell commented Aug 4, 2016

ofrobots commented Aug 4, 2016

MylesBorins commented Oct 10, 2016

fhinkel commented Oct 11, 2016

MylesBorins commented Oct 11, 2016

fhinkel commented Oct 11, 2016