From 3c1a8f45c31e986a807ab66679bb83de9cf0edf7 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 13:41:43 -0400 Subject: [PATCH 1/4] deps: cherry-pick 9eb96bb from upstream V8 Original commit message: [api] Avoid needlessly calling descriptor interceptors Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515. Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82 Reviewed-on: https://chromium-review.googlesource.com/1138808 Commit-Queue: Timothy Gu Reviewed-by: Jakob Kummerow Reviewed-by: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#54492} Refs: https://github.com/v8/v8/commit/9eb96bb431a52568c4964dee03569eef431a4dfa --- common.gypi | 2 +- deps/v8/src/objects.cc | 14 +++++++++----- deps/v8/test/cctest/test-api-interceptors.cc | 12 ++++++------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/common.gypi b/common.gypi index ec877db9900e0e..35c78b7cf4b4b8 100644 --- a/common.gypi +++ b/common.gypi @@ -29,7 +29,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.16', + 'v8_embedder_string': '-node.17', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/src/objects.cc b/deps/v8/src/objects.cc index 80442d5bd89ba1..5c43a911a9f12a 100644 --- a/deps/v8/src/objects.cc +++ b/deps/v8/src/objects.cc @@ -7662,13 +7662,17 @@ namespace { Maybe GetPropertyDescriptorWithInterceptor(LookupIterator* it, PropertyDescriptor* desc) { - bool has_access = true; if (it->state() == LookupIterator::ACCESS_CHECK) { - has_access = it->HasAccess() || JSObject::AllCanRead(it); - it->Next(); + if (it->HasAccess()) { + it->Next(); + } else if (!JSObject::AllCanRead(it) || + it->state() != LookupIterator::INTERCEPTOR) { + it->Restart(); + return Just(false); + } } - if (has_access && it->state() == LookupIterator::INTERCEPTOR) { + if (it->state() == LookupIterator::INTERCEPTOR) { Isolate* isolate = it->isolate(); Handle interceptor = it->GetInterceptor(); if (!interceptor->descriptor()->IsUndefined(isolate)) { @@ -7700,9 +7704,9 @@ Maybe GetPropertyDescriptorWithInterceptor(LookupIterator* it, return Just(true); } + it->Next(); } } - it->Restart(); return Just(false); } } // namespace diff --git a/deps/v8/test/cctest/test-api-interceptors.cc b/deps/v8/test/cctest/test-api-interceptors.cc index 9a5734a9502070..f1dba643fc08f2 100644 --- a/deps/v8/test/cctest/test-api-interceptors.cc +++ b/deps/v8/test/cctest/test-api-interceptors.cc @@ -4768,7 +4768,7 @@ TEST(NamedAllCanReadInterceptor) { ExpectInt32("checked.whatever", 17); CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')") ->IsUndefined()); - CHECK_EQ(6, access_check_data.count); + CHECK_EQ(5, access_check_data.count); access_check_data.result = false; ExpectInt32("checked.whatever", intercept_data_0.value); @@ -4777,7 +4777,7 @@ TEST(NamedAllCanReadInterceptor) { CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')"); CHECK(try_catch.HasCaught()); } - CHECK_EQ(9, access_check_data.count); + CHECK_EQ(8, access_check_data.count); intercept_data_1.should_intercept = true; ExpectInt32("checked.whatever", intercept_data_1.value); @@ -4786,7 +4786,7 @@ TEST(NamedAllCanReadInterceptor) { CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')"); CHECK(try_catch.HasCaught()); } - CHECK_EQ(12, access_check_data.count); + CHECK_EQ(11, access_check_data.count); g_access_check_data = nullptr; } @@ -4855,7 +4855,7 @@ TEST(IndexedAllCanReadInterceptor) { ExpectInt32("checked[15]", 17); CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, '15')") ->IsUndefined()); - CHECK_EQ(6, access_check_data.count); + CHECK_EQ(5, access_check_data.count); access_check_data.result = false; ExpectInt32("checked[15]", intercept_data_0.value); @@ -4864,7 +4864,7 @@ TEST(IndexedAllCanReadInterceptor) { CompileRun("Object.getOwnPropertyDescriptor(checked, '15')"); CHECK(try_catch.HasCaught()); } - CHECK_EQ(9, access_check_data.count); + CHECK_EQ(8, access_check_data.count); intercept_data_1.should_intercept = true; ExpectInt32("checked[15]", intercept_data_1.value); @@ -4873,7 +4873,7 @@ TEST(IndexedAllCanReadInterceptor) { CompileRun("Object.getOwnPropertyDescriptor(checked, '15')"); CHECK(try_catch.HasCaught()); } - CHECK_EQ(12, access_check_data.count); + CHECK_EQ(11, access_check_data.count); g_access_check_data = nullptr; } From cd035f4ef3f7dd28e668e4beadcd5b3e4fda0eea Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 13:41:51 -0400 Subject: [PATCH 2/4] deps: cherry-pick e1a7699 from upstream V8 Original commit message: [api][runtime] Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration - Explicitly allows construction of {Named,Indexed}PropertyHandlerConfiguration with all the members filled. Bug: v8:7612 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded Reviewed-on: https://chromium-review.googlesource.com/1163882 Reviewed-by: Toon Verwaest Reviewed-by: Adam Klein Commit-Queue: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#55142} Refs: https://github.com/v8/v8/commit/e1a76995ef311eb3ca66e12ef1941ed596034d59 --- common.gypi | 2 +- deps/v8/include/v8.h | 39 ++++ deps/v8/src/api.cc | 4 - deps/v8/src/objects.cc | 63 ++++--- .../unittests/api/interceptor-unittest.cc | 176 ++++++++++++++++++ 5 files changed, 248 insertions(+), 36 deletions(-) diff --git a/common.gypi b/common.gypi index 35c78b7cf4b4b8..f94b4b9f5ca02c 100644 --- a/common.gypi +++ b/common.gypi @@ -29,7 +29,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.17', + 'v8_embedder_string': '-node.18', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index 2bacd1a48097a3..b5bb85ca8a4ba4 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -5878,6 +5878,26 @@ enum class PropertyHandlerFlags { }; struct NamedPropertyHandlerConfiguration { + NamedPropertyHandlerConfiguration( + GenericNamedPropertyGetterCallback getter, + GenericNamedPropertySetterCallback setter, + GenericNamedPropertyQueryCallback query, + GenericNamedPropertyDeleterCallback deleter, + GenericNamedPropertyEnumeratorCallback enumerator, + GenericNamedPropertyDefinerCallback definer, + GenericNamedPropertyDescriptorCallback descriptor, + Local data = Local(), + PropertyHandlerFlags flags = PropertyHandlerFlags::kNone) + : getter(getter), + setter(setter), + query(query), + deleter(deleter), + enumerator(enumerator), + definer(definer), + descriptor(descriptor), + data(data), + flags(flags) {} + NamedPropertyHandlerConfiguration( /** Note: getter is required */ GenericNamedPropertyGetterCallback getter = 0, @@ -5929,6 +5949,25 @@ struct NamedPropertyHandlerConfiguration { struct IndexedPropertyHandlerConfiguration { + IndexedPropertyHandlerConfiguration( + IndexedPropertyGetterCallback getter, + IndexedPropertySetterCallback setter, IndexedPropertyQueryCallback query, + IndexedPropertyDeleterCallback deleter, + IndexedPropertyEnumeratorCallback enumerator, + IndexedPropertyDefinerCallback definer, + IndexedPropertyDescriptorCallback descriptor, + Local data = Local(), + PropertyHandlerFlags flags = PropertyHandlerFlags::kNone) + : getter(getter), + setter(setter), + query(query), + deleter(deleter), + enumerator(enumerator), + definer(definer), + descriptor(descriptor), + data(data), + flags(flags) {} + IndexedPropertyHandlerConfiguration( /** Note: getter is required */ IndexedPropertyGetterCallback getter = 0, diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index 87b6b24270777f..fbd0dbc043be0f 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -1811,10 +1811,6 @@ static i::Handle CreateInterceptorInfo( i::Isolate* isolate, Getter getter, Setter setter, Query query, Descriptor descriptor, Deleter remover, Enumerator enumerator, Definer definer, Local data, PropertyHandlerFlags flags) { - // Either intercept attributes or descriptor. - DCHECK(query == nullptr || descriptor == nullptr); - // Only use descriptor callback with definer callback. - DCHECK(query == nullptr || definer == nullptr); auto obj = i::Handle::cast( isolate->factory()->NewStruct(i::INTERCEPTOR_INFO_TYPE, i::TENURED)); obj->set_flags(0); diff --git a/deps/v8/src/objects.cc b/deps/v8/src/objects.cc index 5c43a911a9f12a..6913e68ed9a5f9 100644 --- a/deps/v8/src/objects.cc +++ b/deps/v8/src/objects.cc @@ -7672,41 +7672,42 @@ Maybe GetPropertyDescriptorWithInterceptor(LookupIterator* it, } } - if (it->state() == LookupIterator::INTERCEPTOR) { - Isolate* isolate = it->isolate(); - Handle interceptor = it->GetInterceptor(); - if (!interceptor->descriptor()->IsUndefined(isolate)) { - Handle result; - Handle holder = it->GetHolder(); + if (it->state() != LookupIterator::INTERCEPTOR) return Just(false); - Handle receiver = it->GetReceiver(); - if (!receiver->IsJSReceiver()) { - ASSIGN_RETURN_ON_EXCEPTION_VALUE( - isolate, receiver, Object::ConvertReceiver(isolate, receiver), - Nothing()); - } + Isolate* isolate = it->isolate(); + Handle interceptor = it->GetInterceptor(); + if (interceptor->descriptor()->IsUndefined(isolate)) return Just(false); - PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); - if (it->IsElement()) { - result = args.CallIndexedDescriptor(interceptor, it->index()); - } else { - result = args.CallNamedDescriptor(interceptor, it->name()); - } - if (!result.is_null()) { - // Request successfully intercepted, try to set the property - // descriptor. - Utils::ApiCheck( - PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc), - it->IsElement() ? "v8::IndexedPropertyDescriptorCallback" - : "v8::NamedPropertyDescriptorCallback", - "Invalid property descriptor."); + Handle result; + Handle holder = it->GetHolder(); - return Just(true); - } - it->Next(); - } + Handle receiver = it->GetReceiver(); + if (!receiver->IsJSReceiver()) { + ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, receiver, + Object::ConvertReceiver(isolate, receiver), + Nothing()); + } + + PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, + *holder, kDontThrow); + if (it->IsElement()) { + result = args.CallIndexedDescriptor(interceptor, it->index()); + } else { + result = args.CallNamedDescriptor(interceptor, it->name()); } + if (!result.is_null()) { + // Request successfully intercepted, try to set the property + // descriptor. + Utils::ApiCheck( + PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc), + it->IsElement() ? "v8::IndexedPropertyDescriptorCallback" + : "v8::NamedPropertyDescriptorCallback", + "Invalid property descriptor."); + + return Just(true); + } + + it->Next(); return Just(false); } } // namespace diff --git a/deps/v8/test/unittests/api/interceptor-unittest.cc b/deps/v8/test/unittests/api/interceptor-unittest.cc index 2f9f0e459e6fed..b13384f18adfef 100644 --- a/deps/v8/test/unittests/api/interceptor-unittest.cc +++ b/deps/v8/test/unittests/api/interceptor-unittest.cc @@ -29,4 +29,180 @@ TEST_F(InterceptorTest, FreezeApiObjectWithInterceptor) { } } // namespace + +namespace internal { +namespace { + +class InterceptorLoggingTest : public TestWithNativeContext { + public: + InterceptorLoggingTest() {} + + static const int kTestIndex = 0; + + static void NamedPropertyGetter(Local name, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named getter"); + } + + static void NamedPropertySetter(Local name, Local value, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named setter"); + } + + static void NamedPropertyQuery( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named query"); + } + + static void NamedPropertyDeleter( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named deleter"); + } + + static void NamedPropertyEnumerator( + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named enumerator"); + } + + static void NamedPropertyDefiner( + Local name, const v8::PropertyDescriptor& desc, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named definer"); + } + + static void NamedPropertyDescriptor( + Local name, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "named descriptor"); + } + + static void IndexedPropertyGetter( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed getter"); + } + + static void IndexedPropertySetter( + uint32_t index, Local value, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed setter"); + } + + static void IndexedPropertyQuery( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed query"); + } + + static void IndexedPropertyDeleter( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed deleter"); + } + + static void IndexedPropertyEnumerator( + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed enumerator"); + } + + static void IndexedPropertyDefiner( + uint32_t index, const v8::PropertyDescriptor& desc, + const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed definer"); + } + + static void IndexedPropertyDescriptor( + uint32_t index, const v8::PropertyCallbackInfo& info) { + LogCallback(info, "indexed descriptor"); + } + + template + static void LogCallback(const v8::PropertyCallbackInfo& info, + const char* callback_name) { + InterceptorLoggingTest* test = reinterpret_cast( + info.This()->GetAlignedPointerFromInternalField(kTestIndex)); + test->Log(callback_name); + } + + void Log(const char* callback_name) { + if (log_is_empty_) { + log_is_empty_ = false; + } else { + log_ << ", "; + } + log_ << callback_name; + } + + protected: + void SetUp() override { + // Set up the object that supports full interceptors. + v8::Local templ = v8::ObjectTemplate::New(v8_isolate()); + templ->SetInternalFieldCount(1); + templ->SetHandler(v8::NamedPropertyHandlerConfiguration( + NamedPropertyGetter, NamedPropertySetter, NamedPropertyQuery, + NamedPropertyDeleter, NamedPropertyEnumerator, NamedPropertyDefiner, + NamedPropertyDescriptor)); + templ->SetHandler(v8::IndexedPropertyHandlerConfiguration( + IndexedPropertyGetter, IndexedPropertySetter, IndexedPropertyQuery, + IndexedPropertyDeleter, IndexedPropertyEnumerator, + IndexedPropertyDefiner, IndexedPropertyDescriptor)); + v8::Local instance = + templ->NewInstance(context()).ToLocalChecked(); + instance->SetAlignedPointerInInternalField(kTestIndex, this); + SetGlobalProperty("obj", instance); + } + + std::string Run(const char* script) { + log_is_empty_ = true; + log_.str(std::string()); + log_.clear(); + + RunJS(script); + return log_.str(); + } + + private: + bool log_is_empty_ = false; + std::stringstream log_; +}; + +TEST_F(InterceptorLoggingTest, DispatchTest) { + EXPECT_EQ(Run("for (var p in obj) {}"), + "indexed enumerator, named enumerator"); + EXPECT_EQ(Run("Object.keys(obj)"), "indexed enumerator, named enumerator"); + + EXPECT_EQ(Run("obj.foo"), "named getter"); + EXPECT_EQ(Run("obj[42]"), "indexed getter"); + + EXPECT_EQ(Run("obj.foo = null"), "named setter"); + EXPECT_EQ(Run("obj[42] = null"), "indexed setter"); + + EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 'foo')"), + "named descriptor"); + + EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 42)"), + "indexed descriptor"); + + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {value: 42})"), + "named descriptor, named definer, named setter"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){} })"), + "named descriptor, named definer"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {set(value){}})"), + "named descriptor, named definer"); + EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){}, set(value){}})"), + "named descriptor, named definer"); + + EXPECT_EQ(Run("Object.defineProperty(obj, 42, {value: 'foo'})"), + "indexed descriptor, " + // then attempt definer first and fallback to setter. + "indexed definer, indexed setter"); + + EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 'a')"), + "named query"); + EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 42)"), + "indexed query"); + + EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, 'a')"), + "named query"); + // TODO(cbruni): Fix once hasOnwProperty is fixed (https://crbug.com/872628) + EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, '42')"), ""); +} +} // namespace +} // namespace internal } // namespace v8 From 07cae574b58f1fef715c1d3d83aac8afecf560bd Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 14:04:05 -0400 Subject: [PATCH 3/4] src: implement query callbacks for vm This allows using a Proxy object as the sandbox for a VM context. Fixes: https://github.com/nodejs/node/issues/17480 Fixes: https://github.com/nodejs/node/issues/17481 --- doc/api/vm.md | 42 ++++++++++++++++++ src/node_contextify.cc | 42 +++++++++++++++++- src/node_contextify.h | 6 +++ test/parallel/test-vm-proxy.js | 80 ++++++++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-vm-proxy.js diff --git a/doc/api/vm.md b/doc/api/vm.md index e2e3eba0c12820..defa029821aa00 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -916,6 +916,48 @@ within which it can operate. The process of creating the V8 Context and associating it with the `sandbox` object is what this document refers to as "contextifying" the `sandbox`. +## vm module and Proxy object + +Leveraging a `Proxy` object as the sandbox of a VM context could result in a +very powerful runtime environment that intercepts all accesses to the global +object. However, there are some restrictions in the JavaScript engine that one +needs to be aware of to prevent unexpected results. In particular, providing a +`Proxy` object with a `get` handler could disallow any access to the original +global properties of the new VM context, as the `get` hook does not distinguish +between the `undefined` value and "requested property is not present" – +the latter of which would ordinarily trigger a lookup on the context global +object. + +Included below is a sample for how to work around this restriction. It +initializes the sandbox as a `Proxy` object without any hooks, only to add them +after the relevant properties have been saved. + +```js +'use strict'; +const { createContext, runInContext } = require('vm'); + +function createProxySandbox(handlers) { + // Create a VM context with a Proxy object with no hooks specified. + const sandbox = {}; + const proxyHandlers = {}; + const contextifiedProxy = createContext(new Proxy(sandbox, proxyHandlers)); + + // Save the initial globals onto our sandbox object. + const contextThis = runInContext('this', contextifiedProxy); + for (const prop of Reflect.ownKeys(contextThis)) { + const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop); + Object.defineProperty(sandbox, prop, descriptor); + } + + // Now that `sandbox` contains all the initial global properties, assign the + // provided handlers to the handlers we used to create the Proxy. + Object.assign(proxyHandlers, handlers); + + // Return the created contextified Proxy object. + return contextifiedProxy; +} +``` + [`Error`]: errors.html#errors_class_error [`URL`]: url.html#url_class_url [`eval()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval diff --git a/src/node_contextify.cc b/src/node_contextify.cc index be4c0e5cf303d7..cde903953e7fed 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -143,19 +143,21 @@ Local ContextifyContext::CreateV8Context( NamedPropertyHandlerConfiguration config(PropertyGetterCallback, PropertySetterCallback, - PropertyDescriptorCallback, + PropertyQueryCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, + PropertyDescriptorCallback, CreateDataWrapper(env)); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyDescriptorCallback, + IndexedPropertyQueryCallback, IndexedPropertyDeleterCallback, PropertyEnumeratorCallback, IndexedPropertyDefinerCallback, + IndexedPropertyDescriptorCallback, CreateDataWrapper(env)); object_template->SetHandler(config); @@ -390,6 +392,28 @@ void ContextifyContext::PropertySetterCallback( ctx->sandbox()->Set(property, value); } +// static +void ContextifyContext::PropertyQueryCallback( + Local property, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx = ContextifyContext::Get(args); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + Local context = ctx->context(); + + Local sandbox = ctx->sandbox(); + + PropertyAttribute attributes; + if (sandbox->HasOwnProperty(context, property).FromMaybe(false) && + sandbox->GetPropertyAttributes(context, property).To(&attributes)) { + args.GetReturnValue().Set(attributes); + } +} + + // static void ContextifyContext::PropertyDescriptorCallback( Local property, @@ -535,6 +559,20 @@ void ContextifyContext::IndexedPropertySetterCallback( Uint32ToName(ctx->context(), index), value, args); } +// static +void ContextifyContext::IndexedPropertyQueryCallback( + uint32_t index, + const PropertyCallbackInfo& args) { + ContextifyContext* ctx = ContextifyContext::Get(args); + + // Still initializing + if (ctx->context_.IsEmpty()) + return; + + ContextifyContext::PropertyQueryCallback( + Uint32ToName(ctx->context(), index), args); +} + // static void ContextifyContext::IndexedPropertyDescriptorCallback( uint32_t index, diff --git a/src/node_contextify.h b/src/node_contextify.h index 3d94fbc5c47947..d094692186167e 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -65,6 +65,9 @@ class ContextifyContext { v8::Local property, v8::Local value, const v8::PropertyCallbackInfo& args); + static void PropertyQueryCallback( + v8::Local property, + const v8::PropertyCallbackInfo& args); static void PropertyDescriptorCallback( v8::Local property, const v8::PropertyCallbackInfo& args); @@ -84,6 +87,9 @@ class ContextifyContext { uint32_t index, v8::Local value, const v8::PropertyCallbackInfo& args); + static void IndexedPropertyQueryCallback( + uint32_t index, + const v8::PropertyCallbackInfo& args); static void IndexedPropertyDescriptorCallback( uint32_t index, const v8::PropertyCallbackInfo& args); diff --git a/test/parallel/test-vm-proxy.js b/test/parallel/test-vm-proxy.js new file mode 100644 index 00000000000000..a85bad67e468a6 --- /dev/null +++ b/test/parallel/test-vm-proxy.js @@ -0,0 +1,80 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const globals = {}; +const handlers = {}; +const proxy = vm.createContext(new Proxy(globals, handlers)); + +// One must get the globals and manually assign it to our own global object, to +// mitigate against https://github.com/nodejs/node/issues/17465. +const globalProxy = vm.runInContext('this', proxy); +for (const k of Reflect.ownKeys(globalProxy)) { + Object.defineProperty(globals, k, Object.getOwnPropertyDescriptor(globalProxy, k)); +} +Reflect.setPrototypeOf(globals, Reflect.getPrototypeOf(globalProxy)); + +// Finally, activate the proxy. +const numCalled = {}; +for (const p of Reflect.ownKeys(Reflect)) { + numCalled[p] = 0; + handlers[p] = (...args) => { + numCalled[p]++; + return Reflect[p](...args); + }; +} + +{ + // Make sure the `in` operator only calls `getOwnPropertyDescriptor` and not + // `get`. + // Refs: https://github.com/nodejs/node/issues/17480 + assert.strictEqual(vm.runInContext('"a" in this', proxy), false); + assert.deepStrictEqual(numCalled, { + defineProperty: 0, + deleteProperty: 0, + apply: 0, + construct: 0, + get: 0, + getOwnPropertyDescriptor: 1, + getPrototypeOf: 0, + has: 0, + isExtensible: 0, + ownKeys: 0, + preventExtensions: 0, + set: 0, + setPrototypeOf: 0 + }); +} + +{ + // Make sure `Object.getOwnPropertyDescriptor` only calls + // `getOwnPropertyDescriptor` and not `get`. + // Refs: https://github.com/nodejs/node/issues/17481 + + // Get and store the function in a lexically scoped variable to avoid + // interfering with the actual test. + vm.runInContext('const { getOwnPropertyDescriptor } = Object;', proxy); + + for (const p of Reflect.ownKeys(numCalled)) { + numCalled[p] = 0; + } + + assert.strictEqual( + vm.runInContext('getOwnPropertyDescriptor(this, "a")', proxy), undefined); + assert.deepStrictEqual(numCalled, { + defineProperty: 0, + deleteProperty: 0, + apply: 0, + construct: 0, + get: 0, + getOwnPropertyDescriptor: 1, + getPrototypeOf: 0, + has: 0, + isExtensible: 0, + ownKeys: 0, + preventExtensions: 0, + set: 0, + setPrototypeOf: 0 + }); +} From 3558bd9f093f976c81582d2dc730a3ee1458441c Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 18 Aug 2018 21:05:13 -0400 Subject: [PATCH 4/4] fixup! src: implement query callbacks for vm Fix linting. --- test/parallel/test-vm-proxy.js | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-vm-proxy.js b/test/parallel/test-vm-proxy.js index a85bad67e468a6..e83ba0dee95aed 100644 --- a/test/parallel/test-vm-proxy.js +++ b/test/parallel/test-vm-proxy.js @@ -3,25 +3,25 @@ require('../common'); const assert = require('assert'); const vm = require('vm'); -const globals = {}; -const handlers = {}; -const proxy = vm.createContext(new Proxy(globals, handlers)); +const sandbox = {}; +const proxyHandlers = {}; +const contextifiedProxy = vm.createContext(new Proxy(sandbox, proxyHandlers)); // One must get the globals and manually assign it to our own global object, to // mitigate against https://github.com/nodejs/node/issues/17465. -const globalProxy = vm.runInContext('this', proxy); -for (const k of Reflect.ownKeys(globalProxy)) { - Object.defineProperty(globals, k, Object.getOwnPropertyDescriptor(globalProxy, k)); +const contextThis = vm.runInContext('this', contextifiedProxy); +for (const prop of Reflect.ownKeys(contextThis)) { + const descriptor = Object.getOwnPropertyDescriptor(contextThis, prop); + Object.defineProperty(sandbox, prop, descriptor); } -Reflect.setPrototypeOf(globals, Reflect.getPrototypeOf(globalProxy)); // Finally, activate the proxy. const numCalled = {}; -for (const p of Reflect.ownKeys(Reflect)) { - numCalled[p] = 0; - handlers[p] = (...args) => { - numCalled[p]++; - return Reflect[p](...args); +for (const hook of Reflect.ownKeys(Reflect)) { + numCalled[hook] = 0; + proxyHandlers[hook] = (...args) => { + numCalled[hook]++; + return Reflect[hook](...args); }; } @@ -29,7 +29,7 @@ for (const p of Reflect.ownKeys(Reflect)) { // Make sure the `in` operator only calls `getOwnPropertyDescriptor` and not // `get`. // Refs: https://github.com/nodejs/node/issues/17480 - assert.strictEqual(vm.runInContext('"a" in this', proxy), false); + assert.strictEqual(vm.runInContext('"a" in this', contextifiedProxy), false); assert.deepStrictEqual(numCalled, { defineProperty: 0, deleteProperty: 0, @@ -54,14 +54,17 @@ for (const p of Reflect.ownKeys(Reflect)) { // Get and store the function in a lexically scoped variable to avoid // interfering with the actual test. - vm.runInContext('const { getOwnPropertyDescriptor } = Object;', proxy); + vm.runInContext( + 'const { getOwnPropertyDescriptor } = Object;', + contextifiedProxy); for (const p of Reflect.ownKeys(numCalled)) { numCalled[p] = 0; } assert.strictEqual( - vm.runInContext('getOwnPropertyDescriptor(this, "a")', proxy), undefined); + vm.runInContext('getOwnPropertyDescriptor(this, "a")', contextifiedProxy), + undefined); assert.deepStrictEqual(numCalled, { defineProperty: 0, deleteProperty: 0,