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: variable declaration persists across two runInContext() #5491

Closed
abenhamdine opened this issue Feb 29, 2016 · 6 comments
Closed

vm: variable declaration persists across two runInContext() #5491

abenhamdine opened this issue Feb 29, 2016 · 6 comments
Labels
doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.

Comments

@abenhamdine
Copy link

  • Version: 5.7.0
  • Platform: windows 7.1 Pro 64 bits
  • Subsystem: vm

Code

const vm = require('vm');
const assert = require('assert');
const context = {
    results: {}
};

// 1st execution
const sandBox1 = vm.createContext(context);

let sFormula1 = "";
sFormula1 += "'use strict';\n";
sFormula1 += "let x = 1;\n";
sFormula1 += "let y = 2;\n";
sFormula1 += "let z = 0 ;\n";
sFormula1 += "z = x + y;\n";
sFormula1 += "results.z = z;\n";

vm.runInContext(sFormula1, sandBox1);

// *** 2nd execution ***
// get the value of z from the first sandbox
context.results.z = sandBox1.results.z;
// sandboxing the new context
const sandBox2 = vm.createContext(context);

let sFormula2 = "";
sFormula2 += "'use strict';\n";
sFormula2 += "let z = 10\n";
sFormula2 += "results.z = z;\n";

vm.runInContext(sFormula2, sandBox2);

Expected result

This code should't throw.

Actual result

This code throw the following error:

evalmachine.<anonymous>:1
'use strict';
^

TypeError: Identifier 'z' has already been declared
    at evalmachine.<anonymous>:1:1
    at Object.exports.runInContext (vm.js:44:17)
    at Object.<anonymous> (C:\node-projects\cvl-node-extdirect\bugVM.js:33:4)
    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:141:18)
    at node.js:933:3

let declaration is block-scope, and note that it's not even the same sandbox in the two runInContext(), so how is it possible to get this error ?

Am I wrong somewhere ?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 29, 2016

sandBox1, sandBox2, and context are all the same object. vm.createContext() returns the object that is passed to it.

@silverwind silverwind added the vm Issues and PRs related to the vm subsystem. label Feb 29, 2016
@abenhamdine
Copy link
Author

@cjihrig Ah thanks I did'nt get that !
But I'm still a bit confused even after reading the doc

OK, sandBox1 and sandBox2 are the same object.

But in the first call to runInContext(), z has been declared local :
let z = 0;
So the context should'nt be modified, unlike if z has been declared global z = 0 or var z = 0 and despite this, z already exist when runInContext() is called for the second time.

So I'm missing a point here :

  • does it mean that, if I pass the same sandbox object to 2 differents runInContext(), the 2 "run" will share the same context and the same variables ?
  • if yes, does it mean then createContext() register somewhere a context, associated with a given sandbox ? The doc is not very explicit on this.

Thx in advance for any clarification !

If you think it's useful, I wil send a PR to make the doc more explicit on this.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 1, 2016

This issue might be relevant to what you're seeing - #983.

does it mean that, if I pass the same sandbox object to 2 differents runInContext(), the 2 "run" will share the same context and the same variables ?

Yes.

if yes, does it mean then createContext() register somewhere a context, associated with a given sandbox ? The doc is not very explicit on this.

Yes. See lib/vm.js and src/node_contextify.cc.

Documentation PRs are almost always a good thing, so feel free to make one.

@abenhamdine
Copy link
Author

@cjihrig thx again for your explanation ! It's more clear now, I will send a doc PR soon.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Mar 2, 2016
@bnoordhuis
Copy link
Member

I'll close, looks like it's resolved? Let me know if I should reopen.

@abenhamdine
Copy link
Author

Yes, it's resolved. A doc improvement would be a good thing IMHO, I will send a PR when I have some time, but it's definitely not a bug.

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

No branches or pull requests

5 participants