Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 12:53 PM PT - Aug 16th, 2026
✅ @robobun, your commit 86486a89562bd02d9222b75d85a671a8124930c4 passed in 🧪 To try this PR locally: bunx bun-pr 33715That installs a local version of the PR into your bun-33715 --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 new stat chain mirrors the existing on_chmod pattern closely, but ~80 lines of new unsafe libuv FFI with manual event-loop ref/unref on a Windows-only path is worth a human sanity check.
Extended reasoning...
Overview
This PR fixes Bun.write(dest, Bun.file(src)) resolving with 0 instead of the byte count on Windows. The root cause is that uv_fs_copyfile never populates req->statbuf, but on_copy_file was reading statbuf.size() from a zero-initialized request. The fix chains an async uv_fs_stat on the destination after a successful copy and passes the resulting size to on_complete. A new test in bun-write.test.js covers path→path, Bun.file→Bun.file overwrite, and empty-source variants.
Security risks
None. This is file-I/O return-value plumbing on paths the process already opened and copied; no new user-controlled input reaches a trust boundary, and no auth/crypto/permissions code is touched.
Level of scrutiny
Medium-high. The change is Windows-only (#[cfg(windows)]) and closely mirrors the existing uv_fs_chmod chain in on_complete (same io_request.deinit() → zero → set .data → FFI call → ref_concurrently() sequence, same req->data recovery pattern in the callback). That said, it's ~80 lines of new unsafe libuv FFI with manual event-loop ref/unref balance and raw-pointer provenance, in a file where getting the lifecycle wrong means UAF or a hung promise. I verified the ref/unref pairs balance on all paths (on_copy_file unrefs the copyfile ref; stat_destination_after_copy refs only after successful enqueue; on_stat_after_copy_file unrefs), and the fallback on_complete(0) on stat failure degrades to the pre-fix behavior rather than hanging.
Other factors
The bug-hunting system found nothing. The test is well-placed in the existing suite and follows harness conventions. CI is still building. Given the unsafe-FFI surface and that Windows-only paths are harder to eyeball-verify, I'd rather a human confirm the io_request reuse and path_ptr lifetime (libuv strdups it, so the stack pathbuf scope looks fine) than auto-approve.
|
The diff is ready.
This change is |
robobun
changed the title
There was a problem hiding this comment.
Thanks for the macOS follow-up — the fcopyfile/read-write-loop read_len assignments look right. I don't see further issues, but the new Windows uv_fs_stat chaining (io_request reuse + event-loop ref/unref) is unsafe-FFI lifecycle code, so I'd like a human to sign off on it.
Extended reasoning...
Overview
This PR fixes Bun.write(dest, Bun.file(src)) resolving with 0 instead of the copied byte count. It touches src/runtime/webcore/blob/copy_file.rs on two platforms:
- Windows:
on_copy_filepreviously readio_request.statbuf.size(), which libuv'sfs__copyfilenever populates. The fix chains an asyncuv_fs_staton the destination after a successful copy (newstat_destination_after_copy+on_stat_after_copy_filecallback), then passesstatbuf.size()toon_complete. Falls back toon_complete(0)if the stat can't be issued or fails, since the copy itself already succeeded. - macOS (added in 05439c2 after my earlier review):
do_fcopy_file_with_read_write_loop_fallbacknow setsself.read_lenfromfstat(destination_fd)after a successfulfcopyfile, and fromtotal_writtenon the EBADF read/write-loop branch; the caller caps it atmax_lengthafterftruncate.
A new test in test/js/bun/io/bun-write.test.js asserts the resolved value for path→path, overwrite-existing, and empty-source cases.
Security risks
None identified. No user-controlled input is newly parsed; the added code stats a path that was already opened/copied by the same operation.
Level of scrutiny
This is native runtime code with unsafe libuv FFI, io_request reuse (deinit() + zeroed re-init), and event-loop ref_concurrently/unref_concurrently pairing across an added async hop. The new code closely mirrors the existing on_complete → uv_fs_chmod → on_chmod pattern in the same file (same request reuse, same data provenance recovery, same ref/unref sequencing), and the ref/unref balance looks correct on all paths I traced. The macOS additions are small and match the FreeBSD/clonefile-success paths. Still, per the approval criteria this is not a simple/mechanical change — it modifies async lifecycle in a memory-safety-sensitive area across two #[cfg]-gated platforms — so it warrants a human reviewer familiar with CopyFileWindows lifecycle.
Other factors
- My prior inline concern (macOS
n2assertion failing on the fcopyfile fallback) was addressed in 05439c2 and the thread is resolved. - The bug-hunting pass on the latest revision found no issues.
- CI on the target test file is reported green across all lanes; the red lanes cited are unrelated flakes (napi GC timing, postgres service unavailability).
- Latest build (#70243) for 05439c2 was still running at the time of the last status update, so macOS lane results for the new fcopyfile change aren't yet confirmed in-thread.
… fd (#36758) ## Repro ```sh 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. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… fd (oven-sh#36758) ## Repro ```sh 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 oven-sh#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). oven-sh#32859 reworks the same block to honour source slices and oven-sh#33715 sets `read_len` after `fcopyfile`; both are complementary and will need a small rebase against whichever lands first. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Heads-up from a duplicate report of the Windows 0-byte resolve: this PR is currently showing merge conflicts with main and needs a rebase (copy_file.rs has drifted since July). Also, open PR #37072 adds a test to test/js/bun/io/bun-write.test.js that pins the current Windows behavior with |
|
One more test to reconcile alongside #37072 (from another duplicate report of the Windows 0-byte resolve, still reproducible on current main:
|
…OS overwrite uv_fs_copyfile does not populate req->statbuf, so the Windows copyfile completion handler always reported 0 bytes. After a successful copy, chain an async uv_fs_stat on the destination and report that size. On macOS, when clonefile fails (e.g. EEXIST because the destination already exists) the fcopyfile fallback never assigned read_len. Stat the destination after a successful fcopyfile, record total_written on the EBADF read/write loop, and cap read_len at max_length after ftruncate.
05439c2 to
86486a8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // fcopyfile() doesn't report a byte count; stat the destination | ||
| // (opened with O_TRUNC above) to learn how many bytes were written. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// libuv's `uv_fs_copyfile` does not populate `req->statbuf`, so after a | ||
| /// successful copy we issue an async stat on the destination to learn how | ||
| /// many bytes were written before resolving the promise. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I re-reviewed after the rebase and the macOS fcopyfile fix — no new issues found. The Windows uv_fs_stat chaining mirrors the existing on_chmod pattern (deinit/zero/ref-unref balance checks out), and the macOS read_len assignments now cover both the fcopyfile and EBADF branches as suggested. Given the ~85 lines of new unsafe libuv callback plumbing and the two unresolved comment-cop flags, a human look is still worthwhile.
What was reviewed:
- Event-loop ref/unref balance across
on_copy_file→stat_destination_after_copy→on_stat_after_copy_file→on_complete— paired on every path including stat-enqueue failure andget_fd_pathfailure. path_ptrlifetime acrossuv_fs_stat— points into stackpathbufor Arc-held store; libuvstrdups it, same as theuv_fs_chmodsite.on_complete(0)fallback when stat fails — degrades to the pre-fix truncate behavior rather than rejecting a successful copy.- No
isWindows ? 0 : Nguards from #37072/#32906 are present in the current test file, so nothing to reconcile yet.
Extended reasoning...
Overview
This PR fixes Bun.write(dest, Bun.file(src)) resolving 0 instead of the byte count on two platform-specific paths in src/runtime/webcore/blob/copy_file.rs. On Windows it replaces a read of the never-populated io_request.statbuf with a chained async uv_fs_stat on the destination (new stat_destination_after_copy + on_stat_after_copy_file callback, ~85 lines). On macOS it assigns self.read_len in both success branches of do_fcopy_file_with_read_write_loop_fallback and caps it at max_length after ftruncate. A new test in test/js/bun/io/bun-write.test.js covers path→path, overwrite, and empty-source cases.
Security risks
None identified. This is file-copy byte-count reporting; no untrusted input parsing, auth, or crypto is touched. The new unsafe blocks are FFI calls into libuv following the exact pattern already used for uv_fs_chmod in the same file.
Level of scrutiny
Moderate-to-high. The Windows change adds new async callback chaining over a heap-allocated struct with raw-pointer recovery from req->data, plus event-loop keep-alive ref/unref pairing across three callbacks. While it copies the established on_chmod pattern line-for-line (deinit → zeroed → set data → ref on enqueue → unref in callback), and I traced the ref/unref balance on every exit path, this is exactly the class of code the repo's review guide singles out for careful memory-safety review. The macOS change is simpler (three targeted assignments) and directly addresses the gap I flagged in my earlier review.
Other factors
My previous inline finding (macOS n2 failing on the fcopyfile fallback) was fixed as suggested and the thread is resolved. CI passed on Windows/macOS/Linux for bun-write.test.js per the author's summary; remaining red lanes were unrelated flakes. Two unresolved comment-cop bot flags landed today on the 2- and 3-line explanatory comments — they read as false positives to me (both explain why a stat is needed, which the code cannot say), but a maintainer should confirm. The reconciliation notes for #37072/#32906 are merge-order dependent and neither pattern is present in the current test file. Given the new unsafe FFI surface and platform-gated paths I cannot exercise directly, deferring to human review rather than approving.
|
A third duplicate report of the Windows 0-byte resolve landed. Two notes for whoever merges this:
|
|
Closing in favor of #41209. It fixes the byte count of a whole-file copy on Windows and macOS, and it has the test case from this PR.
|
1 participant
Problem
Bun.write(dest, Bun.file(src))resolves with0instead of the number of bytes copied on two platform-specific paths. The file content is written correctly; only the promise's resolved value is wrong.Any caller that checks
written === expectedas an integrity check fails.Cause
Windows: the
uv_fs_copyfilecompletion handler readsio_request.statbuf.size()to learn how many bytes were copied (on_copy_fileinsrc/runtime/webcore/blob/copy_file.rs). libuv'sfs__copyfilenever writesreq->statbuf; it only sets the result code. The request struct is zero-initialized, so the reported size is always0. This also causeson_completeto run an unnecessarytruncate()against pre-sized destinations.macOS: when the destination already exists,
clonefile(2)fails withEEXISTand control falls through tofcopyfile. Neither thefcopyfilesuccess branch nor itsEBADFread/write-loop fallback assignedself.read_len, so the promise resolved with the initial value0.Fix
Windows: after a successful
uv_fs_copyfile, chain an asyncuv_fs_staton the destination and pass the resultingstatbuf.size()toon_complete. The destination path is re-derived from the file store the same waycopyfile()already does. If the follow-up stat cannot be issued, the promise still resolves since the copy itself succeeded.macOS:
do_fcopy_file_with_read_write_loop_fallbacknow setsself.read_lenon both success branches: anfstatof the (O_TRUNC-opened) destination afterfcopyfile, ortotal_writtenfrom theEBADFloop. The caller capsread_lenatmax_lengthafterftruncate, mirroring the existing clonefile-success and FreeBSD paths.Verification
Windows, before fix:
Windows, after fix: 35/35 tests pass in
test/js/bun/io/bun-write.test.js(including theBUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE=1re-run).Linux: behavior unchanged; the new test passes on POSIX as-is.
macOS:
cargo check -p bun_runtime --target aarch64-apple-darwinpasses; verified by CI.Note: both bugs are platform-specific, so fail-before is reproducible on a Windows build (confirmed above) and a macOS build respectively.