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

Function redefinition in vm.runInContext #548

Closed
kyriosli opened this issue Jan 22, 2015 · 26 comments
Closed

Function redefinition in vm.runInContext #548

kyriosli opened this issue Jan 22, 2015 · 26 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. repro-exists Issues with reproductions. vm Issues and PRs related to the vm subsystem.

Comments

@kyriosli
Copy link

Possible bug of iojs or perhaps v8?

    var vm = require('vm'), ctx = vm.createContext({});

    vm.runInContext('function test(){return 0}', ctx);
    vm.runInContext('function test(){return 1}', ctx);

    var result = vm.runInContext('test()', ctx);
    // returns 0 in io.js, but 1 in node.js

Tested under win7 x64 and linux.

@Fishrock123
Copy link
Contributor

Confirmed result difference, 0 in iojs and 1 in node (all current versions (0.10.33 & 0.11.15)

@kyriosli
Copy link
Author

It seems that a Context is a proxy object in node.js, but a plain object in io.js

@chrisdickinson
Copy link
Contributor

What version of node? Likely this is due to the switch to contextify internally.

@domenic
Copy link
Contributor

domenic commented Jan 22, 2015

This does seem bad, as the intention here is similar to

<script>
function test() { return 0; }
</script>

<script>
function test() { return 1; }
</script>

<script>
alert(test());
</script>

(which alerts 1).

I wonder if it is due to 3c5ea41

@Fishrock123
Copy link
Contributor

@domenic not directly / not alone, that commit exists in 0.11.15, which also appears to have the bug.

@vkurchatkin vkurchatkin added the vm Issues and PRs related to the vm subsystem. label Jan 23, 2015
@domenic
Copy link
Contributor

domenic commented Jan 27, 2015

@Fishrock123 right, I am wondering what the behavior was before that commit but after the contextify switch.

@piscisaureus
Copy link
Contributor

Possibly related: nodejs/node-v0.x-archive#9084

@isaacs isaacs changed the title Function redefination in vm.runInContext Function redefinition in vm.runInContext Feb 5, 2015
@targos
Copy link
Member

targos commented May 24, 2015

Still returning 0 on v2.1.0.
Could be interesting to test on next branch, maybe with #1773 ?

@Fishrock123
Copy link
Contributor

@domenic is this fixed in next?

@domenic
Copy link
Contributor

domenic commented Jun 5, 2015

Haven't been able to test this with the new patches, but I didn't see any reason why the new patches would necessarily fix it. I still have a list of vm bugs to work on :)

@brendanashworth
Copy link
Contributor

Behavior still occurs in 3.0, marking as a bug!

@brendanashworth brendanashworth added the confirmed-bug Issues with confirmed bugs. label Aug 14, 2015
@Trott
Copy link
Member

Trott commented Oct 13, 2015

Still returns 0 in v4.2.0.

@Trott
Copy link
Member

Trott commented Oct 13, 2015

Not sure if this is helpful or stating the obvious, but as @domenic seems to have suspected, 3c5ea41 is the first commit that exhibits the bug. The immediately previous commit does not exhibit the bug.

@targos
Copy link
Member

targos commented Oct 13, 2015

I think I understand the problem here (but I don't know how to fix it):

  • 3c5ea41 added a CopyProperties method to make sure that properties added on global with Object.defineProperty calls are properly put in the context object. This includes function declarations.
  • To check if property is missing in the context, it uses hasOwnProperty
  • If a property is missing, it is added to the context. => This is the source of the issue. Now that the property is an own property of the context, next calls to CopyProperties won't change it!

You don't need to redefine a function to see the problem. Just put a property with the same name in the context before running the code:

> var vm = require('vm'), ctx = vm.createContext({});
> vm.runInContext('function a(){return 0}', ctx);
> ctx
{ a: [Function: a] }

> var vm = require('vm'), ctx = vm.createContext({a: 1});
> vm.runInContext('function a(){return 0}', ctx);
> ctx
{ a: 1 }

@targos
Copy link
Member

targos commented Oct 13, 2015

Wait, I have an idea for a fix

@targos
Copy link
Member

targos commented Oct 13, 2015

So I can't make it work, probably because I don't really understand V8's API.

Here is the patch I tried:

Index: src/node_contextify.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/node_contextify.cc  (revision 6f14b3a7db93d3d2e2c20b3489dd7ddb7171ee97)
+++ src/node_contextify.cc  (revision )
@@ -138,8 +138,8 @@
     int length = names->Length();
     for (int i = 0; i < length; i++) {
       Local<String> key = names->Get(i)->ToString(env()->isolate());
-      bool has = sandbox->HasOwnProperty(key);
-      if (!has) {
+      bool isEqual = sandbox->Get(key)->SameValue(global->Get(key));
+      if (!isEqual) {
         // Could also do this like so:
         //
         // PropertyAttribute att = global->GetPropertyAttributes(key_v);

For some reason, as soon as the property exists on both sides, isEqual is always true.

cc @bnoordhuis ?

@domenic
Copy link
Contributor

domenic commented Oct 13, 2015

I have not confirmed but my guess as to what's wrong with your patch is that by using ->Get, you are triggering the interceptors which contextify installs. You might want to try replacing one or both of those with GetRealNamedPropertyInPrototypeChain or GetRealNamedProperty.

However, I think the ultimate fix here (for this and other issues) might be to try to copy the Blink architecture a bit better. See https://groups.google.com/forum/#!topic/v8-users/OwcOrXYOpUE which was originally about #2734 but might have broader correctness implications.

@idoby
Copy link

idoby commented Oct 22, 2015

The issue seems to be that GlobalPropertySetterCallback isn't even called when defining a function with the form "function a() {}", but everything works as expected when running "a = function () {}".

Test case:

'use strict';
var common = require('../common');
var assert = require('assert');
var vm = require('vm');

var ctx = vm.createContext({a: 1});
// Uncomment the following line to make test pass
//vm.runInContext('a = function () {}', ctx);
vm.runInContext('function a() {}', ctx);

assert.equal(typeof ctx.a, 'function');

cjihrig added a commit to cjihrig/node that referenced this issue Mar 3, 2016
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.

Refs: nodejs#548
cjihrig added a commit that referenced this issue Mar 4, 2016
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.

Refs: #548
PR-URL: #5528
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repro-exists Issues with reproductions. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests