Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesParsing context realm
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on bun 1.4.0 and on main with the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — swaps the realm argument of constructAnonymousFunction to options.parsingContext, matching Node's node_contextify.cc.
What was reviewed:
options.parsingContextis always non-null (defaulted inCompileFunctionOptions::fromJSand thevmModuleCompileFunctionfallback), and captured to a local beforeWTF::move(options).createBufferforcachedDatastill routes throughdefaultGlobalObject()->JSBufferSubclassStructure(), so the buffer stays a caller-realmBuffer(test covers it);lexicallyScopedFeaturesandgetBytecodenow both read the same global on produce and consume.- Realm move is a hardening, not a weakening: the function's structure/prototype now lives in the sandbox realm it was already scoped to, so
fn.constructorno longer hands back the caller'sFunction. - Dead second
if (!function)afterfetcher->owner()correctly removed.
Extended reasoning...
Overview
Changes the first argument of constructAnonymousFunction in vmModuleCompileFunction from the caller's globalObject to options.parsingContext, so compile errors and the returned function are created in the parsing context's realm (matching Node). Also routes the Failed to compile function fallback throwVMError through the same realm, removes an unreachable duplicate null check, and adds a header comment on constructAnonymousFunction documenting the realm parameter. Seven new tests cover the SyntaxError/RangeError realm, the returned function's prototype realm, Error.prepareStackTrace dispatch, the cachedData Buffer realm, and the no-option default.
Security risks
node:vm is isolation-adjacent, so I checked realm direction carefully. The function's scope chain was already parsingContext before this change; only its structure (prototype) moved. That means Object.getPrototypeOf(fn).constructor now resolves to the sandbox's Function instead of the caller's — a hardening, not an escape vector. Compile errors are now created in the context realm, so the context's Error.prepareStackTrace runs; a throw from it is already swallowed by the existing tryClearException (test asserts this). No new caller-realm objects are exposed to the context. cachedData remains a host-realm Buffer because WebCore::createBuffer takes its structure from defaultGlobalObject().
Level of scrutiny
Moderate: C++ JSC bindings in a compat-sensitive path, but the effective change is a one-argument swap plus dead-code deletion. options.parsingContext is guaranteed non-null (fromJS sets it to the caller's global first thing; the !fromJS branch in vmModuleCompileFunction re-defaults it), so there's no new null-deref path. The local parsingContext is captured before WTF::move(options), so the post-move throwVMError use is safe. No new GC roots are needed — the same pointer was already held across the call as functionScope.
Other factors
The PR description traces every use of the realm parameter inside constructAnonymousFunction (ProgramExecutable::create, selectStructureForNewFuncExp, toErrorObject, decorateParseErrorStack, createBuffer, getBytecode) and I spot-checked createBuffer/createUninitializedBuffer and the lexicallyScopedFeatures/SourceCodeKey derivation to confirm produce and consume stay symmetric. Tests are specific, assert both the positive (context realm) and negative (not.toBeInstanceOf(Error)) contract, restore mutated globals in finally, and pin unchanged behavior for the no-option case and cachedData. The bug-hunting pass found nothing.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes which realm vm.compileFunction compiles in — sandbox-boundary code where the fix itself closes an arguments.callee.constructor escape — a human look at the realm semantics is still worthwhile.
What was reviewed:
options.parsingContextis captured into a local beforeWTF::move(options)and is always non-null (defaulted to the caller's global inCompileFunctionOptions::fromJSand the fallback branch).createBufferforcachedDataresolves throughdefaultGlobalObject, so the Buffer stays a caller-realm Buffer (test pins this); theSourceCodeKey'slexicallyScopedFeaturesnow reads from the same global thatsetGlobalScopeExtensionwrites, so produce/consume keys match.- The removed second
if (!function)was unreachable —fetcher->owner()cannot null it.
Extended reasoning...
Overview
Swaps the realm argument of constructAnonymousFunction in vmModuleCompileFunction from the caller's globalObject to options.parsingContext, so the compiled function's structure and any parse-error object are created in the parsing context's realm. Also routes the throwVMError fallback through the same global, deletes an unreachable duplicate null-check, and adds a one-line comment on the constructAnonymousFunction declaration. Eight new tests cover error realm, function realm, the arguments.callee.constructor escape, Error.prepareStackTrace dispatch, cachedData round-trip, and unchanged default behavior.
Security risks
This is node:vm realm-boundary code. The change is a security improvement (closes a documented escape from a parsingContext sandbox back to the caller's globalThis), but any mistake in which realm's intrinsics or prepareStackTrace runs could reintroduce a leak. I traced the affected paths inside constructAnonymousFunction (toErrorObject, selectStructureForNewFuncExp, createBuffer, getBytecode) and each behaves as the PR description states, but sandbox-boundary changes warrant maintainer sign-off.
Level of scrutiny
High. node:vm isolation is security-relevant even though vm is not a hard sandbox; the change alters which realm's user hooks (Error.prepareStackTrace) run and which Function.prototype the returned function inherits.
Other factors
The C++ diff is effectively a one-argument swap with strong test coverage that fails on main and passes with the fix (per the description). parsingContext is captured before WTF::move(options), is guaranteed non-null, and setGlobalScopeExtension was already being called on it before this PR — the read side (globalScopeExtension() inside constructAnonymousFunction) is now aligned with the write side. The comment-cop bot's note was addressed in 41b0956. No outstanding reviewer comments.
|
Updated 6:03 AM PT - Aug 14th, 2026
❌ @robobun, your commit ade2b23 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38297That installs a local version of the PR into your bun-38297 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes which JSC realm constructAnonymousFunction creates its output in — cross-realm semantics in the node:vm sandbox layer — a maintainer look would still be worthwhile.
What was reviewed:
parsingContextis always non-null (CompileFunctionOptions::fromJSdefaults it to the caller's global; the!fromJSfallback re-assigns it), and is captured into a local beforeWTF::move(options).- Traced downstream uses of the swapped
globalObjectargument:createBufferroutes throughdefaultGlobalObject()socachedDatastays a caller-realm Buffer;decorateParseErrorStackmarks the parameter unused;lexicallyScopedFeaturesandgetBytecodenow both read the same global's scope extension (previously they could disagree). - Removed second
if (!function)was provably dead — the first one returns.
Extended reasoning...
Overview
The PR changes one argument at one call site in vmModuleCompileFunction (src/jsc/bindings/NodeVM.cpp): constructAnonymousFunction and the fallback throwVMError now receive options.parsingContext instead of the caller's globalObject. It also removes an unreachable duplicate if (!function) block, adds a one-line doc comment on the header declaration explaining the realm parameter, and adds an 8-test describe("parsingContext") block to test/js/node/vm/vm.test.ts. The net effect is that the compiled function's structure and any compile-time error are created in the parsingContext realm, matching Node.
Security risks
node:vm is not a security boundary per Node's own documentation, but this change is realm-isolation-adjacent: the added test the body cannot reach the caller's realm through its own function object demonstrates that before the fix, arguments.callee.constructor on the compiled function was the caller's Function, letting body code reach the caller's globalThis. The fix closes that. I did not find a way this change weakens isolation — it tightens it. The cachedData Buffer stays in the caller's realm because WebCore::createBuffer unconditionally uses defaultGlobalObject(...)->JSBufferSubclassStructure(), which I verified in JSBuffer.cpp:471.
Level of scrutiny
This is a small, focused change with an unusually thorough PR description that traces every downstream consumer of the swapped argument (ProgramExecutable::create, toErrorObject, selectStructureForNewFuncExp, decorateParseErrorStack, createBuffer, getBytecode). I spot-checked the non-obvious claims (createBuffer's default-global routing, decorateParseErrorStack's UNUSED_PARAM) and they hold. The tests are Node-verified and cover error realm, function realm, the callee-constructor escape, cachedData realm invariance, and Error.prepareStackTrace dispatch (including the throwing case). That said, cross-realm object creation in JSC bindings is subtle territory — the lexicallyScopedFeatures derivation, setGlobalScopeExtension, and SourceCodeKey interaction all now key off a NodeVMGlobalObject rather than the Zig global — and a maintainer familiar with NodeVM's realm handling should confirm the trace.
Other factors
No CODEOWNERS entry covers these paths. The comment-cop bot's feedback (paragraph-long comment) was addressed in commit 41b0956. No outstanding human review comments. The bug-hunting system found nothing. The two sibling entry points with the same class of bug (runInContext/runInNewContext in NodeVMScript.cpp, SourceTextModule in NodeVMSourceTextModule.cpp) are explicitly scoped out with linked follow-up PRs, which is a reasonable split since each derives its target realm differently.
…realm vmModuleCompileFunction passed the caller's global object to constructAnonymousFunction, so a compile error (and the compiled function's prototype) came from the caller's realm while the source was scoped to options.parsingContext. Node compiles inside the parsing context, so the SyntaxError is that context's SyntaxError and the function is that context's Function. Pass the parsing context as the compile realm instead; cachedData is still a Buffer of the caller's realm, as in Node.
…alm via its function object
ade2b23 to
7b8bf9d
Compare
|
On the "context extensions visible on the long-lived parsing context" note in the review summary: that is pre-existing. The |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is small and well-reasoned, and the new tests are thorough. Because it changes which realm compile errors and the returned function are created in — vm sandbox-boundary behavior — and CI shows two macOS-only failures (vite-build.test.ts, astro-post.test.js) that haven't been triaged as unrelated, a human look would still be worthwhile.
What was reviewed
options.parsingContextis always non-null (defaulted to the caller's global in bothfromJSand the fallback branch), and is captured into a local beforeWTF::move(options), so no null deref / use-after-move.createBufferforcachedDataresolves its structure viadefaultGlobalObject(), so the Buffer stays a caller-realm Buffer as the test asserts; theSourceCodeKeyfor produce/consume both readglobalScopeExtension()from the same parsingContext.- Confirmed
setGlobalScopeExtensionwas already called onoptions.parsingContextbefore this change — only the compile-realm argument moved.
Extended reasoning...
Overview
The PR changes vmModuleCompileFunction in src/jsc/bindings/NodeVM.cpp to pass options.parsingContext (instead of the caller's globalObject) as the realm argument to constructAnonymousFunction and to the fallback throwVMError. It also removes an unreachable duplicate if (!function) block and adds a one-line contract comment on the constructAnonymousFunction declaration in NodeVM.h. Eight new tests in test/js/node/vm/vm.test.ts cover the error realm, function realm, arguments.callee.constructor escape, the no-option default, cachedData cross-context reuse, and the context's Error.prepareStackTrace (including a throwing one).
Security risks
This is realm-boundary code for node:vm. The change tightens isolation to match Node: previously a body compiled with parsingContext could reach the caller's globalThis (and process) via arguments.callee.constructor; now it cannot. I did not find a way this weakens isolation — the only realm that shifts is the one the compile output/error is created in, and createBuffer for cachedData still routes through defaultGlobalObject() so the host-realm Buffer subclass is used regardless. Still, cross-realm object creation in JSC is subtle and this is exactly the kind of boundary a maintainer should sign off on.
Level of scrutiny
Medium-high. The native diff is only a few lines, but it changes which JSGlobalObject flows into ParserError::toErrorObject, JSFunction::selectStructureForNewFuncExp, getBytecode, and throwVMError. The PR description audits each of these downstream uses and the tests pin them, but the two macOS-only CI failures (third-party integration tests) should be confirmed as pre-existing flakes before merge — Vite/Astro are unlikely to pass parsingContext, but it's worth a glance.
Other factors
CompileFunctionOptions::fromJS unconditionally sets this->parsingContext = globalObject before reading options, and the caller's fallback branch does the same, so parsingContext cannot be null. The local JSGlobalObject* parsingContext = options.parsingContext; is captured before WTF::move(options), avoiding use-after-move. The comment-cop bot's remark was addressed (comment trimmed to one line). The dead second null-check was correctly removed. No prior automated review from me on this PR.
|
Triage of the two macOS failures from the earlier builds ( |
This branch has not been deployed
1 participant
Problem
vm.compileFunction(code, params, { parsingContext })throws a compile error from the caller's realm:err instanceof SyntaxErroristrueanderr instanceof vm.runInContext("SyntaxError", parsingContext)isfalse. Node (v26.3.0) givesfalse/true. Same for the parser'sRangeErroron a body too deep to parse and for theFailed to compile functionfallback.Object.getPrototypeOf(fn)is the caller'sFunction.prototype; in Node it is the context's. Because of that, the body could get out of the context through its own function object:compileFunction("return new (arguments.callee.constructor)('return globalThis')()", [], { parsingContext })()returns the caller'sglobalThis(withprocesson it) on bun 1.4.0; Node returns the context's.Error.prepareStackTrace; Node runs the context's.vmModuleCompileFunction(src/jsc/bindings/NodeVM.cpp, theconstructAnonymousFunctioncall near line 1625) passes its ownglobalObject, the realmvm.compileFunctionitself lives in, as the realm to compile in;options.parsingContextonly reaches the call as the scope chain.constructAnonymousFunctionthat realm argument is whatParserError::toErrorObjectcreates the error from and whatJSFunction::selectStructureForNewFuncExptakes the function's structure from, so both come out in the wrong realm. The fallbackthrowVMErrorused the same global.Fix
options.parsingContextas the realm argument ofconstructAnonymousFunctionand throw the fallback from it too. It is always set (CompileFunctionOptions::fromJSdefaults it to the caller's global), so without the option the two globals are the same object and nothing changes.node_contextify.ccenters the parsing context beforeScriptCompiler::CompileFunction, so the error and the function are created in it. It is also what JSC does on its own: the parse error of a lazily compiled function is created in the realm of the scope it is linked against (ScriptExecutable::newCodeBlockFor), and a function's structure realm normally equals the realm of its scope chain, which this restores.constructAnonymousFunctionmoves realm:ProgramExecutable::createonly takes the VM from the argument,ProgramCodeBlock::createandJSFunction::createalready received the parsing context's scope,decorateParseErrorStackignores it, andcreateBufferalways buildscachedDatafrom the default global, so it stays a Buffer of the caller's realm like Node'sBuffer::Copy(env, ...). ThecachedDatakey andgetBytecodestill read one and the same global on the produce and consume sides.Error.prepareStackTraceruns with no further change becausecomputeErrorInfo(FormatStackTraceForJS.cpp) dispatches on the error object's own realm; a throwing one is still swallowed by the existingtryClearException, as in the caller-realm case.if (!function)afterfetcher->owner(...).constructAnonymousFunction: it creates the error in whatever realm it is handed, and this PR only changes what is handed in.node:vmhas three compile entry points that create their compile error from the caller's global instead of the target context's. This PR is thecompileFunctionone (NodeVM.cpp), the only one of the three where the compiled code object itself is mis-realmed too.runInContext/runInNewContext(NodeVMScript.cpp, Node'skParsingContext) is node:vm: throw runInContext/runInNewContext compile errors from the context's realm #38317, andnew SourceTextModule(code, { context })(NodeVMSourceTextModule.cpp,createModuleRecord) is left for a separate change; each derives its target realm from a different option and lives in a different constructor, so they are kept as separate changes. node:vm: allocate prepareStackTrace call sites in the vm realm #29998 covers the relatedprepareStackTracecall-site allocations.setGlobalScopeExtension(functionScope)call in the diff is the pre-existing line, now made through the new local. That it installs the contextExtensions chain on the parsing context for good is a separate pre-existing bug, fixed by node:vm: stop leaking compileFunction contextExtensions into later global lookups #38302.compileFunction() > parsingContextblock intest/js/node/vm/vm.test.ts: 8 tests, 6 fail on main and on bun 1.4.0, the 2 that pin unchanged behavior (default realm,cachedData) pass either way, all 8 pass with the fix, and the same assertions hold under node v26.3.0.test/js/node/vm/(script-leak.test.tstimes out locally under ASAN independent of this change),test/js/node/test/parallel/test-vm-basic.js,test-vm-module-basic.js,test-vm-no-dynamic-import-callback.js,test/regression/issue/isArray-proxy-crash.test.ts.Background
Error,SyntaxError,Function.prototype, ...). Eachvmcontext is a realm of its own (aNodeVMGlobalObject), andinstanceofagainst another realm's constructor is false. In JSC an object's realm is recorded in itsStructure, so the realm of a new error or function is decided by which global object its structure is taken from.parsingContext: thecompileFunctionoption naming the context to compile in. Bun keeps it inCompileFunctionOptions::parsingContextand builds the function's scope chain from it; before this change that scope chain was its only use.constructAnonymousFunction: wraps params and body into a(function (...) {...})program, compiles it, and returns the function expression. Its first parameter is the global object the error or the function is created from; the header comment now says so.Error.prepareStackTrace: the V8-style hook on a realm'sErrorconstructor that formats.stack. Bun looks it up on theErrorof the realm the error object was created in, which is why the fix also changes which hook runs.