From 2ac9634186d8631af41997b14aea798652f3db77 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 4 Nov 2022 18:33:03 +0100 Subject: [PATCH 1/2] util: use private symbols in JS land directly Instead of calling into C++ to use the private symbols, use an ObjectTemplate to create an object that holds the symbols and use them directly from JS land. --- lib/internal/bootstrap/node.js | 8 +-- lib/internal/buffer.js | 8 ++- lib/internal/errors.js | 14 ++--- lib/internal/util.js | 15 +++-- src/node_util.cc | 60 ++++--------------- ...test-internal-util-decorate-error-stack.js | 15 ++--- test/parallel/test-util-internal.js | 24 +++----- 7 files changed, 51 insertions(+), 93 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index f98699dc0363f5..0c903fb32877d2 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -80,8 +80,9 @@ const { validateInteger, } = require('internal/validators'); const { - exit_info_private_symbol, - getHiddenValue, + privateSymbols: { + exit_info_private_symbol, + }, kExitCode, kExiting, kHasExitCode, @@ -96,8 +97,7 @@ process.domain = null; // process._exiting and process.exitCode { - const fields = getHiddenValue(process, exit_info_private_symbol); - + const fields = process[exit_info_private_symbol]; ObjectDefineProperty(process, '_exiting', { __proto__: null, get() { diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index bd38cf48a7fc6e..7b8331f47514c4 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -32,9 +32,11 @@ const { utf8Write, getZeroFillToggle } = internalBinding('buffer'); + const { - untransferable_object_private_symbol, - setHiddenValue, + privateSymbols: { + untransferable_object_private_symbol, + }, } = internalBinding('util'); // Temporary buffers to convert numbers. @@ -1048,7 +1050,7 @@ function addBufferPrototypeMethods(proto) { function markAsUntransferable(obj) { if ((typeof obj !== 'object' && typeof obj !== 'function') || obj === null) return; // This object is a primitive and therefore already untransferable. - setHiddenValue(obj, untransferable_object_private_symbol, true); + obj[untransferable_object_private_symbol] = true; } // A toggle used to access the zero fill setting of the array buffer allocator diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 5677b9af679f33..48c99787ab01d9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -809,16 +809,14 @@ const fatalExceptionStackEnhancers = { } }; +const { + privateSymbols: { + arrow_message_private_symbol, + } +} = internalBinding('util'); // Ensures the printed error line is from user code. -let _kArrowMessagePrivateSymbol, _setHiddenValue; function setArrowMessage(err, arrowMessage) { - if (!_kArrowMessagePrivateSymbol) { - ({ - arrow_message_private_symbol: _kArrowMessagePrivateSymbol, - setHiddenValue: _setHiddenValue, - } = internalBinding('util')); - } - _setHiddenValue(err, _kArrowMessagePrivateSymbol, arrowMessage); + err[arrow_message_private_symbol] = arrowMessage; } // Hide stack lines before the first user code line. diff --git a/lib/internal/util.js b/lib/internal/util.js index 4f74f912936611..52b621eae47318 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -42,10 +42,10 @@ const { } = require('internal/errors'); const { signals } = internalBinding('constants').os; const { - getHiddenValue, - setHiddenValue, - arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex, - decorated_private_symbol: kDecoratedPrivateSymbolIndex, + privateSymbols: { + arrow_message_private_symbol, + decorated_private_symbol, + }, sleep: _sleep, toUSVString: _toUSVString, } = internalBinding('util'); @@ -143,15 +143,14 @@ function deprecate(fn, msg, code, useEmitSync) { } function decorateErrorStack(err) { - if (!(isError(err) && err.stack) || - getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true) + if (!(isError(err) && err.stack) || err[decorated_private_symbol]) return; - const arrow = getHiddenValue(err, kArrowMessagePrivateSymbolIndex); + const arrow = err[arrow_message_private_symbol]; if (arrow) { err.stack = arrow + err.stack; - setHiddenValue(err, kDecoratedPrivateSymbolIndex, true); + err[decorated_private_symbol] = true; } } diff --git a/src/node_util.cc b/src/node_util.cc index 5d2b4f2155d3d2..2b47f181207621 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -159,44 +159,6 @@ static void PreviewEntries(const FunctionCallbackInfo& args) { Array::New(env->isolate(), ret, arraysize(ret))); } -inline Local IndexToPrivateSymbol(Environment* env, uint32_t index) { -#define V(name, _) &Environment::name, - static Local (Environment::*const methods[])() const = { - PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) - }; -#undef V - CHECK_LT(index, arraysize(methods)); - return (env->*methods[index])(); -} - -static void GetHiddenValue(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - CHECK(args[0]->IsObject()); - CHECK(args[1]->IsUint32()); - - Local obj = args[0].As(); - uint32_t index = args[1].As()->Value(); - Local private_symbol = IndexToPrivateSymbol(env, index); - Local ret; - if (obj->GetPrivate(env->context(), private_symbol).ToLocal(&ret)) - args.GetReturnValue().Set(ret); -} - -static void SetHiddenValue(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - CHECK(args[0]->IsObject()); - CHECK(args[1]->IsUint32()); - - Local obj = args[0].As(); - uint32_t index = args[1].As()->Value(); - Local private_symbol = IndexToPrivateSymbol(env, index); - bool ret; - if (obj->SetPrivate(env->context(), private_symbol, args[2]).To(&ret)) - args.GetReturnValue().Set(ret); -} - static void Sleep(const FunctionCallbackInfo& args) { CHECK(args[0]->IsUint32()); uint32_t msec = args[0].As()->Value(); @@ -379,8 +341,6 @@ static void ToUSVString(const FunctionCallbackInfo& args) { } void RegisterExternalReferences(ExternalReferenceRegistry* registry) { - registry->Register(GetHiddenValue); - registry->Register(SetHiddenValue); registry->Register(GetPromiseDetails); registry->Register(GetProxyDetails); registry->Register(PreviewEntries); @@ -404,16 +364,22 @@ void Initialize(Local target, Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); -#define V(name, _) \ - target->Set(context, \ - FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ - Integer::NewFromUnsigned(env->isolate(), index++)).Check(); { - uint32_t index = 0; + Local tmpl = v8::ObjectTemplate::New(isolate); +#define V(PropertyName, _) \ + tmpl->Set(FIXED_ONE_BYTE_STRING(env->isolate(), #PropertyName), \ + env->PropertyName()); + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) - } #undef V + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "privateSymbols"), + tmpl->NewInstance(context).ToLocalChecked()) + .Check(); + } + #define V(name) \ target->Set(context, \ FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ @@ -435,8 +401,6 @@ void Initialize(Local target, V(kHasExitCode); #undef V - SetMethodNoSideEffect(context, target, "getHiddenValue", GetHiddenValue); - SetMethod(context, target, "setHiddenValue", SetHiddenValue); SetMethodNoSideEffect( context, target, "getPromiseDetails", GetPromiseDetails); SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails); diff --git a/test/parallel/test-internal-util-decorate-error-stack.js b/test/parallel/test-internal-util-decorate-error-stack.js index b36714b44b8b72..3566d9375fb81c 100644 --- a/test/parallel/test-internal-util-decorate-error-stack.js +++ b/test/parallel/test-internal-util-decorate-error-stack.js @@ -5,12 +5,14 @@ const fixtures = require('../common/fixtures'); const assert = require('assert'); const internalUtil = require('internal/util'); const { internalBinding } = require('internal/test/binding'); -const binding = internalBinding('util'); +const { + privateSymbols: { + arrow_message_private_symbol, + decorated_private_symbol, + } +} = internalBinding('util'); const spawnSync = require('child_process').spawnSync; -const kArrowMessagePrivateSymbolIndex = binding.arrow_message_private_symbol; -const kDecoratedPrivateSymbolIndex = binding.decorated_private_symbol; - const decorateErrorStack = internalUtil.decorateErrorStack; // Verify that decorateErrorStack does not throw with non-objects. @@ -73,9 +75,8 @@ const arrowMessage = 'arrow_message'; err = new Error('foo'); originalStack = err.stack; -binding.setHiddenValue(err, kArrowMessagePrivateSymbolIndex, arrowMessage); +err[arrow_message_private_symbol] = arrowMessage; decorateErrorStack(err); assert.strictEqual(err.stack, `${arrowMessage}${originalStack}`); -assert.strictEqual( - binding.getHiddenValue(err, kDecoratedPrivateSymbolIndex), true); +assert.strictEqual(err[decorated_private_symbol], true); diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js index 36d65e54d5b6ca..e2b500daa70060 100644 --- a/test/parallel/test-util-internal.js +++ b/test/parallel/test-util-internal.js @@ -7,30 +7,24 @@ const fixtures = require('../common/fixtures'); const { internalBinding } = require('internal/test/binding'); const { - getHiddenValue, - setHiddenValue, - arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex + privateSymbols: { + arrow_message_private_symbol, + }, } = internalBinding('util'); -assert.strictEqual( - getHiddenValue({}, kArrowMessagePrivateSymbolIndex), - undefined); - const obj = {}; -assert.strictEqual( - setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'), - true); -assert.strictEqual( - getHiddenValue(obj, kArrowMessagePrivateSymbolIndex), - 'bar'); +assert.strictEqual(obj[arrow_message_private_symbol], undefined); + +obj[arrow_message_private_symbol] = 'bar'; +assert.strictEqual(obj[arrow_message_private_symbol], 'bar'); +assert.deepStrictEqual(Reflect.ownKeys(obj), []); let arrowMessage; try { require(fixtures.path('syntax', 'bad_syntax')); } catch (err) { - arrowMessage = - getHiddenValue(err, kArrowMessagePrivateSymbolIndex); + arrowMessage = err[arrow_message_private_symbol]; } assert.match(arrowMessage, /bad_syntax\.js:1/); From 72fb3c226a40f440802b0b850fd07935b339867a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 15 Nov 2022 17:29:02 +0100 Subject: [PATCH 2/2] fixup! util: use private symbols in JS land directly --- src/node_util.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_util.cc b/src/node_util.cc index 2b47f181207621..e9028c3edc4d4f 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -26,7 +26,6 @@ using v8::Object; using v8::ONLY_CONFIGURABLE; using v8::ONLY_ENUMERABLE; using v8::ONLY_WRITABLE; -using v8::Private; using v8::Promise; using v8::PropertyFilter; using v8::Proxy;