Skip to content
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

VM: weird behaviour in runInContext/runInThisContext #5344

Closed
princejwesley opened this issue Feb 21, 2016 · 10 comments
Closed

VM: weird behaviour in runInContext/runInThisContext #5344

princejwesley opened this issue Feb 21, 2016 · 10 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@princejwesley
Copy link
Contributor

While working on my electron based REPL application, I noticed a weird behaviour in vm.runInContext.

Program
Desktop 🙈  cat repl-vm-strict.js
'use strict';
const vm = require('vm');
const ctx = vm.createContext();

try {
  const result = vm.runInContext('"use strict"; x = 1', ctx);
} catch(e) {
  console.log(e.stack);
  console.log('x is', ctx.x)
}
Output
Desktop 🙈  node repl-vm-strict.js
ReferenceError: x is not defined
    at evalmachine.<anonymous>:1:17
    at Object.exports.runInContext (vm.js:44:17)
    at Object.<anonymous> (/Users/princejohnwesley/Desktop/repl-vm-strict.js:7:21)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:140:18)
    at node.js:1001:3
x is 1
Behaviour
  • vm.runInThisContext/vm.runInContext throws error. ✅
  • context is also updated as if variable is created with var or let binding ❌
Versions

Tested versions: v5.6.0 & v5.1.1

Platform
Desktop 🙈 ₹ uname -a
Darwin Princes-MacBook-Pro.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64
Subsystem

vm

repl with strict is affected by this issue.

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 21, 2016
@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Feb 21, 2016
@bnoordhuis
Copy link
Member

I can confirm the bug but I suspect it's a V8 issue - it calls the global object's named property handlers (specifically the GlobalPropertySetterCallback) when it shouldn't. I'll have to investigate if it still happens with V8 HEAD.

@bnoordhuis
Copy link
Member

Still happens with V8 4.10.253 and 5.0.48. I'll see if I can put a standalone test case together for upstream to take a look at.

@hashseed
Copy link
Member

hashseed commented Mar 1, 2016

I talked to my colleagues. V8 seems to be working as intended in this case. I assume that there is an interceptor on the global object installed by node. The interceptor probably stores the value onto the global object, but returns false, signalling that it did not intercept. V8 then attempts to store and throws due to strict mode.

So the interceptor should either not store and return false, or throw if v8::PropertyCallbackInfo::ShouldThrowOnError() returns true.

@bnoordhuis
Copy link
Member

@hashseed Yang, thanks for chiming in. Is there anything we can do with 4.6 and 4.8? They don't have the ShouldThrowOnError() method.

The best I've been able to come up with is to sniff the script for a 'use strict' string and omit the call to v8::PropertyCallback<T>::Set() in the setter for strict mode scripts if there is no corresponding property on the global object. Basically this:

Local<Object> sandbox = /* ... */;
if (!strict() || sandbox->Has(property)) {
  sandbox->Set(property, value);
  info.GetReturnValue().Set(value);
}

@hashseed
Copy link
Member

hashseed commented Mar 1, 2016

Would this work for you? Whenever the interceptor is going to do a normal store, it does not store and returns false. V8 will then do the store, unless strict mode causes an exception before that.

@thefourtheye
Copy link
Contributor

@bnoordhuis If the property is newly created, neither the sandbox object nor the global object will have the property defined on them when GlobalPropertySetterCallback is called. So, sandbox->Has(property) will fail and the new property will not be added to sandbox, right?

fhinkel added a commit to fhinkel/node that referenced this issue Jul 28, 2016
the global object will fail because we are in strict mode.

Function declarations should not throw, we expect that they are not set
when we define them, so copy them over!

Add test for setting variable in strict mode for issue nodejs#5344.
fhinkel added a commit to fhinkel/node that referenced this issue Jul 30, 2016
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
@jasnell jasnell closed this as completed in 588ee22 Aug 4, 2016
cjihrig pushed a commit that referenced this issue Aug 10, 2016
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]>
@mmc41
Copy link

mmc41 commented Dec 13, 2016

The possible bug behind this "#5344" seems to be fixed now.

@mbroadst
Copy link

mbroadst commented Feb 21, 2017

I'm still experiencing weirdness with a slightly modified version of this test case:

'use strict';
const vm = require('vm');
const ctx = vm.createContext({ x: 42 });

try {
  const result = vm.runInContext('"use strict"; x = 1', ctx);
} catch(e) {
  console.log(e.stack);
  console.log('x is', ctx.x)
}

results in: ReferenceError: x is not defined

mbroadst@gorgor:~$ uname -a && node -v
Darwin gorgor.local 16.3.0 Darwin Kernel Version 16.3.0: Thu Nov 17 20:23:58 PST 2016; root:xnu-3789.31.2~1/RELEASE_X86_64 x86_64
v7.5.0

@mscdex
Copy link
Contributor

mscdex commented Apr 9, 2017

This does seem to be happening still, even on current master.

Thoughts @nodejs/v8?

@bnoordhuis
Copy link
Member

Moved to #12300 so it has its own bug number. It's related but separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants