From 2aa67dc02892307ee4d824e72f914796bfbf4ee2 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 20 Aug 2024 09:30:59 +0100 Subject: [PATCH 1/2] Revert "vm,src: add property query interceptors" This reverts commit c0962dc3bfd94409162fcb77a9da20bce98848c7. --- src/node_contextify.cc | 121 +----------------- src/node_contextify.h | 7 - .../test-vm-global-property-enumerator.js | 49 ------- .../test-vm-global-property-prototype.js | 83 ------------ 4 files changed, 5 insertions(+), 255 deletions(-) delete mode 100644 test/parallel/test-vm-global-property-enumerator.js delete mode 100644 test/parallel/test-vm-global-property-prototype.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 895f7b9d096..f3c1cf3bad9 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,13 +50,10 @@ using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::IndexedPropertyHandlerConfiguration; -using v8::IndexFilter; using v8::Int32; -using v8::Integer; using v8::Intercepted; using v8::Isolate; using v8::Just; -using v8::KeyCollectionMode; using v8::Local; using v8::Maybe; using v8::MaybeLocal; @@ -75,7 +72,6 @@ using v8::Promise; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; -using v8::PropertyFilter; using v8::PropertyHandlerFlags; using v8::Script; using v8::ScriptCompiler; @@ -179,22 +175,20 @@ void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) { NamedPropertyHandlerConfiguration config( PropertyGetterCallback, PropertySetterCallback, - PropertyQueryCallback, + PropertyDescriptorCallback, PropertyDeleterCallback, PropertyEnumeratorCallback, PropertyDefinerCallback, - PropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, IndexedPropertySetterCallback, - IndexedPropertyQueryCallback, + IndexedPropertyDescriptorCallback, IndexedPropertyDeleterCallback, - IndexedPropertyEnumeratorCallback, + PropertyEnumeratorCallback, IndexedPropertyDefinerCallback, - IndexedPropertyDescriptorCallback, {}, PropertyHandlerFlags::kHasNoSideEffect); @@ -359,20 +353,17 @@ void ContextifyContext::RegisterExternalReferences( ExternalReferenceRegistry* registry) { registry->Register(MakeContext); registry->Register(CompileFunction); - registry->Register(PropertyQueryCallback); registry->Register(PropertyGetterCallback); registry->Register(PropertySetterCallback); registry->Register(PropertyDescriptorCallback); registry->Register(PropertyDeleterCallback); registry->Register(PropertyEnumeratorCallback); registry->Register(PropertyDefinerCallback); - registry->Register(IndexedPropertyQueryCallback); registry->Register(IndexedPropertyGetterCallback); registry->Register(IndexedPropertySetterCallback); registry->Register(IndexedPropertyDescriptorCallback); registry->Register(IndexedPropertyDeleterCallback); registry->Register(IndexedPropertyDefinerCallback); - registry->Register(IndexedPropertyEnumeratorCallback); } // makeContext(sandbox, name, origin, strings, wasm); @@ -460,51 +451,6 @@ bool ContextifyContext::IsStillInitializing(const ContextifyContext* ctx) { return ctx == nullptr || ctx->context_.IsEmpty(); } -// static -Intercepted ContextifyContext::PropertyQueryCallback( - Local property, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = ContextifyContext::Get(args); - - // Still initializing - if (IsStillInitializing(ctx)) { - return Intercepted::kNo; - } - - Local context = ctx->context(); - Local sandbox = ctx->sandbox(); - - PropertyAttribute attr; - - Maybe maybe_has = sandbox->HasRealNamedProperty(context, property); - if (maybe_has.IsNothing()) { - return Intercepted::kNo; - } else if (maybe_has.FromJust()) { - Maybe maybe_attr = - sandbox->GetRealNamedPropertyAttributes(context, property); - if (!maybe_attr.To(&attr)) { - return Intercepted::kNo; - } - args.GetReturnValue().Set(attr); - return Intercepted::kYes; - } else { - maybe_has = ctx->global_proxy()->HasRealNamedProperty(context, property); - if (maybe_has.IsNothing()) { - return Intercepted::kNo; - } else if (maybe_has.FromJust()) { - Maybe maybe_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(context, - property); - if (!maybe_attr.To(&attr)) { - return Intercepted::kNo; - } - args.GetReturnValue().Set(attr); - return Intercepted::kYes; - } - } - - return Intercepted::kNo; -} - // static Intercepted ContextifyContext::PropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { @@ -748,68 +694,11 @@ void ContextifyContext::PropertyEnumeratorCallback( if (IsStillInitializing(ctx)) return; Local properties; - // Only get named properties, exclude symbols and indices. - if (!ctx->sandbox() - ->GetPropertyNames( - ctx->context(), - KeyCollectionMode::kIncludePrototypes, - static_cast(PropertyFilter::ONLY_ENUMERABLE | - PropertyFilter::SKIP_SYMBOLS), - IndexFilter::kSkipIndices) - .ToLocal(&properties)) - return; - args.GetReturnValue().Set(properties); -} - -// static -void ContextifyContext::IndexedPropertyEnumeratorCallback( - const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - HandleScope scope(isolate); - ContextifyContext* ctx = ContextifyContext::Get(args); - Local context = ctx->context(); - - // Still initializing - if (IsStillInitializing(ctx)) return; - - Local properties; - - // By default, GetPropertyNames returns string and number property names, and - // doesn't convert the numbers to strings. - if (!ctx->sandbox()->GetPropertyNames(context).ToLocal(&properties)) return; - - std::vector> properties_vec; - if (FromV8Array(context, properties, &properties_vec).IsNothing()) { + if (!ctx->sandbox()->GetPropertyNames(ctx->context()).ToLocal(&properties)) return; - } - // Filter out non-number property names. - std::vector> indices; - for (uint32_t i = 0; i < properties->Length(); i++) { - Local prop = properties_vec[i].Get(isolate); - if (!prop->IsNumber()) { - continue; - } - indices.push_back(prop); - } - - args.GetReturnValue().Set( - Array::New(args.GetIsolate(), indices.data(), indices.size())); -} - -// static -Intercepted ContextifyContext::IndexedPropertyQueryCallback( - uint32_t index, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = ContextifyContext::Get(args); - - // Still initializing - if (IsStillInitializing(ctx)) { - return Intercepted::kNo; - } - - return ContextifyContext::PropertyQueryCallback( - Uint32ToName(ctx->context(), index), args); + args.GetReturnValue().Set(properties); } // static diff --git a/src/node_contextify.h b/src/node_contextify.h index b9e846f70ba..023cdc5d3d8 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -94,9 +94,6 @@ class ContextifyContext : public BaseObject { bool produce_cached_data, v8::Local id_symbol, const errors::TryCatchScope& try_catch); - static v8::Intercepted PropertyQueryCallback( - v8::Local property, - const v8::PropertyCallbackInfo& args); static v8::Intercepted PropertyGetterCallback( v8::Local property, const v8::PropertyCallbackInfo& args); @@ -116,8 +113,6 @@ class ContextifyContext : public BaseObject { const v8::PropertyCallbackInfo& args); static void PropertyEnumeratorCallback( const v8::PropertyCallbackInfo& args); - static v8::Intercepted IndexedPropertyQueryCallback( - uint32_t index, const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertyGetterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertySetterCallback( @@ -132,8 +127,6 @@ class ContextifyContext : public BaseObject { const v8::PropertyCallbackInfo& args); static v8::Intercepted IndexedPropertyDeleterCallback( uint32_t index, const v8::PropertyCallbackInfo& args); - static void IndexedPropertyEnumeratorCallback( - const v8::PropertyCallbackInfo& args); v8::Global context_; std::unique_ptr microtask_queue_; diff --git a/test/parallel/test-vm-global-property-enumerator.js b/test/parallel/test-vm-global-property-enumerator.js deleted file mode 100644 index 7b37c2af410..00000000000 --- a/test/parallel/test-vm-global-property-enumerator.js +++ /dev/null @@ -1,49 +0,0 @@ -'use strict'; -require('../common'); -const vm = require('vm'); -const assert = require('assert'); - -// Regression of https://github.com/nodejs/node/issues/53346 - -const cases = [ - { - get key() { - return 'value'; - }, - }, - { - // Intentionally single setter. - // eslint-disable-next-line accessor-pairs - set key(value) {}, - }, - {}, - { - key: 'value', - }, - (new class GetterObject { - get key() { - return 'value'; - } - }()), - (new class SetterObject { - // Intentionally single setter. - // eslint-disable-next-line accessor-pairs - set key(value) { - // noop - } - }()), - [], - [['key', 'value']], - { - __proto__: { - key: 'value', - }, - }, -]; - -for (const [idx, obj] of cases.entries()) { - const ctx = vm.createContext(obj); - const globalObj = vm.runInContext('this', ctx); - const keys = Object.keys(globalObj); - assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`); -} diff --git a/test/parallel/test-vm-global-property-prototype.js b/test/parallel/test-vm-global-property-prototype.js deleted file mode 100644 index fe8abc8be45..00000000000 --- a/test/parallel/test-vm-global-property-prototype.js +++ /dev/null @@ -1,83 +0,0 @@ -'use strict'; -require('../common'); -const assert = require('assert'); -const vm = require('vm'); - -const sandbox = { - onSelf: 'onSelf', -}; - -function onSelfGetter() { - return 'onSelfGetter'; -} - -Object.defineProperty(sandbox, 'onSelfGetter', { - get: onSelfGetter, -}); - -Object.defineProperty(sandbox, 1, { - value: 'onSelfIndexed', - writable: false, - enumerable: false, - configurable: true, -}); - -const ctx = vm.createContext(sandbox); - -const result = vm.runInContext(` -Object.prototype.onProto = 'onProto'; -Object.defineProperty(Object.prototype, 'onProtoGetter', { - get() { - return 'onProtoGetter'; - }, -}); -Object.defineProperty(Object.prototype, 2, { - value: 'onProtoIndexed', - writable: false, - enumerable: false, - configurable: true, -}); - -const resultHasOwn = { - onSelf: Object.hasOwn(this, 'onSelf'), - onSelfGetter: Object.hasOwn(this, 'onSelfGetter'), - onSelfIndexed: Object.hasOwn(this, 1), - onProto: Object.hasOwn(this, 'onProto'), - onProtoGetter: Object.hasOwn(this, 'onProtoGetter'), - onProtoIndexed: Object.hasOwn(this, 2), -}; - -const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop); -const resultDesc = { - onSelf: getDesc('onSelf'), - onSelfGetter: getDesc('onSelfGetter'), - onSelfIndexed: getDesc(1), - onProto: getDesc('onProto'), - onProtoGetter: getDesc('onProtoGetter'), - onProtoIndexed: getDesc(2), -}; -({ - resultHasOwn, - resultDesc, -}); -`, ctx); - -// eslint-disable-next-line no-restricted-properties -assert.deepEqual(result, { - resultHasOwn: { - onSelf: true, - onSelfGetter: true, - onSelfIndexed: true, - onProto: false, - onProtoGetter: false, - onProtoIndexed: false, - }, - resultDesc: { - onSelf: { value: 'onSelf', writable: true, enumerable: true, configurable: true }, - onSelfGetter: { get: onSelfGetter, set: undefined, enumerable: false, configurable: false }, - onSelfIndexed: { value: 'onSelfIndexed', writable: false, enumerable: false, configurable: true }, - onProto: undefined, - onProtoGetter: undefined, - onProtoIndexed: undefined, - }, -}); From 9f2d89357633963a91ccf29a0db30345beb46bec Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 20 Aug 2024 09:42:03 +0100 Subject: [PATCH 2/2] test: add vm proto property lookup test Add regression tests for vm prototype properties lookup. --- .../test-vm-global-property-enumerator.js | 77 ++++++ .../test-vm-global-property-prototype.js | 224 ++++++++++++++++++ 2 files changed, 301 insertions(+) create mode 100644 test/parallel/test-vm-global-property-enumerator.js create mode 100644 test/parallel/test-vm-global-property-prototype.js diff --git a/test/parallel/test-vm-global-property-enumerator.js b/test/parallel/test-vm-global-property-enumerator.js new file mode 100644 index 00000000000..be6b8352c4a --- /dev/null +++ b/test/parallel/test-vm-global-property-enumerator.js @@ -0,0 +1,77 @@ +'use strict'; +require('../common'); +const vm = require('vm'); +const assert = require('assert'); + +// Regression of https://github.com/nodejs/node/issues/53346 + +const cases = [ + { + get 1() { + return 'value'; + }, + get key() { + return 'value'; + }, + }, + { + // Intentionally single setter. + // eslint-disable-next-line accessor-pairs + set key(value) {}, + // eslint-disable-next-line accessor-pairs + set 1(value) {}, + }, + {}, + { + key: 'value', + 1: 'value', + }, + (new class GetterObject { + get key() { + return 'value'; + } + get 1() { + return 'value'; + } + }()), + (new class SetterObject { + // Intentionally single setter. + // eslint-disable-next-line accessor-pairs + set key(value) { + // noop + } + // eslint-disable-next-line accessor-pairs + set 1(value) { + // noop + } + }()), + [], + [['key', 'value']], +]; + +for (const [idx, obj] of cases.entries()) { + const ctx = vm.createContext(obj); + const globalObj = vm.runInContext('this', ctx); + const keys = Object.keys(globalObj); + assert.deepStrictEqual(keys, Object.keys(obj), `Case ${idx} failed`); +} + +const specialCases = [ + [ + // Prototype named and index properties should not be enumerated. + // FIXME(legendecas): https://github.com/nodejs/node/issues/54436 + { + __proto__: { + key: 'value', + 1: 'value', + }, + }, + ['1', 'key'], + ], +]; +for (const [idx, [obj, expectedKeys]] of specialCases.entries()) { + const ctx = vm.createContext(obj); + const globalObj = vm.runInContext('this', ctx); + const keys = Object.keys(globalObj); + assert.deepStrictEqual(keys, expectedKeys, `Special case ${idx} failed`); +} diff --git a/test/parallel/test-vm-global-property-prototype.js b/test/parallel/test-vm-global-property-prototype.js new file mode 100644 index 00000000000..4ffef470c46 --- /dev/null +++ b/test/parallel/test-vm-global-property-prototype.js @@ -0,0 +1,224 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const outerProto = { + onOuterProto: 'onOuterProto', + bothProto: 'onOuterProto', +}; +function onOuterProtoGetter() { + return 'onOuterProtoGetter'; +} +Object.defineProperties(outerProto, { + onOuterProtoGetter: { + get: onOuterProtoGetter, + }, + bothProtoGetter: { + get: onOuterProtoGetter, + }, + 0: { + value: 'onOuterProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, + 3: { + value: 'onOuterProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, +}); + +// Creating a new intermediate proto to mimic the +// window -> Window.prototype -> EventTarget.prototype chain in JSDom. +const sandboxProto = { + __proto__: outerProto, +}; + +const sandbox = { + __proto__: sandboxProto, + onSelf: 'onSelf', +}; + +function onSelfGetter() { + return 'onSelfGetter'; +} +Object.defineProperties(sandbox, { + onSelfGetter: { + get: onSelfGetter, + }, + 1: { + value: 'onSelfIndexed', + writable: false, + enumerable: false, + configurable: true, + } +}); + +const ctx = vm.createContext(sandbox); + +const result = vm.runInContext(` +Object.prototype.onInnerProto = 'onInnerProto'; +Object.defineProperties(Object.prototype, { + onInnerProtoGetter: { + get() { + return 'onInnerProtoGetter'; + }, + }, + 2: { + value: 'onInnerProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, +}); + +// Override outer proto properties +Object.prototype.bothProto = 'onInnerProto'; +Object.defineProperties(Object.prototype, { + bothProtoGetter: { + get() { + return 'onInnerProtoGetter'; + }, + }, + 3: { + value: 'onInnerProtoIndexed', + writable: false, + enumerable: false, + configurable: true, + }, +}); + +const resultHasOwn = { + onSelf: Object.hasOwn(this, 'onSelf'), + onSelfGetter: Object.hasOwn(this, 'onSelfGetter'), + onSelfIndexed: Object.hasOwn(this, 1), + onOuterProto: Object.hasOwn(this, 'onOuterProto'), + onOuterProtoGetter: Object.hasOwn(this, 'onOuterProtoGetter'), + onOuterProtoIndexed: Object.hasOwn(this, 0), + onInnerProto: Object.hasOwn(this, 'onInnerProto'), + onInnerProtoGetter: Object.hasOwn(this, 'onInnerProtoGetter'), + onInnerProtoIndexed: Object.hasOwn(this, 2), + bothProto: Object.hasOwn(this, 'bothProto'), + bothProtoGetter: Object.hasOwn(this, 'bothProtoGetter'), + bothProtoIndexed: Object.hasOwn(this, 3), +}; + +const getDesc = (prop) => Object.getOwnPropertyDescriptor(this, prop); +const resultDesc = { + onSelf: getDesc('onSelf'), + onSelfGetter: getDesc('onSelfGetter'), + onSelfIndexed: getDesc(1), + onOuterProto: getDesc('onOuterProto'), + onOuterProtoGetter: getDesc('onOuterProtoGetter'), + onOuterProtoIndexed: getDesc(0), + onInnerProto: getDesc('onInnerProto'), + onInnerProtoGetter: getDesc('onInnerProtoGetter'), + onInnerProtoIndexed: getDesc(2), + bothProto: getDesc('bothProto'), + bothProtoGetter: getDesc('bothProtoGetter'), + bothProtoIndexed: getDesc(3), +}; +const resultIn = { + onSelf: 'onSelf' in this, + onSelfGetter: 'onSelfGetter' in this, + onSelfIndexed: 1 in this, + onOuterProto: 'onOuterProto' in this, + onOuterProtoGetter: 'onOuterProtoGetter' in this, + onOuterProtoIndexed: 0 in this, + onInnerProto: 'onInnerProto' in this, + onInnerProtoGetter: 'onInnerProtoGetter' in this, + onInnerProtoIndexed: 2 in this, + bothProto: 'bothProto' in this, + bothProtoGetter: 'bothProtoGetter' in this, + bothProtoIndexed: 3 in this, +}; +const resultValue = { + onSelf: this.onSelf, + onSelfGetter: this.onSelfGetter, + onSelfIndexed: this[1], + onOuterProto: this.onOuterProto, + onOuterProtoGetter: this.onOuterProtoGetter, + onOuterProtoIndexed: this[0], + onInnerProto: this.onInnerProto, + onInnerProtoGetter: this.onInnerProtoGetter, + onInnerProtoIndexed: this[2], + bothProto: this.bothProto, + bothProtoGetter: this.bothProtoGetter, + bothProtoIndexed: this[3], +}; +({ + resultHasOwn, + resultDesc, + resultIn, + resultValue, +}); +`, ctx); + +// eslint-disable-next-line no-restricted-properties +assert.deepEqual(result, { + resultHasOwn: { + onSelf: true, + onSelfGetter: true, + onSelfIndexed: true, + + // The following results should be false in terms of "normal" JavaScript + // objects. However, `in` operator only looks up properties on the inner + // prototype chain, the interceptor has to lookup the outer prototype chain + // to maintain compatibility. + // FIXME(legendecas): https://github.com/nodejs/node/issues/54436 + onOuterProto: true, + onOuterProtoGetter: true, + onOuterProtoIndexed: true, + onInnerProto: true, + onInnerProtoGetter: true, + onInnerProtoIndexed: true, + bothProto: true, + bothProtoGetter: true, + bothProtoIndexed: true, + }, + resultDesc: { + onSelf: { value: 'onSelf', writable: true, enumerable: true, configurable: true }, + onSelfGetter: { get: onSelfGetter, set: undefined, enumerable: false, configurable: false }, + onSelfIndexed: { value: 'onSelfIndexed', writable: false, enumerable: false, configurable: true }, + onOuterProto: undefined, + onOuterProtoGetter: undefined, + onOuterProtoIndexed: undefined, + onInnerProto: undefined, + onInnerProtoGetter: undefined, + onInnerProtoIndexed: undefined, + bothProto: undefined, + bothProtoGetter: undefined, + bothProtoIndexed: undefined, + }, + resultIn: { + onSelf: true, + onSelfGetter: true, + onSelfIndexed: true, + onOuterProto: true, + onOuterProtoGetter: true, + onOuterProtoIndexed: true, + onInnerProto: true, + onInnerProtoGetter: true, + onInnerProtoIndexed: true, + bothProto: true, + bothProtoGetter: true, + bothProtoIndexed: true, + }, + resultValue: { + onSelf: 'onSelf', + onSelfGetter: 'onSelfGetter', + onSelfIndexed: 'onSelfIndexed', + onOuterProto: 'onOuterProto', + onOuterProtoGetter: 'onOuterProtoGetter', + onOuterProtoIndexed: 'onOuterProtoIndexed', + onInnerProto: 'onInnerProto', + onInnerProtoGetter: 'onInnerProtoGetter', + onInnerProtoIndexed: 'onInnerProtoIndexed', + bothProto: 'onOuterProto', + bothProtoGetter: 'onOuterProtoGetter', + bothProtoIndexed: 'onOuterProtoIndexed', + } +});