From ececd225b66f4138977c36b8ee8d570d7d361521 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 Backport-PR-URL: https://github.com/nodejs/node/pull/56927 Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Refs: https://github.com/nodejs/node/issues/52697 --- lib/buffer.js | 6 ++- lib/internal/util.js | 29 ----------- src/node_util.cc | 50 +++++++++++++++++++ .../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, 114 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 e78e2e3bb6053e..86d9803ec19ced 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -77,10 +77,10 @@ const { ONLY_ENUMERABLE, }, getOwnNonIndexProperties, + isInsideNodeModules, } = internalBinding('util'); const { customInspectSymbol, - isInsideNodeModules, lazyDOMException, normalizeEncoding, kIsEncodingSymbol, @@ -176,13 +176,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 ea1ef8ca90c626..7260b8e5e64734 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -3,7 +3,6 @@ const { ArrayBufferPrototypeGetByteLength, ArrayFrom, - ArrayIsArray, ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSort, @@ -35,9 +34,7 @@ const { SafeSet, SafeWeakMap, SafeWeakRef, - StringPrototypeIncludes, StringPrototypeReplace, - StringPrototypeStartsWith, StringPrototypeToLowerCase, StringPrototypeToUpperCase, Symbol, @@ -509,31 +506,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; @@ -916,7 +888,6 @@ module.exports = { guessHandleType, isArrayBufferDetached, isError, - isInsideNodeModules, isUnderNodeModules, join, lazyDOMException, diff --git a/src/node_util.cc b/src/node_util.cc index 019990c688feee..36304498982700 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -254,6 +254,54 @@ static void ParseEnv(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(dotenv.ToObject(env)); } +bool starts_with(std::string_view view, std::string_view prefix) { + return view.size() >= prefix.size() && + view.substr(0, prefix.size()) == prefix; +} + +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 (starts_with(script_name_str, + "node:")) { // Ported to work with C++17 on 20.x. + 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); @@ -269,6 +317,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, @@ -338,6 +387,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/ + } +);