io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request - #37787
io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request#37787robobun wants to merge 4 commits into
Conversation
IoRequestLoop::schedule took &mut Request. The io thread recovers the owning ReadFile/WriteFile from exactly the pointer that was pushed, by container-of, so a pointer made from a &mut of the io_request field does not reach the owner under the aliasing rules (bun_core::container_of's contract says as much), and the reference stayed protected across the push and wake() while the io thread may already be writing the request. The three callers (wait_for_readable, wait_for_writable, FileCloser::do_close) reached it from &mut self chains, so the publishing thread kept protected references to the object on its stack while the io thread, a second pool thread, or (through io_task.finish()) the JS thread wrote to or freed it. schedule now takes *mut Request, projected from the owner. The io thread passes the popped pointer itself to the request callback, and the returned Action carries *mut Poll projected the same way, so the address registered with epoll/kqueue (which the readiness dispatch turns back into the owner) has the owner's provenance too. The callbacks take the pointer. On the pool side, the re-entries that already held the object's pointer (do_read_loop_task, do_write_loop_task, on_close_io_request) now pass it down: update, the read/write loops, on_finish, the two waits and do_close take this: *mut Self, keep their accesses statement scoped, and end with the hand-over. WriteFile::do_write no longer waits from inside; it reports WouldBlock and the loop hands the file over as its last step, where before the caller went on to read self.errno after the object had been handed to the io thread. FileCloser::update, which nothing called, is removed. The first step of a job (JobContext::run -> get_fd -> run_async_with_fd / run_with_fd) still runs under the &mut those traits pass; it now does its work first and hands over through a pointer as its last act, and the traits are left for a separate change. The WorkPool hand-overs in these files (on_ready, on_io_error, on_io_request_closed) are converted separately as well and are not touched here. A source lint bans scheduling an io request spelled from self or through a &mut reborrow; it reports the three converted sites on main. Tests cover the FIFO paths that go through the io thread: reading a FIFO by path, and Bun.write() to a non-blocking FIFO that is already full.
|
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 (7)
WalkthroughThe PR changes Blob I/O scheduling and callbacks from lifetime-bound mutable references to unsafe raw pointers. It adds explicit read and write transition states, updates epoll and kqueue registration, and adds FIFO regression and source-lint tests. ChangesBlob I/O pointer transition
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Status: ready for review. Build 93086 (d02b342) passed on all 194 jobs.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the FIFO-test hang in 58ed1d3 — the childDied race + closeHolder() on early exit now surfaces a child crash as EPIPE instead of blocking a pool thread. I re-reviewed and found no further issues, but given this reshapes the cross-thread ownership hand-over for ReadFile/WriteFile across three threads in unsafe Rust, a human pass is still worthwhile.
What was reviewed:
- The
&mut self→*mut Selfrestructuring (update/proceed/do_close/wait_for_*) — each hand-over is now the last access, anddo_writereturningWriteStepremoves the post-scheduleerrnoread. Actionlosing its lifetime andregister_for_epoll/apply_kqueuederivingudatafrom the raw pointer before reborrowing — the kqueueClosearm'son_donestill fires afterapply_kqueueas before.take_requestclearingscheduledmatches what both tick loops already did; the trampolines' redundant clear is correctly dropped.- The
run_async_with_fd/run_with_fdseam:selfis coerced to*mutandproceedis the last statement, so the caller's&mut self(fromget_fd/JobContext::run) is not touched after the hand-over inside this frame — the residual&mutin those callers is noted in the description as out of scope.
Extended reasoning...
Overview
This PR converts bun_io::IoRequestLoop::schedule and its three callers (ReadFile::wait_for_readable, WriteFile::wait_for_writable, FileCloser::do_close) from &mut Request/&mut self to raw *mut pointers projected from the owner. The stated motivation is pointer provenance (the io thread's container_of recovery needs whole-allocation provenance, which a &mut field reborrow does not carry) and Stacked/Tree Borrows soundness (a protected &mut self on the publishing thread's stack while another thread writes through the object). The change ripples through: RequestCallback becomes unsafe fn(*mut Request) -> Action, Action loses its lifetime and carries *mut Poll, register_for_epoll/apply_kqueue take *mut Poll, and the pool-side control flow in read_file.rs/write_file.rs is restructured around a Next enum + proceed dispatcher so each stage's &mut self borrow ends before the hand-over call. do_write returns a WriteStep enum instead of scheduling the wait itself, which incidentally removes a post-hand-over self.errno read the description identifies as a (theoretical) race. ~800 lines of diff across 4 Rust files plus a new source-lint test and two behavioural FIFO tests.
Security risks
None in the traditional sense — no user-input parsing, auth, or crypto. The risk surface is memory safety: this is unsafe Rust managing cross-thread ownership of heap objects via raw pointers and intrusive fields, on the path that backs Bun.file().text() / Bun.write(). A mistake here is a UAF or double-schedule. The refactor is meant to tighten safety (correct provenance, no live &mut across a hand-over), and I did not find a place where the new shape is less sound than the old one, but the reasoning is subtle enough (e.g. the run_with_fd seam where get_fd's &mut self is still on a caller frame) that it warrants a second pair of eyes.
Level of scrutiny
High. This is exactly the category REVIEW.md's "Native code: memory safety" section calls the most-blocked: cross-thread ownership, raw-pointer hand-off, container-of recovery, and platform-gated code (epoll vs kqueue) where only one half compiles on any given machine. The change is behaviour-preserving by intent (both new FIFO tests pass on main), so correctness rests on whether every converted path still performs the same operations in the same order — which requires reading each converted function against its predecessor. That is not a mechanical review.
Other factors
- My prior inline finding (the FIFO read test hanging a pool thread if the child dies) was addressed in 58ed1d3:
proc.exitednow races the write and closesholderto trigger EPIPE. - The description explicitly scopes out two adjacent changes (#37768's
WorkPool::scheduleconversions in the same files, andJobContext::run/get_fd's&mut self), so a reviewer should confirm the boundary is drawn where the author says it is. - The comment-cop bot has flagged many of the new doc comments; those are
# Safetycontracts on newly-unsafe fns, not workaround justifications, so I don't read them as blocking — but the author may want to trim a few (58ed1d3 already shortened several). - The kqueue path (
tick_kqueue,apply_kqueue) was checked by cargo clippy foraarch64-apple-darwinper the description but not run; the epoll and kqueue tick loops are both restructured, so a macOS CI run matters.
The child's Bun.file(fifo).bytes() did not finish on the darwin lanes while the same setup completes on Linux; the streaming twin of this case in streams.test.js is already todo'd on macOS. The kqueue half of the change is exercised there by the Bun.stdin pipe tests; this test adds the path-opened variant, which is Linux-only for now.
Both tests passed the same way whether or not the ReadFile/WriteFile ever reached the io thread. In debug builds the child now writes its WriteFile scope trace to a file (BUN_DEBUG); the parent waits for the request to show up there before feeding or draining the pipe, which makes at least one round trip certain, and afterwards checks that every request came back as a readiness callback and that the close round-tripped through the io thread. A second write-side case issues two concurrent writes on one fd: the second epoll registration fails with EEXIST, which is the registration-error hand-over, and the trace checks that the failed WriteFile still closed through the io thread before rejecting. The write test's drain also no longer blocks forever in open() if the child died after reporting the pipe full: the open is raced against the child's exit, and a lost race lends the pending open a writer so it can be closed. waitForFileToContain is added to the harness for the trace waits.
There was a problem hiding this comment.
Re-reviewed after d02b342 — that commit addresses my earlier note on the write-side FIFO test (the blocking open is now raced against childDied and a stuck open is unblocked in finally). No further issues found this pass. Given the scope — reshaping the cross-thread ownership hand-over through IoRequestLoop::schedule, both tick loops, and the ReadFile/WriteFile/FileCloser state machines in unsafe Rust — a human look is still worthwhile.
What was reviewed:
take_request/register_epolland the kqueue tick:pollis only dereferenced beforeon_error/on_done, matching the old ordering.do_write→WriteStep: the post-hand-overself.errnoread is gone; the!could_blockEAGAIN retry loop is preserved.read_until_blocked/write_until_blocked: control flow matches the old loop bodies;self.bufferis restored on every exit.stderrin the write FIFO test is not double-consumed (.text()vs thefor awaitloop) — only the async iterator reads it.
Extended reasoning...
Overview
This PR converts the io-thread hand-over in bun_io::IoRequestLoop::schedule from &mut Request to *mut Request projected from the owner, and propagates that shape through every caller: Request::callback becomes unsafe fn(*mut Request) -> Action, Action carries *mut Poll instead of &mut Poll, register_for_epoll/apply_kqueue take the raw pointer, and the ReadFile/WriteFile/FileCloser pool-side entry points (update, do_read_loop/do_write_loop, on_finish, wait_for_*, do_close) are reshaped to take *mut Self and end each stage before the hand-over. The read/write loop bodies are lifted into read_until_blocked/write_until_blocked returning a Next enum, and do_write returns WriteStep instead of calling wait_for_writable internally. Tests: a source-lint banning the old spelling, two FIFO tests (read and write) that trace the hand-overs via debug logging, and a waitForFileToContain harness helper.
Prior feedback
I left two inline comments on earlier revisions. The first (read-side FIFO test hangs on child death) was fixed in 58ed1d3. The second (write-side FIFO test's blocking readFile(fifo) hangs if the child dies after filled) is addressed in d02b342: the test now uses fs.promises.open raced against a childDied rejection, and the finally block unblocks a still-pending open by briefly opening a writer. Once the read fd is open, a dead child's writer fd closing produces EOF on readFile() rather than a hang, so the assertions surface the failure.
Security risks
None user-facing. The change is internal pointer-provenance / aliasing-model correctness with no change to input parsing, auth, or trust boundaries.
Level of scrutiny
High. This is a ~500-line restructuring of unsafe Rust across the io thread's request queue, both epoll and kqueue tick loops, and the ReadFile/WriteFile state machines that hand ownership between three threads. The change is intended to be behaviour-preserving but every hand-over site is rewritten, do_close and on_finish change receiver shape, and do_write's control flow is re-expressed. A mistake here would be a cross-thread UAF or a lost completion, not a compile error.
Other factors
The bug-hunting pass found nothing this run. I traced the epoll/kqueue tick arms against the pre-change code — poll is used only before on_error/on_done, matching the previous ordering; take_request clears scheduled exactly as the old inline reborrow did. read_until_blocked restores self.buffer on both the WaitForReadable return and the fall-through, matching the old do_read_loop. The WriteStep refactor removes the post-hand-over self.errno read the description calls out while keeping the !could_block EAGAIN spin. The ReadFileUV::do_close call site still passes self but io_request returns None there, so it never schedules. Given the breadth of unsafe changes and the number of platform-gated arms (Linux/macOS/FreeBSD/Windows), this is beyond what I'd auto-approve, but I found nothing further to flag.
|
Heads-up on the "Bun.write(Bun.file(fd)) into a full non-blocking FIFO" test: #37852 makes a named pipe on macOS wait in select(2) on the pool thread instead of going through the io thread, so on a macOS debug build it no longer logs |
|
Heads-up on the macOS skip of the FIFO test here: #37823 fixes the hang it skips around (XNU's kqueue filter for a FIFO vnode never fires for the last writer closing, and poll(2) is built on the same filter; the fix waits in select(2) on the pool for named pipes). That PR adds the same 256 KiB scenario, unskipped, plus five more cases to the same describe block in |
1 participant
Problem
bun_io::IoRequestLoop::schedulehands aReadFile/WriteFileto the io thread, and all three callers passed it&mut self.io_requestfrom a&mut selfmethod. Same class as blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 / fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703 / bundler: hand a finished Bun.build back through its pointer, not a &mut receiver #37723; the io-thread twin of Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768.&mut Pollin the returnedAction) later comes back from the io thread or the kernel and the owner is recovered from it by container-of. A pointer made from&mutto one field only covers that field, so the walk back to the owner leaves its bounds;container_ofdocuments that as disallowed.schedulereturns, and the JS thread may then free it, while every&mut selfframe on the publishing thread's stack is still live. Both aliasing models (Tree Borrows is whatbun run rust:miriuses) reject a foreign write or free under a live&mut, used again or not.WriteFile::do_writescheduled the wait onEAGAINand its caller then readself.errno, racing the io thread. In a tiny window both threads could runon_finishand schedule the close twice. There is no report of it.Fix
scheduletakes*mut Request, projected from the owner, and the push is the caller's last access. The io thread passes the popped pointer straight to the callback (nowunsafe fn(*mut Request) -> Action), andActioncarries*mut Poll, registered with the kernel as given. So every pointer that comes back was derived from the whole owner.update, the read / write loops,on_finish,wait_for_*,FileCloser::do_close) takesthis: *mut Selfand hands over as its last act. Loop bodies are unchanged; they return what to do next instead of doing it.do_writereports would-block instead of waiting from inside, which removes theerrnoread.schedulecall nothing on the calling thread touches the object and no&mutinto it is still live. Behaviour is otherwise meant to be preserved. Left alone on purpose: the&muta job's first step receives fromJobContext::run/get_fd, and theWorkPoolhand-overs in these files (Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768's).EEXISTerror path) through the io thread and check the debug trace; both also pass on main, so only the lint tells the trees apart. The read case is skipped on macOS (the child never finished there; reported separately). These plus neighbouring file tests pass in a debug ASAN build; clippy is clean on Linux and darwin.Background
read()/write()on aReadFile/WriteFilewould block, it pushes the request onto a lock-free queue and wakes that thread, which registers the fd and, when it fires or registration fails, hands the object back to a pool thread.scheduleis the only cross-thread entry point.RequestandPollare fields embedded in theReadFile/WriteFile. The io thread and the kernel are given a field's address and the owner is recovered by subtracting the field offset (container-of). That is only defined if the pointer was made from the whole owner (&raw mut (*this).field), not from a&mutto the field.&mut Tclaims exclusive access to the bytes it was made from. Under Stacked / Tree Borrows (what miri checks), another thread writing to or freeing those bytes while it is live is UB even if it is never used again. A raw pointer claims nothing, so a cross-thread hand-over goes through one and is the frame's last use.FileCloser: the shared close path for these tasks. On Linux a registered fd must be unregistered before close, so closing also round-trips through the io thread (schedule_close, back viaon_close_io_request); that round trip is the thirdschedulecaller.test/internal/source-lints/are tests that scan bun's own source for banned patterns; the new one bans ascheduleargument spelled fromselfor written as a&mutreborrow.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 test/js/bun/util/bun-file-read.test.ts
Original description
What
bun_io::IoRequestLoop::schedule(src/io/lib.rs) is how aReadFile/WriteFileis handed to the io thread: to wait for its fd to become readable or writable, and (on Linux) to have the fd unregistered before it is closed. It took&mut Request, and all three callers reached it from&mut self:No crash is known. This is the io-thread twin of the hand-over #37768 converts for
WorkPool::schedule, and the same class as #37681 / #37703 / #37723; none of those touch this entry point.Why the old shape is wrong
schedulepushedNonNull::from(request), and the io thread hands that exact pointer back to the request's callback (on_request_readable,on_request_writable, theschedule_closegenerated byimpl_file_closer!), which walks back to theReadFile/WriteFilewithcontainer_of. A pointer made from&mut self.io_requestonly has provenance over theRequestfield, so that walk leaves its bounds;bun_core::container_of's own contract says a&mut fieldreborrow does not suffice, andIntrusiveIoRequest::from_io_requestrequires whole-allocation provenance that nothing was providing. The same thing happened one level down: theActionreturned by those callbacks carriedpoll: &mut Poll,register_for_epoll/apply_kqueuederived the address they register with the kernel from it, and__bun_io_pollable_on_ready(src/runtime/dispatch.rs) later turns that address back into the owner bycontainer_ofas well. (src/runtime/webcore/blob/copy_file.rs already carries a comment explaining why its libuv request must not be recovered this way; this is the same hazard.)scheduled, register the fd and, on a registration error or a close, hand the object straight on to a pool thread, all beforeschedulehas returned fromwake().schedule's own&mut Requestargument, and the&mut selfof every frame between the pool's entry point and the call (do_read_loop_task->update->do_read_loop->wait_for_readable;on_finish->do_close; the write side likewise), stayed protected on the publishing thread's stack while another thread wrote through the object. The same frames also end inio_task.finish(), after which the JS thread frees the object. A foreign write to, or deallocation of, memory a protected reference covers is rejected by both aliasing models (Tree Borrows is whatbun run rust:miriuses) whether or not the reference is used again; the reduction in blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681's description shows the dealloc case of this shape.WriteFile::do_writecalledwait_for_writable()itself onEAGAINand returnedfalse, anddo_write_loop_posixthen readself.errnoto tell "waiting" from "failed". That read happens after the hand-over, so it raced the io thread / the next pool thread; had the fd become writable and the nextwrite()failed inside that window, both threads would have runon_finishand scheduled the close twice. The window is tiny and there is no report of it; it is fixed because the restructuring below removes it for free.Fix
IoRequestLoop::scheduletakes*mut Requestand documents that it must be projected from the owner (&raw mut (*this).io_request) and that the push is the caller's last access. The io thread's two tick loops go through onetake_request, which passes the popped pointer itself to the callback (no&mutreborrow in between).Request::callbackbecomesRequestCallback = unsafe fn(*mut Request) -> Action, mirroringWorkPoolTask::callback.Actionloses its lifetime:pollis*mut Poll, projected from the owner like the request, andregister_for_epoll/apply_kqueuetake that pointer and register its address as given, so the pointer that comes back from the kernel reaches the owner too. The ticks usepollbefore callingon_error/on_done(the hand-on) and never after; the existing ordering of those calls is unchanged.*mut Request, recover the owner withfrom_io_request, and projectio_poll. Their redundantscheduled = falsewent away (take_requestclears it, as the ticks already did).do_read_loop_task,do_write_loop_taskandon_close_io_requestalready hold the object's pointer and now pass it down instead of forming&mut.update,do_read_loop/do_write_loop,on_finish,wait_for_readable/wait_for_writableandFileCloser::do_closetakethis: *mut Self; each does its reads through place expressions or call-scoped reborrows and ends with the hand-over. The loop bodies themselves are unchanged apart from their exits:read_until_blocked/write_until_blockedare the old loops returning what to do next (Finish/WaitFor*), andprepare_read/prepare_writeare the old first steps doing the same, with oneproceedper type performing the step through the pointer.do_writereturnsWriteStep::{Wrote, WouldBlock, Failed}instead of waiting from inside (this is what removes theerrnoread above); its!could_blockEAGAINbranch is kept exactly as it was, since Bun.write: poll instead of spin when a nonblocking stdout pipe returns EAGAIN #35953 / Bun.write: deliver the whole payload to a FIFO instead of a torn prefix #36025 are changing that behaviour.FileCloser::io_requestreturns the projected pointer (NoneforReadFileUV, as before),schedule_closehas the callback's new signature, andFileCloser::update, which had no callers, is deleted.&mutthatJobContext::run(src/jsc/job.rs, 18 implementors) andFileOpener::get_fdpass in;run_async_with_fd/run_with_fdnow finish their own use ofselffirst and hand over through a pointer as their last act, and the two traits are a separate change. TheWorkPoolhand-overs in these files (on_ready,on_io_error,on_io_request_closed) are Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768's and are not touched here, so the two rebase mechanically onto each other. The kqueueClosearm's ordering relative to thekevent()call is pre-existing and reported separately.Tests
test/internal/source-lints/self-receiver-io-schedule.test.tsbans anIoRequestLoop::scheduleargument spelled fromself(directly, or via a local bound from aself.expression in the same function) or written as any&mutreborrow. On main it reports exactly the three sites above:and it passes with this branch (the header names the converted functions as the templates).
test/js/bun/util/bun-file-read.test.ts:Bun.file(fifo).bytes()on a FIFO opened by path. The test holds the write end but writes nothing until, in debug builds, the child's trace (theWriteFiledebug scope, which both files log under, sent to a file withBUN_DEBUG) showsReadFile.onRequestReadable, i.e. the ReadFile has been handed to the io thread and the io thread has run the converted trampoline while there is no data yet; then it feeds 256 KiB through a blocking fd and closes. Afterwards it checks the trace:waitForReadableat least once, exactly as manyonRequestReadableandonReadyas waits, noonIOError, oneonFinish() = deferred(the close handed to the io thread) and oneonFinish() = immediately(the round trip came back), i.e. every hand-over this PR converts on the read side, including the one that closes the fd it opened. In release builds the scope is compiled out and only the bytes, the hash and the exit are checked. Linux only: on the darwin lanes the child never finished this read (same symptom as the FIFO read case that test/js/web/streams/streams.test.js has todo'd on macOS; the kqueue code this PR touches is signature-only, and the read side is exercised on macOS by the Bun.stdin pipe tests), so it is skipped there and the macOS behaviour is reported separately.test/js/bun/io/bun-write.test.js:Bun.write(Bun.file(fd), 256 KiB)into a non-blocking FIFO the child has already filled toEAGAIN, so the WriteFile has to be handed to the io thread before its firstwrite(). The test does not drain the pipe until the child reports the fill and, in debug builds, until the trace shows the request on the io thread (onRequestWritable), which makes that first round trip certain rather than likely; it then checks that every request came back as anonReady, that there was noonIOError, and thatonFinishran twice (the close round-tripped through the io thread; once on macOS, which clears that flag when the one-shot registration fires). A second, Linux and debug only case issues two concurrent writes on the same fd and drains only once both requests have reached the io thread: the secondepoll_ctlregistration then deterministically fails withEEXIST, which is the registration-error hand-over (register_epoll->on_io_error), and the test checks that the failed write rejects with{code: "EEXIST", syscall: "epoll_ctl"}, that the other one's payload arrives intact, and in the trace that exactly one request ended inonIOErrorand that both WriteFiles still closed through the io thread (onFinishfour times). The EEXIST itself is pre-existing behaviour (one poller per fd); the comment says the case turns into two successful writes if that ever changes. Thedest.sizeaccess is what makes the destination known to be a pipe, which is what the write side polls on at all today; 256 KiB skipsBun.write's synchronous path (the full pipe would make it fall back to WriteFile anyway) and spans several pipe buffers. If the child dies after reporting the fill, the drain's blocking open of the read end is raced against the child's exit instead of waiting for a writer that will never come.Both FIFO scenarios also pass on main (the change is meant to be behaviour preserving); the lint is what distinguishes the two trees.
waitForFileToContainis added to test/harness.ts for the trace waits.Verification
Debug (ASAN) build: the two test files above,
test/regression/issue/07500(1 MBBun.stdin.text()from a pipe),bun-stdin-slice,bun-file-fd-read,bun-file,bun-serve-file, regression27849, andtest/internal/source-lints/(85 pass) all pass. Runningbun-write.test.jsas a whole file in this container intermittently times out a varying set of its tests, with or without this change (the synchronous 256 MB copy test in itsdescribe.concurrentstarves the others under ASAN; reported separately), andbun-write-leak.test.tstrips its 256 MB cap on a debug binary not namedbun-asan, which src/runtime/webcore/blob/write_file.rs already documents; neither involves this diff.cargo clippy -p bun_io -p bun_runtimeis clean on Linux and foraarch64-apple-darwin(the kqueue half of the tick lives only there);cargo checkof both crates also passes forx86_64-pc-windows-msvc, andbun_ioforx86_64-unknown-freebsd;cargo fmtis clean.