Skip to content

Commit

Permalink
src: implement IsInsideNodeModules() in C++
Browse files Browse the repository at this point in the history
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: #55286
Backport-PR-URL: #55217
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Refs: #52697
  • Loading branch information
joyeecheung authored and ruyadorno committed Nov 27, 2024
1 parent fcdd616 commit 2af72a7
Show file tree
Hide file tree
Showing 10 changed files with 108 additions and 31 deletions.
6 changes: 4 additions & 2 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ const {
ONLY_ENUMERABLE,
},
getOwnNonIndexProperties,
isInsideNodeModules,
} = internalBinding('util');
const {
customInspectSymbol,
isInsideNodeModules,
lazyDOMException,
normalizeEncoding,
kIsEncodingSymbol,
Expand Down Expand Up @@ -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;
}

Expand Down
29 changes: 0 additions & 29 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const {
ArrayFrom,
ArrayIsArray,
ArrayPrototypePush,
ArrayPrototypeSlice,
ArrayPrototypeSort,
Expand Down Expand Up @@ -34,9 +33,7 @@ const {
SafeSet,
SafeWeakMap,
SafeWeakRef,
StringPrototypeIncludes,
StringPrototypeReplace,
StringPrototypeStartsWith,
StringPrototypeToLowerCase,
StringPrototypeToUpperCase,
Symbol,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -911,7 +883,6 @@ module.exports = {
getSystemErrorName,
guessHandleType,
isError,
isInsideNodeModules,
isUnderNodeModules,
isMacOS,
isWindows,
Expand Down
44 changes: 44 additions & 0 deletions src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,48 @@ 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);
Expand All @@ -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<Object> target,
Expand Down Expand Up @@ -396,6 +439,7 @@ 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);
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/warning_node_modules/new-buffer-cjs.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/warning_node_modules/new-buffer-esm.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions test/parallel/test-buffer-constructor-node-modules.js
Original file line number Diff line number Diff line change
@@ -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/
}
);

0 comments on commit 2af72a7

Please sign in to comment.