From 54404c64e7de0e9f0eb6552c089fa05fa7f2303c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 8 Oct 2024 12:19:46 +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. PR-URL: https://github.com/nodejs/node/pull/55286 Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu --- lib/buffer.js | 6 ++- lib/internal/util.js | 29 ----------- src/node_util.cc | 44 +++++++++++++++++ .../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, 108 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 8cba1c90ab8189..a3ffbbdc6b350f 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -2,7 +2,6 @@ const { ArrayFrom, - ArrayIsArray, ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSort, @@ -34,9 +33,7 @@ const { SafeSet, SafeWeakMap, SafeWeakRef, - StringPrototypeIncludes, StringPrototypeReplace, - StringPrototypeStartsWith, StringPrototypeToLowerCase, StringPrototypeToUpperCase, Symbol, @@ -513,31 +510,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; @@ -911,7 +883,6 @@ module.exports = { getSystemErrorName, guessHandleType, isError, - isInsideNodeModules, isUnderNodeModules, isMacOS, isWindows, diff --git a/src/node_util.cc b/src/node_util.cc index c99e26752c7d93..77da3235182efe 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -298,6 +298,48 @@ static void GetCallSite(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(callsites); } +static void IsInsideNodeModules(const FunctionCallbackInfo& 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()->Value(); + Local 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]); + } + + bool result = false; + for (int i = 0; i < frame_count; ++i) { + Local stack_frame = stack->GetFrame(isolate, i); + Local script_name = stack_frame->GetScriptName(); + + if (script_name.IsEmpty() || script_name->Length() == 0) { + continue; + } + Utf8Value script_name_utf8(isolate, script_name); + std::string_view script_name_str = script_name_utf8.ToStringView(); + 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) { registry->Register(GetPromiseDetails); registry->Register(GetProxyDetails); @@ -313,6 +355,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(FastGuessHandleType); registry->Register(fast_guess_handle_type_.GetTypeInfo()); registry->Register(ParseEnv); + registry->Register(IsInsideNodeModules); } void Initialize(Local target, @@ -396,6 +439,7 @@ void Initialize(Local 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..e865b9e0ae2f0b --- /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/ + } +);