Skip to content
This repository has been archived by the owner. It is now read-only.

runInThisContext does not run in context of the caller #898

Closed
ghost opened this Issue Apr 11, 2011 · 15 comments

Comments

Projects
None yet
5 participants
@ghost
Copy link

ghost commented Apr 11, 2011

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

This comment has been minimized.

Copy link
Author

ghost commented Apr 11, 2011

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

This comment has been minimized.

Copy link
Author

ghost commented Apr 11, 2011

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

This comment has been minimized.

Copy link
Author

ghost commented Apr 11, 2011

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

This comment has been minimized.

Copy link
Author

ghost commented Apr 23, 2011

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

This comment has been minimized.

Copy link

mmalecki commented Sep 5, 2011

+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

This comment has been minimized.

Copy link

thejh commented Sep 5, 2011

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

@mmalecki

This comment has been minimized.

Copy link

mmalecki commented Sep 5, 2011

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

This comment has been minimized.

Copy link

medikoo commented Sep 6, 2011

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

This comment has been minimized.

Copy link
Author

ghost commented Sep 7, 2011

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

@medikoo

This comment has been minimized.

Copy link

medikoo commented Sep 7, 2011

@Herby thanks for clarification

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 12, 2011

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

This comment has been minimized.

Copy link
Member

bnoordhuis commented Dec 12, 2011

@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

This comment has been minimized.

Copy link
Author

ghost commented Dec 12, 2011

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

This comment has been minimized.

Copy link

pedronasser commented Aug 20, 2013

Should change the name for runInGlobalContext. lol

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Aug 20, 2013

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.

@bnoordhuis bnoordhuis closed this Aug 20, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.