Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughString-valued export replacements now use arena-backed expressions. A subprocess regression test checks replacement values after CommonJS and JSON module loads in synchronous and asynchronous transforms. ChangesExport replacement lifetime
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to String-valued export replacements remain available after other modules load, and the regression test covers both transform APIs. No actionable merge-blocking risk is established. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix and test are in this PR (head 7a815dd), waiting on CI. Reproduced on a debug build at With the change, all of those forms print the replacement after the Review so far: the two flagged comments were trimmed in d96761b (threads resolved); 7a815dd added the debug guard and the release-detectable fixture described in the PR body. Nothing outstanding. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line allocator fix moving the E::EString node from the thread-local AST store into the instance arena, with a subprocess regression test.
What was reviewed:
- Confirmed
Expr::allocate(bump, ..)exists and places the node in the caller's bump arena; thearenapassed here is the boxedJSTranspiler.arena, which outlivesconfig.runtime.replace_exports. - Test covers plain, inject, and
defaultstring forms via bothtransformSync()andtransform(), drains pipes concurrently, and asserts exact output;tempDir/bunExe/bunEnvare already imported at line 2. - The 60s timeout is justified by the comment (only the pre-fix failing path is slow, symbolizing the child's sanitizer report).
Extended reasoning...
Overview
The PR changes one call in export_replacement_value (src/runtime/api/JSTranspiler.rs) from Expr::init(..) to Expr::allocate(arena, ..), updates the field comment on JSTranspiler.arena to mention the new resident, and adds a spawned-child regression test to test/bundler/transpiler/transpiler.test.js.
The mechanism is exactly as described: Expr::init (src/ast/expr.rs:1061) puts heap-sized node kinds like E::EString into the thread-local data::Store, which is reset by the next main-thread parse; Expr::allocate (src/ast/expr.rs:1052) puts the node into the caller-supplied bump arena instead. The arena argument threaded through Config::from_js → export_replacement_value is arena_ref, a &'static view of the Box<Arena> that becomes JSTranspiler.arena and is dropped only with the JSTranspiler itself. The string bytes were already copied into that arena via alloc_slice_copy; now the node struct joins them, so the Expr stored in config.runtime.replace_exports no longer dangles after an intervening require().
Security risks
None. This is an internal allocator-choice fix for a use-after-free; no new input parsing, no boundary changes.
Level of scrutiny
Memory-safety fix in native code, so I traced the arena provenance end-to-end (constructor → from_js → export_replacement_value) and confirmed Expr::allocate's signature matches. The functional change is a single well-understood substitution; the rest is comments and a test. Both Expr::init and Expr::allocate call data::Store::assert(), so no new precondition is introduced.
Other factors
The test follows the harness conventions closely: tempDir + Bun.spawn with bunEnv, concurrent Promise.all on stdout/stderr/exited, stderr asserted empty before parsing stdout, exact-value toEqual on the six outputs, exitCode asserted last. It exercises every string-value code path (plain replace, inject-form array, default) through both sync and async transforms after two different store-resetting operations (CJS require and JSON import). The 60s per-test timeout was flagged and ruled out — the comment names a real reason (child sanitizer symbolization on the pre-fix build), and the passing path is fast. No prior human review comments to address.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 7:29 PM PT - Sep 28th, 2026
✅ @robobun, your commit 1e3a2881ca37a8f73d18e34a9eb8c30b1e59938c passed in 🧪 To try this PR locally: bunx bun-pr 38541That installs a local version of the PR into your bun-38541 --bun |
|
Two small follow-up pushes since the review above, no functional change:
|
There was a problem hiding this comment.
LGTM — one-line allocator swap that moves the E::EString node for exports.replace string values into the same instance arena that already holds its bytes, fixing a UAF when the thread-local AST store is reset between construction and transform.
What was reviewed:
Expr::allocatesemantics insrc/ast/expr.rs— its doc comment matches this exact use case (node must outlivereset()), andStore::assert()there is the same guardExpr::initalready passed.- Arena lifetime:
JSTranspiler.arenais aBox<Arena>never reset and dropped with theConfigholding theExpr; the asyncTransformTaskpath holds aStrongon the wrapper so the arena outlives the off-thread read. - Test: hermetic spawned fixture in the existing
transpiler.test.js, drains pipes concurrently, covers sync/async × plain/inject/default, asserts exact output;tempDir/bunExe/bunEnvalready imported at line 2.
Extended reasoning...
Overview
The PR changes one call site in src/runtime/api/JSTranspiler.rs::export_replacement_value: Expr::init(EString::init(data), ..) → Expr::allocate(arena, EString::init(data), ..). The arena is the JSTranspiler's own Box<Arena>, already threaded through Config::from_js and already used one line above to hold the string bytes (arena.alloc_slice_copy(&buf)). The change puts the E::EString struct in the same arena as its payload instead of the thread-local AST slab, which is bulk-reset by any subsequent main-thread parse. A regression test is added to test/bundler/transpiler/transpiler.test.js.
Security risks
None. This is an internal allocation-lifetime fix in the transpiler option parser; no input validation, auth, crypto, or trust boundary is touched. The change strictly narrows a UAF window (moving an allocation from a shorter-lived store to a longer-lived one that already owns related data).
Level of scrutiny
Low-to-medium. The functional change is a single call-site swap between two well-documented Expr constructors whose doc comment (src/ast/expr.rs:1048-1050) describes exactly this scenario. I verified: (a) Expr::allocate's signature and semantics match — it bump-allocates the node in the caller's arena via into_data_alloc; (b) the Store::assert() it calls is the same debug guard Expr::init already satisfied at this site, so no new precondition is introduced; (c) the arena passed here is JSTranspiler.arena: Box<Arena>, address-stable, never reset, dropped with the struct that holds the Expr in config.runtime.replace_exports; (d) the async transform() path clones replace_exports.entries and keeps the JSTranspiler alive via TransformJs._transpiler: Strong, so the arena outlives the off-thread parse's read of the node.
Other factors
The test follows repo harness conventions: spawned subprocess with bunExe()/bunEnv, tempDir fixture, Promise.all over stdout/stderr/exited, exact-value assertions on JSON-parsed output before the exit-code check, no network, no sleeps. It covers the variant matrix that goes through the changed function (plain replace, inject form, export default, sync and async) and the two module-load paths that reset the store (require of CJS, import of JSON). The PR description documents that the test crashes under ASAN without the fix and passes with it, and that the full transpiler.test.js suite still passes. The two comment-cop inline threads were addressed in d96761b (comments trimmed) and are resolved. No outstanding human review comments.
|
Pushed 7a815dd after a second look at the change, two things came out of it (PR description updated to match):
Also fixed the The previous build (96157) ended with no test failures; its two macOS aarch64 jobs expired waiting for an agent, so this push also gives it a fresh run. |
There was a problem hiding this comment.
LGTM — targeted use-after-reset fix.
What was reviewed:
Expr::allocate(arena, ..)vsExpr::init: verifiedinto_data_allocputs theE::EStringnode in the caller's bump (the boxedJSTranspiler.arena, which lives as long asconfig.runtime.replace_exports), whileinto_data_storewould append to the thread-local slab thatStore::begin()resets — matches the pattern used fordefinevalues.Disabler::scope()guard: debug-only (bodies are#[cfg(debug_assertions)]-gated, no-op in release);Expr::allocatecallsStore::assert()but notDisabler::assert(), and the inline bool/number/null/undefined arms don't touch the store, so the guard cannot fire on the current code — it just fences future reintroduction.- Test follows harness conventions (
tempDir/bunEnv/bunExealready imported, concurrent pipe drain, exact-value assertions,exitCodelast) and the 300-stringreset.cjsrefills the slab so the assertion also fails on release builds, not just via debug poisoning.
Extended reasoning...
Overview
One-line functional change in src/runtime/api/JSTranspiler.rs: export_replacement_value now builds string replacement nodes with Expr::allocate(arena, ..) instead of Expr::init(..), so the E::EString struct lands in the same Box<Arena> that already holds its bytes rather than in the thread-local AST slab that any subsequent main-thread parse resets. A debug-only bun_ast::expr::Disabler::scope() guard is armed for the duration of the function to catch any future store-backed allocation in this scope. src/ast/expr.rs gets a one-line doc tweak on Expr::allocate (the old comment referenced a non-existent Expr.alloc). A new spawned-fixture test in test/bundler/transpiler/transpiler.test.js covers the plain, inject and default replacement forms through both transformSync() and transform().
Security risks
None. This is a lifetime fix for constructor-time option data in Bun.Transpiler; no new inputs, parsing, or trust boundaries. The change removes a use-after-reset (freed-store read) rather than introducing one.
Level of scrutiny
Memory-safety adjacent, so I traced the mechanism rather than taking the description at face value. Expr::init → into_data_store → data::Store::append writes into the per-thread slab (new_store.rs:470-482) that begin() resets; Expr::allocate → into_data_alloc → bump.alloc writes into the passed arena (expr.rs:1023-1031, 1052-1057). The arena passed here is the Box<Arena> field on JSTranspiler, which is never reset and drops with the struct that owns config.runtime.replace_exports — so node lifetime now matches pointer lifetime exactly. The string bytes were already in that arena (arena.alloc_slice_copy), so this just brings the containing struct to the same place. I confirmed the Disabler guard is release-no-op (all three methods are #[cfg(debug_assertions)]-gated bodies) and that neither Expr::allocate nor the four inline-ExprData arms hit Disabler::assert(), so the guard is inert on the fixed code and only trips if someone reintroduces a store-backed node here.
Other factors
The test is well-constructed: it spawns a child so the store state is clean, forces a slab reset via both require() of a CJS file and import() of JSON, and — importantly — has reset.cjs declare 300 string constants so the reused slab slot actually holds different bytes in release builds (where the store isn't 0xAA-poisoned), making the JSON.parse(stdout) equality assertion fail without the fix even outside debug/ASAN. Harness conventions are followed (tempDir, bunEnv, concurrent Promise.all drain, stderr/stdout asserted before exitCode, no per-test timeout override). The two comment-cop bot flags were addressed in d96761b and both threads are marked resolved; the remaining src/ diff is the allocate change plus one explanatory line and the guard. Nothing else outstanding on the timeline.
…e arena export_replacement_value copied the string bytes into the JSTranspiler's arena but built the E::EString node with Expr::init, which appends the node to the thread-local AST store. The Expr is kept in config.runtime.replace_exports for the life of the transpiler, while the next synchronous parse on the thread (a require() of a CommonJS file, a JSON import, ...) resets that store, so later transforms printed the replacement from a freed slot. Allocate the node in the same arena as its bytes with Expr::allocate; the arena lives exactly as long as the config that holds the Expr. Disabler::scope() makes a debug build panic if anything in this function appends to the thread-local Expr store again. The required module in the test declares enough strings to reuse the slot the replacement node used to occupy, so the test also fails on a release build without the fix.
|
Rebased onto The rebase had one conflict, in Verification on the new head, with #44137 fixes an abort of |
7a815dd to
4881aa2
Compare
There was a problem hiding this comment.
LGTM. The comment nit from the previous review is addressed in a90fada, and the fix itself is a one-call change with a regression test.
What was reviewed:
- Arena lifetime for the replacement
Expr:JSTranspiler._arenais aBox<Arena>dropped with the instance, and the async path'sTransformJsholds ajsc::Strongon the wrapper, so the node outlives every sync and async transform that reads it. - The remaining arms of
export_replacement_value(boolean/number/null/undefined) buildExprDatainline with no store append, so theDisabler::scope()guard cannot trip on them;Expr::allocateonly callsStore::assert(), which checks initialization, not the disabled flag. - Same bug class elsewhere: the
defineoption goes throughDefineData::parse, which alreadydeep_clones into the bump arena, so no sibling site is left on the thread-local store. - Test:
await usingon the subprocess, pipes drained viaPromise.all, stderr asserted before the exit code, exact expected output for all six sync/async transforms; the serialit(...)matches the neighboring tests in this describe block.
Extended reasoning...
The diff touches src/runtime/api/JSTranspiler.rs (swap Expr::init for Expr::allocate(arena, ..) in export_replacement_value plus a debug-only Disabler guard), a doc-comment rewrite in src/ast/expr.rs, and a new subprocess regression test in test/bundler/transpiler/transpiler.test.js. It touches no security-sensitive surface; it is a memory-lifetime fix for AST nodes that previously pointed into a thread-local slab reset by later parses. Approval is decided by the change being three lines of production code whose ownership story checks out from the code (boxed instance arena, Strong ref held by the async job), the only sibling site (defines) already handling the same class, no CODEOWNERS entry covering the changed files, and the prior review's single nit having been addressed by the follow-up commit.
|
This PR is closed and its change is in #44137. A follow-up for the change is in #44137 (comment):
|
1 participant
Problem
new Bun.Transpiler({ exports: { replace: { foo: "bar" } } })followed by any synchronous parse on the same thread (arequire()of a CommonJS file, animportof a.jsonfile, ...) and then atransformSync()/transform()of a module that exportsfooprints the replacement from memory that no longer belongs to it.0xAAon reset):export const foo = "";) or dies withpanic(main thread): Segmentation fault ... Crashed while printing input.jsx, depending on what landed there.export_replacement_valueinsrc/runtime/api/JSTranspiler.rscopies the string bytes into theJSTranspiler's own arena but builds the node withExpr::init, which appends theE::EStringstruct to the thread-local AST store. The returnedExpr(a pointer to that struct) is kept inconfig.runtime.replace_exportsfor the life of the transpiler and copied into every later parse, while the next parse on the main thread resets that store and fills it with its own nodes.nullandundefinedare stored inline inExprData. Both the plain form (foo: "bar") and the inject form (foo: ["name", "bar"]) go through this function.Fix
Expr::allocate(arena, ..)instead ofExpr::init(..), so theE::EStringstruct lives in the same arena as its bytes.JSTranspiler.arena) is never reset and is dropped together with theConfigholding theExpr, so the node lives exactly as long as the pointer to it. This is also what the code did before the port (allocator.create(E.String)on the instance allocator), and whatDefineData::parseinsrc/bundler/defines.rsdoes for the same reason withdefinevalues.export_replacement_valuenow also holdsbun_ast::expr::Disabler::scope(), the existing debug-only guard used byPackageJSONEditorfor the same situation: if this function ever appends to the thread-local store again, debug builds panic at construction time ([bun_ast::expr::Expr] called while disabled), which every existingexports.replacetest would hit. Checked by temporarily puttingExpr::initback:transpiler.test.jsaborts at module load with that panic.Expr::allocatenever reaches the store, so the guard is inert for the fixed code, and it compiles to nothing in release.Expr::allocatepointed at a non-existentExpr.alloc; it now says when to useExpr::initinstead.test/bundler/transpiler/transpiler.test.js("string replacement values survive other modules being loaded after the transpiler is created"): a spawned fixture creates the transpiler,require()s a CommonJS module with 300 string declarations, imports a JSON file, then transforms anexport const, anexport function(inject form) and anexport defaultthrough bothtransformSync()andtransform(). The required module is deliberately large so that release builds reuse the slot too: without the fix the test fails on a release build (all six outputs print"") and on a debug build (sanitizer report from the child); with the fix it passes on both.transpiler.test.jspasses with the change (189 pass, 0 fail).Background
Expris a smallCopyvalue; for heap-sized node kinds such asE::EStringit holds a pointer into a thread-local slab (src/ast/new_store.rs). The slab is bulk-reset at the start of every parse that runs on that thread without a per-parse allocator scope, and the following parse writes its own nodes over the same memory, so anything built withExpr::initis only valid until the next such parse.Expr::allocate(bump, ..)is the variant for nodes that must outlive that reset; it puts the node in the caller's arena instead. Debug builds additionally fill the slab with0xAAon reset, which is why the stale read is a deterministic crash there.bun_ast::expr::Disableris a debug-only flag checked by the slab'sappend;Disabler::scope()sets it for the current scope, turning anyExpr::initof a slab-backed node inside that scope into a panic. It is a no-op in release builds.Bun.Transpileroption parsing runs in the constructor with no allocator scope active, soExpr::initthere lands in the thread-local slab.transformSync()/transform()parse into their own per-call arena, which is why the bug only shows up once some other synchronous parse on the main thread (module loading) has reset the slab.Per-form probe on a debug build, before and after the change
Each row is a separate process: construct the transpiler,
require("./reset.cjs"), then transform the given source.foo: "bar"export const foo = 1;(sync)export const foo = "bar";getStaticProps: ["__N_SSG", "ssg"]export function getStaticProps() {}(sync)export var __N_SSG = "ssg";default: "dflt"export default 1;(sync)export default "dflt";foo: "bar"export const foo = 1;(async)export const foo = "bar";getStaticProps: ["__N_SSG", "ssg"]export function getStaticProps() {}(async)export var __N_SSG = "ssg";num: 42export const num = 1;export const num = 42;inj: ["__INJ", true]export function inj() {}export var __INJ = true;foo: "bar", norequire()in betweenexport const foo = 1;export const foo = "bar";Release build without the fix, by size of the module required in between
Same fixture as the test, release binary at
b7a043103,reset.cjsholding N string declarations. 5, 10 and 20 declarations: correct output on every run (the slot is not reached). 40 and more: wrong output or a segmentation fault on every run. The test uses 300.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
String-valued
exports.replaceentries were built withExpr::init, which placed the replacementExprin the thread-local expression store that every subsequent parse resets, so later synchronous or asynchronous transforms on the sameBun.Transpilerinstance read a dangling node and produced wrong or corrupted replacement values. The fix switchesexport_replacement_valuetoExpr::allocatewith the transpiler's own boxed arena, which lives as long as the instance and is also kept alive by theStrongreference held during async transforms. A subprocess regression test confirms rep…