From 52739821e8a5b1811755f63e3949a11ca4e80811 Mon Sep 17 00:00:00 2001 From: Joyee Cheung <joyeec9h3@gmail.com> Date: Fri, 4 Oct 2024 13:52:53 +0200 Subject: [PATCH] src: implement IsInsideNodeModules() in C++ This previously compiles a script and run it in a new context to avoid global pollution, which is more complex than necessary and can be too slow for it to be reused in other cases. The new implementation just checks the frames in C++ which is safe from global pollution, faster and simpler. The previous implementation also had a bug when the call site is in a ESM, because ESM have URLs as their script names, which don't start with '/' or '\' and will be skipped. The new implementation removes the skipping to fix it for ESM. --- lib/buffer.js | 6 ++- lib/internal/util.js | 26 ---------- src/node_util.cc | 46 ++++++++++++++++-- .../warning_node_modules/new-buffer-cjs.js | 1 + .../warning_node_modules/new-buffer-esm.mjs | 1 + .../node_modules/new-buffer-cjs/index.js | 1 + .../node_modules/new-buffer-cjs/package.json | 3 ++ .../node_modules/new-buffer-esm/index.js | 2 + .../node_modules/new-buffer-esm/package.json | 4 ++ .../test-buffer-constructor-node-modules.js | 48 +++++++++++++++++++ 10 files changed, 107 insertions(+), 31 deletions(-) create mode 100644 test/fixtures/warning_node_modules/new-buffer-cjs.js create mode 100644 test/fixtures/warning_node_modules/new-buffer-esm.mjs create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js create mode 100644 test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json create mode 100644 test/parallel/test-buffer-constructor-node-modules.js diff --git a/lib/buffer.js b/lib/buffer.js index 6294eae2fb5093..2c56b6c504db9b 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -79,10 +79,10 @@ const { ONLY_ENUMERABLE, }, getOwnNonIndexProperties, + isInsideNodeModules, } = internalBinding('util'); const { customInspectSymbol, - isInsideNodeModules, lazyDOMException, normalizeEncoding, kIsEncodingSymbol, @@ -178,13 +178,15 @@ function showFlaggedDeprecation() { if (bufferWarningAlreadyEmitted || ++nodeModulesCheckCounter > 10000 || (!require('internal/options').getOptionValue('--pending-deprecation') && - isInsideNodeModules())) { + isInsideNodeModules(100, true))) { // We don't emit a warning, because we either: // - Already did so, or // - Already checked too many times whether a call is coming // from node_modules and want to stop slowing down things, or // - We aren't running with `--pending-deprecation` enabled, // and the code is inside `node_modules`. + // - We found node_modules in up to the topmost 100 frames, or + // there are more than 100 frames and we don't want to search anymore. return; } diff --git a/lib/internal/util.js b/lib/internal/util.js index bf1622aa5305ce..eafce4190bce46 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -516,31 +516,6 @@ function getStructuredStack() { return getStructuredStackImpl(); } -function isInsideNodeModules() { - const stack = getStructuredStack(); - - // Iterate over all stack frames and look for the first one not coming - // from inside Node.js itself: - if (ArrayIsArray(stack)) { - for (const frame of stack) { - const filename = frame.getFileName(); - - if ( - filename == null || - StringPrototypeStartsWith(filename, 'node:') === true || - ( - filename[0] !== '/' && - StringPrototypeIncludes(filename, '\\') === false - ) - ) { - continue; - } - return isUnderNodeModules(filename); - } - } - return false; -} - function once(callback, { preserveReturnValue = false } = kEmptyObject) { let called = false; let returnValue; @@ -914,7 +889,6 @@ module.exports = { getSystemErrorName, guessHandleType, isError, - isInsideNodeModules, isUnderNodeModules, isMacOS, isWindows, diff --git a/src/node_util.cc b/src/node_util.cc index c99e26752c7d93..f986f1c591e2bd 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -292,10 +292,47 @@ static void GetCallSite(const FunctionCallbackInfo<Value>& args) { callsite_objects.push_back(obj); } +} + +static void IsInsideNodeModules(const FunctionCallbackInfo<Value>& args) { + Isolate* isolate = args.GetIsolate(); + CHECK_EQ(args.Length(), 2); + CHECK(args[0]->IsInt32()); // frame_limit + // The second argument is the default value. + + int frames_limit = args[0].As<v8::Int32>()->Value(); + Local<StackTrace> stack = StackTrace::CurrentStackTrace(isolate, frames_limit); + int frame_count = stack->GetFrameCount(); + + // If the search requires looking into more than |frames_limit| frames, give up + // and return the specified default value. + if (frame_count == frames_limit) { + return args.GetReturnValue().Set(args[1]); + } - Local<Array> callsites = - Array::New(isolate, callsite_objects.data(), callsite_objects.size()); - args.GetReturnValue().Set(callsites); + bool result = false; + for (int i = 0; i < frame_count; ++i) { + Local<StackFrame> stack_frame = stack->GetFrame(isolate, i); + Local<String> script_name = stack_frame->GetScriptName(); + + if (script_name.IsEmpty() || script_name->Length() == 0) { + continue; + } + + std::string script_name_str = Utf8Value(isolate, script_name).ToString(); + if (script_name_str.starts_with("node:")) { + continue; + } + if (script_name_str.find("/node_modules/") != std::string::npos || + script_name_str.find("\\node_modules\\") != std::string::npos || + script_name_str.find("/node_modules\\") != std::string::npos || + script_name_str.find("\\node_modules/") != std::string::npos) { + result = true; + break; + } + } + + args.GetReturnValue().Set(result); } void RegisterExternalReferences(ExternalReferenceRegistry* registry) { @@ -313,6 +350,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(FastGuessHandleType); registry->Register(fast_guess_handle_type_.GetTypeInfo()); registry->Register(ParseEnv); + registry->Register(IsInsideNodeModules); } void Initialize(Local<Object> target, @@ -396,6 +434,8 @@ void Initialize(Local<Object> target, target->Set(context, env->constants_string(), constants).Check(); } + SetMethod( + context, target, "isInsideNodeModules", IsInsideNodeModules); SetMethodNoSideEffect( context, target, "getPromiseDetails", GetPromiseDetails); SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails); diff --git a/test/fixtures/warning_node_modules/new-buffer-cjs.js b/test/fixtures/warning_node_modules/new-buffer-cjs.js new file mode 100644 index 00000000000000..be2877fa30c46d --- /dev/null +++ b/test/fixtures/warning_node_modules/new-buffer-cjs.js @@ -0,0 +1 @@ +require('new-buffer-cjs'); diff --git a/test/fixtures/warning_node_modules/new-buffer-esm.mjs b/test/fixtures/warning_node_modules/new-buffer-esm.mjs new file mode 100644 index 00000000000000..9aa56f759f8ae4 --- /dev/null +++ b/test/fixtures/warning_node_modules/new-buffer-esm.mjs @@ -0,0 +1 @@ +import 'new-buffer-esm' diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js new file mode 100644 index 00000000000000..514db554ed6edf --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/index.js @@ -0,0 +1 @@ +new Buffer(10); diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json new file mode 100644 index 00000000000000..f2c75cf8e443b4 --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-cjs/package.json @@ -0,0 +1,3 @@ +{ + "main": "index.js" +} \ No newline at end of file diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js new file mode 100644 index 00000000000000..9dadaeb12413c2 --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/index.js @@ -0,0 +1,2 @@ +import { Buffer } from 'node:buffer'; +new Buffer(10); diff --git a/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json new file mode 100644 index 00000000000000..07fe0622822c0e --- /dev/null +++ b/test/fixtures/warning_node_modules/node_modules/new-buffer-esm/package.json @@ -0,0 +1,4 @@ +{ + "main": "index.js", + "type": "module" +} \ No newline at end of file diff --git a/test/parallel/test-buffer-constructor-node-modules.js b/test/parallel/test-buffer-constructor-node-modules.js new file mode 100644 index 00000000000000..1c915a7fc91405 --- /dev/null +++ b/test/parallel/test-buffer-constructor-node-modules.js @@ -0,0 +1,48 @@ +'use strict'; + +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); + +if (process.env.NODE_PENDING_DEPRECATION) + common.skip('test does not work when NODE_PENDING_DEPRECATION is set'); + +spawnSyncAndAssert( + process.execPath, + [ fixtures.path('warning_node_modules', 'new-buffer-cjs.js') ], + { + trim: true, + stderr: '', + } +); + +spawnSyncAndAssert( + process.execPath, + [ fixtures.path('warning_node_modules', 'new-buffer-esm.mjs') ], + { + trim: true, + stderr: '', + } +); + +spawnSyncAndAssert( + process.execPath, + [ + '--pending-deprecation', + fixtures.path('warning_node_modules', 'new-buffer-cjs.js') + ], + { + stderr: /DEP0005/ + } +); + +spawnSyncAndAssert( + process.execPath, + [ + '--pending-deprecation', + fixtures.path('warning_node_modules', 'new-buffer-esm.mjs') + ], + { + stderr: /DEP0005/ + } +);