node:fs: skip zero-fill of 256 KB pre-stat buffer in async readFile#32295
node:fs: skip zero-fill of 256 KB pre-stat buffer in async readFile#32295robobun wants to merge 1 commit into
Conversation
The async flavor of readFileWithOptions allocates a 256 KB scratch buffer for the read-before-stat optimization. The Zig port used a comptime-sized uninitialised stack array; the Rust port heap-allocates (unavoidable, the array size depends on `flavor`), but used `vec![0u8; 256*1024]`, which zero-fills the slab on every call even though it is write-only (immediately passed to `Syscall::read` and only the kernel-filled prefix is ever observed). Switch to `Vec::with_capacity` via `try_reserve_exact` + `VecExt::expand_to_capacity`, matching the pattern already used for the write-only slab in `copy_file_using_read_write_loop` and the post-stat read loop later in the same function. On allocation failure the buffer is left empty and the pre-stat loop is skipped, falling through to fstat (same as sync-without-VM already does).
|
Warning Review limit reached
More reviews will be available in 4 minutes and 13 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 4:31 AM PT - Jun 15th, 2026
❌ @robobun, your commit 61f26c6 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32295That installs a local version of the PR into your bun-32295 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the change mirrors the exact try_reserve_exact + expand_to_capacity pattern already used a few lines below (and in copy_file_using_read_write_loop), but since it adds a new unsafe block in the async readFile hot path it's worth a quick human glance.
Extended reasoning...
Overview
This PR replaces vec![0u8; 256 * 1024] with Vec::new() + try_reserve_exact + unsafe { expand_to_capacity() } for the pre-stat scratch buffer in the async branch of readFileWithOptions (src/runtime/node/node_fs.rs:7145-7152), and hoists the use bun_collections::vec_ext::VecExt import up so it covers both call sites. Net change is ~10 lines in a single function.
Correctness
I traced the buffer's lifetime: pre_stat_buf is sliced from async_stack_buffer and handed to Syscall::read(fd, available) in a loop; only &pre_stat_buf[..total] (the kernel-filled prefix) is ever read afterward. The SAFETY comment is accurate. On try_reserve_exact failure the vec stays empty, the pre-stat loop is skipped (available.is_empty() is immediately true), and execution falls through to fstat — identical to the existing sync-without-VM path at line 7159. One minor behavioural delta: expand_to_capacity() sets len = capacity(), so if the allocator rounds 256 KB up the loop may read slightly past 256 KB, whereas the old vec![0u8; N] set len to exactly N. This is harmless (still bounded, same semantics) and matches the precedent at lines 4896-4903.
Security risks
None identified. The uninit bytes are never observed by user code — they're write-only into read(2). This is the same uninit-slab idiom already accepted in this function at line 7296 and in copy_file_using_read_write_loop at line 4902, with the same VecExt::expand_to_capacity helper whose documented contract (vec_ext.rs:121-125) covers exactly this use.
Level of scrutiny
Medium. It's a small, mechanical perf-parity fix that copies an established in-file pattern verbatim, and the bug-hunting pass found nothing. But it introduces a new unsafe block in fs.readFile/fs.promises.readFile — one of the hottest, most widely-exercised paths in the runtime — so I'd rather a human confirm the SAFETY reasoning than auto-approve.
Other factors
readFile tests pass per the PR description. No outstanding reviewer comments. No CODEOWNER signal available to me.
|
CI: all individual lanes passed. The aggregate is red because of No |
1 participant
What
The async flavor of
readFileWithOptionsallocates a 256 KB scratch buffer for the read-before-stat optimization. In Zig (node_fs.zig:5193) this was a comptime-sized uninitialised stack array:The Rust port (
node_fs.rs:7145) cannot size a stack array onflavor, so it heap-allocates, but usedvec![0u8; 256 * 1024], which also zero-fills the slab on every asyncfs.readFile/fs.promises.readFilecall. The buffer is write-only: it is handed straight toSyscall::readand only the[..total]prefix the kernel filled is ever observed. The 256 KB memset is wasted work the Zig version did not do.Fix
Allocate via
try_reserve_exact+VecExt::expand_to_capacity, matching the uninit-slab pattern already used incopy_file_using_read_write_loop(node_fs.rs:4896) and the post-stat read loop later in the same function. On allocation failure the buffer is left empty and the pre-stat loop is skipped, falling through to fstat (same behaviour as the sync-without-VM path already has).Verification