From 2b7fe69e23d2f023383bf5114b0d72a3165cca79 Mon Sep 17 00:00:00 2001 From: "Anna M. Kedzierska" Date: Sun, 28 May 2017 00:39:33 +0100 Subject: [PATCH] vm: remove CopyProperties() for named properties Extend NamedPropertyHandlerConfiguration with PropertyDescriptorCallback and PropertyDefinerCallback, which query ES6 property descriptors and use DefineProperty() API. CopyProperties() is removed. A number of tests can be moved from /known_issues to /parallel. This is a code review PR: - it uses V8 API changes, which are yet to land, - it temporarily breaks property defining for indexed properties. --- deps/v8/include/v8.h | 17 +- deps/v8/src/api.cc | 10 +- deps/v8/src/builtins/builtins-object.cc | 5 +- deps/v8/src/objects.cc | 76 ++++--- deps/v8/src/objects.h | 28 +-- deps/v8/test/cctest/test-api-interceptors.cc | 41 ++++ src/node_contextify.cc | 202 +++++++++--------- ...t-vm-attributes-property-not-on-sandbox.js | 25 --- .../test-vm-indexed-properties.js | 0 .../test-vm-attributes-property-on-sandbox.js | 17 ++ .../test-vm-data-property-writable.js | 0 .../test-vm-getters.js | 0 .../test-vm-global-define-property.js | 2 +- .../test-vm-global-non-writable-properties.js | 2 +- .../test-vm-proxies-without-CP.js} | 2 +- 15 files changed, 244 insertions(+), 183 deletions(-) delete mode 100644 test/known_issues/test-vm-attributes-property-not-on-sandbox.js rename test/{parallel => known_issues}/test-vm-indexed-properties.js (100%) create mode 100644 test/parallel/test-vm-attributes-property-on-sandbox.js rename test/{known_issues => parallel}/test-vm-data-property-writable.js (100%) rename test/{known_issues => parallel}/test-vm-getters.js (100%) rename test/{known_issues => parallel}/test-vm-global-non-writable-properties.js (89%) rename test/{known_issues/test-vm-proxy-failure-CP.js => parallel/test-vm-proxies-without-CP.js} (86%) diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index eaac6db0a17dad..04c8cb3c859418 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -3033,6 +3033,14 @@ enum class IndexFilter { kIncludeIndices, kSkipIndices }; */ enum class IntegrityLevel { kFrozen, kSealed }; +/** + * DONT_SKIP_INTERCEPTORS is the default behaviour and triggers the definer + * interceptor when setting an own property on an object using + * DefineProperty, while SKIP_INTERCEPTORS bypasses the interceptors. + */ +enum CallInterceptors { DONT_SKIP_INTERCEPTORS, SKIP_INTERCEPTORS }; + + /** * A JavaScript object (ECMA-262, 4.3.3) */ @@ -3070,7 +3078,8 @@ class V8_EXPORT Object : public Value { // Returns true on success. V8_WARN_UNUSED_RESULT Maybe DefineOwnProperty( Local context, Local key, Local value, - PropertyAttribute attributes = None); + PropertyAttribute attributes = None, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); // Implements Object.DefineProperty(O, P, Attributes), see Ecma-262 19.1.2.4. // @@ -3084,9 +3093,13 @@ class V8_EXPORT Object : public Value { // // The PropertyDescriptor can change when redefining a property. // + // CallInterceptors call_interceptors = SKIP_INTERCEPTORS + // bypasses interceptors when setting property on an object. + // // Returns true on success. V8_WARN_UNUSED_RESULT Maybe DefineProperty( - Local context, Local key, PropertyDescriptor& descriptor); + Local context, Local key, PropertyDescriptor& descriptor, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); // Sets an own property on this object bypassing interceptors and // overriding accessors or read-only properties. diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index 671960a0b72924..aa0741eccb8983 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -4436,7 +4436,8 @@ bool v8::PropertyDescriptor::has_configurable() const { Maybe v8::Object::DefineOwnProperty(v8::Local context, v8::Local key, v8::Local value, - v8::PropertyAttribute attributes) { + v8::PropertyAttribute attributes, + CallInterceptors call_interceptors) { auto isolate = reinterpret_cast(context->GetIsolate()); ENTER_V8(isolate, context, Object, DefineOwnProperty, Nothing(), i::HandleScope); @@ -4450,7 +4451,7 @@ Maybe v8::Object::DefineOwnProperty(v8::Local context, desc.set_configurable(!(attributes & v8::DontDelete)); desc.set_value(value_obj); Maybe success = i::JSReceiver::DefineOwnProperty( - isolate, self, key_obj, &desc, i::Object::DONT_THROW); + isolate, self, key_obj, &desc, i::Object::DONT_THROW, call_interceptors); // Even though we said DONT_THROW, there might be accessors that do throw. RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool); return success; @@ -4458,7 +4459,8 @@ Maybe v8::Object::DefineOwnProperty(v8::Local context, Maybe v8::Object::DefineProperty(v8::Local context, v8::Local key, - PropertyDescriptor& descriptor) { + PropertyDescriptor& descriptor, + CallInterceptors call_interceptors) { auto isolate = reinterpret_cast(context->GetIsolate()); ENTER_V8(isolate, context, Object, DefineOwnProperty, Nothing(), i::HandleScope); @@ -4467,7 +4469,7 @@ Maybe v8::Object::DefineProperty(v8::Local context, Maybe success = i::JSReceiver::DefineOwnProperty( isolate, self, key_obj, &descriptor.get_private()->desc, - i::Object::DONT_THROW); + i::Object::DONT_THROW, call_interceptors); RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool); return success; } diff --git a/deps/v8/src/builtins/builtins-object.cc b/deps/v8/src/builtins/builtins-object.cc index 95d2149f31a21e..00ba9dfc0a107d 100644 --- a/deps/v8/src/builtins/builtins-object.cc +++ b/deps/v8/src/builtins/builtins-object.cc @@ -117,9 +117,8 @@ Object* ObjectDefineAccessor(Isolate* isolate, Handle object, // To preserve legacy behavior, we ignore errors silently rather than // throwing an exception. Maybe success = JSReceiver::DefineOwnProperty( - isolate, receiver, name, &desc, - FLAG_harmony_strict_legacy_accessor_builtins ? Object::THROW_ON_ERROR - : Object::DONT_THROW); + isolate, receiver, name, &desc, Object::DONT_THROW, + DONT_SKIP_INTERCEPTORS); MAYBE_RETURN(success, isolate->heap()->exception()); if (!success.FromJust()) { isolate->CountUsage(v8::Isolate::kDefineGetterOrSetterWouldThrow); diff --git a/deps/v8/src/objects.cc b/deps/v8/src/objects.cc index 6a6d265f2c0eb9..5a4613632c44d5 100644 --- a/deps/v8/src/objects.cc +++ b/deps/v8/src/objects.cc @@ -6445,7 +6445,8 @@ Maybe JSReceiver::DeletePropertyOrElement(Handle object, // static Object* JSReceiver::DefineProperty(Isolate* isolate, Handle object, Handle key, - Handle attributes) { + Handle attributes, + CallInterceptors call_interceptors) { // 1. If Type(O) is not Object, throw a TypeError exception. if (!object->IsJSReceiver()) { Handle fun_name = @@ -6464,7 +6465,8 @@ Object* JSReceiver::DefineProperty(Isolate* isolate, Handle object, } // 6. Let success be DefinePropertyOrThrow(O,key, desc). Maybe success = DefineOwnProperty( - isolate, Handle::cast(object), key, &desc, THROW_ON_ERROR); + isolate, Handle::cast(object), key, &desc, THROW_ON_ERROR, + call_interceptors); // 7. ReturnIfAbrupt(success). MAYBE_RETURN(success, isolate->heap()->exception()); CHECK(success.FromJust()); @@ -6556,18 +6558,20 @@ Maybe JSReceiver::DefineOwnProperty(Isolate* isolate, Handle object, Handle key, PropertyDescriptor* desc, - ShouldThrow should_throw) { + ShouldThrow should_throw, + CallInterceptors call_interceptors) { if (object->IsJSArray()) { return JSArray::DefineOwnProperty(isolate, Handle::cast(object), - key, desc, should_throw); + key, desc, should_throw, call_interceptors); } if (object->IsJSProxy()) { return JSProxy::DefineOwnProperty(isolate, Handle::cast(object), - key, desc, should_throw); + key, desc, should_throw, call_interceptors); } if (object->IsJSTypedArray()) { return JSTypedArray::DefineOwnProperty( - isolate, Handle::cast(object), key, desc, should_throw); + isolate, Handle::cast(object), key, desc, should_throw, + call_interceptors); } // TODO(neis): Special case for JSModuleNamespace? @@ -6575,20 +6579,28 @@ Maybe JSReceiver::DefineOwnProperty(Isolate* isolate, // DefineOwnPropertyIgnoreAttributes, can handle arguments // (ES#sec-arguments-exotic-objects-defineownproperty-p-desc). return OrdinaryDefineOwnProperty(isolate, Handle::cast(object), key, - desc, should_throw); + desc, should_throw, call_interceptors); } // static Maybe JSReceiver::OrdinaryDefineOwnProperty(Isolate* isolate, - Handle object, - Handle key, - PropertyDescriptor* desc, - ShouldThrow should_throw) { + Handle object, + Handle key, + PropertyDescriptor* desc, + ShouldThrow should_throw, + CallInterceptors call_interceptors) { bool success = false; DCHECK(key->IsName() || key->IsNumber()); // |key| is a PropertyKey... + + LookupIterator::Configuration iterator_config = LookupIterator::OWN; + if (call_interceptors == CallInterceptors::SKIP_INTERCEPTORS) { + iterator_config = LookupIterator::OWN_SKIP_INTERCEPTOR; + } + LookupIterator it = LookupIterator::PropertyOrElement( - isolate, object, key, &success, LookupIterator::OWN); + isolate, object, key, &success, iterator_config); + DCHECK(success); // ...so creating a LookupIterator can't fail. // Deal with access checks first. @@ -6985,13 +6997,14 @@ bool PropertyKeyToArrayIndex(Handle index_obj, uint32_t* output) { Maybe JSArray::DefineOwnProperty(Isolate* isolate, Handle o, Handle name, PropertyDescriptor* desc, - ShouldThrow should_throw) { + ShouldThrow should_throw, + CallInterceptors call_interceptors) { // 1. Assert: IsPropertyKey(P) is true. ("P" is |name|.) // 2. If P is "length", then: // TODO(jkummerow): Check if we need slow string comparison. if (*name == isolate->heap()->length_string()) { // 2a. Return ArraySetLength(A, Desc). - return ArraySetLength(isolate, o, desc, should_throw); + return ArraySetLength(isolate, o, desc, should_throw, call_interceptors); } // 3. Else if P is an array index, then: uint32_t index = 0; @@ -7018,7 +7031,8 @@ Maybe JSArray::DefineOwnProperty(Isolate* isolate, Handle o, } // 3g. Let succeeded be OrdinaryDefineOwnProperty(A, P, Desc). Maybe succeeded = - OrdinaryDefineOwnProperty(isolate, o, name, desc, should_throw); + OrdinaryDefineOwnProperty(isolate, o, name, desc, should_throw, + call_interceptors); // 3h. Assert: succeeded is not an abrupt completion. // In our case, if should_throw == THROW_ON_ERROR, it can be! // 3i. If succeeded is false, return false. @@ -7031,7 +7045,7 @@ Maybe JSArray::DefineOwnProperty(Isolate* isolate, Handle o, // OrdinaryDefineOwnProperty(A, "length", oldLenDesc). succeeded = OrdinaryDefineOwnProperty(isolate, o, isolate->factory()->length_string(), - &old_len_desc, should_throw); + &old_len_desc, should_throw, call_interceptors); // 3j iii. Assert: succeeded is true. DCHECK(succeeded.FromJust()); USE(succeeded); @@ -7041,7 +7055,8 @@ Maybe JSArray::DefineOwnProperty(Isolate* isolate, Handle o, } // 4. Return OrdinaryDefineOwnProperty(A, P, Desc). - return OrdinaryDefineOwnProperty(isolate, o, name, desc, should_throw); + return OrdinaryDefineOwnProperty(isolate, o, name, desc, should_throw, + call_interceptors); } @@ -7086,12 +7101,13 @@ bool JSArray::AnythingToArrayLength(Isolate* isolate, // static Maybe JSArray::ArraySetLength(Isolate* isolate, Handle a, PropertyDescriptor* desc, - ShouldThrow should_throw) { + ShouldThrow should_throw, + CallInterceptors call_interceptors) { // 1. If the [[Value]] field of Desc is absent, then if (!desc->has_value()) { // 1a. Return OrdinaryDefineOwnProperty(A, "length", Desc). return OrdinaryDefineOwnProperty( - isolate, a, isolate->factory()->length_string(), desc, should_throw); + isolate, a, isolate->factory()->length_string(), desc, should_throw, call_interceptors); } // 2. Let newLenDesc be a copy of Desc. // (Actual copying is not necessary.) @@ -7121,7 +7137,8 @@ Maybe JSArray::ArraySetLength(Isolate* isolate, Handle a, new_len_desc->set_value(isolate->factory()->NewNumberFromUint(new_len)); return OrdinaryDefineOwnProperty(isolate, a, isolate->factory()->length_string(), - new_len_desc, should_throw); + new_len_desc, should_throw, + call_interceptors); } // 13. If oldLenDesc.[[Writable]] is false, return false. if (!old_len_desc.writable()) { @@ -7150,7 +7167,7 @@ Maybe JSArray::ArraySetLength(Isolate* isolate, Handle a, readonly.set_writable(false); Maybe success = OrdinaryDefineOwnProperty( isolate, a, isolate->factory()->length_string(), &readonly, - should_throw); + should_throw, call_interceptors); DCHECK(success.FromJust()); USE(success); } @@ -7174,7 +7191,8 @@ Maybe JSArray::ArraySetLength(Isolate* isolate, Handle a, Maybe JSProxy::DefineOwnProperty(Isolate* isolate, Handle proxy, Handle key, PropertyDescriptor* desc, - ShouldThrow should_throw) { + ShouldThrow should_throw, + CallInterceptors call_interceptors) { STACK_CHECK(isolate, Nothing()); if (key->IsSymbol() && Handle::cast(key)->IsPrivate()) { return SetPrivateProperty(isolate, proxy, Handle::cast(key), desc, @@ -7204,7 +7222,7 @@ Maybe JSProxy::DefineOwnProperty(Isolate* isolate, Handle proxy, if (trap->IsUndefined(isolate)) { // 7a. Return target.[[DefineOwnProperty]](P, Desc). return JSReceiver::DefineOwnProperty(isolate, target, key, desc, - should_throw); + should_throw, call_interceptors); } // 8. Let descObj be FromPropertyDescriptor(Desc). Handle desc_obj = desc->ToObject(isolate); @@ -16728,10 +16746,11 @@ bool CanonicalNumericIndexString(Isolate* isolate, Handle s, // ES#sec-integer-indexed-exotic-objects-defineownproperty-p-desc // static Maybe JSTypedArray::DefineOwnProperty(Isolate* isolate, - Handle o, - Handle key, - PropertyDescriptor* desc, - ShouldThrow should_throw) { + Handle o, + Handle key, + PropertyDescriptor* desc, + ShouldThrow should_throw, + CallInterceptors call_interceptors) { // 1. Assert: IsPropertyKey(P) is true. DCHECK(key->IsName() || key->IsNumber()); // 2. Assert: O is an Object that has a [[ViewedArrayBuffer]] internal slot. @@ -16792,7 +16811,8 @@ Maybe JSTypedArray::DefineOwnProperty(Isolate* isolate, } } // 4. Return ! OrdinaryDefineOwnProperty(O, P, Desc). - return OrdinaryDefineOwnProperty(isolate, o, key, desc, should_throw); + return OrdinaryDefineOwnProperty(isolate, o, key, desc, should_throw, + call_interceptors); } ExternalArrayType JSTypedArray::type() { diff --git a/deps/v8/src/objects.h b/deps/v8/src/objects.h index ba02ebeefedaef..2eee4b767bc5cb 100644 --- a/deps/v8/src/objects.h +++ b/deps/v8/src/objects.h @@ -2038,17 +2038,18 @@ class JSReceiver: public HeapObject { Handle object, uint32_t index, LanguageMode language_mode = SLOPPY); - MUST_USE_RESULT static Object* DefineProperty(Isolate* isolate, - Handle object, - Handle name, - Handle attributes); + MUST_USE_RESULT static Object* DefineProperty( + Isolate* isolate, Handle object, Handle name, + Handle attributes, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); MUST_USE_RESULT static MaybeHandle DefineProperties( Isolate* isolate, Handle object, Handle properties); // "virtual" dispatcher to the correct [[DefineOwnProperty]] implementation. MUST_USE_RESULT static Maybe DefineOwnProperty( Isolate* isolate, Handle object, Handle key, - PropertyDescriptor* desc, ShouldThrow should_throw); + PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); // ES6 7.3.4 (when passed DONT_THROW) MUST_USE_RESULT static Maybe CreateDataProperty( @@ -2057,7 +2058,8 @@ class JSReceiver: public HeapObject { // ES6 9.1.6.1 MUST_USE_RESULT static Maybe OrdinaryDefineOwnProperty( Isolate* isolate, Handle object, Handle key, - PropertyDescriptor* desc, ShouldThrow should_throw); + PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); MUST_USE_RESULT static Maybe OrdinaryDefineOwnProperty( LookupIterator* it, PropertyDescriptor* desc, ShouldThrow should_throw); // ES6 9.1.6.2 @@ -6272,7 +6274,8 @@ class JSProxy: public JSReceiver { // ES6 9.5.6 MUST_USE_RESULT static Maybe DefineOwnProperty( Isolate* isolate, Handle object, Handle key, - PropertyDescriptor* desc, ShouldThrow should_throw); + PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); // ES6 9.5.7 MUST_USE_RESULT static Maybe HasProperty(Isolate* isolate, @@ -6686,7 +6689,8 @@ class JSTypedArray: public JSArrayBufferView { // ES6 9.4.5.3 MUST_USE_RESULT static Maybe DefineOwnProperty( Isolate* isolate, Handle o, Handle key, - PropertyDescriptor* desc, ShouldThrow should_throw); + PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); DECL_CAST(JSTypedArray) @@ -6816,15 +6820,15 @@ class JSArray: public JSObject { // ES6 9.4.2.1 MUST_USE_RESULT static Maybe DefineOwnProperty( Isolate* isolate, Handle o, Handle name, - PropertyDescriptor* desc, ShouldThrow should_throw); + PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); static bool AnythingToArrayLength(Isolate* isolate, Handle length_object, uint32_t* output); MUST_USE_RESULT static Maybe ArraySetLength(Isolate* isolate, - Handle a, - PropertyDescriptor* desc, - ShouldThrow should_throw); + Handle a, PropertyDescriptor* desc, ShouldThrow should_throw, + CallInterceptors call_interceptors = DONT_SKIP_INTERCEPTORS); // Checks whether the Array has the current realm's Array.prototype as its // prototype. This function is best-effort and only gives a conservative diff --git a/deps/v8/test/cctest/test-api-interceptors.cc b/deps/v8/test/cctest/test-api-interceptors.cc index f81d48eb58ed4d..edad3b4f074440 100644 --- a/deps/v8/test/cctest/test-api-interceptors.cc +++ b/deps/v8/test/cctest/test-api-interceptors.cc @@ -1608,6 +1608,8 @@ THREADED_TEST(NamedPropertyHandlerGetter) { } namespace { +int definer_was_called; + void NotInterceptingPropertyDefineCallback( Local name, const v8::PropertyDescriptor& desc, const v8::PropertyCallbackInfo& info) { @@ -1618,6 +1620,7 @@ void InterceptingPropertyDefineCallback( Local name, const v8::PropertyDescriptor& desc, const v8::PropertyCallbackInfo& info) { // Intercept the callback by setting a non-empty handle + definer_was_called++; info.GetReturnValue().Set(name); } @@ -1851,6 +1854,44 @@ THREADED_TEST(PropertyDefinerCallbackForFreeze) { .FromJust()); } +// Check that the define interceptor is not triggered for DefineProperty +// with the SKIP_INTERCEPTORS flag. +THREADED_TEST(DefinePropertyWithoutInterceptors) { + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope scope(isolate); + v8::Local templ = ObjectTemplate::New(isolate); + + templ->SetHandler(v8::NamedPropertyHandlerConfiguration( + nullptr, nullptr, nullptr, nullptr, nullptr, + InterceptingPropertyDefineCallback)); + + LocalContext context; + v8::Local p; + + v8::Local obj = + templ->NewInstance(context.local()).ToLocalChecked(); + + definer_was_called = 0; + + // Use a generic descriptor. + v8::PropertyDescriptor desc_generic; + + p = v8_str("foo"); + + // DONT_SKIP_INTERCEPTORS (default) triggers the interceptors when setting + // own property of an object. + CHECK(obj->DefineProperty(context.local(), p, desc_generic).FromJust()); + CHECK_EQ(1, definer_was_called); + + definer_was_called = 0; + + // SKIP_INTERCEPTORS does not trigger the interceptors. + CHECK(obj->DefineProperty(context.local(), p, desc_generic, + v8::SKIP_INTERCEPTORS) + .FromJust()); + CHECK_EQ(0, definer_was_called); +} + // Check that the descriptor passed to the callback is enumerable. namespace { void CheckEnumerablePropertyDefineCallback( diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c73db420f18b4e..02bfe7102dae90 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -111,93 +111,6 @@ class ContextifyContext { return Local::Cast(context()->GetEmbedderData(kSandboxObjectIndex)); } - // XXX(isaacs): This function only exists because of a shortcoming of - // the V8 SetNamedPropertyHandler function. - // - // It does not provide a way to intercept Object.defineProperty(..) - // calls. As a result, these properties are not copied onto the - // contextified sandbox when a new global property is added via either - // a function declaration or a Object.defineProperty(global, ...) call. - // - // Note that any function declarations or Object.defineProperty() - // globals that are created asynchronously (in a setTimeout, callback, - // etc.) will happen AFTER the call to copy properties, and thus not be - // caught. - // - // The way to properly fix this is to add some sort of a - // Object::SetNamedDefinePropertyHandler() function that takes a callback, - // which receives the property name and property descriptor as arguments. - // - // Luckily, such situations are rare, and asynchronously-added globals - // weren't supported by Node's VM module until 0.12 anyway. But, this - // should be fixed properly in V8, and this copy function should be - // removed once there is a better way. - void CopyProperties() { - HandleScope scope(env()->isolate()); - - Local context = PersistentToLocal(env()->isolate(), context_); - Local global = - context->Global()->GetPrototype()->ToObject(env()->isolate()); - Local sandbox_obj = sandbox(); - - Local clone_property_method; - - Local names = global->GetOwnPropertyNames(); - int length = names->Length(); - for (int i = 0; i < length; i++) { - Local key = names->Get(i)->ToString(env()->isolate()); - Maybe has = sandbox_obj->HasOwnProperty(context, key); - - // Check for pending exceptions - if (has.IsNothing()) - return; - - if (!has.FromJust()) { - Local desc_vm_context = - global->GetOwnPropertyDescriptor(context, key) - .ToLocalChecked().As(); - - bool is_accessor = - desc_vm_context->Has(context, env()->get_string()).FromJust() || - desc_vm_context->Has(context, env()->set_string()).FromJust(); - - auto define_property_on_sandbox = [&] (PropertyDescriptor* desc) { - desc->set_configurable(desc_vm_context - ->Get(context, env()->configurable_string()).ToLocalChecked() - ->BooleanValue(context).FromJust()); - desc->set_enumerable(desc_vm_context - ->Get(context, env()->enumerable_string()).ToLocalChecked() - ->BooleanValue(context).FromJust()); - CHECK(sandbox_obj->DefineProperty(context, key, *desc).FromJust()); - }; - - if (is_accessor) { - Local get = - desc_vm_context->Get(context, env()->get_string()) - .ToLocalChecked().As(); - Local set = - desc_vm_context->Get(context, env()->set_string()) - .ToLocalChecked().As(); - - PropertyDescriptor desc(get, set); - define_property_on_sandbox(&desc); - } else { - Local value = - desc_vm_context->Get(context, env()->value_string()) - .ToLocalChecked(); - - bool writable = - desc_vm_context->Get(context, env()->writable_string()) - .ToLocalChecked()->BooleanValue(context).FromJust(); - - PropertyDescriptor desc(value, writable); - define_property_on_sandbox(&desc); - } - } - } -} - - // This is an object that just keeps an internal pointer to this // ContextifyContext. It's passed to the NamedPropertyHandler. If we // pass the main JavaScript context object we're embedded in, then the @@ -229,9 +142,10 @@ class ContextifyContext { NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback, GlobalPropertySetterCallback, - GlobalPropertyQueryCallback, + GlobalPropertyDescriptorCallback, GlobalPropertyDeleterCallback, GlobalPropertyEnumeratorCallback, + GlobalPropertyDefinerCallback, CreateDataWrapper(env)); object_template->SetHandler(config); @@ -448,29 +362,108 @@ class ContextifyContext { } - static void GlobalPropertyQueryCallback( + static void GlobalPropertyDescriptorCallback( Local property, - const PropertyCallbackInfo& args) { + const PropertyCallbackInfo& info) { ContextifyContext* ctx; - ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); + ASSIGN_OR_RETURN_UNWRAP(&ctx, info.Data().As()); // Still initializing if (ctx->context_.IsEmpty()) return; - Local context = ctx->context(); - Maybe maybe_prop_attr = - ctx->sandbox()->GetRealNamedPropertyAttributes(context, property); + v8::Isolate* isolate = context->GetIsolate(); + HandleScope scope(isolate); + + Local key = property->ToString(isolate); + Local sandbox = ctx->sandbox(); - if (maybe_prop_attr.IsNothing()) { - maybe_prop_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(context, - property); + Maybe has = sandbox->HasOwnProperty(context, key); + Local descriptor_intercepted = has.IsNothing() ? + ctx->global_proxy()->GetOwnPropertyDescriptor(context, key) + .ToLocalChecked() : + sandbox->GetOwnPropertyDescriptor(context, key).ToLocalChecked(); + + // If the property had already been defined on the sandbox or global object, + // we intercept, query and return it. + // Else, there is no need to intercept with first time definitions. + if (!descriptor_intercepted->IsUndefined()) { + info.GetReturnValue().Set(descriptor_intercepted); } + } + + static void GlobalPropertyDefinerCallback( + Local property, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& info) { + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, info.Data().As()); + Local context = ctx->context(); - if (maybe_prop_attr.IsJust()) { - PropertyAttribute prop_attr = maybe_prop_attr.FromJust(); - args.GetReturnValue().Set(prop_attr); + v8::Isolate* isolate = context->GetIsolate(); + HandleScope scope(isolate); + if (ctx->context_.IsEmpty()) + return; + + auto attributes = PropertyAttribute::None; + bool is_declared = + ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), + property) + .To(&attributes); + bool read_only = + static_cast(attributes) & + static_cast(PropertyAttribute::ReadOnly); + + // if it is set on the global as read_only, return + if (is_declared && read_only) + return; + + Local sandbox = ctx->sandbox(); + + auto define_prop_on_sandbox = + [&] (PropertyDescriptor* desc_for_sandbox) { + if (desc.has_enumerable()) { + desc_for_sandbox->set_enumerable(desc.enumerable()); + } + if (desc.has_configurable()) { + desc_for_sandbox->set_configurable(desc.configurable()); + } + // Set the property on the sandbox. + sandbox->DefineProperty(context, property, *desc_for_sandbox); + // Set the property on the global object bypassing interceptors + // by using the v8::SKIP_INTERCEPTORS flag (default is + // v8::DONT_SKIP_INTERCEPTORS). + // With default behaviour, DefineProperty() triggers + // the Definer callback, which in turn calls the Descriptor + // callback to query property descriptor from the sandbox. + // Here, it finds it (we define it above), sees that no changes + // were introduced (property is the same thing as the property + // we just set on the sandbox), assumes it is already set + // and never proceeds to set it on the global object. + ctx->global_proxy()->DefineProperty(context, property, + *desc_for_sandbox, v8::SKIP_INTERCEPTORS); + // Intercept to refrain from re-setting on the global object. + info.GetReturnValue().Set(property); + }; + + if (desc.has_get() || desc.has_set()) { + PropertyDescriptor desc_for_sandbox( + desc.has_get() ? desc.get() : v8::Undefined(isolate).As(), + desc.has_set() ? desc.set() : v8::Undefined(isolate).As()); + + define_prop_on_sandbox(&desc_for_sandbox); + } else { + Local value = + desc.has_value() ? desc.value() : + v8::Undefined(isolate).As(); + + if (desc.has_writable()) { + PropertyDescriptor desc_for_sandbox(value, desc.writable()); + define_prop_on_sandbox(&desc_for_sandbox); + } else { + PropertyDescriptor desc_for_sandbox(value); + define_prop_on_sandbox(&desc_for_sandbox); + } } } @@ -702,15 +695,12 @@ class ContextifyScript : public BaseObject { TryCatch try_catch(env->isolate()); // Do the eval within the context Context::Scope context_scope(contextify_context->context()); - if (EvalMachine(contextify_context->env(), + EvalMachine(contextify_context->env(), timeout, display_errors, break_on_sigint, args, - &try_catch)) { - contextify_context->CopyProperties(); - } - + &try_catch); if (try_catch.HasCaught()) { try_catch.ReThrow(); return; diff --git a/test/known_issues/test-vm-attributes-property-not-on-sandbox.js b/test/known_issues/test-vm-attributes-property-not-on-sandbox.js deleted file mode 100644 index d9534c3d4393a9..00000000000000 --- a/test/known_issues/test-vm-attributes-property-not-on-sandbox.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; -require('../common'); -const assert = require('assert'); -const vm = require('vm'); - -// The QueryCallback explicitly calls GetRealNamedPropertyAttributes -// on the global proxy if the property is not found on the sandbox. -// -// foo is not defined on the sandbox until we call CopyProperties(). -// In the QueryCallback, we do not find the property on the sandbox -// and look up its PropertyAttributes on the global_proxy(). -// PropertyAttributes are always flattened to a value -// descriptor. -const sandbox = {}; -vm.createContext(sandbox); -const code = `Object.defineProperty( - this, - 'foo', - { get: function() {return 17} } - ); - var desc = Object.getOwnPropertyDescriptor(this, 'foo');`; - -vm.runInContext(code, sandbox); -// The descriptor is flattened. We wrongly have typeof desc.value = 'number'. -assert.strictEqual(typeof sandbox.desc.get, 'function'); diff --git a/test/parallel/test-vm-indexed-properties.js b/test/known_issues/test-vm-indexed-properties.js similarity index 100% rename from test/parallel/test-vm-indexed-properties.js rename to test/known_issues/test-vm-indexed-properties.js diff --git a/test/parallel/test-vm-attributes-property-on-sandbox.js b/test/parallel/test-vm-attributes-property-on-sandbox.js new file mode 100644 index 00000000000000..4ff15eaa02a74e --- /dev/null +++ b/test/parallel/test-vm-attributes-property-on-sandbox.js @@ -0,0 +1,17 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const sandbox = {}; +vm.createContext(sandbox); +const code = `Object.defineProperty( + this, + 'foo', + { get: function() {return 17} } + ); + var desc = Object.getOwnPropertyDescriptor(this, 'foo');`; + +vm.runInContext(code, sandbox); +// Accessor properties are not flattened. +assert.strictEqual(typeof sandbox.desc.get, 'function'); diff --git a/test/known_issues/test-vm-data-property-writable.js b/test/parallel/test-vm-data-property-writable.js similarity index 100% rename from test/known_issues/test-vm-data-property-writable.js rename to test/parallel/test-vm-data-property-writable.js diff --git a/test/known_issues/test-vm-getters.js b/test/parallel/test-vm-getters.js similarity index 100% rename from test/known_issues/test-vm-getters.js rename to test/parallel/test-vm-getters.js diff --git a/test/parallel/test-vm-global-define-property.js b/test/parallel/test-vm-global-define-property.js index 30709fccaab453..00bd21052884d8 100644 --- a/test/parallel/test-vm-global-define-property.js +++ b/test/parallel/test-vm-global-define-property.js @@ -44,4 +44,4 @@ assert(res); assert.strictEqual(typeof res, 'object'); assert.strictEqual(res, x); assert.strictEqual(o.f, res); -assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']); +assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']); diff --git a/test/known_issues/test-vm-global-non-writable-properties.js b/test/parallel/test-vm-global-non-writable-properties.js similarity index 89% rename from test/known_issues/test-vm-global-non-writable-properties.js rename to test/parallel/test-vm-global-non-writable-properties.js index 8d7dfce55e3d81..0f5d79596d5893 100644 --- a/test/known_issues/test-vm-global-non-writable-properties.js +++ b/test/parallel/test-vm-global-non-writable-properties.js @@ -7,7 +7,7 @@ const vm = require('vm'); const ctx = vm.createContext(); vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx); -assert.strictEqual(ctx.x, undefined); // Not copied out by cloneProperty(). +assert.strictEqual(ctx.x, 42); assert.strictEqual(vm.runInContext('x', ctx), 42); vm.runInContext('x = 0', ctx); // Does not throw but x... assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered. diff --git a/test/known_issues/test-vm-proxy-failure-CP.js b/test/parallel/test-vm-proxies-without-CP.js similarity index 86% rename from test/known_issues/test-vm-proxy-failure-CP.js rename to test/parallel/test-vm-proxies-without-CP.js index cb636f2ae2bb80..6ca70704aa9ce0 100644 --- a/test/known_issues/test-vm-proxy-failure-CP.js +++ b/test/parallel/test-vm-proxies-without-CP.js @@ -1,6 +1,6 @@ 'use strict'; -// Sandbox throws in CopyProperties() despite no code being run +// Fixed by removing CopyProperties() // Issue: https://github.com/nodejs/node/issues/11902