Bun.build: read env from a per-build copy instead of the VM's live loader - #37780
Bun.build: read env from a per-build copy instead of the VM's live loader#37780robobun wants to merge 6 commits into
Conversation
…s live loader JSBundleCompletionTask stored a pointer to the calling VM's dotenv Loader and handed it to Transpiler::init on the bundle thread, where configure_defines (load_process, load, load_defines) and the resolver read it, and where `env: "inline"` / prefix defines borrowed the map's value bytes for the whole build. The JS thread keeps mutating that loader: `process.env.HTTPS_PROXY = ...` goes through Bun__setEnvValue, whose Map::put frees the value it replaces, so a build in flight read (and inlined) freed memory, and any such write raced the bundle thread's reads. The task now owns a Loader copied from the VM's when Bun.build() (or an HTML route build) is scheduled, on the JS thread, and both the build's transpiler and its resolver point at that copy. Loader::clone keeps the loaded-files bookkeeping and `quiet`, so load_process/load on the copy skip exactly what they skipped on the VM's loader. The compile step's download settings come from the same copy.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status Reproduced on the released build ( Review: everything raised so far is addressed (OOM past the plugin handle 5ebdf02, macro VM env escape 457c64d, copy freed when VM creation fails 0033f85); all threads resolved. Needs a maintainer's look at the ownership change. CI: build for 0033f85 pending; 92977 (an earlier revision) had 192 passes and 0 test failures, 93413 died on github.com 503s while downloading dependencies. |
|
Updated 11:25 AM PT - Aug 12th, 2026
❌ @robobun, your commit 0033f85 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37780That installs a local version of the PR into your bun-37780 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review follow-ups, all pushed:
|
There was a problem hiding this comment.
I re-reviewed after 5ebdf02 and 3010ed1 — the earlier OOM/plugins leak concern is resolved (handle_oom aborts, so create_and_schedule_completion_task stays effectively infallible as before). No new issues found. Given this is a cross-thread memory-safety fix touching JSBundleCompletionTask's ownership model and raw-pointer lifetime through the bundle thread, a human sign-off on the ownership reasoning is still worthwhile.
What was reviewed:
Loader::clonecopies map + load-state flags soload_process/loadstay no-ops on the copy; derived caches left empty is fine (rebuilt from the copied map, and the build path doesn't use them).- Box'd
envgives a stable heap address;&raw mut *self.envhanded toTranspiler::initoutlives the transpiler (task is only dropped indeinit/free_released_unstarted, both after the bundle thread handsselfback). - No
proxy_env_storagelock needed at the copy site — same JS thread as the only writer (Bun__setEnvValue); workers write their own loader. - Tests cover
env: "inline",env: "HTTPS_*", and theBun.serveHTML-route path; each spawns a subprocess, drains both pipes, asserts the at-call value is inlined and the mid-build value is not.
Extended reasoning...
Overview
Fixes a heap-use-after-free in Bun.build() / Bun.serve HTML routes: the bundle thread previously read the calling VM's live bun_dotenv::Loader, whose Box<[u8]> values can be freed on the JS thread by process.env.HTTPS_PROXY = ... (Bun__setEnvValue → Map::put) while a build's inlined defines still borrow them. The fix gives each JSBundleCompletionTask its own Box<bun_dotenv::Loader> cloned at call time; the transpiler and resolver now point at that copy. Touches src/dotenv/env_loader.rs (new Loader::clone), src/runtime/api/js_bundle_completion_task.rs (field type change and wiring), and adds three subprocess tests in test/bundler/bundler_env.test.ts.
Security risks
None introduced. The change tightens isolation (build reads a snapshot instead of shared mutable state). No new user-controlled input paths, no auth/crypto/permissions.
Level of scrutiny
High — this is native memory-safety code on a cross-thread boundary (Send task, non-atomic refcount, raw pointers into a Box field consumed on the bundle thread). The reasoning is subtle enough (why no lock, why the Box address is stable through &raw mut *self.env, why handle_oom instead of ?) that a maintainer should confirm it matches the surrounding lifecycle invariants.
Other factors
- My earlier inline finding (the
.clone()?early return leaking the protectedJSBundlerPlugin) was addressed in 5ebdf02 by routing OOM throughbun_core::handle_oom, which aborts — matching the pre-PR behavior where nothing beforeBox::newwas fallible. Verifiedhandle_oomcallsout_of_memory()onErr. Loader::cloneuses the existing fallibleMap::clone_with_allocatorandStringSet::clone;reject_unauthorized/aws_credentialsare correctly reset (they're lazy caches derived from the map).- The comment-cop threads were resolved (comments trimmed in 3010ed1; the two flagged pre-existing comments were only re-wrapped).
- CI build #92977 was still running at the time of the last timeline update; result not yet visible.
- The PR description's semantics argument (call-time snapshot is the right behavior, not just a safe one) is sound and consistent with how defines work.
…uild's Macro::init creates one VM per bundler thread the first time a macro runs there and reuses it for every later build; it was created with the env loader pointer the current build passed in. Now that a Bun.build owns its env and frees it with the build, the next build running a macro on that thread read the freed copy. The VM now clones the loader it is given, and the clone lives as long as the VM, which is never destroyed. The new test runs four builds with macros on a two-thread pool, so the later builds necessarily reuse a VM created by an earlier build.
|
Pushed 457c64d (plus f8f60b5, comments only). Self-review of the first revision found a consumer of the env pointer that outlives a build: the per-thread macro VM that
The CodeRabbit comments are still rate-limit notices; the comment threads are answered and resolved. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser_jsc/Macro.rs:436-447— Nit:into_raw_nn(Box::new(copy))leaks the boxedLoaderifVirtualMachine::init(...)?returnsErr—InitOptions::env_loaderis a caller-owned borrow (BORROW_PARAM), so init never frees it, andOption<NonNull<T>>has noDrop. Before this PR the slot heldNonNull::new(env)(a borrow), so nothing leaked here. Trivial fix: keep theBoxin a local and onlyBox::leak/into_rawafterinitsucceeds — same pattern you already applied in 5ebdf02 for thepluginshandle.Extended reasoning...
What the bug is
The new-VM branch of
Macro::initnow heap-allocates the env copy viabun_core::heap::into_raw_nn(Box::new(copy))(line 440) and stores the resultingOption<NonNull<Loader>>in a local before passing it asInitOptions::env_loader. Immediately after,VirtualMachine::init(...)?runs (lines 442–447). IfinitreturnsErr, the?propagates and the boxedbun_dotenv::Loaderis never reclaimed —Option<NonNull<T>>isCopyand has noDrop, and per theInitOptionsdoc (src/jsc/VirtualMachine.rs, "BORROW_PARAM — caller-owned; the loader outlives the VM") the VM never takes ownership of that pointer on either the success or error path.Before this PR the same slot was
NonNull::new(env), a borrow of the caller's loader, so nothing leaked on the same error path. This is exactly the REVIEW.md pattern "Arm a Drop/RAII guard before any fallible call" / "New early returns or fallible calls → re-audit everything acquired above them": a newly-introduced acquisition placed ahead of an existing fallible call.The specific code path
Macro::initreaches theelsebranch (!VirtualMachine::is_loaded()) on a bundler worker thread the first time a macro runs there.- Line 436–441:
NonNull::new(env).map(|env| { let copy = handle_oom(env.as_ref().clone()); into_raw_nn(Box::new(copy)) })— theBox<Loader>is created and immediately converted to a rawNonNull, so noDropguard exists. - Line 442–447:
VirtualMachine::init(VirtualMachineInitOptions { env_loader, ... })?. Insideinit,(hooks.init_runtime_state)(vm, &mut opts)?atVirtualMachine.rs:2501can fail — the documented case is "e.g. a deleted cwd → getcwd ENOENT out of Transpiler::init". - The
?returnsErrfromMacro::init.env_loader: Option<NonNull<Loader>>drops with no destructor. TheBox<Loader>(map +custom_files_loaded+ bookkeeping) leaks.
Why existing code doesn't prevent it
InitOptions::env_loaderis documented as a caller-owned borrow that is "forwarded to RuntimeHooks::init_runtime_state"; the VM stores the pointer but never frees it. There is no scopeguard around the raw pointer.handle_oomon theclone()itself only handles the OOM case (aborting), not a subsequentinitfailure.Step-by-step proof
Concretely, with
envnon-null:env.as_ref().clone()succeeds →copy: Loader.Box::new(copy)succeeds;into_raw_nnreturnsNonNull<Loader>; theBox's destructor is disarmed.env_loader = Some(ptr).VirtualMachine::initrunsalloc_zeroedfor the VM, sets the thread-localVMslot, boxes the console, then calls(hooks.init_runtime_state)(vm, &mut opts)?. SupposeTranspiler::initinside that hook fails withENOENT(deleted cwd).initreturnsErr(e). Back inMacro::init,?desugars toreturn Err(e.into()).- Rust drops the locals in scope:
env_loader: Option<NonNull<Loader>>—Copy, noDrop, nothing runs. The heap allocation atptris unreachable.
Impact — why this is a nit, not blocking
- The same error path already leaks larger allocations:
VirtualMachine::initat line 2369–2375alloc_zeroeds the VM struct and never frees it on the?at 2501, and the boxedConsoleObject(line 2393–2394) and optional log box are likewise leaked. Adding oneBox<Loader>(~30 env-var strings) to a path that already leaks the whole VM struct is not a new bug class. VM.set(Some(vm))runs at line 2376 before the fallible?, sois_loaded()returnstrueafterward and this new-VM branch is reached at most once per bundler worker thread — after which macros on that thread are broken anyway (half-initialized VM in the thread-local).- The success path intentionally leaks the same box ("it is never destroyed", per the comment at lines 432–435) — the error path merely extends that policy to an unreachable box.
- The trigger (VM init failing on a bundler worker thread mid-
Bun.build) is rare and effectively fatal to macros on that thread.
How to fix
Keep the
Boxalive across the fallible call and only leak it on success, e.g.:let env_loader_box = NonNull::new(env).map(|env| { Box::new(bun_core::handle_oom(unsafe { env.as_ref() }.clone())) }); let _vm = VirtualMachine::init(VirtualMachineInitOptions { log: Some(NonNull::from(&mut *log)), env_loader: env_loader_box.as_deref_mut().map(NonNull::from), is_main_thread: false, ..Default::default() })?; // init succeeded — the VM (and this thread) keep the loader forever. if let Some(b) = env_loader_box { Box::leak(b); }
(or equivalently
core::mem::forget(env_loader_box)after the?). This is the same shape as 5ebdf02, where thepluginshandle was kept out of an early-return path by making the clone abort on OOM instead of returning past it.
|
On the |
|
Pushed 0033f85: took the The previous CI build (93413) failed in the dependency-download steps with HTTP 503s from github.com on every affected lane (lolhtml, c-ares, mimalloc, libuv, WebKit tarballs), unrelated to the change; this push starts a fresh build. |
There was a problem hiding this comment.
Both earlier findings are addressed — the create_and_schedule_completion_task OOM path now aborts (5ebdf02) and the macro VM's env copy is freed on VirtualMachine::init failure (0033f85). This pass found no further issues. Given this changes cross-thread ownership of the env loader (bundle thread, resolver workers, and the per-thread macro VM all now read a per-build copy that drops with the task), a maintainer's look at the lifetime story is still warranted.
What was reviewed
Loader::clone: copies the map and the loaded-files bookkeeping soload_process/loadon the copy are no-ops; derived caches (reject_unauthorized, S3) start empty and rebuild from the copied map — no consumer of those caches is reached on the build path.Macro::initerror arm: verified againstVirtualMachine.rs:2498— oninit_runtime_statefailurevm.transpileris never written, so nothing retains the freed copy; the leaked VM'stranspiler.envstays null and lateris_loaded()short-circuits.free_released_unstarted/deinit: the boxed env drops with the task on both the bundle-thread free path and the JS-thread refcount-zero path.- Tests exercise
Bun.build(bothenv: "inline"and prefix), theBun.serveHTML-route path, and macro-VM reuse across builds; each is a spawned child asserting the inlined value andexitCode.
Extended reasoning...
Overview
Fixes a heap-use-after-free where Bun.build() handed the calling VM's live bun_dotenv::Loader to the bundle thread by pointer while the JS thread kept mutating it (process.env.HTTPS_PROXY = ... → Bun__setEnvValue → Map::put frees the Box<[u8]> a build define still borrows). The fix gives JSBundleCompletionTask its own Box<Loader> cloned at call time, points the build's transpiler/resolver at that copy, and gives the per-thread macro VM its own clone (since that VM outlives any one build). A new Loader::clone copies the map plus did_load_process / default_files_loaded / custom_files_loaded / quiet so configure_defines on the copy does not re-read environ or the .env files. Four new subprocess tests cover env: "inline", env: "HTTPS_*", Bun.serve static HTML routes, and macro-VM reuse across sequential builds.
Security risks
None identified. The change narrows what the bundle thread can see (a snapshot instead of live process env), which is strictly safer than before. No auth, crypto, or permission surfaces touched.
Level of scrutiny
High. This is memory-safety work in Rust unsafe code that changes which thread owns and frees a shared resource. It threads a raw *mut Loader through Transpiler::init, for_worker's lifetime-widening transmute, and Macro::init's VM creation, and it adds a boxed field whose drop must be reached on every task-teardown path (normal completion, VM-teardown-while-queued, VM-teardown-while-running). Two revisions were needed during review to close leak paths this change itself introduced (the plugins handle on the OOM early return, and the macro VM's copy on VirtualMachine::init failure); the second was found by the author's own re-audit after the first. That is exactly the kind of change a maintainer who owns the bundler thread's lifecycle should sign off on.
Other factors
- Prior review threads are all resolved except my note on
Macro.rs:451, which 0033f85 addresses; theinspect_errfree is sound perVirtualMachine.rs:2498("vm.transpilerwas never written" on the failure path). - The author's status comment explicitly requests a maintainer's look at the ownership change.
- CI on the previous revision was green (192 passes, 0 failures) with two darwin-26 jobs that expired for lack of an agent; the current revision's build is pending.
- Test coverage is good: each variant is a spawned fixture that fails on the released build (freed-buffer bytes in the output) and under ASAN on the unfixed debug build, per the description.
1 participant
Problem
Bun.build({ env: "inline" })(or anenv: "PREFIX_*"pattern) whose plugin assignsprocess.env.HTTPS_PROXYwhile the build is in flight emits a freed buffer: release inlines"0\x02\x00\x00roxy-at-build-call...", ASAN reportsheap-use-after-freeinbest_quote_char_for_stringonBun Pool 0, freed byBun__setEnvValueon the JS thread.Bun.serveHTML routes without HMR build through the same task.Fix
Bun.build()call, the build's transpiler and resolver read that copy, and it is freed with the task. The build never reads the VM's loader and the VM can never reach the copy..envfiles andenvironare not re-read on the copy; derived caches start empty and are rebuilt on demand. Semantically a build's defines are a snapshot anyway, so the output now reflects the env at the momentBun.build()was called instead of an unspecified later point.env: "inline",env: "HTTPS_*", aBun.serveHTML route) fail on the released build and abort under ASAN without the fix, and pass with it. A fourth test (four builds using a macro withUV_THREADPOOL_SIZE=2) aborted under ASAN with the first revision and passes with the macro change. The copy costs well under a millisecond per build with a 30-variable environment.Background
.envfiles). Each VM materializes itsprocess.envfrom it, and only a few assignments (the proxy variables, since fetch: observe runtime process.env proxy var changes #28614) write back through to the native map.env: "inline"andenv: "PREFIX_*"build options turn matching variables into defines, constants substituted into the output. A define points at the loader's stored bytes rather than holding a copy.Bun.build()returns a promise and bundles on a process-wide thread pool while JS, including pluginonLoadcallbacks, keeps running on the calling thread, so anything the build reads from the VM must be copied or never written during the build.Original description
What
Bun.build()(andBun.serveHTML routes without HMR, which go through the sameJSBundleCompletionTask) handed the calling VM's live dotenvLoaderto the bundle thread. The task storedvm.transpiler.envandcreate_and_configure_transpilerpassed it toTranspiler::init, so on the bundle threadconfigure_definesranload_process()/load()/load_defines()against it, the resolver (and its worker threads) readNODE_PATHthrough it, and withenv: "inline"or anenv: "PREFIX_*"pattern every define was anE::Stringborrowing the value bytes stored in that map for the rest of the build (defines.rs,env_string_store_put).The JS thread keeps mutating that same loader while a build is in flight:
process.env.HTTPS_PROXY = ...(andHTTP_PROXY/NO_PROXYand the lowercase variants, since #28614) goes throughBun__setEnvValue, whoseMap::putassigns over theHashTableValueand frees theBox<[u8]>the build's define points at. A new key can also grow the map under aload_definesthat is iterating it. The original Zig code had the same design, so this is not a port regression.Repro
No thread race is needed; a plugin is enough, because the defines are built before any
onLoadruns:Release build (
bun 1.4.0-canary): the inlined string is the freed buffer, with the allocator's free-list pointer in its first bytes:Debug (ASAN) build:
Fix
JSBundleCompletionTask.envis now aBox<bun_dotenv::Loader>of its own, copied from the VM's loader increate_and_schedule_completion_task(JS thread, inside theBun.build()call), andcreate_and_configure_transpiler/configure_bundlerpoint the build's transpiler and resolver at that copy. Nothing the build runs reads the VM's loader anymore, and nothing the VM does can reach the build's copy; the copy is dropped with the task.Loader::clone(new,bun_dotenv) copies the map together withdefault_files_loaded/custom_files_loaded/did_load_process/quiet, soload_process()andload()on the copy are the same no-ops (and print the same, or nothing) that they were on the VM's loader; a copy of just the map would have re-readenvironover runtime-assigned values and re-read the.envfiles on every build. The derived caches (S3 credentials,NODE_TLS_REJECT_UNAUTHORIZED) start empty; they are computed from the map on first use and the build path does not use them.Why a copy at call time is the right semantics, not just a safe one: a build's defines are a snapshot by nature (they are baked into the output), and the only thing the previous code "gained" from reading live was an unspecified point in time between
Bun.build()returning its promise and the bundle thread getting around toconfigure_defines. Now the environment that a build inlines is the environment at the momentBun.build()was called. The compile step's download settings (proxy / TLS for fetching a cross-compile target) come from the same copy, which is the same call-time semantics #37507 is moving that code to.The macro VM. One consumer of the transpiler's env pointer outlives a build:
Macro::initcreates a VM on each bundler thread the first time a macro runs there, created with the env pointer of the build that happened to be running, and reuses that VM for every later build (it is never destroyed; the pool threads are process-wide). With a per-build env, the next build running a macro on that thread read the freed copy (Bun__getEnvValue->Loader::geton a freed map, ASAN heap-use-after-free onBun Pool 0, freed by the task drop on the JS thread; this was surfaced in review of the first revision). Before this PR the pointer happened to be the calling VM's loader, which outlives everything on the main thread (aBun.build()inside a Worker had the same problem, since the worker's loader dies with the worker).Macro::initnow gives the VM a clone of the loader it is handed, which lives as long as the VM does, the same arrangement Worker VMs have. Semantically the macro VM's env was already fixed at VM creation (process.envis materialized per VM), so macros observe what they did before; the per-thread macro VM'sconfigure_definesalso stops writing.envcontents into a loader it shares with other threads (previously the calling VM's live loader, in the first revision the build's copy).Copying the env is per build, roughly one small allocation per variable; under the ASAN debug build a 500-module
Bun.build()takes ~320 ms and this adds well under a millisecond with a 30-variable environment.Related, independent: #37766 removes the
quietwriteTranspiler::initdid on a passed-in loader (with this change that write would only ever have hit the build's copy); #37775 gives the debugger thread's VM its own loader. The bake dev server inlines env through the same define path against the VM's loader for the server's lifetime; that is a separate fix (copying the bytes inenv_string_store_put) and has been filed separately.Tests
test/bundler/bundler_env.test.ts, "env is copied when the build is scheduled": the scenario above run in a child process forBun.build({ env: "inline" }),Bun.build({ env: "HTTPS_*" }), and aBun.serve({ development: false })HTML route with[serve.static] env = "inline"and the plugin loaded from bunfig. Each asserts the bundle inlines the value that was set when the build was scheduled and not the one assigned mid-build. All three fail on the released build (the inlined string is the clobbered buffer shown above) and on the unfixed debug build (ASAN abort), and pass with this change.A fourth test covers the macro VM: four sequential
Bun.build()calls in one child process withUV_THREADPOOL_SIZE=2, each bundling a file whose macro returns a key ofprocess.envno earlier build has read, so the lookup reaches the VM's loader. With two pool threads the third and fourth builds necessarily reuse a VM created by an earlier build. It passes on the released build (where the VM points at the process-lifetime loader), aborted under ASAN with the first revision of this PR (builds 0 and 1 print, build 2 dies inLoader::get), and passes with theMacro::initchange.Also ran locally with the debug build: the rest of
bundler_env.test.ts,bun-build-api.test.ts(the 400-build stress test lands at ~167 s of its 180 s budget on this machine with or without the env copy being a factor),bun-serve-html.test.ts(its fivedevelopment: truetests fail here withEMFILE while initializing file watcheron the released build as well; thedevelopment: falseones pass), and aBun.build({ compile })of the host target.