Bun.serve: release the FIFO file reader when the client disconnects

Closed
RO
17d ago
+422-12

Problem

  • Bun.serve answers with Bun.file(fifo). After a client disconnects, the server keeps the reader fd and its poll until the writer closes. 100 clients pin 100 fds, and dead readers take later bytes.
  • With Bun.stdin or Bun.file(fd) the fd stays open, so the poll fires on the freed stream: AddressSanitizer: heap-use-after-free in PosixBufferedReader::begin_read.
  • Cause: finish() (src/runtime/server/FileResponseStream.rs:570) keeps the ref for the read in flight. The reader keeps its FilePoll unless it owns the fd (src/io/PipeReader.rs:339).

Fix

  • finish() pauses the reader, which unregisters the poll, and drops the read ref.
  • PosixBufferedReader returns its FilePoll in deinit and Drop, whoever owns the fd.
  • Windows: the stream hands the fd to the reader, which closes it after a read in flight.
  • Verified: test/js/bun/http/bun-serve-file.test.ts. Five new tests fail on main, and a sixth covers a mid-body disconnect on every platform. Self-reviewed: 5 concerns raised, 5 addressed.

Background

  • FileResponseStream streams an fd into a response. For a pipe, its BufferedReader reads until EAGAIN, then waits on a FilePoll (an epoll or kqueue registration).
  • start() takes a ref for the read in flight. Only a reader callback released it.
  • Considered the poll release in the stream's Drop (#37083). The shell IOReader had that copy, so the reader does it.

Downsides

  • Still open: a pipe response that ends with no bytes has no framing. The client waits for the idle timeout. #37082 fixes that.
  • Cost: one epoll_ctl(EPOLL_CTL_DEL) when a pipe, socket or device response ends (8 streams, 8 calls). Regular files log the same syscalls as main.
Notes

All numbers are from debug (ASAN) builds on Linux x64, main at 36cd1514ec, unless a line says otherwise.

After the rebase onto main 468efacace (the one conflict was the node:fs import line of the test file):

  • The report's script: 100 of 100 fds stay open on main for the fetch handler, the static route and .stream(). This branch: 0.
  • printf hello | bun with new Response(Bun.stdin): main reports the use-after-free in 6 of 6 runs, now at src/io/PipeReader.rs:663. This branch: 0 of 6.
  • bun-serve-file.test.ts: main has 172 pass and 5 fail, and the 5 are the new tests. This branch has 178 pass and 0 fail, with the sixth test.
  • bun run rust:check-all: 12 of 12 targets.

The report's script: 100 clients request a FIFO body and disconnect, half with a reset and half in order. Main: signal aborted=100 pendingRequests=0 FIFO reader fds open=100. This branch: 0 open, for new Response(Bun.file(F)), routes: {"/f": Bun.file(F)} and new Response(Bun.file(F).stream()).

What each change does (8 aborted FIFO responses, then printf hello | bun with a server that answers new Response(Bun.stdin), 6 runs):

buildFilePoll slots usedpolls unregisteredwake-ups of dead readersfds closeduse-after-free
main802806 of 6
only the finish() change88080 of 6
only the reader change802800 of 6
this branch18080 of 6

Either change removes the use-after-free. Only finish() fixes the abort. Only both together return the FilePoll slot to the pool, so a later stream can use it again.

The use-after-free on main: the read is in PosixBufferedReader::begin_read (src/io/PipeReader.rs:655 at 36cd1514ec), from FilePoll::on_update. The free is the drop of FileResponseStream. A release build does not report it. It fails in other ways: ECONNRESET on the next request, a timeout, or RangeError: Out of memory.

A failed poll registration in start(): PosixBufferedReader::start reports that failure through on_reader_error and returns Ok. start() now returns when the stream is already finished. Without that, the paused reader never released the read ref that start() took next. Two responses over the same fd (the second registration fails with EEXIST): 2 streams started and 1 freed before e8826ff8b5, 2 and 2 after it. No fd leaks and the process exits in both cases. LeakSanitizer sees the leak: with the test runner's settings for the ASAN lane (detect_leaks=1, BUN_DESTRUCT_VM_ON_EXIT=1), a build without that guard reports Direct leak of 216 byte(s) allocated from FileResponseStream::start, and this branch is clean. A test for that lane is written but not in this PR, because I could not run it through the test runner.

Windows: a debug build has no ASAN there, so I used temporary log lines (not in this PR) where the read is issued, in on_file_read, and in the reader's Drop. 3 runs of 400 aborted responses over a 16 MiB file: 200 readers were dropped with a read in flight, all 200 callbacks arrived detached, and none of 2423 callbacks with a live reader pointed at a dropped reader. Full responses afterwards matched the file. bun-serve-file, serve.test.ts and bun-serve-static pass on Windows x64 (491 pass, 0 fail). The new tests need mkfifo and are POSIX-only. After the rebase I did not run Windows by hand again. Main's changes to the Windows reader since then only remove #[cfg(windows)] attributes (#44086), and the sixth test runs on the Windows lanes in CI. The sixth test passed its first Windows run, x64 and aarch64, in CI build 123104.

The case that stays open: with this branch, a response over a pipe that is at EOF and holds no bytes took 3988 to 3995 ms with idleTimeout 2 and 4, and did not end with idleTimeout: 0. The head has no Content-Length and no Transfer-Encoding. #37082 changes finish() to write the framing.

A pipe fd in blocking mode blocked the event loop when this PR was opened (bun script.mjs 0<>/tmp/fifo with new Response(Bun.stdin)). #43868 fixed that on main: the reader polls before a plain read. Checked on 468efacace: main and this branch both pass.

Not in this PR: a Windows named pipe whose start fails. A Source::Pipe reader closes its handle in close_impl, and the stream's Drop closes the fd when auto_close is set. So a start that fails after the reader adopted the pipe closes twice. Main does the same. After a successful start this PR leaves one close, because the stream hands the fd to the reader. Not verified on Windows.

Tests

  • The five new tests take 0.5 to 1.5 s each on the debug build. The fixtures use Bun.connect clients. In this debug build node:net cost 1.7 to 3.3 s per fixture (import and 8 clients) against 0.1 to 0.5 s.
  • #44036 adds a block at the same place in bun-serve-file.test.ts. Keep both blocks. No source line conflicts.
  • The sixth test serves test/js/web/encoding/utf8-encoding-fixture.bin (4.25 MiB) through a fetch handler and a static route. 40 clients leave mid-body. It then counts the open fds, compares a full download of each route with the file, and the fixture must exit without process.exit(). It has no pipe, so it also runs on Windows, where the reader closes the fd after a read in flight.
  • The sixth test passes on main's Linux build too, because this PR does not change the sendfile path there. Checks of the test: a fixture variant that leaks one fd per request reports 16 leaked fds for 16 requests, and the server sees an abort for 20 of 20 requests on the fetch route.
  • Response(Bun.file) does not double-close the fd on Windows (already in this file) aborts 48 responses in each of 160 rounds and checks a canary fd. It covers a double close on the Windows path. It has no check for an fd that stays open.
  • Limit of the sixth test: with the Windows hand-off removed it still passes, because the stream's Drop then closes the fd. It fails when the reader does not close the fd that it was handed. A rework (full downloads that run while clients leave, and two cases that only LeakSanitizer can fail) is written but not in this PR, because I could not run it.

Suites run on Linux: bun-serve-file, bun-serve-static, serve-file-slice-read-error, tls-bunfile-leak, body-stream, serve.test.ts, spawn.test.ts, bunshell, file-io, shell-blocking-pipe, shell-pipe-read-fault. bun run rust:check-all: 12 of 12 targets.

Failures here that main's build has too

  • The three backpressure tests from #41244 in bun-serve-file.test.ts exceed the 5 s limit in this container, on main's binary and on this branch's. The host load average was 350 to 590 on 16 cores. Their fixture alone took 3.8 to 6.6 s on main and 3.6 to 8.3 s on this branch (9 interleaved runs each), with the right output in all 18 runs.
  • shell-blocking-pipe.test.ts "send buffer size doesn't block the main thread" takes about 6.9 s on both binaries (5 of 5 runs each).
  • serve.test.ts: "root range port (#7187)" and "/bun:info loopback" (the container runs as root and has no IPv6).
  • After the rebase the host load average was 700 to 1100. Five tests of shell-pipe-read-fault.test.ts exceeded 5 s when six test files ran together. Alone, with a 60 s limit, main's binary and this branch's each pass 11 of 11 in 2 interleaved rounds (0.6 to 5.0 s per test on both).
  • A run of serve.test.ts and spawn.test.ts on this branch overlapped other test runs at that load and had 15 and 10 tests over their limits. I did not repeat it. CI build 122853 (546703b3b8, the same source) passes both files on every Linux, Windows and macOS arm64 lane.

Regular files: 8 complete responses over a 13 KB file use the reader, not sendfile. The debug log shows the same 9 operations on main and on this branch (9 close, no poll call).

Not measured: instruction and syscall counts from a tracer. This container has no perf, valgrind or strace. The unregister count above comes from the debug log. The release binary size is not measured. The debug binary is 3,472 bytes smaller than main's (810,618,496 against 810,621,968).

Review concerns and what changed

  1. The poll release was in the stream's Drop, a second copy of the code in IOReader. It moved into PosixBufferedReader.
  2. A Windows-only reader API repeated what File::detach does. It is gone. The stream sets CLOSE_HANDLE after start().
  3. No Windows test: the sixth test runs on every platform (155fff3fd2) and passes on Windows in CI. See its limit under Tests.
  4. and 5. Comments: shortened.

Alternative not taken: the reader could own the fd on every platform. That moves the close of every regular file response on POSIX from the stream to the reader. This PR keeps the POSIX owner as it is.

Prior art: #37083 fixed the same two leaks in August. It conflicted with main after #38886, #40478 and #41244, and it is closed in favour of this PR.


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/http/bun-serve-file.test.ts