From 6a18b93ebcba254cd62ddfc268524c2b363da2bb Mon Sep 17 00:00:00 2001 From: Carlos Espa <43477095+Ceres6@users.noreply.github.com> Date: Sat, 28 Sep 2024 11:46:03 +0200 Subject: [PATCH] src: add receiver to fast api callback methods When creating an fast api the callback might use the receiver. In that case if the internal binding is destructured the method won't have access to the reciver and it will throw. Passing the receiver as second argument ensures the receiver is available. PR-URL: https://github.com/nodejs/node/pull/54408 Reviewed-By: Stephen Belanger Reviewed-By: Joyee Cheung Reviewed-By: Yagiz Nizipli Reviewed-By: Santiago Gimeno --- doc/api/cli.md | 2 +- doc/api/permissions.md | 2 +- doc/contributing/adding-v8-fast-api.md | 19 +++++++++- lib/fs.js | 2 +- lib/internal/fs/promises.js | 2 +- lib/internal/modules/cjs/loader.js | 4 +-- lib/internal/modules/esm/resolve.js | 7 ++-- src/histogram.cc | 31 +++++++++------- src/histogram.h | 35 +++++++++++++------ src/node_external_reference.h | 23 ++++++++---- src/node_file.cc | 6 ++-- src/node_process.h | 6 ++-- src/node_wasi.cc | 1 + src/node_wasi.h | 3 +- src/timers.cc | 15 +++++--- src/timers.h | 14 +++++--- ...test-permission-fs-internal-module-stat.js | 2 +- typings/internalBinding/fs.d.ts | 2 +- 18 files changed, 123 insertions(+), 53 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 23e080965107b2..96139b83b0e2af 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -220,7 +220,7 @@ The initializer module also needs to be allowed. Consider the following example: ```console $ node --experimental-permission t.js node:internal/modules/cjs/loader:162 - const result = internalModuleStat(filename); + const result = internalModuleStat(receiver, filename); ^ Error: Access to this API has been restricted diff --git a/doc/api/permissions.md b/doc/api/permissions.md index fd1a8e5859fae1..e37c2982bc146a 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -48,7 +48,7 @@ will be restricted. ```console $ node --experimental-permission index.js node:internal/modules/cjs/loader:171 - const result = internalModuleStat(filename); + const result = internalModuleStat(receiver, filename); ^ Error: Access to this API has been restricted diff --git a/doc/contributing/adding-v8-fast-api.md b/doc/contributing/adding-v8-fast-api.md index 7f7da32656d81b..1869c12731f7f3 100644 --- a/doc/contributing/adding-v8-fast-api.md +++ b/doc/contributing/adding-v8-fast-api.md @@ -24,7 +24,7 @@ for example, they may not trigger garbage collection. [`node_external_reference.h`](../../src/node_external_reference.h) file. Although, it would not start failing or crashing until the function ends up in a snapshot (either the built-in or a user-land one). Please refer to the - [binding functions documentation](../../src#binding-functions) for more + [binding functions documentation](../../src/README.md#binding-functions) for more information. * To test fast APIs, make sure to run the tests in a loop with a decent iterations count to trigger relevant optimizations that prefer the fast API @@ -38,6 +38,23 @@ for example, they may not trigger garbage collection. * The fast callback must be idempotent up to the point where error and fallback conditions are checked, because otherwise executing the slow callback might produce visible side effects twice. +* If the receiver is used in the callback, it must be passed as a second argument, + leaving the first one unused, to prevent the JS land from accidentally omitting the receiver when + invoking the fast API method. + + ```cpp + // Instead of invoking the method as `receiver.internalModuleStat(input)`, the JS land should + // invoke it as `internalModuleStat(binding, input)` to make sure the binding is available to + // the native land. + static int32_t FastInternalModuleStat( + Local unused, + Local recv, + const FastOneByteString& input, + FastApiCallbackOptions& options) { + Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked()); + // More code + } + ``` ## Fallback to slow path diff --git a/lib/fs.js b/lib/fs.js index 1497c0935a130c..11c53abc917875 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1414,7 +1414,7 @@ function readdirSyncRecursive(basePath, options) { for (let i = 0; i < readdirResult.length; i++) { const resultPath = pathModule.join(path, readdirResult[i]); const relativeResultPath = pathModule.relative(basePath, resultPath); - const stat = binding.internalModuleStat(resultPath); + const stat = binding.internalModuleStat(binding, resultPath); ArrayPrototypePush(readdirResults, relativeResultPath); // 1 indicates directory if (stat === 1) { diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 88473afc1a5220..2f37c8d7f8b6f9 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -914,7 +914,7 @@ async function readdirRecursive(originalPath, options) { const { 0: path, 1: readdir } = ArrayPrototypePop(queue); for (const ent of readdir) { const direntPath = pathModule.join(path, ent); - const stat = binding.internalModuleStat(direntPath); + const stat = binding.internalModuleStat(binding, direntPath); ArrayPrototypePush( result, pathModule.relative(originalPath, direntPath), diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index c959170e57337d..b0210e4d96348c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -235,9 +235,9 @@ function stat(filename) { const result = statCache.get(filename); if (result !== undefined) { return result; } } - const result = internalFsBinding.internalModuleStat(filename); + const result = internalFsBinding.internalModuleStat(internalFsBinding, filename); if (statCache !== null && result >= 0) { - // Only set cache when `internalModuleStat(filename)` succeeds. + // Only set cache when `internalModuleStat(internalFsBinding, filename)` succeeds. statCache.set(filename, result); } return result; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index c83778cc00e6f1..35925ef0817273 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -241,8 +241,10 @@ function finalizeResolution(resolved, base, preserveSymlinks) { throw err; } - const stats = internalFsBinding.internalModuleStat(StringPrototypeEndsWith(path, '/') ? - StringPrototypeSlice(path, -1) : path); + const stats = internalFsBinding.internalModuleStat( + internalFsBinding, + StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path, + ); // Check for stats.isDirectory() if (stats === 1) { @@ -802,6 +804,7 @@ function packageResolve(specifier, base, conditions) { let lastPath; do { const stat = internalFsBinding.internalModuleStat( + internalFsBinding, StringPrototypeSlice(packageJSONPath, 0, packageJSONPath.length - 13), ); // Check for !stat.isDirectory() diff --git a/src/histogram.cc b/src/histogram.cc index 50605515dc8826..c62a5b8952400f 100644 --- a/src/histogram.cc +++ b/src/histogram.cc @@ -169,7 +169,8 @@ void HistogramBase::RecordDelta(const FunctionCallbackInfo& args) { (*histogram)->RecordDelta(); } -void HistogramBase::FastRecordDelta(Local receiver) { +void HistogramBase::FastRecordDelta(Local unused, + Local receiver) { HistogramBase* histogram; ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver); (*histogram)->RecordDelta(); @@ -189,7 +190,8 @@ void HistogramBase::Record(const FunctionCallbackInfo& args) { (*histogram)->Record(value); } -void HistogramBase::FastRecord(Local receiver, +void HistogramBase::FastRecord(Local unused, + Local receiver, const int64_t value, FastApiCallbackOptions& options) { if (value < 1) { @@ -436,7 +438,9 @@ void IntervalHistogram::Start(const FunctionCallbackInfo& args) { histogram->OnStart(args[0]->IsTrue() ? StartFlags::RESET : StartFlags::NONE); } -void IntervalHistogram::FastStart(Local receiver, bool reset) { +void IntervalHistogram::FastStart(Local unused, + Local receiver, + bool reset) { IntervalHistogram* histogram; ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver); histogram->OnStart(reset ? StartFlags::RESET : StartFlags::NONE); @@ -448,7 +452,7 @@ void IntervalHistogram::Stop(const FunctionCallbackInfo& args) { histogram->OnStop(); } -void IntervalHistogram::FastStop(Local receiver) { +void IntervalHistogram::FastStop(Local unused, Local receiver) { IntervalHistogram* histogram; ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver); histogram->OnStop(); @@ -564,42 +568,45 @@ void HistogramImpl::DoReset(const FunctionCallbackInfo& args) { (*histogram)->Reset(); } -void HistogramImpl::FastReset(Local receiver) { +void HistogramImpl::FastReset(Local unused, Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); (*histogram)->Reset(); } -double HistogramImpl::FastGetCount(Local receiver) { +double HistogramImpl::FastGetCount(Local unused, Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return static_cast((*histogram)->Count()); } -double HistogramImpl::FastGetMin(Local receiver) { +double HistogramImpl::FastGetMin(Local unused, Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return static_cast((*histogram)->Min()); } -double HistogramImpl::FastGetMax(Local receiver) { +double HistogramImpl::FastGetMax(Local unused, Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return static_cast((*histogram)->Max()); } -double HistogramImpl::FastGetMean(Local receiver) { +double HistogramImpl::FastGetMean(Local unused, Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return (*histogram)->Mean(); } -double HistogramImpl::FastGetExceeds(Local receiver) { +double HistogramImpl::FastGetExceeds(Local unused, + Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return static_cast((*histogram)->Exceeds()); } -double HistogramImpl::FastGetStddev(Local receiver) { +double HistogramImpl::FastGetStddev(Local unused, + Local receiver) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return (*histogram)->Stddev(); } -double HistogramImpl::FastGetPercentile(Local receiver, +double HistogramImpl::FastGetPercentile(Local unused, + Local receiver, const double percentile) { HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver); return static_cast((*histogram)->Percentile(percentile)); diff --git a/src/histogram.h b/src/histogram.h index c3d8fe5f207274..362e82e4436a90 100644 --- a/src/histogram.h +++ b/src/histogram.h @@ -101,14 +101,22 @@ class HistogramImpl { static void GetPercentilesBigInt( const v8::FunctionCallbackInfo& args); - static void FastReset(v8::Local receiver); - static double FastGetCount(v8::Local receiver); - static double FastGetMin(v8::Local receiver); - static double FastGetMax(v8::Local receiver); - static double FastGetMean(v8::Local receiver); - static double FastGetExceeds(v8::Local receiver); - static double FastGetStddev(v8::Local receiver); - static double FastGetPercentile(v8::Local receiver, + static void FastReset(v8::Local unused, + v8::Local receiver); + static double FastGetCount(v8::Local unused, + v8::Local receiver); + static double FastGetMin(v8::Local unused, + v8::Local receiver); + static double FastGetMax(v8::Local unused, + v8::Local receiver); + static double FastGetMean(v8::Local unused, + v8::Local receiver); + static double FastGetExceeds(v8::Local unused, + v8::Local receiver); + static double FastGetStddev(v8::Local unused, + v8::Local receiver); + static double FastGetPercentile(v8::Local unused, + v8::Local receiver, const double percentile); static void AddMethods(v8::Isolate* isolate, @@ -158,10 +166,12 @@ class HistogramBase final : public BaseObject, public HistogramImpl { static void Add(const v8::FunctionCallbackInfo& args); static void FastRecord( + v8::Local unused, v8::Local receiver, const int64_t value, v8::FastApiCallbackOptions& options); // NOLINT(runtime/references) - static void FastRecordDelta(v8::Local receiver); + static void FastRecordDelta(v8::Local unused, + v8::Local receiver); HistogramBase( Environment* env, @@ -233,8 +243,11 @@ class IntervalHistogram final : public HandleWrap, public HistogramImpl { static void Start(const v8::FunctionCallbackInfo& args); static void Stop(const v8::FunctionCallbackInfo& args); - static void FastStart(v8::Local receiver, bool reset); - static void FastStop(v8::Local receiver); + static void FastStart(v8::Local unused, + v8::Local receiver, + bool reset); + static void FastStop(v8::Local unused, + v8::Local receiver); BaseObject::TransferMode GetTransferMode() const override { return TransferMode::kCloneable; diff --git a/src/node_external_reference.h b/src/node_external_reference.h index 38a4ff7e6c2c99..8d49a119c21832 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -12,19 +12,25 @@ namespace node { using CFunctionCallbackWithOneByteString = uint32_t (*)(v8::Local, const v8::FastOneByteString&); -using CFunctionCallback = void (*)(v8::Local receiver); +using CFunctionCallback = void (*)(v8::Local unused, + v8::Local receiver); using CFunctionCallbackReturnDouble = - double (*)(v8::Local receiver); + double (*)(v8::Local unused, v8::Local receiver); using CFunctionCallbackReturnInt32 = - int32_t (*)(v8::Local receiver, + int32_t (*)(v8::Local unused, + v8::Local receiver, const v8::FastOneByteString& input, // NOLINTNEXTLINE(runtime/references) This is V8 api. v8::FastApiCallbackOptions& options); using CFunctionCallbackValueReturnDouble = double (*)(v8::Local receiver); -using CFunctionCallbackWithInt64 = void (*)(v8::Local receiver, +using CFunctionCallbackValueReturnDoubleUnusedReceiver = + double (*)(v8::Local unused, v8::Local receiver); +using CFunctionCallbackWithInt64 = void (*)(v8::Local unused, + v8::Local receiver, int64_t); -using CFunctionCallbackWithBool = void (*)(v8::Local receiver, +using CFunctionCallbackWithBool = void (*)(v8::Local unused, + v8::Local receiver, bool); using CFunctionCallbackWithString = bool (*)(v8::Local, const v8::FastOneByteString& input); @@ -50,11 +56,15 @@ using CFunctionCallbackWithUint8ArrayUint32Int64Bool = using CFunctionWithUint32 = uint32_t (*)(v8::Local, const uint32_t input); using CFunctionWithDoubleReturnDouble = double (*)(v8::Local, + v8::Local, const double); using CFunctionWithInt64Fallback = void (*)(v8::Local, + v8::Local, const int64_t, v8::FastApiCallbackOptions&); -using CFunctionWithBool = void (*)(v8::Local, bool); +using CFunctionWithBool = void (*)(v8::Local, + v8::Local, + bool); using CFunctionWriteString = uint32_t (*)(v8::Local receiver, @@ -83,6 +93,7 @@ class ExternalReferenceRegistry { V(CFunctionCallbackReturnDouble) \ V(CFunctionCallbackReturnInt32) \ V(CFunctionCallbackValueReturnDouble) \ + V(CFunctionCallbackValueReturnDoubleUnusedReceiver) \ V(CFunctionCallbackWithInt64) \ V(CFunctionCallbackWithBool) \ V(CFunctionCallbackWithString) \ diff --git a/src/node_file.cc b/src/node_file.cc index 0b40cb63fae014..ab2961ca8cf3f2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1034,8 +1034,9 @@ static void ExistsSync(const FunctionCallbackInfo& args) { static void InternalModuleStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - CHECK(args[0]->IsString()); - BufferValue path(env->isolate(), args[0]); + CHECK_GE(args.Length(), 2); + CHECK(args[1]->IsString()); + BufferValue path(env->isolate(), args[1]); CHECK_NOT_NULL(*path); ToNamespacedPath(env, &path); THROW_IF_INSUFFICIENT_PERMISSIONS( @@ -1053,6 +1054,7 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { } static int32_t FastInternalModuleStat( + Local unused, Local recv, const FastOneByteString& input, // NOLINTNEXTLINE(runtime/references) This is V8 api. diff --git a/src/node_process.h b/src/node_process.h index 142d0e63e18c46..d4f1c0d45dec50 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -72,7 +72,8 @@ class BindingData : public SnapshotableObject { static BindingData* FromV8Value(v8::Local receiver); static void NumberImpl(BindingData* receiver); - static void FastNumber(v8::Local receiver) { + static void FastNumber(v8::Local unused, + v8::Local receiver) { NumberImpl(FromV8Value(receiver)); } @@ -80,7 +81,8 @@ class BindingData : public SnapshotableObject { static void BigIntImpl(BindingData* receiver); - static void FastBigInt(v8::Local receiver) { + static void FastBigInt(v8::Local unused, + v8::Local receiver) { BigIntImpl(FromV8Value(receiver)); } diff --git a/src/node_wasi.cc b/src/node_wasi.cc index ad1da44a01f437..d8b226f40a9edc 100644 --- a/src/node_wasi.cc +++ b/src/node_wasi.cc @@ -241,6 +241,7 @@ inline void EinvalError() {} template R WASI::WasiFunction::FastCallback( + Local unused, Local receiver, Args... args, // NOLINTNEXTLINE(runtime/references) This is V8 api. diff --git a/src/node_wasi.h b/src/node_wasi.h index 25551936e6be36..ef7d2e83b6728a 100644 --- a/src/node_wasi.h +++ b/src/node_wasi.h @@ -160,7 +160,8 @@ class WASI : public BaseObject, v8::Local); private: - static R FastCallback(v8::Local receiver, + static R FastCallback(v8::Local unused, + v8::Local receiver, Args..., v8::FastApiCallbackOptions&); diff --git a/src/timers.cc b/src/timers.cc index 127806fbcdfd3e..c7ba22a078fb5f 100644 --- a/src/timers.cc +++ b/src/timers.cc @@ -33,7 +33,8 @@ void BindingData::SlowGetLibuvNow(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(Number::New(args.GetIsolate(), now)); } -double BindingData::FastGetLibuvNow(Local receiver) { +double BindingData::FastGetLibuvNow(Local unused, + Local receiver) { return GetLibuvNowImpl(FromJSObject(receiver)); } @@ -47,7 +48,9 @@ void BindingData::SlowScheduleTimer(const FunctionCallbackInfo& args) { ScheduleTimerImpl(Realm::GetBindingData(args), duration); } -void BindingData::FastScheduleTimer(Local receiver, int64_t duration) { +void BindingData::FastScheduleTimer(Local unused, + Local receiver, + int64_t duration) { ScheduleTimerImpl(FromJSObject(receiver), duration); } @@ -61,7 +64,9 @@ void BindingData::SlowToggleTimerRef( args[0]->IsTrue()); } -void BindingData::FastToggleTimerRef(Local receiver, bool ref) { +void BindingData::FastToggleTimerRef(Local unused, + Local receiver, + bool ref) { ToggleTimerRefImpl(FromJSObject(receiver), ref); } @@ -75,7 +80,9 @@ void BindingData::SlowToggleImmediateRef( args[0]->IsTrue()); } -void BindingData::FastToggleImmediateRef(Local receiver, bool ref) { +void BindingData::FastToggleImmediateRef(Local unused, + Local receiver, + bool ref) { ToggleImmediateRefImpl(FromJSObject(receiver), ref); } diff --git a/src/timers.h b/src/timers.h index d514a70fc25990..fd3cbf66f7c516 100644 --- a/src/timers.h +++ b/src/timers.h @@ -26,23 +26,29 @@ class BindingData : public SnapshotableObject { static void SetupTimers(const v8::FunctionCallbackInfo& args); static void SlowGetLibuvNow(const v8::FunctionCallbackInfo& args); - static double FastGetLibuvNow(v8::Local receiver); + static double FastGetLibuvNow(v8::Local unused, + v8::Local receiver); static double GetLibuvNowImpl(BindingData* data); static void SlowScheduleTimer( const v8::FunctionCallbackInfo& args); - static void FastScheduleTimer(v8::Local receiver, + static void FastScheduleTimer(v8::Local unused, + v8::Local receiver, int64_t duration); static void ScheduleTimerImpl(BindingData* data, int64_t duration); static void SlowToggleTimerRef( const v8::FunctionCallbackInfo& args); - static void FastToggleTimerRef(v8::Local receiver, bool ref); + static void FastToggleTimerRef(v8::Local unused, + v8::Local receiver, + bool ref); static void ToggleTimerRefImpl(BindingData* data, bool ref); static void SlowToggleImmediateRef( const v8::FunctionCallbackInfo& args); - static void FastToggleImmediateRef(v8::Local receiver, bool ref); + static void FastToggleImmediateRef(v8::Local unused, + v8::Local receiver, + bool ref); static void ToggleImmediateRefImpl(BindingData* data, bool ref); static void CreatePerIsolateProperties(IsolateData* isolate_data, diff --git a/test/parallel/test-permission-fs-internal-module-stat.js b/test/parallel/test-permission-fs-internal-module-stat.js index b39c276f3c1d99..0075afb8bb55cd 100644 --- a/test/parallel/test-permission-fs-internal-module-stat.js +++ b/test/parallel/test-permission-fs-internal-module-stat.js @@ -18,7 +18,7 @@ const internalFsBinding = internalBinding('fs'); // Run this inside a for loop to trigger the fast API for (let i = 0; i < 10_000; i++) { assert.throws(() => { - internalFsBinding.internalModuleStat(blockedFile); + internalFsBinding.internalModuleStat(internalFsBinding, blockedFile); }, { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index c8af56bcc8a44f..889769de209a16 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -113,7 +113,7 @@ declare namespace InternalFSBinding { function futimes(fd: number, atime: number, mtime: number): void; function futimes(fd: number, atime: number, mtime: number, usePromises: typeof kUsePromises): Promise; - function internalModuleStat(path: string): number; + function internalModuleStat(receiver: unknown, path: string): number; function lchown(path: string, uid: number, gid: number, req: FSReqCallback): void; function lchown(path: string, uid: number, gid: number, req: undefined, ctx: FSSyncContext): void;