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.
nodejs · nodejs-github-bot · Oct 8, 2024 · Oct 4, 2024 · Oct 7, 2024 · Oct 7, 2024
commit eb8eba815f64d375f46224c9934079e0297e8a8a
diff --git 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
@@ -2,7 +2,6 @@
 
 const {
   ArrayFrom,
-  ArrayIsArray,
   ArrayPrototypePush,
   ArrayPrototypeSlice,
   ArrayPrototypeSort,
@@ -34,9 +33,7 @@ const {
   SafeSet,
   SafeWeakMap,
   SafeWeakRef,
-  StringPrototypeIncludes,
   StringPrototypeReplace,
-  StringPrototypeStartsWith,
   StringPrototypeToLowerCase,
   StringPrototypeToUpperCase,
   Symbol,
@@ -516,31 +513,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 +886,6 @@ module.exports = {
   getSystemErrorName,
   guessHandleType,
   isError,
-  isInsideNodeModules,
   isUnderNodeModules,
   isMacOS,
   isWindows,

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -298,6 +298,47 @@ static void GetCallSite(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(callsites);
 }
 
+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]);
+  }
+
+  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;
+    }
+    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 +354,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 +438,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
diff --git a/test/fixtures/warning_node_modules/new-buffer-esm.mjs b/test/fixtures/warning_node_modules/new-buffer-esm.mjs
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
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
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
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
diff --git a/test/parallel/test-buffer-constructor-node-modules.js 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/
+  }
+);