Repository navigation
windows: heap-allocate bunx fast-path environment block - #32644
Conversation
The .bunx fast path on Windows wrote the process environment into a fixed [u16; 32767] buffer. That limit belongs to CreateProcessA (ANSI); CreateProcessW with CREATE_UNICODE_ENVIRONMENT has no documented block size limit, and CI environments routinely exceed 32 KB of env data. The panic reported in BUN-3MAQ (index out of bounds writing past the buffer) was already prevented on main by the length pre-checks added in #30722, which made the fast path bail and fall through to the libuv slow path instead of crashing. This change removes the cap entirely: write_windows_env_block now sizes a Vec<u16> to the actual contents so the fast path works regardless of environment size, and the now-unused 64 KB static scratch buffer and error branch in the caller are removed.
|
Updated 10:34 AM PT - Jun 23rd, 2026
✅ @robobun, your commit ada400abcf12a4e4427b739719fa05023d781020 passed in 🧪 To try this PR locally: bunx bun-pr 32644That installs a local version of the PR into your bun-32644 --bun |
|
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 (1)
Walkthrough
ChangesHeap-allocated Windows environment block
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for the test-comment update in ada400a.
Extended reasoning...
Overview
Rewrites Map::write_windows_env_block (src/dotenv/env_loader.rs) to return a heap-allocated Vec<u16> sized to the actual environment contents instead of writing into a fixed [u16; 32767] and returning Result<*const u16, Error>. The single caller in BunXFastPath::try_launch (src/runtime/cli/run_command.rs) is updated to hold the Vec as a local and pass env_block.as_ptr() into FromBunRunContext.environment; the now-dead ENVIRONMENT_BUFFER static and the Err(_) => return fallback branch are removed. A Windows-only integration test is added to bun-run.test.ts.
Correctness
- Capacity bound:
4 + Σ(key_bytes + 1 + value_bytes + 1)is a valid upper bound on UTF-16 code units because UTF-8 byte count ≥ UTF-16 code-unit count for every codepoint (1→1, 2→1, 3→1, 4→2). The buffer is zero-initialized viavec![0u16; capacity], sotruncate(i + 4)leaves four real NUL terminators. The empty-map case yields[0,0,0,0], identical to the old code. - Pointer lifetime:
env_blockis a local that stays in scope acrosstry_startup_from_bun_js(run_ctx)(run_command.rs:4089), which is whereCreateProcessWis called synchronously and copieslpEnvironmentbefore returning. TheVecis dropped only after that call returns, so no UAF. - Single caller: grep confirms
write_windows_env_blockhas exactly one Rust call site, so the signature change is fully covered.
Security risks
None. This is buffer-sizing for an env block handed to CreateProcessW; no parsing, no auth, no untrusted input shape change. The data flowing through is the same process environment that was already being serialized.
Level of scrutiny
Low–medium. Windows-only (#[cfg(windows)]) fast-path code with a safe slow-path fallback that previously absorbed the failure case being removed here. The change is mechanical (fixed buffer → sized Vec) and mirrors what create_null_delimited_env_map already does for POSIX. No CODEOWNERS entry for the touched paths.
Other factors
My earlier nit about the test comment (it reads as if it asserts the fast path specifically, when it actually guards the user-visible contract) was addressed in ada400a — the comment now explicitly states it does not assert which spawn path was taken. The inline thread is resolved. No other reviewer comments are outstanding; CodeRabbit had no actionable findings; the bug-hunting system found nothing.
There was a problem hiding this comment.
Thanks for the comment update — that addresses my earlier nit. I'd still like a human to glance at this since it rewires the Windows .bunx fast-path env-block construction (heap Vec → raw ptr into CreateProcessW) and drops the slow-path fallback branch; also worth confirming the musl build-bun failures on #64238 are unrelated infra before merging.
Extended reasoning...
Overview
This PR rewrites Map::write_windows_env_block in src/dotenv/env_loader.rs to return a heap-allocated Vec<u16> sized to the actual environment contents (UTF-8 byte length is a provably-safe upper bound on UTF-16 code units), instead of writing into a fixed [u16; 32767] and returning Err(TooManyEnvironmentVariables) on overflow. The sole caller in src/runtime/cli/run_command.rs (BunXFastPath::try_launch) is updated to drop the static ENVIRONMENT_BUFFER scratch cell and the Err(_) => { close handle; return } slow-path fallback, passing env_block.as_ptr() directly into FromBunRunContext.environment. A Windows-only integration test in test/cli/install/bun-run.test.ts exercises a ~50 KB env block through a .bunx bin.
I verified there is exactly one caller of write_windows_env_block and no remaining references to ENVIRONMENT_BUFFER. The env_block: Vec<u16> local outlives the synchronous try_startup_from_bun_js → launcher → CreateProcessW call (consumed at bun_shim_impl.rs:1381 as lpEnvironment), so the raw-pointer lifetime is sound. The capacity computation (4 + Σ(key_bytes + 1 + value_bytes + 1)) is a correct upper bound and truncate(i + 4) cannot exceed it, so the four zero-initialized trailing NULs are always present.
Security risks
None identified. The change relaxes an artificial size cap on the environment block passed to CreateProcessW with CREATE_UNICODE_ENVIRONMENT; the env contents themselves are unchanged (same iterator over self.map, same UTF-8→UTF-16 converter). No new untrusted input is introduced.
Level of scrutiny
Medium. This is a small, well-reasoned change, but it sits directly on the Windows process-spawn fast path and removes a defensive fallback (Err(_) => return → slow path). Post-PR, any future regression in env-block construction would surface as a fast-path failure rather than a silent fallthrough, which is arguably better but is a behavioral shift a maintainer should sign off on. The raw-pointer-into-local-Vec across the shim boundary is correct but is exactly the sort of thing worth a second pair of eyes.
Other factors
- My earlier nit (test comment implied it was exercising the fast path specifically; on the parent commit it passes via slow-path fallthrough) was addressed in ada400a — the comment now states it guards the user-visible contract and does not assert which spawn path was taken. That thread is resolved.
- robobun reports musl (
x64-musl,x64-musl-baseline,aarch64-musl) build-bun failures on Build #64238 for commit 010361c.write_windows_env_blockis not#[cfg(windows)]-gated so it does compile on Linux, but the author reportscargo checkpasses on host, and the failures are inscripts/build/ci.ts(build infra) rather than test failures — likely unrelated, but worth a human confirming before merge. - No CODEOWNERS entries match the touched paths.
|
Re the musl All three Windows test lanes pass on #64244. |
2 participants
What
write_windows_env_block(used by the Windows.bunxfast path inbun run/bunx) wrote the process environment into a fixed[u16; 32767]buffer. That 32,767-character limit belongs toCreateProcessA(ANSI);CreateProcessWwithCREATE_UNICODE_ENVIRONMENThas no documented block-size limit, and CI environments routinely exceed 32 KB of env data.This changes
write_windows_env_blockto size aVec<u16>to the actual environment contents (UTF-8 byte length is a safe upper bound on UTF-16 code units) so the fast path works regardless of environment size. The now-unused 64 KB static scratch buffer and the error-handling branch in the caller are removed.Background (BUN-3MAQ)
Sentry BUN-3MAQ reports the Zig-era panic in bun 1.3.14:
where a ~280 KB environment value overran the fixed buffer because the bounds check happened after the write. That crash was already prevented on
mainby the length pre-checks added in #30722: instead of overrunning, the Rust port returnsTooManyEnvironmentVariables, the fast path bails, andrun_binaryfalls through to the libuvuv_spawnslow path which heap-allocates the env block. So there is no crash on currentmain; this PR removes the remaining artificial cap so the fast path doesn't have to bail.Gate note
The changed code is
#[cfg(windows)], the new test isit.if(isWindows), and on currentmain(where #30722 already landed) a large environment silently falls back to the slow path rather than failing. So there is no fail-before to observe on the Linux gate; the Windows CI lane exercises the new test.Verification
cargo check -p bun_dotenv -p bun_runtime --target x86_64-pc-windows-msvccargo check -p bun_dotenv -p bun_runtime(host)bun bd test test/cli/install/bun-run.test.ts(291 pass, 1 skip on Linux)