runInThisContext does not run in context of the caller

[ghost]

https://gist.github.com/913539

The wrong thing are the output lines "new this" and "ctx this", which should show 2, but show 1 instead.
This may lead to strange REPL behaviour as well (it probably does, indeed, if require uses runInThisContext).

[ghost]

Btw, v0.2.6, I am unlucky by having freebsd/amd64 which in uncompilable for a past few months, but this code was not changed very much, so it should be the same in 0.4/0.5-pre.

[ghost]

Since runInThisContext is de facto the same code as process.compile of old API had, this issue is present for process.compile as well - just checked.

[ghost]

The problem is, very probably,on using FunctionTemplate, in NODE_SET_XXX macros. It's fine for majority of cases, but FunctionTemplate is context-bounjd - that is, the function created from it run in context to which it is bound. Thus, runInThisContext is not running in active context of the time of the call (which it should), but in the context that was active when GetFunction() gets called.
For these two functions (runInThisContext in class and in prototype) probably ObjectTemplate combined with SetCallAsFunctionHandler (or other v8 trick) should be used.
(if there are other functions in node that are sensitive to be called in context of the caller, it applies to them as well)

[ghost]

Any plans to fix this? runInThisContext is used by module system and its behaviour is broken inside any context beyond the starting one, which is not good...

[mmalecki]

+1 on this issue. Happens on v0.5.5 as well. It can turn into quite serious security issue if you trust VM module and include it in your context.

[thejh]

@mmalecki, the docs clearly say that the VM module is not for security :D

[mmalecki]

vm.runInThisContext does not have access to the local scope, so localVar is unchanged. eval does have access to the local scope, so localVar is changed.

Docs.

It fails to do that and it's insecure, but sure, it wasn't what VM was designed for.

[medikoo]

I guess problem lies in name of this function and not in how it works, I assume that it works as author intended.

Context is resolved lexically, vm.runInThisContext was declared in root context and will always fallback to root context.
Due to to lexical nature of things it's unable to retrieve global of context to which it was passed. It's like passing already declared function to other closure and expect it to see it's closure variables.

Example:

var showThisA = (function () {
    var a = 'foo';
    return function () {
        return a;
    };
}());

var otherScope = (function () {
    var a = 'bar';
    return function (showThisA) {
        return showThisA();
    };
}());

console.log(otherScope(showThisA)); // "foo" and we're not surprised

Reaching global context works same. it's just end of closure chain.

Solution would be to declare context's own runInThisContext within new context and use it.

What is unfortunate is that runInThisContext should rather be named runInRootContext or runInInitialContext,

I assume that vm can have real runInThisContext which will work as you expect but it would introduce some extra complexity to the vm module, as context will need to be tracked dynamically.

[ghost]

@mmedikoo: False. I am the author and runInThisContext is named exactly after what it should do.

[medikoo]

@Herby thanks for clarification

[ghost]

What about this? It is the bug. Makes vm behaviour strange when used in other-than-main context. No v8 / C++ guru to fix this?

[bnoordhuis]

@Herby: Does the issue boil down to this?

Object.x = 42; 
var vm = require('vm'), ctx = vm.createContext({vm:vm});
vm.runInContext('Object.x = 1337', ctx);
vm.runInContext('vm.runInThisContext("Object.x")', ctx); // returns 42, not 1337

If yes, then I think you're looking at expected behaviour: the vm object in vm.runInThisContext() is attached to the global scope so it makes sense that it's returning the global Object.x, not the context-ified Object.x

[ghost]

It is not expected behaviour. Expected behaviour of runInThisContext is really to run in context of the caller. It should be context-unbound. This is what this bug is about the whole time (I designed the name and I knew what was the expected behaviour - the context of the caller. It is there for good reason).

The problem is really the fact that it is bound to the context - it should do its work without declaring a (finished) function, but to use some other kind of handling. It is too v8-specific. This one method (two, in fact, instance and class one) should be implemented specially (all others are bound by using that pair of macros, which is fine for them; this one should be exempt from context-binding).

[pedronasser]

Should change the name for runInGlobalContext. lol

[bnoordhuis]

Is this still open? Okay, I'll close it because it's not a bug.

Reduced test case reproduced here for posterity:

var vm = require('vm');
var ctx = vm.createContext({vm:vm});
Object.x = 1;
vm.runInContext('Object.x = 2', ctx);
console.log(vm.runInContext('Object.x', ctx));
console.log(vm.runInContext('vm.runInThisContext("Object.x")', ctx));
// prints:
// 2
// 1

Why? It passes the vm object to the newly created context object.

Objects and functions in V8 belong to the context they're created in, not the context they run in. That's why the call to vm.runInThisContext() returns 1 - the vm object and the runInThisContext function belong to the main context, the one where Object.x = 1.