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

Change from 0.10 => 1.2; enumerable of this.escape in vm.runInNewContext was false, now true #864

Closed
smikes opened this issue Feb 17, 2015 · 8 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@smikes
Copy link
Contributor

smikes commented Feb 17, 2015

Here's the behavior from node 0.10:

$ node -p 'require("vm").runInNewContext("Object.getOwnPropertyDescriptor(this,\"escape\")", {})'
{ value: [Function: escape],
  writable: true,
  enumerable: false,
  configurable: true }

And in io.js@1.2.0:

$ nvm use 1.2
Now using io.js v1.2.0
$ node -p 'require("vm").runInNewContext("Object.getOwnPropertyDescriptor(this,\"escape\")", {})'
{ value: [Function: escape],
  writable: true,
  enumerable: true,
  configurable: true }

Is this an intentional change? Is it documented anywhere? FWIW this is present in node@0.11 as well

@anba
Copy link

anba commented Feb 17, 2015

I guess this is related to https://code.google.com/p/v8/issues/detail?id=3861. GetOwnPropertyNames() is called here.

@smikes
Copy link
Contributor Author

smikes commented Feb 17, 2015

Testing with cloneProperty in the REPL correctly generates an escape with enumerable: false:

> c = require('./cloneProperty')
[Function: cloneProperty]
> s = {}
{}
> c(global, "escape", s)
undefined
> s
{}
> Object.getOwnPropertyDescriptor(s, "escape")
{ value: [Function: escape],
  writable: true,
  enumerable: false,
  configurable: true }

where cloneProperty.js is:

function cloneProperty(source, key, target) {
                if (key === 'Proxy') return;
                try {
                  var desc = Object.getOwnPropertyDescriptor(source, key);
                  if (desc.value === source) desc.value = target;
                  Object.defineProperty(target, key, desc);
                } catch (e) {
                 // Catch sealed properties errors
                }
              }

module.exports = cloneProperty;

@bnoordhuis
Copy link
Member

I suspect that's caused by the named property interceptor for the global object here.

if (!in_sandbox || !in_proxy_global) {
  args.GetReturnValue().Set(None);
}

Not sure what the best way to fix it is. The interceptor needs to retrieve the actual property attributes somehow without causing infinite recursion.

@Fishrock123 Fishrock123 added the vm Issues and PRs related to the vm subsystem. label Feb 17, 2015
smikes added a commit to smikes/io.js that referenced this issue Feb 19, 2015
GlobalPropertyQueryCallback() was returning `None`
as attributes for all properties, without regard
to actual properties settings.

This patch looks up attributes for sandbox-defined
properties and returns them.  Partial fix for nodejsgh-864

Add test for nodejsgh-864, non-default settings of
object properties (e.g., `enumerable: false`) are not
visible when object is used as a sandbox
@smikes
Copy link
Contributor Author

smikes commented Feb 19, 2015

@boordhuis - Indeed, getting attributes for properties of the global is tricky. I have a PR (#885) for the sandbox side, which was easy.

I tried something like this:

    Local<Context> context = PersistentToLocal(isolate, ctx->context_);
    attr = context->Global()->GetPropertyAttributes(property);

But that set off an infinite recursion, as you predicted. Any suggestions?

@bnoordhuis
Copy link
Member

@smikes I commented on that here. It looks like V8 needs to grow some new APIs to fix this issue. I have it on my radar but I'm stretched rather thin at the moment.

@bnoordhuis
Copy link
Member

kisg pushed a commit to paul99/v8mips that referenced this issue Feb 25, 2015
Add v8::Object::GetRealNamedPropertyAttributes() and
v8::Object::GetRealNamedPropertyAttributesInPrototypeChain().

See nodejs/node#864 for background.

Review URL: https://codereview.chromium.org/942003003

Cr-Commit-Position: refs/heads/master@{#26855}
@Fishrock123
Copy link
Contributor

Looks like the v8 patch landed.

domenic added a commit to domenic/io.js that referenced this issue May 22, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of nodejs#885.

Fixes nodejs#885; fixes nodejs#864.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of nodejs#885.

Fixes nodejs#885; fixes nodejs#864.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of nodejs#885.

Fixes nodejs#885; fixes nodejs#864.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of nodejs#885.

Fixes nodejs#885; fixes nodejs#864.
chrisdickinson pushed a commit that referenced this issue Jun 3, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@domenic
Copy link
Contributor

domenic commented Jun 5, 2015

This can be closed as it's fixed in next.

domenic added a commit that referenced this issue Jun 17, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Jul 22, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Jul 24, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Jul 30, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Aug 1, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Aug 3, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Aug 4, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
domenic added a commit that referenced this issue Aug 4, 2015
The GlobalPropertyQueryCallback was changed in 2010 to return an
integer instead of a boolean:

https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU

This integer communicates the property descriptors of the property,
instead of just its presence or absence. However, the original
contextify code was probably written before this change, and it was
not updated when porting to Node.js.

Credit to @smikes for the test and the original PR of #885.

Fixes: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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

No branches or pull requests

5 participants