Surface ENOMEM instead of aborting when a whole-file read cannot allocate st_size - #33178
Conversation
File::read_to_end presized its destination with reserve_exact on a size
taken from a fresh fstat. st_size is only a hint (sparse or truncated
files, writers racing the read), so an oversized value made the
infallible reservation call handle_alloc_error and abort the process
with SIGABRT instead of returning an error.
The cheapest reproduction is the automatic .env loader, which runs
before any user code:
truncate -s 1T .env
bun app.js
# memory allocation of 1099511627792 bytes failed
# SIGABRT before app.js runs
The env loader already treats ENOMEM while reading an env file as
recoverable, but that branch was unreachable because the reservation
aborted before read() could return.
Use try_reserve/try_reserve_exact and surface failures as ENOMEM via the
existing bun_sys::Error::oom(), for both the fstat-derived presize and
the incremental growth in read_fill_vec. Every consumer of read_to_end
(env loader, bunx, bun pack, lockfile, Bun.Image, standalone graph) now
gets a regular error instead of an abort.
The ".env with 50000 entries" test also gets an explicit debug-build
timeout: the spawned debug ASAN child alone needs ~8s on slower
machines, independent of this change.
|
Updated 7:05 AM PT - Jul 1st, 2026
❌ @robobun, your commit 380a123 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33178That installs a local version of the PR into your bun-33178 --bun |
WalkthroughFile reading reservation logic in file.rs and lib.rs was changed from panicking allocation calls to fallible try_reserve/try_reserve_exact calls returning Error::oom() on failure. Corresponding env.test.ts tests were updated with timeout adjustments and a new ASAN-only oversized-file regression test. ChangesFallible allocation handling
Sequence Diagram(s)sequenceDiagram
participant Test as env.test.ts
participant Bun as bun process
participant Loader as env loader
Test->>Bun: spawn with oversized sparse .env
Bun->>Loader: read_to_end_with_array_list
Loader->>Loader: try_reserve_exact(want)
Loader-->>Bun: Err(Error::oom()) if allocation fails
Bun-->>Test: reaches user code, exit code 0
Compact metadata
Related issues: None referenced in provided data. Related PRs: None referenced in provided data. Suggested labels: rust, tests, memory-safety Suggested reviewers: None specified in provided data. Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/env.test.ts`:
- Around line 341-364: The .env stress test in env.test.ts is avoiding the
preferred fix by increasing the per-test timeout; instead, reduce the workload
in the .env with 50000 entries case so it finishes within the normal budget.
Update the large-entry setup in the test callback (and any related expected loop
in index.ts) to use fewer entries while still validating parsing, rather than
relying on the isDebug timeout branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d89d9d0a-537a-4529-8835-1014b1b13cee
📒 Files selected for processing (3)
src/sys/file.rssrc/sys/lib.rstest/cli/run/env.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap from infallible reserve* to try_reserve* so allocation failure surfaces as ENOMEM instead of aborting; no signature or success-path changes.
Extended reasoning...
Overview
This PR touches three files: src/sys/file.rs (the read_to_end_with_array_list reservation), src/sys/lib.rs (the shared read_fill_vec growth loop), and test/cli/run/env.test.ts (one new ASAN-gated regression test plus a debug-build timeout on an existing slow test). The native change is a mechanical swap of Vec::reserve / reserve_exact for try_reserve / try_reserve_exact, returning the crate's existing bun_sys::Error::oom() on failure. The + 16 on the fstat hint also becomes .saturating_add(16), which is strictly safer.
Security risks
None introduced. If anything this is a hardening change: an attacker-controlled sparse file or racing writer that inflates st_size can no longer abort the process via handle_alloc_error before user code runs — it now surfaces as a catchable ENOMEM through paths that already handle Err. No new inputs are trusted, no auth/crypto/permissions touched.
Level of scrutiny
Low-to-moderate. read_fill_vec and read_to_end_with_array_list are on a widely-used file-read path, but the change does not alter control flow, signatures, or success-path semantics — try_reserve* behaves identically to reserve* when allocation succeeds. Every caller already handled Maybe<T> failure (the pre-existing get_end_pos()? could already fail), so the new error case flows through existing plumbing. The .env loader's ENOMEM branch that this now makes reachable was already written for exactly this case.
Other factors
The new test follows repo conventions (bunEnv spread, concurrent pipe draining, await using, {stdout, exitCode} object assertion, no stderr-empty check) and is deterministically gated to ASAN via allocator_may_return_null=1 so it doesn't depend on host memory. The added timeout on .env with 50000 entries is a benign accommodation for debug+ASAN slowness with an explanatory comment; workload and assertions are unchanged. No CODEOWNERS cover these paths, no bugs were flagged by the bug-hunting pass, and no prior reviewer comments are outstanding.
|
CI status, final for both runs of this PR. The change is green on every lane that ran it; the remaining failures are infrastructure or a known pre-existing flake, with evidence below.
Nothing left to act on from my side: the darwin job needs a retry by someone with Buildkite access (the agent fails to download the build artifact regardless of the diff), and the musl failures are #33044. I am not pushing further empty commits to retrigger the pipeline. |
There was a problem hiding this comment.
The change looks correct and well-tested to me, but since read_to_end / read_fill_vec are the shared whole-file read primitives reached from the env loader, bunx, lockfile reader, standalone module graph, and more, it's worth a maintainer's eyes before landing.
Extended reasoning...
Overview
This PR swaps infallible Vec::reserve / reserve_exact calls for their fallible try_reserve / try_reserve_exact counterparts in src/sys/file.rs::read_to_end_with_array_list and src/sys/lib.rs::read_fill_vec, returning the existing bun_sys::Error::oom() (ENOMEM) on failure instead of aborting the process via Rust's handle_alloc_error. It also replaces get_end_pos()? + 16 with saturating_add(16) for defensive correctness. A new ASAN-gated test creates a 1 TiB sparse .env and asserts user code is reached; an existing 50000-entry test gains a debug-only timeout.
Security risks
None identified. The change strictly reduces attack surface: an untrusted st_size (sparse file, racing writer, hostile filesystem) can no longer abort the process before user code runs. No new inputs are accepted and no validation is loosened.
Level of scrutiny
Moderate-to-high. The diff itself is mechanically simple — try_reserve has identical success-path semantics to reserve, and every caller already handles Err from these functions (the fstat inside could always fail). However, bun_sys::File::read_to_end is a foundational primitive with a wide blast radius (env loader, bunx, bun pack, lockfile reader, standalone module graph, Bun.Image). Changes at this layer warrant a maintainer sign-off even when they look obviously correct.
Other factors
- No bugs surfaced by the bug-hunting system.
- The one CodeRabbit nit (about the timeout bump) was discussed and withdrawn as resolved.
- The PR description is thorough with a reproducible fail-before/pass-after demonstration.
- No CODEOWNERS entry covers
src/sys/. - The change is at the current HEAD (
e2e732e8 sys: make whole-file read reservations fallible), suggesting it may already be integrated.
2 participants
Problem
bun_sys::File::read_to_endpresizes its destination withlist.reserve_exact(get_end_pos()? + 16)(src/sys/file.rs:217), whereget_end_pos()is a freshfstat.st_sizeis only a hint: sparse files, files truncated or grown by a racing writer, and unusual filesystems can report sizes that have nothing to do with what is readable or allocatable.reserve_exactis infallible, so when the allocator refuses the request, Rust'shandle_alloc_erroraborts the whole process. No JS code can catch it.The cheapest reproduction is the automatic
.envloader, which runs before any user code:1099511627792 is exactly
st_size + 16. Verified on bun 1.4.0 (release, under an 8 GB address space limit) and on a debug ASAN build (ASAN caps a single request at 1 TiB). Without a memory limit the release build instead commits the lying size and reads sparse zeros until the OOM killer ends it, still before user code.The
.envloader has an explicit branch that treatsENOMEMwhile reading an env file as recoverable (src/dotenv/env_loader.rs,read_env_file_contents), but it was unreachable: the abort happened in the reservation, beforeread()could return anything. The sameread_to_endis reached frombunx'spackage.jsonread,bun pack, the lockfile reader, the standalone module graph, andBun.Image(path), where a secondfstatinsideread_to_endcan also disagree with thest_sizethe caller just validated againstMAX_INPUT_FILE_BYTES.Fix
Treat the fstat-derived size as a hint and make every reservation in the read path fallible:
read_to_end_with_array_list:try_reserve_exactthe saturatingst_size + 16hint and return the existingbun_sys::Error::oom()(ENOMEM) on failure.read_fill_vec: the incrementalreserve(grow_by)in the read loop becomestry_reservewith the same error.No signatures change and every caller already handles
Errfrom these functions (thefstatitself could fail before). With the fix, the.envcase takes the already-designed recoverable path and startup reaches user code; the other callers report a normalENOMEMerror.Files whose content really is that large still cost that much memory to read, same as
node --env-file; this change only guarantees that allocation failure surfaces asENOMEMinstead ofSIGABRT.Tests
test/cli/run/env.test.ts:.env with a huge lying st_size does not abort the process. Creates a 1 TiB sparse.env, runsbun app.js, asserts user code runs and the process exits 0. Gated to ASAN builds (skipIf(!isASAN || isWindows)): ASAN deterministically refuses the 1 TiB request (allocator_may_return_null=1), so the test does not depend on machine memory. On the unfixed build the child dies with exit 134 and empty stdout; on the fixed build it printsreached user codeand exits 0. A release build would treat the sparse file as a genuinely huge one, so there is nothing deterministic to assert there..env with 50000 entriesnow has an explicit debug-build timeout. The spawned debug ASAN child alone takes about 8 seconds on a slower machine (measured the same, 8.2s, with and without this change), which cannot fit the default 5s budget. Workload and assertions are unchanged.fail before / pass after