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

vm: access to Symbols on global context does not work across sandbox boundary #884

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

Comments

@Sebmaster
Copy link
Contributor

This is more a guess than anything, but apparently accessing Symbols in a vm context is not forwarded to the original object handle, resulting in different values depending on which side of the sandbox boundary the access happens.

Reduced test case:

"use strict";

var vm = require("vm");

var symbol = Symbol();

function Document() {
  this[symbol] = "foo";
  this.prop = "bar";
}

Document.prototype.symbol = function () {
  return this[symbol];
};

Document.prototype.property = function () {
  return this.prop;
};

var context = new Document();
vm.createContext(context);

console.log(context.symbol());
console.log(vm.runInContext("this.symbol()", context)); // should be foo, returns undefined

// compare:

console.log(context.property());
console.log(vm.runInContext("this.property()", context)); // should be bar, correct
@domenic
Copy link
Contributor

domenic commented Feb 18, 2015

Might be more global vs. global proxy stuff (#855), or might be because @isaacs's good ol' hack uses the V8 GetOwnPropertyNames API which probably doesn't give back symbols.

@bnoordhuis
Copy link
Member

I think it's a bit of both. v8::Object::GetOwnPropertyNames() indeed doesn't return symbols. That could be scripted around if it weren't for #864 because:

  1. v8::Object::GetOwnPropertyNames is subtly incompatible with Object.getOwnPropertyNames(), see https://code.google.com/p/v8/issues/detail?id=3861
  2. There is no v8::Object::GetOwnPropertySymbols() counterpart to Object.getOwnPropertySymbols(), see https://code.google.com/p/v8/issues/detail?id=3901
  3. Thanks to Change from 0.10 => 1.2; enumerable of this.escape in vm.runInNewContext was false, now true #864, it's not possible to tell built-in properties apart from user-defined ones because everything is enumerable.

Combined, it makes it pretty much impossible to make it work in either C++ or JS. Fixing #864 isn't easy either because you can't look up a property's attributes without going through interceptors (creating infinite recursion) like you can for a property's value.

EDIT: s/#855/#864/ - linked to the wrong issue.

@bnoordhuis
Copy link
Member

On the upside, adding a v8::Object::GetRealNamedPropertyAttributes() method turned out pretty straightforward. When I have some time, I'll try to get it landed upstream.

diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc
index 88d3c88..e16a594 100644
--- a/deps/v8/src/api.cc
+++ b/deps/v8/src/api.cc
@@ -3774,6 +3774,23 @@ Local<Value> v8::Object::GetRealNamedProperty(Handle<String> key) {
 }


+PropertyAttribute v8::Object::GetRealNamedPropertyAttributes(
+    Handle<String> key) {
+  i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
+  ON_BAILOUT(isolate, "v8::Object::GetRealNamedPropertyAttributes()",
+             return static_cast<PropertyAttribute>(NONE));
+  ENTER_V8(isolate);
+  i::Handle<i::JSObject> self_obj = Utils::OpenHandle(this);
+  i::Handle<i::String> key_obj = Utils::OpenHandle(*key);
+  i::LookupIterator it(self_obj, key_obj,
+                       i::LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
+  Maybe<PropertyAttributes> result = self_obj->GetPropertyAttributes(&it);
+  DCHECK(result.has_value);
+  if (result.value == ABSENT) return static_cast<PropertyAttribute>(NONE);
+  return static_cast<PropertyAttribute>(result.value);
+}
+
+
 // Turns on access checks by copying the map and setting the check flag.
 // Because the object gets a new map, existing inline cache caching
 // the old map of this object will fail.

@domenic
Copy link
Contributor

domenic commented May 22, 2015

One thing that might be helpful in fixing this is using ObjectTemplate::SetHandler instead of SetNamedPropertyHandler. The latter calls the former with PropertyHandlerFlags::kOnlyInterceptStrings which sounds like exactly the opposite of what we want. Going to try it soon...

@Sebmaster
Copy link
Contributor Author

Seems like this is a change from how v8-master to how it is handled in the version currently in node. Seems like SetHandler takes a bool in the currently pulled version, indicating whether it takes into account symbols.

@domenic
Copy link
Contributor

domenic commented May 23, 2015

Yeah I am working in the next branch on this.

@domenic
Copy link
Contributor

domenic commented May 23, 2015

This is harder than it seems because to use v8::Name you need to buy in to the MaybeLocal revolution.

domenic added a commit to domenic/io.js that referenced this issue May 23, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now.

Fixes nodejs#884.
domenic added a commit to domenic/io.js that referenced this issue May 23, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now.

Fixes nodejs#884.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now.

Fixes nodejs#884.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now.

Fixes nodejs#884.
domenic added a commit to domenic/io.js that referenced this issue Jun 1, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes nodejs#884.
chrisdickinson pushed a commit that referenced this issue Jun 3, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@bnoordhuis
Copy link
Member

Fixed by 9002cc2.

domenic added a commit that referenced this issue Jun 17, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Jul 22, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Jul 24, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Jul 30, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Aug 1, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Aug 3, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Aug 4, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
domenic added a commit that referenced this issue Aug 4, 2015
By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: #884
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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

4 participants