[WIP] src: intercept property descriptors on sandbox #15114

targos · 2017-08-31T09:38:21Z

The main intent of this PR is to fix #2734.
It's not completely ready because one existing test is broken. I'm opening the PR now to get some help.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src,vm

targos · 2017-08-31T09:40:14Z

src/node_contextify.cc

+      if (desc.has_configurable()) {
+        desc_copy.set_configurable(desc.configurable());
+      }
+      Maybe<bool> result = ctx->sandbox()->DefineProperty(context, property, desc_copy);


@fhinkel It feels wrong to me having to make manual copies of the PropertyDescriptor. Am I missing something?

It's more or less correct. You have to know whether you want a value or accessor descriptor. enumerable, configurable, and DefineProp() is the same for both cases though.

@targos Can you refactor the copying process into a new function? I suspect it might come handy in the future, and it'll also be nice to write the DefineProperty bit only once.

I haven't tried playing around with it, but it does seem like one should be able to just pass in desc without making a copy. I'm sure there's an excellent reason why not tho :-)

@jasnell I can't just pass desc because it's a const reference. DefineProperty wants a non-const reference.

sigh... ok yeah, that's the kind of question that is easily answered by reading the docs ;-) That is unfortunate.

targos · 2017-08-31T09:42:01Z

The failing test is here:

node/test/parallel/test-vm-context.js

Lines 118 to 119 in 365c245

    
           assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx), 
        
                         /Cannot assign to read only property 'x'/);

targos · 2017-08-31T09:43:19Z

/cc @TimothyGu @fhinkel @domenic

fhinkel · 2017-08-31T12:15:50Z

I think @AnnaMag already has a PR open for this. Her PR removes the copyProperties() and thus fixes most of the vm issues.

TimothyGu

@AnnaMag's PR: #13265

I suppose it's okay to apply the ES6-aware parts first, before the more complete CopyProperties() removal. However, I think https://codereview.chromium.org/2807333003/ might be a crucial part of getting this fully working even not counting CopyProperties() removal, so I'd wait for @AnnaMag's reply before proceeding.

TimothyGu · 2017-08-31T12:31:22Z

src/node_contextify.cc

-      args.GetReturnValue().Set(prop_attr);
+    if (!maybe_prop_desc.IsEmpty()) {
+      Local<Value> prop_desc = maybe_prop_desc.ToLocalChecked();
+      if (!prop_desc->IsUndefined()) {


Is this check necessary?

When the property does not exist on the sandbox, the property descriptor is undefined. In that case we don't intercept the call (so that Object.getOwnPropertyDescriptor(this, 'Object') can get it from the global object)

TimothyGu · 2017-08-31T12:36:38Z

src/node_contextify.cc

+      if (desc.has_configurable()) {
+        desc_copy.set_configurable(desc.configurable());
+      }
+      Maybe<bool> result = ctx->sandbox()->DefineProperty(context, property, desc_copy);


@targos Can you refactor the copying process into a new function? I suspect it might come handy in the future, and it'll also be nice to write the DefineProperty bit only once.

TimothyGu · 2017-08-31T12:38:21Z

src/node_contextify.cc

+      if (result.IsJust()) {
+        args.GetReturnValue().Set(result.FromJust());
+      }
+    }


else { UNREACHABLE(); }?

targos · 2017-08-31T15:18:42Z

@TimothyGu I don't see how I can refactor the copy to a function. Do you have an idea, knowing that PropertyDescriptor has no copy constructor?

jasnell · 2017-08-31T15:42:07Z

@targos... for that failing test, looking at it now. This is the bit of code that is failing: https://github.com/targos/node/blob/3eceb415fa3049764befd0e3f2149df00228d9f6/src/node_contextify.cc#L431-L432

There are two specific issues here:

The attributes do not appear to be getting set correctly and
When the value is read_only, it simply returns without throwing

I'm still looking at why the first one is happening, but for the second one, to match the expected behavior, the setter should likely throw instead of simply returning.

jasnell · 2017-08-31T16:50:29Z

@targos ... a patch like this gets the test-vm-context.js test working with one small tweak..

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 10bd46b..e40039f 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -414,6 +414,7 @@ class ContextifyContext {
       const PropertyCallbackInfo<Value>& args) {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+    Environment* env = Environment::GetCurrent(args.GetIsolate());

     // Still initializing
     if (ctx->context_.IsEmpty())
@@ -421,15 +422,16 @@ class ContextifyContext {

     auto attributes = PropertyAttribute::None;
     bool is_declared =
-        ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
-                                                            property)
+        ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), property)
         .To(&attributes);
     bool read_only =
         static_cast<int>(attributes) &
         static_cast<int>(PropertyAttribute::ReadOnly);
-
-    if (is_declared && read_only)
+    if (is_declared && read_only) {
+      if (args.ShouldThrowOnError())
+        env->ThrowError("Cannot assign to read only property");
       return;
+    }

     // true for x = 5
     // false for this.x = 5
diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js
index 7e54047..9e770f5 100644
--- a/test/parallel/test-vm-context.js
+++ b/test/parallel/test-vm-context.js
@@ -116,5 +116,5 @@ vm.runInContext('x = 0', ctx);                      // Does not throw but x...
 assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.

 assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
-              /Cannot assign to read only property 'x'/);
+              /Cannot assign to read only property/);
 assert.strictEqual(vm.runInContext('x', ctx), 42);

To make the error message match perfectly, we would need to grab the name of the property (just slightly trickier with Symbol props) and include that in the error message.

targos · 2017-09-01T12:15:58Z

@jasnell thanks. I made that change. I also added a check to the PropertyDefinerCallback for when the property is non-configurable.

targos · 2017-09-01T12:18:05Z

While I was working on this, I found a possibly unexpected behavior with calls like Reflect.ownKeys(this). I added a failing test here: https://github.com/targos/node/blob/8176f21cc70467899c83c2a072569550ddf08b25/test/known_issues/test-vm-ownkeys.js
Am I correct to think that it should work?

Edit: currently, the three tests return ['a']

TimothyGu · 2017-09-01T15:13:06Z

@targos In case you want to go down the route of throwing the error manually, V8 uses the following algorithm:

"Cannot assign to read only property '%s' of %s '%s'",
    property->ToDetailString(), ctx->sandbox()->TypeOf(), ctx->sandbox()->ToDetailString()

Still not a fan though. Also note that certain other errors need to be checked as well, such as an accessor property w/o a setter.

jasnell · 2017-09-01T17:25:57Z

I'll have to look into the Reflect.ownKeys(this) issue further but so far this change LGTM

targos · 2017-09-01T20:31:49Z

So... I did what I could for the error messages with my limited C++ knowledge... Please make suggestions if you know how to improve that :)

@TimothyGu thank you, we're almost there! Compared to the error thrown by V8, only the last string is inconsistent: ctx->sandbox->ToDetailString() returns [object Object]. The V8 error has #<Object>.

addaleax · 2017-09-01T20:35:19Z

src/node_contextify.cc

+inline const char* ToString(Local<Value> value) {
+  String::Utf8Value utf8(value);
+  return *utf8;
+}


Uh – This won’t work. :/ Once utf8 goes out of scope, it also potentially frees the memory used for storing value in its UTF-8 form … I think you need to inline this everywhere (i.e. create distinct Utf8Values for the different values).

Or, alternatively, you could probably do this (warning, haven’t compiled or tried it):

std::ostream& operator<<(std::ostream& os, Local<Value> value) { String::Utf8Value utf8(value); return os << *utf8; }

Thanks. That seems to work.

targos · 2017-09-04T11:52:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9935/

TimothyGu · 2017-09-05T04:10:17Z

@targos

Compared to the error thrown by V8, only the last string is inconsistent: ctx->sandbox->ToDetailString() returns [object Object]. The V8 error has #<Object>.

Hmm. ToDetailString uses the same algorithm as V8 when figuring out what to print... in particular it looks at the toString property to determine whether to use #<X> (for when toString === Object.prototype.toString) or [object X]. I'm frankly not quite sure what's going on here.

Fixes: nodejs#2734 Fixes: nodejs#5350 Fixes: nodejs#5679

targos · 2017-09-05T07:53:06Z

@TimothyGu I thought it could be because the wrong context was used so I tried with the main context and the contextified one but the result is the same :(

targos · 2017-09-05T08:15:28Z

Confirmed this condition is not met:

node/deps/v8/src/objects.cc

Line 361 in a4e923f

} else if (*to_string == *isolate->object_to_string()) {

@nodejs/v8 any idea?

bmeurer · 2017-09-05T08:33:44Z

cc @fhinkel @isheludko

BridgeAR · 2017-09-13T18:42:53Z

Ping @nodejs/v8

psmarshall · 2017-09-18T11:11:48Z

@hashseed Any ideas?

BridgeAR · 2017-09-22T22:10:51Z

I am not sure how we can properly progress further. It seems hard to get any further insight into the issue. @targos is the current status quo of the PR already better than the situation before?

AnnaMag · 2017-10-03T12:16:51Z

My apologies for the delay in my reaction to this.
As @fhinkel mentioned, #13265 provides a fix for this and most other vm issues. The PR was stalled due to https://codereview.chromium.org/2807333003/.
IMHO it is worth waiting for #13265 to land. I'm working on moving things forward.

targos · 2017-10-21T06:22:13Z

Closing in favor of #16293.

Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for nodejs#10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag nodejs#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f Refs: nodejs#6283 Refs: nodejs#15114 Refs: nodejs#13265 Fixes: nodejs#2734 Fixes: nodejs#10223 Fixes: nodejs#11803 Fixes: nodejs#11902

Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for nodejs#10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag nodejs#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f PR-URL: nodejs#16293 Fixes: nodejs#2734 Fixes: nodejs#10223 Fixes: nodejs#11803 Fixes: nodejs#11902 Ref: nodejs#6283 Ref: nodejs#15114 Ref: nodejs#13265 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for nodejs/node#10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag nodejs/node#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f PR-URL: nodejs/node#16293 Fixes: nodejs/node#2734 Fixes: nodejs/node#10223 Fixes: nodejs/node#11803 Fixes: nodejs/node#11902 Ref: nodejs/node#6283 Ref: nodejs/node#15114 Ref: nodejs/node#13265 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Aug 31, 2017

targos commented Aug 31, 2017

View reviewed changes

TimothyGu reviewed Aug 31, 2017

View reviewed changes

targos force-pushed the fix-2734 branch from 70a9565 to a96be8a Compare August 31, 2017 15:41

targos force-pushed the fix-2734 branch from 8176f21 to e9266c3 Compare September 1, 2017 12:53

addaleax reviewed Sep 1, 2017

View reviewed changes

targos force-pushed the fix-2734 branch from 85482cb to 7c65e46 Compare September 4, 2017 11:51

targos added 2 commits September 5, 2017 09:30

src: intercept property descriptors on sandbox

8497889

Fixes: nodejs#2734 Fixes: nodejs#5350 Fixes: nodejs#5679

test: add known issue vm test for Reflect.ownKeys

bd90db9

try ToDetailString

5f2c4fa

targos force-pushed the fix-2734 branch from 7c65e46 to 5f2c4fa Compare September 5, 2017 08:15

fhinkel mentioned this pull request Oct 18, 2017

src: use new V8 API in vm module #16293

Closed

8 tasks

targos closed this Oct 21, 2017

targos deleted the fix-2734 branch October 17, 2020 12:51

[WIP] src: intercept property descriptors on sandbox #15114

[WIP] src: intercept property descriptors on sandbox #15114

Conversation

targos commented Aug 31, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Aug 31, 2017

targos commented Aug 31, 2017

fhinkel commented Aug 31, 2017 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Aug 31, 2017

jasnell commented Aug 31, 2017

jasnell commented Aug 31, 2017 • edited by gibfahn Loading

targos commented Sep 1, 2017

targos commented Sep 1, 2017 • edited Loading

TimothyGu commented Sep 1, 2017

jasnell commented Sep 1, 2017

targos commented Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 4, 2017

TimothyGu commented Sep 5, 2017

targos commented Sep 5, 2017

targos commented Sep 5, 2017 • edited Loading

bmeurer commented Sep 5, 2017

BridgeAR commented Sep 13, 2017

psmarshall commented Sep 18, 2017

BridgeAR commented Sep 22, 2017

AnnaMag commented Oct 3, 2017 • edited Loading

targos commented Oct 21, 2017

fhinkel commented Aug 31, 2017 •

edited

Loading

jasnell commented Aug 31, 2017 •

edited by gibfahn

Loading

targos commented Sep 1, 2017 •

edited

Loading

targos commented Sep 1, 2017 •

edited

Loading

addaleax Sep 1, 2017 •

edited

Loading

targos commented Sep 5, 2017 •

edited

Loading

AnnaMag commented Oct 3, 2017 •

edited

Loading