Skip to content

Proxy passed as vm context hides built-in properties. #6158

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
psrok1 opened this issue Apr 11, 2016 · 3 comments
Closed

Proxy passed as vm context hides built-in properties. #6158

psrok1 opened this issue Apr 11, 2016 · 3 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@psrok1
Copy link

psrok1 commented Apr 11, 2016

Proxy object is not correctly contextified by vm.createContext routine.

I've tried to create vm with traps on global context, so I made one based on Proxy object.
Unfortunately, code executed inside vm doesn't have any access to built-ins.

> vm.runInNewContext("String", {})
[Function: String]
> vm.runInNewContext("String", new Proxy({},{}))
undefined

Passing required built-ins this way:

ctx = new Proxy({String:String}, {})
vm.runInNewContext("String", ctx)

seems to be (a bit dirty) workaround, but it doesn't resolve a problem at all...

vm.runInNewContext("String.prototype.f = function(){}; ''.f()", ctx)
... because passed String differs from used built-in by string literal, so this code raises an exception (''.f is undefined)

Tested on builds from branches node-vee-eight-4.9/5.0
Proxies doesn't work with older V8s and VMs, because of "not an object" bug.

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Apr 11, 2016
@Fishrock123
Copy link
Contributor

cc ... @domenic maybe? @ofrobots any idea?

@ofrobots
Copy link
Contributor

I think the issue is because v8::Object::GetRealNamedProperty in node_contextify.cc ends up behaving differently for Proxy objects. I'm not sure this behaviour is intended. I have opened an issue upstream: https://bugs.chromium.org/p/v8/issues/detail?id=4932.

@ofrobots
Copy link
Contributor

ofrobots commented May 6, 2016

This should be fixed upstream in V8 5.2: https://codereview.chromium.org/1929853002. I will request a merge back to V8 5.0 in upstream.

@targos targos closed this as completed in 4453c0c May 25, 2016
targos added a commit to targos/node that referenced this issue May 26, 2016
A Proxy context should not hide built-in global objects.
Ref: nodejs#6158

PR-URL: nodejs#6967
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue May 30, 2016
Pick up the latest set of patch level updates from the V8 5.0 branch.
v8/v8@5.0.71.47...5.0.71.52

Fixes: nodejs#6158
PR-URL: nodejs#6928
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue May 30, 2016
A Proxy context should not hide built-in global objects.
Ref: nodejs#6158

PR-URL: nodejs#6967
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this issue Jun 2, 2016
Pick up the latest set of patch level updates from the V8 5.0 branch.
v8/v8@5.0.71.47...5.0.71.52

Fixes: #6158
PR-URL: #6928
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this issue Jun 2, 2016
A Proxy context should not hide built-in global objects.
Ref: #6158

PR-URL: #6967
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants