From 470384e7be37a1f2c303f068deae5abbf7b22318 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 4 Nov 2022 18:33:03 +0100 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/45379 Backport-PR-URL: https://github.com/nodejs/node/pull/45674 Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell Signed-off-by: Daeyeon Jeong --- 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 | 61 ++++--------------- ...test-internal-util-decorate-error-stack.js | 15 ++--- test/parallel/test-util-internal.js | 24 +++----- 7 files changed, 51 insertions(+), 94 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 723dc19a602a00..d3c18419fda0ac 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -76,8 +76,9 @@ const { exposeLazyInterfaces, } = require('internal/util'); const { - exiting_aliased_Uint32Array, - getHiddenValue, + privateSymbols: { + exiting_aliased_Uint32Array, + }, } = internalBinding('util'); setupProcessObject(); @@ -87,8 +88,7 @@ setupBuffer(); process.domain = null; { - const exitingAliasedUint32Array = - getHiddenValue(process, exiting_aliased_Uint32Array); + const exitingAliasedUint32Array = process[exiting_aliased_Uint32Array]; 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 da7a36e593621b..b807428a981f7a 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -816,16 +816,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 f47efe1040a15a..f95a37753ca19e 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 9ca2ab96c473d1..7228123ac00351 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; @@ -159,44 +158,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(); @@ -386,8 +347,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); @@ -412,16 +371,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), \ @@ -432,8 +397,6 @@ void Initialize(Local target, V(kRejected); #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/);