vm module behaves oddly W.R.T. context-objects and functions in exterior scopes

[ELLIOTTCABLE]

I’m not even sure how to go about describing this. What could possibly be going wrong is still far beyond my ability to understand, but I have derived a simple testcase to demonstrate the issue.

The first three testcases demonstrate that the error isn’t anything obvious:

• Everything works fine without the vm module
• Everything works fine if the object used is a separate object instead of the context object itself
• Everything looks fine after the vm is complete

// passes
function node_function_context_behaves_normally() {
   var assert = require('assert'), vm = require('vm')
   var o = new Object(); o.oo = o

   o.outFunc = function(){
      assert.equal(typeof o.inFunc, 'function', "(WON'T TRIGGER) a normal function behaves oddly with faux-context") }
  ;(function(oo){
      oo.inFunc = function(){ oo.widget = 'dat widget' }
      oo.outFunc()                                          })(o.oo)
}
node_function_context_behaves_normally()

// passes
function node_vm_normal_object_behaves_normally() {
   var assert = require('assert'), vm = require('vm')
   var o = new Object(); o.oo = o

   o.normalObject = {}
   o.normalObject.outFunc = function(){
      assert.equal(typeof o.normalObject.inFunc, 'function', "(WON'T TRIGGER) vm module behaves oddly with normal object") }
   vm.runInNewContext("                                                               \
      oo.normalObject.inFunc = function(){ oo.normalObject.widget = 'dat widget' };   \
      oo.normalObject.outFunc()                                                      ", o)
}
node_vm_context_behaves_oddly()

// passes
function node_vm_context_appears_to_behave_normally() {
   var assert = require('assert'), vm = require('vm')
   var o = new Object(); o.oo = o

   vm.runInNewContext("                                     \
      oo.inFunc = function(){ oo.widget = 'dat widget' };   ", o)
   assert.equal(typeof o.inFunc, 'function', "(WON'T TRIGGER) vm module still behaves oddly with context *after*")
}
node_vm_context_appears_to_behave_normally()

// fails
function node_vm_context_behaves_oddly() {
   var assert = require('assert'), vm = require('vm')
   var o = new Object(); o.oo = o

   o.outFunc = function(){
      assert.equal(typeof o.inFunc, 'function', "vm module behaves oddly with context") }
   vm.runInNewContext("                                     \
      oo.inFunc = function(){ oo.widget = 'dat widget' };   \
      oo.outFunc()                                          ", o)
}
node_vm_context_behaves_oddly()

The first three testcases throw no errors; only the last one does.

I’m using the current stable Node as of this posting, Node.js v0.4.12.

[ELLIOTTCABLE]

P.S. I’ve looked into this a bit, with the assistance of others from the IRC channel; it looks like @ry’s Script implementation is simply copying properties from the context object onto the vm’s global-sandbox before executing the code, and then manually copying everything that has changed back afterwards.

It sounds like we can rewrite that to utilize the passed context object itself as the prototype for the global, and yank out all the ugly manual copying (also solving this bug); I’ll have to look into it more tomorrow.

[ELLIOTTCABLE]

So, solved and implemented. Tests updaded to ensure the functionality.

I’d love input especially from @ry, @isaacs, @mraleph, and @bnoordhuis, as I’ve discussed this with you all previously. This is my first commit to Node since late 2009, so please let me know if there’s any styling or conventions that I messed up; I’m sorry if my code doesn’t quite “fit in” at first blush. (I did my best!)

Worth noting: All tests that passed before the commits pass after, though some have had minor functionality removed intentionally (stuff that doesn’t make any sense with the new system.) Tested on OS X and Linux, can’t imagine this causing any other platform-specific issues though.

[ghost]

-1 to forcing customized prototyope to the global object.
It's strange and I feel it dangerous.

[ELLIOTTCABLE]

Short summary of the points I’ve considered, considering exploring the global state of the Context from the outside.

The old methodology of copying any values of the Context’s globals back to the passed sandbox after the VM is finished is obviously inadequate. Copying causes all sorts of issues (such as those brought up by previous issue reports with depth-of-recursive-references, getters/setters, non-enumerable/invisible properties, blahblahblahblah), and to boot, it messes up whatever sandbox object you passed in (which is unacceptable if you intend to re-use it consistently as the same prototype of multiple Contexts’ globals.)

Some alternatives include:

• Changing the API such that all of the run-methods return the globals (or v8 global-proxy) after execution has completed, instead of the value of the last expression
• Providing a separate API to acquire a reference to the globals (or global-proxy) for a given Node::WrappedContext JavaScript-side, as new Context().global_proxy() or similar
• Duplicating the old shallow-copy functionality, but with a separate API as either of the last two describe
• Provide a GLOBAL_PROXY global from within Node to all environments, a lá GLOBALS, that can then be passed back out of the inner environment of a VM however the API consumer desires

I’m not sure which of these (or something else entirely) is the most friendly / useful API. I’m open to suggestions as to where to continue from here.

[ELLIOTTCABLE]

@Herby any particular reasoning behind “it’s dangerous?”

I’ve talked to the v8 guys, there’s explicitly Nothing Wrong™ with doing this. All of the magic happens with the global_proxy → globals prototype chain, and you shouldn’t modify the prototype of global_proxy if you value your skin … but there’s nothing unusual or wrong about setting the prototype of globals itself.

As for the “JavaScript”-ness of it, which I can see why you might be a little confused about … well, it’s exactly the same ‘hack’ that v8 itself uses to handle placing of the Context-specific values into the “global scope”: their solution to the similar issue was the same as mine; “use prototypal inheritance on the object representing the top of the scope chain.” If it’s good enough for v8::Context, it’s good enough for Node::WrappedContext. :D

[ghost]

@ELLIOTTCABLE: Well, if they say it's fine, I'm ok with it. I just have the experience that doing nasty things can lead to plethora of other problems that are initially hidden but begin to show themselves through use, so I rather defensively avoid doing thing that I do not believe to be right. But since they say this is not a nasty thing, why not?

[rehanift]

If I understand your suggested alternatives, does #1 change this behavior?

var sandbox = { plus: function(a,b){ return a + b } }
var result = vm.runInNewContext('plus(1,1)');
//=> result = 4

Personally, I use the last-evaluated expression from vm.run* methods, I would like that to stay the same if possible.

[ELLIOTTCABLE]

@rehanift I’m basically agreed, there. It’s sort of an eval()-esque API.

Perhaps that’s yet another alternative: changing the current APIs to be more VM-like and less eval()-like (in that they don’t treat any particular JavaScript expression, in this case the last one, as particularly special in any way), and then provide evalInNewContext() API alternatives to the new runInNewContext() that return the value of the last expression instead of the global scope?

Then again of course, there’s the consideration of backwards compatibility: how much code out there is currently using VM? I mean, we’re pre-1.0.0, so it’s not such a consideration, but there’s still the argument that “even if the API is better designed, it’s not worth the improvement to break backwards compatibility.” I’m personally very anti-backwards-compatibility whenever it is at the expense of API quality, but I’m pretty sure @ry et. al might disagree with me there. ;D

[ELLIOTTCABLE]

So, a discussion with @isaacs in IRC resulted in us deciding to restrict this pull-request itself to the non-API-changing prototypal-insertion elements of this code. I’ve reverted the API-changing elements, and we’re going to discuss how, exactly, these are best implemented on the mailing list, with a goal of implementing a replacement for 0.7.

[isaacs]

Here's what I'd like to see:

1. For 0.6, a cleanup to make it behave a bit more reasonably with respect to nested objects, getter/setters, etc. Otherwise, no API changes. Imperfection is ok, as long as it doesn't make things any worse. I think this pull req does that, and we should merge it in. The scary bits have been given the stamp of approval by @mraleph, and are apparently the normal v8-ism for such things, so I'm +1 on this.
2. In 0.7, we should make this api actually work how it seems like it should. That is, the sandbox object, and the global object in the vm, should be, for all intents and purposes, the same object. Adding/removing/modifying a property on one should be immediately visible on the other, and vice versa, without any "copying" step. It's yet to be seen if this is possible, but it's certainly a bigger change. If it's impossible/expensive/bad to do this, then we should explicitly remove the copy-back step, and provide some new API for getting the global object from the VM.

@Herby @bnoordhuis @mraleph Does this plan sound sane? Comments on this pull req for item 1?

@ELLIOTTCABLE Can you add another commit to make sure it doesn't add any new lint errors? python tools/cpplint.py src/node_javascript.{cc,h} Thanks.

[ghost]

@isaacs: Yes, it looks fine. Except the " If it's impossible/expensive/bad to do this, then we should explicitly remove the copy-back step, and provide some new API for getting the global object from the VM", it looks, well, ugly to me.
As I understand v8 global_proxy --> global thing used in context, making sandbox == context's global is impossible. Though, I would not get such a defensive position, I would instead try to make this feature into v8 itself (when creating context, having ability to pass a global sandbox, which will be used as the global_proxy, resetting its _ _ proto _ _ to global; possibly throwing TypeError if its _ _ proto _ _ before is not null or Object.prototype, that is, only accept flat object that does not inherit anything unusual).
I hope I was understood, I wrote it with densely and with lots of parentheses :-/ .

[Ayms]

Pass global to the vm (!!!!!!!!! ???????????)

Maybe we should wait a little bit before proceeding.

I think we are addressing here a non-issue.

Let's start from @ELLIOTTCABLE tests above, modifying :

function node_vm_context_behaves_oddly() {
var assert = require('assert'), vm = require('vm')
var o = new Object(); o.oo = o

o.inFunc=function() {};

o.outFunc = function(){
assert.equal(typeof o.inFunc, 'function', "vm module behaves oddly with context") }
vm.runInNewContext("
oo.inFunc = function(){ oo.widget = 'dat widget' };
oo.outFunc() ", o)
}

Just works, because outFunc is defined in the initial context and does execute there, so does not know inFunc defined in vm context unless inFunc exists in initial context (my modification)

For me, this is exactly the same issue as #1770 answered and closed by @bennoordhuis (and he was right)

Then I think that the tests above should replace o by "this" inside the functions (and then we come back to #1674, see below)

I have released today the node-dom project (not documented yet) which uses vm in all possible ways I believe, and I have explained today what I think should happen in #1674 and I do propose to try some modifications in cloneObjectMethod, at least node-dom gives a real example of implementation, I could give more tricky examples (like handling in node-dom something like body onload="console.log(this)" and how to have "this" refers to body which is not trivial)

VM is indeed confusing whether we are in the calling context or VM's one (I tried to give enough details in #1674 again), while having to do the same things in both to be consistent.

But to close the loop and allow modifications in both, we just have to do : context.context=context (@isaacs comment Adding/removing/modifying a property on one should be immediately visible on the other,)

This is not an invention, global is doing the very same thing for example, context will behave the same but still will be separated from other contexts, then limits security pb

I would then agree with @Herby changes are strange and dangerous

As another example I was thinking about some deviation or how not to use vm, so everything happens in the same context, which is not far from what is proposed here, we could do :

//from initial context
var code='my code'
--> save code somewhere in code.js
--> do something like : require('./code.js')
--> delete code.js

Then if code is doing something like 'remove all files from your server using fs'..........

I might be wrong in some parts but node-dom gave me so many problems with vm/initial context that I think these remarks are valid, context in vm should have nothing to do with others, and I will propose something for the "this" pb.

[Ayms]

@bnoordhuis not bennoordhuis

[ELLIOTTCABLE]

@nais what you suggest is unclear; I’m not seeing what you’re talking about.

As I said above, @Herby was simply a little unclear on the v8 APIs, and what we’re doing here is perfectly safe.

Regarding security, that’s a non-argument; the security or lack thereof of VMs are irrelevant, as VMs are not intended to be a secure API. (On that note, @isaacs and I are discussing a new truely secure Prison module for ~0.7+)

For this commit, what we’re doing is neither dangerous, nor unintuitive; it’s simply using a straightforward technique to fix an outstanding bug: that the sandbox is static-copy (a hack), not dynamic.

[ELLIOTTCABLE]

Final note for anybody following this: the clean-up functionality originally discussed here, and then discarded from this pull request, will be discussed on the mailing list for ~0.7. Additionally, it’s possible that we’re going to one-up this entire tactic, by patching v8 to allow arbitrary external objects to be set as the actual global object (predicate on exploring the difficulty and performance penalites of that approach.)

[Ayms]

@isaacs @ry @elliotcable

Could we please revert back for now ? With this + "vm context with accessors" everything is broken now in VM, see the test case below (I have followed what Elliot said in #1674), and it does not even solve #1674, the issues are :

• "vm context with accessors" does not take into account the fact that getOwnPropertyDescriptor can return undefined (not difficult to solve this one)
• after createContext, the context has nothing to do with the initial object and vice-versa (I believe this is related to this comment for 0.7 "Adding/removing/modifying a property on one should be immediately visible on the other,", so why do we not wait that it works ?)
• createContext/runInXXContext : "this" does not refer to the correct object #1674 is not solved

If you uncomment the getter to window in the code, then tests will work but this can not be the solution

var vm = require('vm');

var WINDOW = function() {
    this.name = "window";
    this.fn = function() {
        console.log(this.name);
    };
}

WINDOW.prototype.Image = function() {
    console.log(this.name);
}

window = new WINDOW(); // sandbox

window.Image(); //window //OK

console.log(Object.getOwnPropertyDescriptor(window,'Image'));//undefined

Object.defineProperty(window,'test',{value:function() {console.log(this.name)}});

window.test();//window //OK

ctx=vm.createContext(window); //window=vm.createContext(window) does not work either for below tests

console.log(ctx.Image); //undefined
                //Image is missing - separate issue, cloneObjectMethod does not take into account the fact that getOwnPropertyDescriptor can be undefined 

console.log(ctx.test); //OK

//Object.defineProperty(global, 'window', {get : function() {return ctx}, set : function() {}}); //uncommenting this will make the below tests work

window.window=window; //assign in sandbox self-reference back to sandbox (apparently useless since last changes)


window.console=console; //apparently useless since last changes

ctx.console=console; //assign console to ctx

ctx.window=window; //if not window does not exist in ctx

vm.createScript('test()').runInContext(ctx);//undefined "this" in test function does not refer to ctx (#1674)

vm.createScript('console.log(name)').runInContext(ctx);//window//OK

vm.createScript('fn()').runInContext(ctx);//window//OK

window.document='document';

vm.createScript('console.log(document);console.log(window.document)').runInContext(ctx); //should be document + document//"document is not defined"//NOK

vm.createScript('var div="div";console.log(window.div)').runInContext(ctx); //should be "div"//undefined//NOK

console.log(window.div); //should be div//undefined//NOK

[Ayms]

@ELLIOTTCABLE

[ry]

Reverted in 9d27faa until I have a chance to review.

[Ayms]

Thanks, we should revert too "vm context with accessors", looks not so easy to solve finally (getters/setters), I don't know why ecma specs do allow it but it's not rare that getOwnPropertyDescriptor returns undefined (proto for example), #1674 (even if disturbing) is not blocking, others issues mentionned above are.

[ELLIOTTCABLE]

Er, @nais, as I’ve said in #1674, you’re misunderstanding how this is supposed to work. The API is, definitely, a little unclear right now; we’re planning on re-working the entire API for this in 0.7, to make it very explicit and easy to use.

The change in this pull request is definitely not reponsible for the “brokenness” you are experiencing, though; that’s simply because you’re using the API in an unintended way. As you put it, “after createContext, the context has nothing to do with the initial object and vice-versa …,” that is intended functionality.

@nais, on a related note, please reply to my latest comment on #1674; I’d love to figure out what you were expecting out of v8::Context objects, as a user; once I understand, maybe I can clean up the API to make more intuitive sense to you.

[ELLIOTTCABLE]

Er, I don’t know how this got closed. @github’s screwing up. M’bad.

[TooTallNate]

Haha, I don't get how I "referenced" the revert commit either. So I agree, GitHub's screwin' up...

[ELLIOTTCABLE]

@TooTallNate that’s probably entirely my fault. I used undocumented APIs to turn this “Issue” into a “Pull Request,” which @github normally treats as a different thing. I probably borked teh ’hub in the process. #mybad

[Ayms]

@ELLIOTTCABLE, I will reply (again) on #1674, I am not misunderstanding how it is supposed to work, maybe it will be more clear once I have documented https://github.com/Nais/node-dom, you must be able to access window context in the initial context and in vm, then turning window into a context and assigning window global var into that context referring to the context itself so it is available in vm and initial context seems perfectly logical.

I gave plenty of examples in both posts, right now if I set a global var in vm, it is not reflected in window for example, so this breaks everything, I know the API is not finished yet but it appears context and window are separate and there is no way to handle this for me.

Since there are plenty of parallel discussions, I would appreciate on your side a simple example of what current changes are doing and supposed to solve and what you think should be the result of NOK tests above, if you don't agree with the NOK statement

Indeed last changes do break for me the "logical" behavior, since you say it is intended, then please let me know how I should proceed now to correlate window in both contexts.

[ELLIOTTCABLE]

@nais I messaged, let’s talk about this in IRC and get everything clear, so we stop wasting space on Issues until we understand eachother.

[trevnorris]

I think this was squashed into 200df86. Should be closed?

[bnoordhuis]

I think this was squashed into 200df86. Should be closed?

Yes, and reverted again in 9d27faa. I see no compelling reason to land it (never mind the fact that it's bitrotted), closing.