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

sandbox inheritied properties flattened after vm.runInNewContext #5350

Closed
waynedpj opened this issue Feb 21, 2016 · 12 comments
Closed

sandbox inheritied properties flattened after vm.runInNewContext #5350

waynedpj opened this issue Feb 21, 2016 · 12 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@waynedpj
Copy link

on node-4.5.1 when using a sandbox Object with inherited properties, after running sandbox in vm.runInNewContext all properties up the prototype chain are copied to sandbox Object, even if they are not changed by code, though the prototype chain is still there. here is a failing test case

'use strict';

let vm = require('vm'), assert = require('assert');

let base = Object.create(null);
base.x = 1;
base.y = 2;

let sandbox = Object.create(base);
sandbox.z = 3;

console.log('base:\n%j', base);
console.log('sandbox:\n%j', sandbox);
console.log("sandbox's own keys:\n%j", Object.keys(sandbox));

let code = 'x = 0; z = 4;';
let result = vm.runInNewContext(code, sandbox);
console.log('result: %s', result);

console.log('base:\n%j', base);
console.log('sandbox:\n%j', sandbox);
console.log("sandbox's own keys:\n%j", Object.keys(sandbox));

assert((Object.keys(sandbox)).indexOf('y') === -1);  // fails

note that the same problem occurs even if code is empty, as well as with vm.runInContext.

not sure if this is expected behavior or not, but i was surprised to have sandbox modified like that. if it is normal, i did not find any mention of it in the docs, and would ask if someone could kindly explain why?

thanks.

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 21, 2016
@abenhamdine
Copy link

@waynedpj do you see the same behaviour with the last stable version of nodejs ?
I also see some strange behaviours with the sandbox object properties on node 5.7 (but not exactly the same problem as your), but i'm still investigating before opening an issue.

@waynedpj
Copy link
Author

@abenhamdine yes, i still see the same issue w/ 5.7. i have been investigating this further, and though i am not totally following what is going on yet, i suspect that this flattening of sandbox's prototype chain might be happening here in CopyProperties. but i am not familiar enough yet w/ Node.js's C++ code yet ;)

does your issue involve the sandbox being modified unexpectedly?

please let us know if you find anything and thanks.

@waynedpj
Copy link
Author

PS. @abenhamdine which strange behaviors are you seeing?

@abenhamdine
Copy link

I just opened an issue : #5491

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

Pretty certain this is the same issue as #2734

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

@nodejs/v8 ... can anyone on the v8 site assist with this one?

@jeisinger
Copy link
Contributor

I'll sync with Domenic and Ali to understand the exact requirements here.

fhinkel added a commit to fhinkel/node that referenced this issue Jul 28, 2016
AnnaMag added a commit to AnnaMag/node that referenced this issue Dec 17, 2016
italoacasas pushed a commit that referenced this issue Dec 21, 2016
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
evanlucas pushed a commit that referenced this issue Jan 3, 2017
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
evanlucas pushed a commit that referenced this issue Jan 4, 2017
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 23, 2017
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 24, 2017
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 31, 2017
PR-URL: #10319
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 15, 2017

Should this be closed as a duplicate of #2734?

@Trott Trott marked this as a duplicate of #2734 Jul 15, 2017
@TimothyGu
Copy link
Member

@Trott Sure.

@TimothyGu
Copy link
Member

Actually, this is a different issue, as it describes the sandbox object being changed.

@TimothyGu TimothyGu reopened this Jul 17, 2017
targos added a commit to targos/node that referenced this issue Sep 5, 2017
@fhinkel
Copy link
Member

fhinkel commented Oct 23, 2017

This got fixed in #16293.

@waynedpj
Copy link
Author

thanks!

fhinkel added a commit to fhinkel/node that referenced this issue Oct 25, 2017
The known issue is fixed with
nodejs#16293.

The text needs to call `Object.hasOwnProperty(this)`
instead of `this.hasOwnProperty()`, otherwise `this` is
from the wrong context is used.

Add a second test case taken verbatim from issue
nodejs#5350

Fixes: nodejs#5350
Refs: nodejs#16293
fhinkel added a commit to fhinkel/node that referenced this issue Oct 25, 2017
The known issue is fixed with
nodejs#16293.

The text needs to call `Object.hasOwnProperty(this)`
instead of `this.hasOwnProperty()`, otherwise `this` is
from the wrong context is used.

Add a second test case taken verbatim from issue
nodejs#5350

PR-URL: nodejs#16411
Fixes: nodejs#5350
Ref: nodejs#16293
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
The known issue is fixed with
nodejs/node#16293.

The text needs to call `Object.hasOwnProperty(this)`
instead of `this.hasOwnProperty()`, otherwise `this` is
from the wrong context is used.

Add a second test case taken verbatim from issue
nodejs/node#5350

PR-URL: nodejs/node#16411
Fixes: nodejs/node#5350
Ref: nodejs/node#16293
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
The known issue is fixed with
#16293.

The text needs to call `Object.hasOwnProperty(this)`
instead of `this.hasOwnProperty()`, otherwise `this` is
from the wrong context is used.

Add a second test case taken verbatim from issue
#5350

PR-URL: #16411
Fixes: #5350
Ref: #16293
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
The known issue is fixed with
nodejs/node#16293.

The text needs to call `Object.hasOwnProperty(this)`
instead of `this.hasOwnProperty()`, otherwise `this` is
from the wrong context is used.

Add a second test case taken verbatim from issue
nodejs/node#5350

PR-URL: nodejs/node#16411
Fixes: nodejs/node#5350
Ref: nodejs/node#16293
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants