Repository navigation
fix: zero-initialize primitive scratch buffers (EXP-089) - #31258
Closed
Dicklesworthstone wants to merge 1 commit into
Closed
Dicklesworthstone wants to merge 1 commit into
Dicklesworthstone wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
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 (2)
WalkthroughThree buffer initialization constructors across two files were updated to return safe zero-initialized values instead of unsafe uninitialized buffers, removing associated unsafe contracts and simplifying the safety model for path and depth buffer construction. ChangesSafe Buffer Initialization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EXP-089 showed that three safe scratch-buffer constructors built primitive
arrays with MaybeUninit::uninit().assume_init():
- PathBuffer::uninit()
- WPathBuffer::uninit()
- depth_buf_uninit()
Miri rejects the construction itself before any caller can establish a
write-before-read discipline:
encountered uninitialized memory, but expected an integer
Even for integer element types where every bit pattern is valid, uninitialized
memory is not an initialized Rust value. Return zero-initialized storage from
the existing constructors instead. This keeps the call-site API stable and
removes the invalid_value allowances and comments that treated Zig-style
undefined scratch storage as a valid Rust array value.
This is the emergency correctness patch recommended by the audit. A
performance-preserving follow-up can wrap the storage in MaybeUninit and expose
only initialized prefixes, but the current safe API cannot return an
uninitialized primitive array as [T; N].
Verification:
- bun run fmt:rust
- git diff --check
- cargo check -p bun_core
- cargo check -p bun_install
- cargo check --workspace
- CARGO_TARGET_DIR=/tmp/exp-089-postfix-target cargo +nightly miri run
in a temp crate depending on local bun_core and calling the real constructors
- bun bd test test/cli/install/hoist.test.ts
Dicklesworthstone
force-pushed
the
claude/ub-fix-scratch-buffer-init
branch
from
May 23, 2026 04:26
7017bba to
868caff
Compare
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 28, 2026
…40714) ### Problem - `URL::join_normalize`, `join_write` and `join_alloc` (`src/url/lib.rs:521`) built their 2 KB scratch buffers with `MaybeUninit::<[u8; 2048]>::uninit().assume_init()` under a lint allow. An integer array from uninitialized memory is undefined behavior even when every byte is written before it is read. Miri: `constructing invalid value of type [u8; 2048]: at [0], encountered uninitialized memory, but expected an integer`. - `lockfile::tree::depth_buf_uninit` (`src/install/lockfile/Tree.rs:82`) did the same for a `[u32; MAX_DEPTH]`. ### Fix - The concatenation scratch in `join_normalize` is `[MaybeUninit<u8>; 2048]`, filled with `write_copy_of_slice`. Only the written prefix becomes a `&[u8]`, the idiom `bun_paths::resolve_path::join_string_buf_t` uses for the same step. - The normalize output in `join_write` comes from `bun_paths::path_buffer_pool`, so there is no per-call memset. Joins longer than `MAX_PATH_BYTES` keep the zeroed heap fallback. `join_alloc` now writes through `join_write` into its `Vec`. - `DepthBuf` is `[MaybeUninit<Id>; MAX_DEPTH]`. `relative_path_and_depth` writes slots `1..=depth`, then reads them back with `assume_init`. The never-read `depth_buf[0] = 0` is gone. - Verified: `bun run rust:miri -p bun_url` (4 new unit tests, `bun_url` joins the Miri crate set). `test/internal/source-lints/uninit-assumed-init.test.ts` pins the `#[allow(invalid_value)]` sites to `src/bun_core/util.rs`. Also `test/js/bun/util/filesystem_router.test.ts` and four `test/cli/install` files. ### Background - `MaybeUninit<T>` is the std wrapper for memory that may not be initialized. `write_copy_of_slice` fills a `[MaybeUninit<u8>]`, and `assume_init_ref` views a filled range as `&[u8]`. - `path_buffer_pool` is a thread-local cache of up to 4 `PathBuffer`s (4 KB Linux, 1 KB macOS, 98 KB Windows). `get()` pops one or allocates it zeroed. The guard returns it on drop. - `normalize_string_buf` writes its output from index 0 and reads only indices it already wrote. Stale bytes in a pooled buffer are never observed. <details><summary>Notes</summary> Fail-before: with the tests from this PR appended to the unfixed `src/url/lib.rs`, `MIRIFLAGS=-Zmiri-tree-borrows cargo miri test -p bun_url` stops at `join_buf_uninit` with the error quoted above. With the fix all 4 tests pass. `cargo test -p bun_url` without Miri does not link (the Highway SIMD symbols come from C++), which is why the Miri lane is where these tests run, same as `bun_paths`. `join_alloc` used to size its `Vec` exactly from the normalized length. It now reserves the upper bound (`origin + 1 + needed`) and `into_boxed_slice` shrinks it. The callers are the legacy linker (`src/bundler/linker.rs`) and `FileSystemRouter`/dev-server `src` generation, not hot paths. Install tests run: `test/cli/install/bun-dedupe.test.ts`, `bun-prune.test.ts`, `bun-lock.test.ts`, `bun-install-native-binlink.test.ts`. Not in this PR: `PathBuffer::uninit()` and `WPathBuffer::uninit()` in `src/bun_core/util.rs:727,774` have the same `assume_init` pattern. They have 348 + 24 call sites in 118 files (plus 34 `PathBuffer::default()`), and the `uninit` body exists because zeroing the 98 KB Windows buffer at every site caused test timeouts. A sound version needs either a `MaybeUninit`-backed buffer type with an initialized-prefix API or moving the sites to `path_buffer_pool`. That is a separate change. #31258 (open, external) proposes to zero them, which brings the Windows memset back. Also ran: `cargo clippy -p bun_url -p bun_install --tests --no-deps` (clean), `cargo fmt --check`. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 3 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/uninit-assumed-init.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/25] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/25] gen JS modules (bundle-modules) Preprocess modules (8034ms) Bundle modules (54ms) Postprocesss modules (218ms) Bundle Functions (527ms) Generate Code (47ms) [8.89s] Bundled "src/js" for development 2787 kb 197 internal modules 13 native modules 50 internal functions across 16 files [2/7] cargo bun_runtime → libbun_runtime.a [4/7] cc obj/codegen/InternalModuleRegistryConstants.S.o FAILED: rust-target/x86_64-unknown-linux-gnu/debug/libbun_runtime.a /workspace/bun/build/release/bun /workspace/bun/scripts/build/stream.ts rust --console --cwd=/workspace/bun --env=CARGO_TERM_COLOR=always --env=BUN_CODEGEN_DIR=/workspace/bun/build/debug/codegen --env=CC=/usr/lib/llvm-21/bin/clang --env=CXX=/usr/lib/llvm-21/bin/clang++ --env=AR=/usr/lib/llvm-21/bin/llvm-ar --env=CARGO_TARGET_ ... (truncated) release without fix: 1 FAILED bun test v1.4.1-canary.1 (65362b5) test/internal/source-lints/uninit-assumed-init.test.ts: (pass) scans a non-empty set of tracked Rust sources [0.11ms] 70 | }); 71 | 72 | test("no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)]", () => { 73 | // `[^)]*` spans the multi-line form of the attribute; lint paths never 74 | // contain `)`. 75 | expect(scan(/#!?\[(?:allow|expect)\([^)]*\b(?:invalid_value|clippy::uninit_assumed_init)\b/)).toEqual(KNOWN_SITES); ^ error: expect(received).toEqual(expected) [ "src/bun_core/util.rs", + "src/install/lockfile/Tree.rs", + "src/url/lib.rs", ] - Expected - 0 + Received + 2 at <anonymous> (/workspace/bun/test/internal/source-lints/uninit-assumed-init.test.ts:75:97) (fail) no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)] [13.02ms] 1 pass 1 fail 2 expect() calls Ran 2 tests across 1 file. [1.89s] __F:1:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/uninit-assumed-init.test.ts bun test v1.4.1 (65362b5) test/internal/source-lints/uninit-assumed-init.test.ts: (pass) scans a non-empty set of tracked Rust sources [2.38ms] (pass) no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)] [173.11ms] 2 pass 0 fail 2 expect() calls Ran 2 tests across 1 file. [27.20s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 737ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/22] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/22] gen JS modules (bundle-modules) Preprocess modules (9162ms) Bundle modules (87ms) Postprocesss modules (392ms) Bundle Functions (653ms) Generate Code (44ms) [10.35s] Bundled "src/js" for production 2595 kb 197 internal modules 13 native modules 50 internal functions across 16 files [2/6] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_paths v0.0.0 (/workspace/bun/src/paths) �[1m�[92m Compiling�[0m bun_sys v0.0.0 (/workspace/bun/src/sys) �[1m�[92m Compiling�[0m bun_url v0.0.0 (/workspace/bun/src/url) �[1m�[92m Compiling�[0m bun_http_types v0.0.0 (/workspace/bun/src/http_types) �[1m�[92m Compiling�[0m bun_perf v0.0.0 (/workspace/bun/src/perf) �[1m�[92m Compiling�[0m bun_threading v0.0.0 (/workspace/bun/src/threading) �[1m�[92m Compiling�[0m bun_which ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/rust-miri.ts | 1 + src/install/PackageInstaller.rs | 2 +- src/install/lockfile/Tree.rs | 24 ++--- .../lockfile_json_stringify_for_debugging.rs | 4 +- src/url/lib.rs | 120 +++++++++++++++------ .../source-lints/uninit-assumed-init.test.ts | 76 +++++++++++++ 6 files changed, 175 insertions(+), 52 deletions(-) ``` </details> **gate history** · 2 passed · 1 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/rust-miri.ts 1 1 0 src/install/PackageInstaller.rs 1 2 0 src/install/lockfile/Tree.rs 4 5 0 …stall/lockfile/lockfile_json_stringify_for_debugging.rs 2 2 0 src/url/lib.rs 4 10 0 test/internal/source-lints/uninit-assumed-init.test.ts 0 1 0 ``` </details> <!-- robobun:evidence:end -->
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 6, 2026
…k arrays (#41442) ### Problem - `PathBuffer::uninit()` and `WPathBuffer::uninit()` (`src/bun_core/util.rs`) return a `[u8; N]` / `[u16; N]` built with `MaybeUninit::uninit().assume_init()`. An integer must be initialized, so this is undefined behavior at the constructor, before any byte is read. Miri: `constructing invalid value of type bun_core::PathBuffer: at .0[0], encountered uninitialized memory, but expected an integer` (`cargo miri test -p bun_sys dir::tests::borrow_does_not_close`). - `bufs_storage_init` (`src/resolver/resolver.rs`) does the same with `Box::<Bufs>::new_uninit().assume_init()`. Both sites silenced the `invalid_value` and `clippy::uninit_assumed_init` lints that catch this. ### Fix - Remove both constructors and `impl Default` for both types. Every scratch call site (about 440, mechanical one-line edits) takes a buffer from `bun_paths::path_buffer_pool::get()` or its `w_` / `os_` siblings. The pool zeroes a buffer once on allocation and reuses it, so there is no per-call memset and no 98 KB stack frame on Windows. Structs built per item (`NodeFS`, `bin::NamesIterator`, `PosixToWinNormalizer`) hold a pool guard. Long-lived struct fields use an explicit `ZEROED`. - The pool gains: a `try_with` fallback to the heap while TLS destructors run, `Default` for `PoolGuard` so structs can derive it, and `__lsan_ignore_object` on each allocation. The last one matters because a guard live at `Global::exit` never drops, and in an optimized build its stack slot is dead, so LeakSanitizer reported the buffer and the ASAN lane aborted every `bun <file.md>` run. - The resolver's `Bufs` uses `Box::new_zeroed()` (once per thread). - Verified: `test/internal/source-lints/uninit-assumed-init.test.ts` (its known-site list is now empty, so it fails on main), new pool unit tests under Miri (`cargo miri test -p bun_paths`, 29 pass), a release ASAN build with the CI `ASAN_OPTIONS` on the markdown, `bun exec` and `bunx` paths, `cargo check` for linux, macOS, FreeBSD, Windows x64 and arm64, clippy, and the fs, shell, spawn, path, resolve and `bun add` suites. ### Background - `PathBuffer` is a `[u8; MAX_PATH_BYTES]` newtype used as write-then-read scratch around path syscalls. `MAX_PATH_BYTES` is 4 096 on Linux, 1 024 on macOS and 98 302 on Windows. `WPathBuffer` is the `[u16; 32767]` sibling for Windows wide paths. - "Every bit pattern is a valid `u8`" is true, but uninitialized memory is not a bit pattern. The Rust reference lists an integer read from uninitialized memory as undefined behavior. - `bun_paths::path_buffer_pool` is a per-thread LIFO of up to four boxed buffers with an RAII guard. It already existed with about 180 callers. A cache miss allocates with `alloc_zeroed`, which is usually fresh OS-zeroed pages. <details><summary>Notes</summary> Why not `ZEROED` at the call sites: on Windows that is a 98 KB memset per call at about 440 sites. A previous attempt did exactly that and the leak and stress tests timed out, which is why `uninit()` was introduced. #31258 proposes `ZEROED` again and has the same cost. Why not `[MaybeUninit<u8>; N]` inside `PathBuffer`: every syscall wrapper in `bun_sys` takes `&mut [u8]`, so an initialized-prefix API would change the whole syscall surface. The pool gives the same result with no API change. Why `impl Default` is gone: `Default` has to return a by-value buffer, so it can only be `ZEROED`. With it, `#[derive(Default)]` on a struct that embeds a `PathBuffer` silently picked up a 98 KB memset per construction on Windows (`PosixToWinNormalizer`, built per resolved import). Every zero-fill is now a visible `ZEROED` at its site: the crash handler (must not allocate), `bun_core::spawn_sync` (cold, linux/freebsd), and once-per-task fields in `PackageInstaller`, `extract_tarball`, the test `Scanner`, `Tree` iterators, `PatchFile`, `WindowsWatcher` and the resolver's `top_level_dir_buf`. Overlap with #40680: that PR moves `NodeFS::sync_error_buf` to a pool guard and the `fetch_impl` locals to the pool for Windows stack depth. This PR contains the same source changes (the `fetch_impl` locals were part of the sweep). #40680's test is not included here. LeakSanitizer and `Global::exit`: reproduced with `bun run build:asan` and `ASAN_OPTIONS=detect_leaks=1:abort_on_error=1`. `render_markdown_file_and_exit` held two guards at `exit`, and LSan reported the 4 KB buffer allocated at `path_buffer_pool::get` (`run_command.rs:3423`). `new_boxed` now calls `__lsan_ignore_object` on each buffer (under `cfg(bun_asan)`), which fixes every such site. The markdown body was also split into a function that returns the exit code. Verified both ways: with the original `-> !` shape plus only the LSan call, the test passes. Perf check (debug builds, main vs this branch, three runs each): `Glob("**/*.rs").scan` over `src/` 457/475/459 ms vs 478/453/452 ms, 20 000 `statSync` + `realpathSync` 2663/2674/2663 ms vs 2663/2717/2683 ms, 2 000 `readdirSync` 617/618/617 vs 617/628/626 ms, 4 000 `Bun.resolveSync` 261/262/262 vs 263/266/265 ms. No difference outside noise. No Windows measurement. Windows has the most to gain (no 98 KB stack frames) and nothing in the hot paths zero-fills per call. `bun_sys` cannot join `MIRI_CRATES` in `scripts/rust-miri.ts`: after this change Miri gets past the constructor and stops at `can't call foreign function openat64`, which it has no shim for. The pool and its unit tests stay in `bun_paths`, which is in `MIRI_CRATES`. `test/js/bun/glob/scan.test.ts` times out in this container on main and on this branch alike: it scans the whole checkout, which holds 685 000 files of build output here. Test suites run with the debug build: `test/js/node/fs/fs.test.ts` (565 pass), `test/js/node/fs/fs-mkdir.test.ts`, `test/js/bun/shell/bunshell.test.ts` (431 pass), `test/js/bun/spawn/spawn.test.ts` (148 tests), `test/js/node/path/path.test.js`, `test/js/bun/resolve/` (390 pass, one 5 s timeout that also fails on main here), `test/cli/install/bun-add.test.ts` (71 pass), `test/cli/run/markdown-entrypoint.test.ts`. With the release ASAN build and the CI leak settings: `test/cli/run/` (1177 pass, 4 environment failures: FUSE, uid/gid, a 60 s timeout) and `test/js/bun/shell/exec.test.ts`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 129 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/source-lints/uninit-assumed-init.test.ts bun test v1.4.3 (e0a2b82) test/internal/source-lints/uninit-assumed-init.test.ts: (pass) scans a non-empty set of tracked Rust sources [1.99ms] 68 | }); 69 | 70 | test("no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)]", () => { 71 | // `[^)]*` spans the multi-line form of the attribute; lint paths never 72 | // contain `)`. 73 | expect(scan(/#!?\[(?:allow|expect)\([^)]*\b(?:invalid_value|clippy::uninit_assumed_init)\b/)).toEqual(KNOWN_SITES); ^ error: expect(received).toEqual(expected) - [] + [ + "src/bun_core/util.rs", + ] - Expected - 1 + Received + 3 at <anonymous> (/workspace/bun/test/internal/source-lints/uninit-assumed-init.test.ts:73:97) (fail) no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)] [126.91ms] 1 pass 1 fail 2 expect() calls Ran 2 tests across 1 file. [23.59s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: 1 FAILED bun test v1.4.3-canary.1 (1491acd) test/internal/source-lints/uninit-assumed-init.test.ts: (pass) scans a non-empty set of tracked Rust sources [0.11ms] 68 | }); 69 | 70 | test("no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)]", () => { 71 | // `[^)]*` spans the multi-line form of the attribute; lint paths never 72 | // contain `)`. 73 | expect(scan(/#!?\[(?:allow|expect)\([^)]*\b(?:invalid_value|clippy::uninit_assumed_init)\b/)).toEqual(KNOWN_SITES); ^ error: expect(received).toEqual(expected) - [] + [ + "src/bun_core/util.rs", + ] - Expected - 1 + Received + 3 at <anonymous> (/workspace/bun/test/internal/source-lints/uninit-assumed-init.test.ts:73:97) (fail) no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)] [12.05ms] 1 pass 1 fail 2 expect() calls Ran 2 tests across 1 file. [561.00ms] __F:1:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/source-lints/uninit-assumed-init.test.ts bun test v1.4.3 (e0a2b82) test/internal/source-lints/uninit-assumed-init.test.ts: (pass) scans a non-empty set of tracked Rust sources [2.21ms] (pass) no new #[allow(invalid_value)] or #[allow(clippy::uninit_assumed_init)] [122.33ms] 2 pass 0 fail 2 expect() calls Ran 2 tests across 1 file. [24.05s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 575ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited [1/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_brotli_sys v0.0.0 (/workspace/bun/src/brotli_sys) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compi ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/bun_core/lib.rs | 9 ++ src/bun_core/util.rs | 49 ++------- src/bundler/OutputFile.rs | 7 +- src/bundler/linker_context/computeChunks.rs | 4 +- .../linker_context/generateChunksInParallel.rs | 2 +- .../linker_context/writeOutputFilesToDisk.rs | 6 +- src/bundler/options.rs | 4 +- src/bundler/transpiler.rs | 4 +- src/bunfig/arguments.rs | 6 +- src/crash_handler/lib.rs | 4 +- src/dotenv/env_loader.rs | 4 +- src/event_loop/MiniEventLoop.rs | 2 +- src/glob/GlobWalker.rs | 14 ++- src/install/PackageInstall.rs | 12 +-- src/install/PackageInstaller.rs | 22 ++-- src/install/PackageManager.rs | 14 +-- src/install/PackageManager/CommandLineArguments.rs | 6 +- .../PackageManager/PackageManagerDirectories.rs | 16 +-- .../PackageManager/PackageManagerEnqueue.rs | 8 +- .../PackageManager/PackageManagerOptions.rs | 9 +- .../PackageManager/PackageManagerResolution.rs | 3 +- .../PackageManager/WorkspacePackageJSONCache.rs | 4 +- src/install/PackageManager/patchPackage.rs | 22 ++-- .../PackageManager/updatePackageJSONAndInstall.rs | 9 +- src/install/TarballStream.rs | 6 +- src/install/bin.rs | 12 +-- src/install/extract_tarball.rs | 14 ++- src/install/hoisted_install.rs | 4 +- src/install/isolated_install.rs | 6 +- src/install/lib.rs | 6 +- src/install/lockfile.rs | 8 +- src/install/lockfile/Package.rs | 8 +- src/install/loc ... (truncated) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/bun_core/lib.rs 1 2 11 src/bun_core/util.rs 1 3 14 src/bundler/OutputFile.rs 0 0 9 src/bundler/linker_context/computeChunks.rs 0 0 9 src/bundler/linker_context/generateChunksInParallel.rs 0 0 9 src/bundler/linker_context/writeOutputFilesToDisk.rs 0 0 9 src/bundler/options.rs 0 0 9 src/bundler/transpiler.rs 0 0 9 src/bunfig/arguments.rs 0 0 9 src/crash_handler/lib.rs 0 0 9 src/dotenv/env_loader.rs 0 0 9 src/event_loop/MiniEventLoop.rs 0 0 9 src/glob/GlobWalker.rs 0 0 11 src/install/PackageInstall.rs 0 0 9 src/install/PackageInstaller.rs 0 0 9 src/install/PackageManager.rs 0 0 9 (+ 113 more files) ``` </details> **root cause** · written by the author bot `PathBuffer::uninit`, `WPathBuffer::uninit`, and the resolver's `bufs_storage_init` materialized `[u8; N]` and `[u16; N]` values from `MaybeUninit::uninit().assume_init()`, which is undefined behavior because integer types require initialized memory regardless of whether the bytes are later read, and the `invalid_value` and `uninit_assumed_init` lints that diagnose this were suppressed at those sites. The fix removes those constructors and migrates their call sites to the existing `path_buffer_pool`, which produces buffers through `Box::new_zeroed().assume_init()` so every byte is written b… <!-- robobun:evidence:end -->
Collaborator
|
Thank you for the report and the patch. main has since fixed this a different way, so I am closing this one as superseded.
If you see a site that is still uninitialized on main, please open a new issue or PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes audit finding EXP-089 by making the broad primitive scratch-buffer constructors return initialized Rust values:
PathBuffer::uninit()now returnsPathBuffer::ZEROEDWPathBuffer::uninit()now returnsWPathBuffer::ZEROEDdepth_buf_uninit()now returns[0; MAX_DEPTH]The old bodies used
MaybeUninit::uninit().assume_init()to model Zigundefinedscratch storage. Even for integer element types where every bit pattern is valid, uninitialized memory is not an initialized Rust value: Miri rejects the constructor itself withencountered uninitialized memory, but expected an integer, before caller write-before-read discipline can matter.This is the audit's emergency correctness patch. A performance-preserving follow-up could wrap the storage in
MaybeUninitand expose only initialized prefixes, but the current safe API cannot return an uninitialized primitive array as[T; N].This also removes the stale
invalid_valueallowances and comments that claimed the constructors were sound because callers used the buffers as write-only scratch space.Duplicate check
Before opening this, I checked existing open/all PRs and local/remotes for EXP-089,
PathBuffer,WPathBuffer,depth_buf_uninit, and scratch-buffer fixes. I did not find an existing PR that covers this constructor-validity issue.Verification
bun run fmt:rustgit diff --checkcargo check -p bun_corecargo check -p bun_installcargo check --workspaceCARGO_TARGET_DIR=/tmp/exp-089-postfix-target cargo +nightly miri runin a temp crate depending on localbun_coreand calling the realPathBuffer::uninit()/WPathBuffer::uninit()constructorsbun bd test test/cli/install/hoist.test.ts