Bun.write: don't over-copy or ftruncate a caller-supplied destination fd

[robobun]

Repro

head -c 200000 /dev/zero | tr '\0' S > /tmp/src.bin
printf 'DDDDDDDDDDDDDDDDDDDDDDDDDDDDDD' > /tmp/dst.bin
BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 bun -e '
  const fs = require("fs");
  const fd = fs.openSync("/tmp/dst.bin", "r+");
  console.log(await Bun.write(Bun.file(fd).slice(0, 5), Bun.file("/tmp/src.bin")));
  fs.closeSync(fd);
'
wc -c /tmp/dst.bin
# Linux (fallback): 200000 (expected 30; whole source over-copied then ftruncated)
# macOS:            5      (expected 30; fcopyfile rewrote from 0 then ftruncate(5))

Same for Bun.stdout under >> redirection: the pre-existing bytes in the redirected file are gone after the write.

Cause

CopyFile::run_async dispatched fd-backed destinations through the same path as path destinations.

• macOS: fcopyfile(COPYFILE_DATA) rewrites the destination from offset 0, and when stat.st_size > max_length Bun then calls ftruncate(dest, max_length) to trim the over-copied tail. Both are safe when Bun opened the destination with O_CREAT|O_TRUNC, but for a caller-supplied fd they discard whatever the file already contained.
• FreeBSD and the Linux read/write fallback (kernel < 4.5, EXDEV, ENOTSUP, EINVAL, or BUN_CONFIG_DISABLE_COPY_FILE_RANGE): copy_file_using_read_write_loop reads the source to EOF regardless of max_length and then ftruncate(dest, total_written) sets the file length, with the same effect.

The Linux copy_file_range/sendfile/splice happy path already copies exactly max_length bytes and does not truncate, so this only shows up there when the kernel syscall is unavailable for the fd pair.

Fix

Add read_write_loop_capped, a bounded read()/write() loop that transfers exactly cap bytes (or to EOF) and never touches the destination's length. Route every fd-backed destination through it:

• macOS/FreeBSD run_async: take the fcopyfile/ftruncate branch only when destination_file_store.pathlike is a path.
• Linux do_copy_file_range: the three identical fallback sites are factored into read_write_fallback, which keeps the existing copy-to-EOF + ftruncate for path destinations and uses the bounded loop for fd destinations.

Verification

Two tests in test/js/bun/io/bun-write.test.js under Bun.write(Bun.file(fd), Bun.file(path)) does not truncate the fd spawn a child with BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 so they fail-before on every POSIX lane:

=== system bun ===
(fail) preserves bytes past the slice window in an r+ fd
  resolved: "200000", content: 200000 x "S"   (expected "5", "SSSSS" + 25 x "D")
(fail) preserves pre-existing bytes when stdout is redirected with >>
  resolved: "1000", content: 1000 bytes        (expected "100", 110 bytes)

=== this branch ===
(pass) preserves bytes past the slice window in an r+ fd
(pass) preserves pre-existing bytes when stdout is redirected with >>

The existing Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe test is widened from Linux-only to every POSIX lane, covering the bounded loop's EOF-termination branch on macOS/FreeBSD. cargo check -p bun_runtime --target {aarch64,x86_64}-apple-darwin and --target x86_64-unknown-freebsd pass.

Related

Found during review of #36692 (which routes a FIFO source through splice on Linux; the bun_opened_dest guard it adds to the same three fallback sites is subsumed by read_write_fallback here). #32859 reworks the same block to honour source slices and #33715 sets read_len after fcopyfile; both are complementary and will need a small rebase against whichever lands first.

---

no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js

[coderabbitai]

Walkthrough

Changes

Copy fallbacks distinguish path destinations from existing descriptors. Descriptor writes use bounded loops, while path destinations are truncated. Size limits apply only to regular files. POSIX tests cover sliced writes, redirected stdout, and pipe copying.

Changes

Copy fallback behavior

Layer / File(s)	Summary
Fallback dispatch and copy limits src/runtime/webcore/blob/copy_file.rs	Fallback dispatch tracks destination origin, limits source-size calculations and preallocation to regular files, and routes unsupported operations through the shared descriptor-aware helper.
POSIX destination-aware copying src/runtime/webcore/blob/copy_file.rs	macOS, FreeBSD, Linux, and Android fallbacks truncate path destinations, use bounded read/write copying for descriptors, handle partial writes, and record copied bytes.
POSIX regression coverage test/js/bun/io/bun-write.test.js	Tests validate sliced writes through existing descriptors, redirected stdout, preserved trailing bytes, and pipe copying on non-Windows platforms.

Possibly related PRs

• oven-sh/bun#35953: Modifies POSIX descriptor-based Bun.write handling and related tests.
• oven-sh/bun#36590: Overlaps with Linux and unknown-size copy behavior and regression coverage.
• oven-sh/bun#36692: Modifies copy_file.rs and pipe-copy behavior for FIFO sources.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main fix for caller-supplied destination file descriptors.
Description check	✅ Passed	The description explains the problem, cause, fix, verification steps, tests, and platform coverage in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

No related open issues found.

[claude]

No bugs found, but this changes Bun.write I/O semantics for fd-backed destinations on macOS/FreeBSD and the author flags it needs coordination with #32859 and #33715 which touch the same block — worth a human look before landing.

What was reviewed:

• do_read_write_loop_capped: bounds arithmetic (SizeType = u64, want capped at 64 KiB, remaining -= amt can't underflow), short-write handling, error paths set system_error + read_len.
• Path-vs-fd gating in the macOS/FreeBSD blocks: do_close() is still reached on every branch; the macOS ftruncate call now uses the safe bun_sys::ftruncate(Fd, i64) wrapper (matches the FreeBSD path).
• The 64 KiB stack buffer matches existing usage in read_file.rs / node_fs.rs; bun_sys::read/write on macOS use $NOCANCEL (same as the existing fallback loop).
• Tests: fd is closed in finally, subprocess pipes drained concurrently, exit code asserted last; the Linux/Windows skip rationale checks out (copy_file_range rejects O_APPEND before writing).

Extended reasoning...

Overview

This PR fixes data loss in Bun.write(Bun.file(fd), Bun.file(path)) on macOS and FreeBSD. Previously, an fd-backed destination went through the same fcopyfile(COPYFILE_DATA) (macOS) or unbounded copy_file_using_read_write_loop + ftruncate (FreeBSD) path as a path destination. Both operations are safe when Bun opened the destination itself with O_CREAT|O_TRUNC, but for a caller-supplied fd (e.g. Bun.stdout under >> redirection, or Bun.file(fs.openSync(..., 'r+'))) they discard whatever the file already contained. The fix adds a bounded read()/write() loop (do_read_write_loop_capped) that transfers exactly max_length bytes and never touches the destination's length, and gates the destructive path on PathOrFileDescriptor::Path(_).

Two files touched: src/runtime/webcore/blob/copy_file.rs (+~90/-30 across cfg-gated macOS/FreeBSD blocks) and test/js/bun/io/bun-write.test.js (+50, two new tests skipped on Linux/Windows).

Security risks

None identified. The change narrows what Bun.write does to a caller-supplied fd (write only the requested bytes at the fd's current position, honoring O_APPEND) rather than expanding it. No new user-controlled data reaches size arithmetic — cap is derived from min(stat.st_size, max_length) - offset as before, and the loop's per-iteration want is clamped to 64 KiB.

Level of scrutiny

Medium-high. This is a semantic change to a core user-facing I/O API on two platforms, one of which (FreeBSD) has no CI coverage and the other (macOS) is a smaller CI lane than Linux. The new tests are describe.skipIf(isWindows || isLinux), so they won't run on the majority of CI. The author reports cargo check for all three targets passes, but runtime verification is macOS-only. The change also swaps the macOS raw bun_sys::darwin::ftruncate for the safe bun_sys::ftruncate wrapper — a small drive-by that matches the FreeBSD block.

Other factors

• The PR description explicitly names two in-flight PRs (#32859, #33715) that rework the same run_async block and will need rebasing against whichever lands first. That merge-order decision is a maintainer call.
• I traced the new loop against SizeType = u64 / MAX_SIZE = (1<<52)-1 from src/jsc/webcore_types.rs: when the source has st_size == 0 (pipe/fifo), max_length stays at MAX_SIZE and the loop correctly reads to EOF; when st_size != 0, max_length is already clamped before the call.
• bun_sys::read/write on macOS use $NOCANCEL variants without an EINTR retry loop (check_once!), same as the existing copy_file_using_read_write_loop fallback, so no behavior divergence there.
• The 64 KiB stack buffer pattern is used in read_file.rs, node_fs.rs, and elsewhere in src/sys/, so it's an accepted idiom on the work-pool thread.
• Tests follow the harness conventions (tempDir, Buffer.alloc(n, fill), pipes drained via Promise.all, cleanup in finally before assertions could throw, exit code asserted last).

Given the platform-specific behavior change, limited CI exercise, and the explicit coordination note with two other PRs, I'm deferring rather than approving.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/js/bun/io/bun-write.test.js:490 — Changing it.skipIf(!isLinux) → it.skipIf(isWindows) removed the only use of isLinux in this file, so the isLinux import from harness on line 9 is now dead — drop it from the destructured import list.

  Extended reasoning...

  What

  The PR changes the FIFO copy test's guard from it.skipIf(!isLinux) to it.skipIf(isWindows) (so it runs on macOS/FreeBSD as well). That was the only reference to the isLinux helper in test/js/bun/io/bun-write.test.js; the destructured import on line 9 now binds a name nothing reads.

  Verification

  $ rg -n isLinux test/js/bun/io/bun-write.test.js
  9:  isLinux,
  

  The one place elsewhere in the file that still needs a Linux gate uses process.platform === "linux" directly (the copyFileRange block), so it does not depend on this import.

  Why it matters

  REVIEW.md ("Code style & idioms reviewers enforce") lists this as required scope for the same PR:

  Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): … helpers whose last caller you rewired …

  An unused named import is exactly that case — the last consumer was rewired to isWindows.

  Impact

  None at runtime — the test file still executes correctly and Bun's test runner does not fail on unused bindings. This is purely hygiene / repo-convention compliance, hence nit severity (non-blocking).

  Fix

  Remove isLinux from the destructured harness import on line 9:

   import {
     bunEnv,
     bunExe,
     exampleHtml,
     exampleSite,
     gcTick,
  -  isLinux,
     isWindows,
     tempDir,
     withoutAggressiveGC,
   } from "harness";

• 🔴 src/runtime/webcore/blob/copy_file.rs:889-893 — On macOS/FreeBSD, fstat() on a pipe reports the bytes currently buffered in st_size (unlike Linux's 0), so the earlier if stat.st_size != 0 { self.max_length = min(st_size, max_length)... } clamp fires and caps max_length to ~16–64 KiB. The new fd-dest branch then calls do_read_write_loop_capped(self.max_length), which hard-stops at that cap — so Bun.write(Bun.stdout, Bun.stdin) now copies only what was in the pipe buffer at fstat time and drops the rest, and the newly-un-skipped 1 MiB pipe test (skipIf(isWindows) instead of skipIf(!isLinux)) will fail/flake on macOS CI. Skip the st_size clamp when !S::ISREG(stat.st_mode), or pass MAX_SIZE to do_read_write_loop_capped for non-regular sources.

  Extended reasoning...

  What the bug is

  On macOS and FreeBSD, fstat(2) on a pipe/FIFO reports the number of bytes currently available for reading in st_size. XNU's pipe_stat (bsd/kern/sys_pipe.c) sets sb->st_size = rpipe->pipe_buffer.cnt, and FreeBSD's sys/kern/sys_pipe.c does the same. Linux always reports 0 for pipes — which is why the existing if stat.st_size != 0 guard in run_async was sufficient there, and why the test comment ("fstat on a FIFO reports st_size == 0") is only true on Linux.

  In run_async, after fstat(source_fd), this block runs unconditionally (no S_ISREG guard) before the platform-specific copy blocks:

  if stat.st_size != 0 {
      self.max_length = (SizeType::try_from(stat.st_size).expect("int cast")
          .min(self.max_length))
          .max(self.offset) - self.offset;
      ...
  }

  So on macOS/FreeBSD with a pipe source that has data queued, self.max_length is clamped to whatever happened to be in the pipe buffer at the moment of the fstat (typically ≤ 16–64 KiB).

  Why it wasn't a problem before this PR

  Before this PR, on macOS the fd-destination path went through do_fcopy_file_with_read_write_loop_fallback(); fcopyfile on a non-seekable pipe fails with EBADF and falls back to copy_file_using_read_write_loop(..., stat_size=0, ...), which reads the source to EOF regardless of max_length. On FreeBSD it called copy_file_using_read_write_loop(..., 0, ...) directly. The subsequent if st_size > max_length { ftruncate } check evaluates N > N == false (they're equal after the clamp), so the clamped max_length was harmless — the whole stream was copied and nothing was truncated.

  Why it's a problem now

  After this PR, when the destination is a caller-supplied fd, the macOS/FreeBSD blocks call self.do_read_write_loop_capped(self.max_length). read_write_loop_capped's loop is:

  let mut remaining = cap;
  while remaining > 0 {
      let want = (buf.len() as SizeType).min(remaining) as usize;
      let amt = bun_sys::read(src_fd, &mut buf[..want])?;
      if amt == 0 { break; }
      remaining -= amt as SizeType;
      ...
  }

  It stops after exactly cap bytes even if the source has more. So only the bytes that were in the pipe buffer at fstat time are copied; the rest of the stream is silently dropped.

  Step-by-step proof

  For the test Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe (> 4096 bytes) on macOS:

  1. sh -c 'head -c 1048576 /dev/zero | bun -e ... | wc -c' — stdin and stdout are both real kernel pipes (fd-backed, not paths).
  2. By the time bun starts and reaches fstat(stdin), head has almost certainly filled some of the pipe buffer (bun startup latency ≫ head write latency), so st_size is non-zero and ≤ ~16 KiB (macOS default pipe buffer).
  3. stat.st_size != 0 → self.max_length = min(16384, MAX_SIZE) = 16384.
  4. S_ISDIR is false; the Linux #[cfg] block is skipped; the macOS block runs.
  5. destination_file_store.pathlike is Fd(_) (Bun.stdout), so the else-branch calls do_read_write_loop_capped(16384).
  6. The loop reads/writes exactly 16384 bytes and returns; read_len = 16384.
  7. The test asserts { piped: '1048576', resolved: '1048576' } but gets { piped: '16384', resolved: '16384' } (or whatever was buffered) → fail.

  The PR changes this test from it.skipIf(!isLinux) to it.skipIf(isWindows), so it now runs on macOS CI and will fail (or flake, if fstat happens to race ahead of head's first write and see st_size == 0).

  Impact

  This is a functional regression: Bun.write(fd_dest, pipe_source) on macOS/FreeBSD — including the common Bun.write(Bun.stdout, Bun.stdin) shell-pipe pattern — now silently truncates the stream to whatever fit in the pipe buffer at startup.

  Fix

  Either skip the st_size clamp for non-regular sources:

  if stat.st_size != 0 && bun_sys::S::ISREG(stat.st_mode as _) {
      self.max_length = ...;
  }

  or, in the macOS/FreeBSD fd-dest branches, pass MAX_SIZE to do_read_write_loop_capped when !S::ISREG(stat.st_mode) so it reads to EOF.

[robobun]

Good catch on the BSD pipe st_size semantics. Fixed in 0f8c74d by gating the max_length clamp on S_ISREG(st_mode); the bounded loop now sees MAX_SIZE for a pipe source and reads to EOF. The isLinux import was already removed by autofix.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/runtime/webcore/blob/copy_file.rs (1)

869-935: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Assign self.read_len on successful macOS path copies. The fcopyfile helper does not update it on success, including the successful fallback path. The Path-destination branch then resolves Bun.write() with the initial 0 despite copying data.

🤖 Prompt for 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.

In `@src/runtime/webcore/blob/copy_file.rs` around lines 869 - 935, Update the
successful macOS Path-destination branch around
do_fcopy_file_with_read_write_loop_fallback so self.read_len is assigned to the
number of bytes copied, including when fcopyfile or its fallback succeeds.
Preserve the existing truncation behavior and ensure Bun.write resolves with the
copied length instead of the initial zero.

🤖 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 `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 950-973: The read_write_fallback function must enforce finite cap
values when bun_opened_dest is true. Route finite caps through
read_write_loop_capped, while preserving the existing
copy_file_using_read_write_loop and final truncation behavior for cap ==
MAX_SIZE.

---

Outside diff comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 869-935: Update the successful macOS Path-destination branch
around do_fcopy_file_with_read_write_loop_fallback so self.read_len is assigned
to the number of bytes copied, including when fcopyfile or its fallback
succeeds. Preserve the existing truncation behavior and ensure Bun.write
resolves with the copied length instead of the initial zero.

🪄 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: 55fe91f7-971b-4e39-82ef-d8f7f76895a1

📥 Commits

Reviewing files that changed from the base of the PR and between 9395d0e and 0f8c74d.

📒 Files selected for processing (2)

• src/runtime/webcore/blob/copy_file.rs
• test/js/bun/io/bun-write.test.js

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/runtime/webcore/blob/copy_file.rs (2)

792-806: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Preserve the destination length during preallocation. preallocate_file calls fallocate with mode 0, which extends st_size. A failed or short Linux copy can therefore leave trailing zeroes for both Path and Fd destinations. Use FALLOC_FL_KEEP_SIZE or truncate to the bytes written on every incomplete copy path.

🤖 Prompt for 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.

In `@src/runtime/webcore/blob/copy_file.rs` around lines 792 - 806, Update the
preallocation flow around preallocate_file and the incomplete-copy cleanup paths
so allocation does not extend the destination’s logical length: use
FALLOC_FL_KEEP_SIZE when supported, or truncate the destination to the number of
bytes actually written before returning. Apply this consistently for both Path
and Fd destinations while preserving normal completion behavior.

---

322-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Pass only the remaining copy budget to fallback paths.

fallback_cap remains unchanged after optimized bytes are written. The fallback can therefore copy beyond max_length. Also, NodeFS::copy_file_using_read_write_loop continues to EOF after a finite stat_size. Use the remaining budget and disable that unbounded phase for bounded copies.

🤖 Prompt for 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.

In `@src/runtime/webcore/blob/copy_file.rs` around lines 322 - 326, Update the
fallback setup around fallback_cap so it reflects the remaining copy budget
after optimized bytes are written, rather than the original limit. For bounded
copies, pass that remaining budget to fallback paths and disable the unbounded
NodeFS::copy_file_using_read_write_loop phase that continues to EOF after a
finite stat_size; preserve unlimited behavior for unknown-size copies.

🤖 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 `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 975-978: Update the documentation comment for the read/write
helper in src/runtime/webcore/blob/copy_file.rs to remove the inaccurate claim
that it never changes the destination length. State only the durable guarantees:
it never truncates the destination, never reads past cap, and stops at cap or
EOF.

---

Outside diff comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 792-806: Update the preallocation flow around preallocate_file and
the incomplete-copy cleanup paths so allocation does not extend the
destination’s logical length: use FALLOC_FL_KEEP_SIZE when supported, or
truncate the destination to the number of bytes actually written before
returning. Apply this consistently for both Path and Fd destinations while
preserving normal completion behavior.
- Around line 322-326: Update the fallback setup around fallback_cap so it
reflects the remaining copy budget after optimized bytes are written, rather
than the original limit. For bounded copies, pass that remaining budget to
fallback paths and disable the unbounded NodeFS::copy_file_using_read_write_loop
phase that continues to EOF after a finite stat_size; preserve unlimited
behavior for unknown-size copies.

🪄 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: 64418344-a8c6-4107-9ae4-dd3a2655b5da

📥 Commits

Reviewing files that changed from the base of the PR and between 0f8c74d and b13543b.

📒 Files selected for processing (1)

• src/runtime/webcore/blob/copy_file.rs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/runtime/webcore/blob/copy_file.rs (1)

344-355: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Apply self.offset to every source read path.

do_open_file opens path sources at offset zero. self.read_off is incremented but never used, and all copy_file_range, fallback, macOS, and FreeBSD reads use the descriptor’s current position. Sliced copies can start at byte zero while reporting the shortened read_len. Apply the offset before dispatch and test the first copied byte and read_len.

🤖 Prompt for 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.

In `@src/runtime/webcore/blob/copy_file.rs` around lines 344 - 355, Update the
copy operation’s source-read setup to apply self.offset (including self.read_off
as it advances) before every read path, covering copy_file_range, fallback,
macOS, and FreeBSD dispatches. Ensure do_open_file sources begin at the
requested offset rather than descriptor position zero, while preserving accurate
read_len and validating that the first copied byte matches the slice start.

🤖 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.

Outside diff comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 344-355: Update the copy operation’s source-read setup to apply
self.offset (including self.read_off as it advances) before every read path,
covering copy_file_range, fallback, macOS, and FreeBSD dispatches. Ensure
do_open_file sources begin at the requested offset rather than descriptor
position zero, while preserving accurate read_len and validating that the first
copied byte matches the slice start.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0a8083b9-84f3-4cd2-9e6e-7454c6d766b6

📥 Commits

Reviewing files that changed from the base of the PR and between 0fbca83 and 4a3f8f0.

📒 Files selected for processing (1)

• src/runtime/webcore/blob/copy_file.rs

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/runtime/webcore/blob/copy_file.rs:803-808 — preallocate_file here still runs on caller-supplied destination fds. On Linux it is fallocate(fd, mode=0, 0, max_length), and mode-0 fallocate extends the file to offset+len — the same length-mutating-syscall-on-caller-fd class this PR fixes for ftruncate/fcopyfile. With the PR's own >>-redirect test but a >2 MiB source, fallocate(1, 0, 0, 3_000_000) zero-extends the caller's log file before the copy starts (fallocate addresses by explicit offset, so O_APPEND doesn't help), and the subsequent O_APPEND writes then append after the zeros. Gate the preallocate on matches!(self.destination_file_store.pathlike, PathOrFileDescriptor::Path(_)) — same guard as the other sites.

  Extended reasoning...

  What the bug is

  bun_sys::preallocate_file (src/sys/lib.rs:7546-7558) is fallocate(fd, /*mode=*/0, offset, len) on Linux. Per fallocate(2), mode 0 (the default "allocate" mode) says: "If the size of the file is less than offset+len, then the file size will be changed to offset+len." It is a length-mutating syscall on the destination fd — exactly the invariant this PR establishes must not happen for caller-supplied fds.

  The call at copy_file.rs:805-813 (which this PR's diff touches — it removed the ISREG clause from this exact condition) has no destination_file_store.pathlike guard, so it fires for a caller-supplied fd whenever the source is a regular file with a computed max_length > PREALLOCATE_LENGTH (2 MiB) and != MAX_SIZE.

  Step-by-step proof (the PR's own >> scenario, source > 2 MiB)

  head -c 3000000 /dev/zero > /tmp/src.bin           # 3 MB regular-file source
  printf 'AAAAAAAAAA' > /tmp/log.txt                 # 10-byte caller file
  bun -e 'await Bun.write(Bun.stdout, Bun.file("/tmp/src.bin"))' >> /tmp/log.txt

  1. run_async: source fstat → st_size = 3_000_000, S_IFREG. Enter the outer if stat.st_size != 0 && ISREG(...) block.
  2. self.max_length = min(3_000_000, MAX_SIZE).max(0) - 0 = 3_000_000.
  3. PREALLOCATE_SUPPORTED (Linux) && 3_000_000 > 2_097_152 && 3_000_000 != MAX_SIZE → fallocate(1, 0, 0, 3_000_000). fallocate addresses by explicit offset (only write(2) honours O_APPEND), so log.txt is zero-extended from 10 bytes to 3 000 000 bytes right here.
  4. Linux dispatch: source ISREG → do_copy_file_range::<CopyFileRange, _>. copy_file_range(2) on an O_APPEND fd_out returns EBADF (man page: "the O_APPEND flag is set for the open file description referred to by fd_out"). EBADF is not in the {ENOSYS, EXDEV, ENOTSUP, EINVAL} fallback set, so do_copy_file_range sets system_error and Bun.write rejects — but the caller's file has already been silently blown out to 3 MB of zeros.
  5. With BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 (the env the PR's tests set), step 4 instead goes straight to read_write_fallback → read_write_loop_capped, whose write()s honour O_APPEND and land after the fallocate-created 3 MB, giving ~6 MB instead of the expected 3 000 010 bytes.

  Either way the caller's file layout is corrupted before the copy proper even starts.

  Why existing code doesn't prevent it

  The PR adds matches!(destination_file_store.pathlike, Path(_)) guards to the macOS fcopyfile/ftruncate block, the FreeBSD block, and the three Linux read/write fallback sites (via bun_opened_dest in read_write_fallback). The preallocate_file call sits above all of those, in the shared ISREG-source setup, with no equivalent guard. PREALLOCATE_SUPPORTED is Linux-only, so this is a Linux/Android-specific hole.

  The PR's new tests use 200 KB and 1 KB sources, both under PREALLOCATE_LENGTH = 2 MiB, so they don't exercise this branch — per REVIEW.md "Cover the variant matrix", the >2 MiB source variant is the missing case in the same test.

  Why this belongs in this PR

  This is pre-existing on main (the PR only hoisted the ISREG guard from the inner preallocate condition to the outer if; reachability is unchanged). But REVIEW.md's "Fix the whole class in the same PR — same-class sites are ONE concern, not scope creep" applies directly: it is the identical bug class (length-mutating syscall against a caller-supplied destination fd), in the same function, on lines the diff already edits, and the PR's title/description explicitly establishes "never mutate a caller-supplied fd's length" as the invariant. This is not the path-destination slice handling deferred to #32859 — this is the caller-fd class this PR owns.

  Fix

  One line, same shape as the other guards:

  if PREALLOCATE_SUPPORTED
      && matches!(self.destination_file_store.pathlike, PathOrFileDescriptor::Path(_))
      && self.max_length > PREALLOCATE_LENGTH
      && self.max_length != MAX_SIZE
  {

  and bump the >>-redirect test's source above 2 MiB (e.g. Buffer.alloc(3_000_000, "S"), slice (0, 3_000_000)) so it fail-befores on Linux without needing BUN_CONFIG_DISABLE_COPY_FILE_RANGE.

[robobun]

Good catch on preallocate_file: fallocate(fd, 0, 0, len) is length-mutating and was also unguarded for caller-supplied fds. Gated on PathOrFileDescriptor::Path(_) in da64c88 and added a >2 MiB-source test that fails on main with a 6 MB zero-padded result and passes with the fix.

[claude]

My earlier findings (BSD pipe st_size clamp, missing #[inline(never)]) are both addressed; the follow-up commits also gate preallocate_file on a path destination and derive the fallback cap from the live remain. No new issues in this pass, but this is cross-platform I/O with macOS/FreeBSD-specific branches that only CI can exercise, and it intersects three other in-flight PRs (#36692, #32859, #33715) — worth a human look before merging.

Checked: read_write_loop_capped short-write loop and 0-write early-return; ISREG gate now covers both the max_length clamp and the preallocate block (source-is-regular still enforced by the outer condition); fallback_cap(remain) is evaluated at each of the three Linux fallback sites and remain is undecremented on the error branches, so the cap is correct even if the first kernel-copy iteration errored.
Ruled out: the widened Bun.write(Bun.stdout, Bun.stdin) pipe test on macOS now reaches do_read_write_loop_capped(MAX_SIZE) because the ISREG gate keeps max_length at MAX_SIZE for a FIFO source.

Extended reasoning...

Overview

The PR stops Bun.write(Bun.file(fd), Bun.file(path)) from over-copying and ftruncate-ing a caller-supplied destination fd. It adds a bounded read_write_loop_capped helper, factors the three identical Linux fallback sites into read_write_fallback (which branches on whether Bun opened the destination), gates the macOS fcopyfile+ftruncate and FreeBSD read/write+ftruncate paths on a path destination, gates the st_size→max_length clamp on S_ISREG (so BSD pipe st_size doesn't cap the copy), and gates preallocate_file on a path destination. Three new subprocess tests exercise the fd-destination path with BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1, and the existing FIFO→FIFO test is widened from Linux-only to all POSIX.

Since my previous review

Commits 0f8c74d, b13543b, 0fbca83, d9aa656, da64c88 landed after my last comments. They (a) gate the st_size clamp on ISREG, (b) add #[inline(never)] to read_write_loop_capped, (c) drop a doc comment, (d) compute fallback_cap from the live remain at each call site, and (e) gate preallocate_file on a path destination with a new >2 MiB test. All of my previous findings and CodeRabbit's are addressed and resolved in the thread.

Security risks

None identified. This is local file I/O; no untrusted-length parsing, no auth/crypto. The bounded loop uses a fixed 64 KB stack buffer with #[inline(never)] matching the sibling copy_file_using_read_write_loop convention.

Level of scrutiny

High. This is core Bun.write behaviour with per-OS branches (#[cfg(target_os = "macos")], freebsd, linux|android) whose correctness depends on kernel semantics (fcopyfile rewriting from offset 0, BSD pipe_stat reporting buffered bytes in st_size, fallocate extending an O_APPEND fd). The PR description itself defers macOS/FreeBSD verification to CI. It also explicitly overlaps with #36692, #32859, and #33715, so a maintainer should sequence the merges.

Other factors

• Path-destination fallback semantics are intentionally left unchanged (verbatim factor-out); the author and CodeRabbit agreed the sliced-path-destination cap is #32859's scope.
• Tests follow harness conventions (tempDir, bunEnv spread, combined-object assertions before exitCode, Buffer.alloc(n, fill) over repeat).
• The macOS ftruncate call site was also switched from raw unsafe { darwin::ftruncate } to the bun_sys::ftruncate wrapper, a small cleanup.

Given the platform-specific reach and the coordination needed with three related PRs, I'm deferring rather than approving.

[robobun]

^{Updated 11:05 PM PT - Aug 3rd, 2026}

@autofix-ci[bot], your commit 43b197b is building: #88699

[robobun]

CI on a559220: 194/196 jobs passed. bun-write.test.js (with the three new fd-destination tests and the widened pipe test) is green on every Linux lane and on all five macOS test lanes that ran (darwin 26 aarch64 x2, darwin 14 aarch64, darwin 14 x64 x2). The two non-passing jobs are unrelated to this diff:

• darwin 14 aarch64 (second shard) timed out during the tart VM checkout sync and never ran any tests.
• windows 11 aarch64 fails bun-upgrade.test.ts with "Canary builds are not available for this platform yet", which is a missing artifact for that platform and not touched here.

Ready for review.

[Jarred-Sumner]

@robobun fix conflicts, rebase, close if unneeded

[robobun]

Rebased onto main (43b197b) and resolved the import conflict in bun-write.test.js. The bug still reproduces on the current release and none of #36692 / #32859 / #33715 have merged, so this is still needed. All three fd tests plus the widened pipe test fail-before / pass-after locally; cargo check is clean on aarch64-apple-darwin and x86_64-unknown-freebsd.

[claude]

No bugs found, but this reworks platform-gated file-copy dispatch across macOS/FreeBSD/Linux with a new hand-written bounded read/write loop and known merge interactions with #36692/#32859/#33715 on the same block — worth a human look before landing.

What was reviewed:

• read_write_loop_capped handles partial writes and EOF correctly; #[inline(never)] matches the sibling helper's 64 KB stack-buffer convention.
• read_write_fallback's bun_opened_dest arm is a verbatim factor-out of the three pre-PR call sites (copy-to-EOF + ftruncate(total)); path-destination behavior is unchanged.
• The st_size clamp's new ISREG guard subsumes the old inner preallocate_file ISREG check, so swapping that for the Path(_) gate doesn't lose the source-is-regular condition.
• fallback_cap(remain) at the mid-loop ENOSYS/EXDEV/ENOTSUP site uses the already-decremented remain, so a partial fast-path copy before fallback keeps the correct budget.

Extended reasoning...

Overview

This PR fixes data loss in Bun.write(Bun.file(fd), Bun.file(path)) when the destination is a caller-supplied file descriptor rather than a path Bun opened with O_CREAT|O_TRUNC. It touches src/runtime/webcore/blob/copy_file.rs (~150 lines net across macOS, FreeBSD, and Linux/Android #[cfg] blocks) and adds three subprocess tests plus widens one existing test in test/js/bun/io/bun-write.test.js.

The core change: fd-backed destinations now route through a new read_write_loop_capped that copies at most cap bytes and never calls ftruncate/fallocate. Path destinations keep the existing semantics unchanged (verified by tracing read_write_fallback's bun_opened_dest arm against the three pre-PR call sites — same stat_size derivation, same trailing ftruncate, only the raw libc::ftruncate swapped for the bun_sys::ftruncate wrapper).

Security risks

None identified. This is local file I/O with no parsing of untrusted input; the change strictly narrows what Bun does to a caller-owned fd (stops writing past the requested window, stops truncating). No new unsafe blocks are introduced — two raw libc::ftruncate calls are replaced with the safe bun_sys::ftruncate wrapper.

Level of scrutiny

High. This is cross-platform I/O in a user-facing API with per-OS #[cfg] branches whose correctness depends on kernel-specific semantics (XNU/FreeBSD pipe_stat reporting buffered bytes in st_size, fcopyfile rewriting from offset 0, fallocate extending an O_APPEND fd). The PR has already been through several review rounds that caught real issues (BSD pipe st_size clamp, missing #[inline(never)], preallocate_file gating, deriving the fallback cap from live remain), which is exactly the signal that this code is subtle enough to want a maintainer's eyes on the final state.

Other factors

• CI on the pre-rebase head was green on all Linux and macOS lanes for the new tests (194/196 jobs, both failures unrelated).
• The PR description explicitly names three open PRs (#36692, #32859, #33715) that touch the same block and will need rebasing against whichever lands first — a maintainer should decide merge order.
• Jarred already engaged ("fix conflicts, rebase, close if unneeded") and robobun rebased; the post-rebase state hasn't had a human sign-off.
• The tests follow harness conventions well (tempDir, Buffer.alloc over .repeat, concurrent pipe drain, combined-object assertion before exitCode, BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 to force fail-before on Linux).