Bun.write(file, file): check the source before opening the destination - #38186
Bun.write(file, file): check the source before opening the destination#38186robobun wants to merge 1 commit into
Conversation
CopyFile opened the destination with O_CREAT | O_TRUNC and only then
fstat'ed the source, so a directory source ("That doesn't work on
folders"), a source kind Linux can't copy ("Non-regular files aren't
supported yet"), or a failing fstat rejected the write after the
destination had already been emptied (or created).
Open the source, fstat it, and pick the Linux copy syscall first; the
destination is opened only once the source is known to be usable. The
Windows read/write fallback had the same order and now opens the source
first as well.
|
Warning Review limit reached
Next review available in: 29 seconds 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 |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reorders the open/validate/open sequence across four platform branches in CopyFile::run_async (a core Bun.write path), a human look would still be worthwhile.
Checked: open_source/open_destination/pick_copy_syscall are the old do_open_file<IOWhich> and Linux dispatch bodies verbatim; every new early-return path (fstat fail, ISDIR, pick_copy_syscall → None, open_destination fail) calls do_close() so the Bun-opened source fd is released; Retry::Fail still sets system_error via mkdir_if_not_exists → set_system_error. The macOS clonefile guard (both pathlikes are Path) is equivalent to the old both fds INVALID — the block is only de-indented. On Windows, read_write_loop.close() before mkdirp() correctly resets must_close_source_fd/source_fd so the copyfile() re-entry re-opens the source without leaking; prepare_pathlike only sets must_close on success, so throw() after a failed source open is a no-op close.
Extended reasoning...
Overview
Fixes a data-loss bug in file-to-file Bun.write: when the source turns out to be unusable (a directory, or on Linux a FIFO → regular file), the destination was already opened O_CREAT|O_TRUNC and left as a 0-byte file. The fix reorders run_async to open the source, fstat it, run all source-rejection checks, and only then open the destination. do_open_file<IOWhich> is split into open_source/open_destination; the Linux syscall-choice conditions are hoisted into pick_copy_syscall so the FIFO rejection can happen before the destination exists. The Windows read/write fallback (prepare_read_write_loop) gets the same reordering, plus a read_write_loop.close() before the mkdirp retry so the already-opened source fd doesn't leak when copyfile() re-enters. Tests cover the {path, Bun.file} × {path, fd} directory-source matrix, an absent destination, a Linux FIFO source, and a missing source (which also exercises the Windows fallback via the file's self-rerun under BUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE=1).
Security risks
None. This is local file I/O ordering; no untrusted-input parsing, no permission or auth logic. The destination-open flags and mode are unchanged.
Level of scrutiny
High — CopyFile::run_async is the production path for Bun.write(BunFile, BunFile) on every POSIX target, with distinct cfg branches for macOS (clonefile/fcopyfile), Linux/Android (copy_file_range/splice/sendfile), FreeBSD, and a separate Windows implementation. Most of the ~250-line Rust diff is de-indentation of the macOS clonefile block (previously nested under if both fds INVALID) and moving code into helpers verbatim, but the reordering itself needs the fd-lifecycle and error-path audit that REVIEW.md calls for.
Other factors
I traced every new early-return in run_async and confirmed each releases the Bun-opened source via do_close() (which is guarded on pathlike being a Path and the fd being valid, so caller-supplied fds are untouched). The Retry::Fail arm in open_destination no longer closes the source inline, but its only caller now calls do_close() on Err, and mkdir_if_not_exists still sets system_error on that path so then() rejects. The removed if system_error.is_some() { return; } was dead (every system_error-setting path in the old do_open_file also returned Err), and the removed empty if let Fd(_) = dest.pathlike {} block did nothing. The macOS clonefile guard change from both fds INVALID to both pathlikes are Path(_) is equivalent given the two if let Fd(fd) assignments immediately above. On Windows, prepare_pathlike only sets must_close after a successful open, so the new source-first order doesn't leak on failure, and the added close() before mkdirp() handles the one case where the source was opened but the destination open failed with ENOENT. Tests use tempDir, close their fds in finally, hold the FIFO open r+ so Bun's O_RDONLY open doesn't block, and assert exact rejection messages plus untouched destination content. The PR description documents fail-before verification and cross-target cargo check. Given the size and the four-way platform branching, deferring rather than approving.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Updated 11:32 AM PT - Aug 13th, 2026
✅ @robobun, your commit 819807e075ec23026b019c50b6acfe28f67f51b1 passed in 🧪 To try this PR locally: bunx bun-pr 38186That installs a local version of the PR into your bun-38186 --bun |
|
Not a duplicate of #31515: that PR adds source slice windows and, as part of its Windows plumbing, reorders |
Problem
Bun.write(dest, Bun.file(dir))rejects withThat doesn't work on folders, butdestis 0 bytes afterwards. Same with a directory fd as the source, with a FIFO source on Linux (Non-regular files aren't supported yet), and with aBun.file(dest)destination; a destination that did not exist is left behind as an empty file.CopyFile::do_open_fileinsrc/runtime/webcore/blob/copy_file.rsopened the source and right after it the destination withO_CREAT | O_WRONLY | O_TRUNC. The sourcefstat, the directory check and the Linux source-kind dispatch (where the FIFO rejection comes from) all ran later, inrun_async.fs.copyFileandcprefuse the same inputs without touching the destination, andBun.writewith a missing source already did, because the source open happened first.Fix
run_asyncnow opens the source (if it is a path),fstats it, rejects a directory, picks the Linux copy syscall, and only then callsopen_destination(). The position of that call is the fix; the checks themselves are the existing ones.do_open_file<IOWhich>is split intoopen_source/open_destination. ItsBothmode encoded the old order; the dest-open failure path now closes a Bun-opened source through the existingdo_close().pick_copy_syscallis the dispatch block's three conditions, unchanged, returningNonefor the unsupported case so it can be evaluated before the destination exists; the dispatch block now matches on its result.clonefileattempt, size clamp, preallocate and copy code are unchanged apart from indentation. The deadif destination is Fd {}block and the redundantsystem_error.is_some()check in the rewritten region are removed.uv_fs_copyfile) never touches the destination for an unusable source (probed with a directory, a directory fd and a missing source), so it is unchanged.prepare_read_write_loop) had the same order: underBUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE=1a missing source truncated the destination. It now opens the source first, and closes it before a mkdirp retry becausecopyfile()re-runs from the top. It still cannot recognise a directory source at open time (GetFileTypereports directories as files); with a path source that fallback is only reachable under the test flag, so that is left alone.test/js/bun/io/bun-write.test.js,Bun.write(dest, Bun.file(unusable source)) leaves the destination alone: directory source for {path,Bun.file} destination x {path, fd} source, directory source with an absent destination, FIFO source (Linux only; macOS copies FIFOs through its read/write fallback), missing source. The six directory/FIFO cases fail on the released build and pass with this change. The missing-source case passes on POSIX either way; it is there for the Windows fallback, where it failed before (the file re-runs itself with the flag on Windows).cargo check -p bun_runtimeforaarch64-apple-darwinandx86_64-unknown-freebsd,cargo clippy -p bun_runtime,cargo fmt --check: clean.ftruncateafter the sourcefstat, which would still create an absent destination and still truncate before the Linux FIFO rejection. Bun.write(file, Bun.stdin): splice a pipe source into any destination on Linux #36692 would make Linux copy FIFO sources; if it lands, the FIFO case here becomes a successful copy and that test goes away. blob: copy the source slice window in Bun.write(file, file.slice()), not the whole file #31515 (source slice windows) contains the same Windowsprepare_read_write_loopreorder as part of its Windows plumbing but leaves the POSIX order alone; whichever lands second has a trivial conflict in that one Windows function.Background
Bun.writeis handled byCopyFile(POSIX) andCopyFileWindows, not by the generic blob write path.CopyFile::run_asyncruns on the work pool and reports throughself.system_error: set it and return, andthen()rejects the promise with it.Bun.stdout,Bun.file(fd)), which Bun never opens, truncates or closes. That is why both opens are conditional on*_fd == Fd::INVALIDand whydo_close()only closes what Bun opened.copy_file_range(regular to regular),splice(pipe to pipe) orsendfile(regular file, char device or socket source), each with a read/write fallback; any other source kind is rejected. The choice depends only on the source'sst_modeand the destination store's cached mode, so it can be made before the destination is opened.destination_file_store.modeis 0 unless thatBunFilewas stat'ed earlier (.size,.lastModified, or an fd store likeBun.stdout);pick_copy_syscallkeeps the existing rule that 0 counts as a regular file.Local note on an unrelated test in the same file
should work when copyFileRange is not available > on large filesexceeds the 5 s default per-test timeout in my environment with both the released and the patched build (it moves 256 MB through the disk; CI runs the file with a much larger per-test timeout). #37792 is already about that test.no test proof · iteration 0 · 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