Repository navigation
Socket syscall fault injection for net/tls/http/http2/fetch/WebSocket fuzzing + 7 bug fixes - #32681
Merged
Merged
Conversation
Collaborator
|
Updated 4:39 PM PT - Jun 25th, 2026
❌ @cirospaciari, your commit 6e11400 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32681That installs a local version of the PR into your bun-32681 --bun |
cirospaciari
changed the title
cirospaciari
changed the title
cirospaciari
changed the title
cirospaciari
changed the title
cirospaciari
marked this pull request as ready for review
June 25, 2026 18:38
… fuzzing + 7 bug fixes
Adds a compile-time-gated fault-injection layer at the bsd_* syscall
wrappers in usockets, with runtime control via bun:internal-for-testing.
This is the single chokepoint for all node:net, node:tls, Bun.connect,
Bun.serve, fetch, node:http2 and WebSocket TCP traffic, so a small set
of hooks covers the whole networking stack including the TLS BIO.
Build flag: --socket-fault-injection (defaults to follow asan, so on for
local debug Linux/arm64-mac and CI release-asan, off elsewhere). When
compiled out the bsd_* hot path is byte-identical; when compiled in but
disarmed it costs one acquire atomic load. Rules are process-global with
per-fd targeting; counters/disarm are atomic; WSASetLastError on Windows.
Tests: deterministic recv/send/connect/accept errno injection, partial
read/write reassembly, TLS record-layer + handshake under 1-byte I/O,
seeded short-I/O fuzzers for net/tls/http/https/fetch/h2/WebSocket, a
curl-style every-i-th-recv torture loop, and split-process scenarios for
the bug classes below. All waits are on observable conditions (no
Bun.sleep / setImmediate polling).
Bugs found and fixed by the harness:
- NodeHTTPResponse.rs: heap-use-after-free in UpgradeCTX header
preservation when the upgrade request arrives across multiple recv()
calls — drop the post-upgrade set_on_aborted_handler() call (after uWS
adopts the socket the HttpParser::fallback buffer is freed).
- net.ts / socket_body.rs: surface any recv errno as 'error' on close
(Node-compat); previously only ECONNRESET. The native side now filters
the libus CloseCode enum (0/1/2) so it is never mistaken for an errno.
- websocket_client: ws.close(code, reason) never delivered the close
frame to the peer (server saw 1006). Three layered causes —
shutdown_read() before write let SHUT_RD-triggered EOF race into an
RST via SO_LINGER{1,0}; under partial writes handle_end/handle_close
fired before the deferred dispatch_close; C++ WebSocket::close()
cleared m_connectedWebSocketKind so the deferred didClose()
early-returned. Now deferred via close_dispatch_pending and finished
in handle_writable / shutdown_after_close_frame.
- uWS WebSocketProtocol.h parseClosePayload rejected close codes
1012-3999 as abnormal; per RFC 6455 §7.4 the reserved range is
1016-2999.
- h2_frame_parser: on_native_writable now loops flush() until real
backpressure or no progress, so corked frames re-arm WRITABLE instead
of stranding under partial sends.
- h2_frame_parser: uncork() is self-scoped — previously a client's
flush() could drain the server's thread-local CORKED_H2 slot to the
server fd from inside the client's writable callback.
- loop.c SEMI_SOCKET dispatch: test the WRITABLE bit, not exact
equality. A pre-connect partial write adds READABLE, and the old
`== WRITABLE` check fell through to the listen-socket arm and
busy-spun on accept4(connecting_fd)=EINVAL forever.
Also: socket_body.rs requires rule.errno when action is 'errno' and
lists all 13 accepted names; clear() throws when not compiled in;
test/no-validate-leaksan.txt adds s3-stream-cancel-leak.test.ts
(pre-existing exit-time leak, root cause documented).
cirospaciari
force-pushed
the
claude/socket-fault-injection
branch
from
June 25, 2026 18:39
1beab3a to
a3e02ba
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds process-wide socket fault injection from build configuration through native uSockets hooks and JS test surfaces, and updates socket close, writable, and HTTP/2 flush behavior. It also adds Bun, Node, and web regression tests under injected short I/O and errno faults. ChangesSocket fault injection and transport behavior
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I didn't find any bugs, but this is a large PR with seven non-trivial behavioral fixes across the core networking stack (loop.c semi-socket dispatch, WebSocket close-frame state machine, h2 uncork semantics, errno filtering at the native/JS boundary) — worth a human pass on the production-path changes even though the fault-injection layer itself is gated to asan builds.
Extended reasoning...
Overview
This PR adds a build-gated socket syscall fault-injection layer (compiled only in asan builds, zero-cost in release) plus 9 new test suites, and uses it to surface and fix seven bugs across the networking stack. It spans 29 files: usockets C (bsd.c, loop.c, new fault_inject.{c,h}), uWS C++ (WebSocketProtocol.h), the build system (config.ts/flags.ts/rust.ts/Cargo.toml), Rust runtime (websocket_client.rs, h2_frame_parser.rs, NodeHTTPResponse.rs, socket_body.rs, uws_sys/lib.rs), C++ bindings (WebSocket.cpp), and JS (net.ts, internal-for-testing.ts).
Security risks
None apparent. The fault-injection control surface is exposed only via bun:internal-for-testing (already restricted to debug/CI), and the hooks compile out entirely in shipped release builds via #define US_FAULT_CHECK(...) 0. The bug fixes themselves close a heap-use-after-free and tighten error propagation rather than relaxing any checks.
Level of scrutiny
High. While the injection harness is test-only infrastructure, the seven bug fixes change always-on production code paths:
loop.cSEMI_SOCKET dispatch switches from exact equality to a bit test — this is the core event-loop poll dispatch.websocket_client.rsadds a newclose_dispatch_pendingstate that threads throughhandle_close/handle_end/handle_writable/clear_data, deferring teardown and shutdown ordering.h2_frame_parser.rschangesuncork()from draining whatever owns the thread-local cork slot to self-scoped only, andon_native_writablenow loopsflush()until backpressure.socket_body.rsfilterserr > 2to distinguish libus CloseCode enum values from real recv errnos — a hard-coded assumption about the enum's value space.net.tsnow surfaces any recv errno as'error'instead of whitelisting ECONNRESET.NodeHTTPResponse.rsremoves aset_on_aborted_handler()call post-upgrade.WebSocketProtocol.hwidens the accepted close-code range (1012–1014, 3000–3999).
Each fix is well-reasoned and individually small, but together they touch the hot path of every TCP/TLS/HTTP/HTTP2/WebSocket connection.
Other factors
The PR description is unusually thorough and the new tests give strong coverage of the fixed behaviors. CI has one failure (terminal.test.ts on macOS x64) that appears unrelated to this change. No prior human or bot reviews exist on the timeline. Given the breadth of production-critical code touched, this warrants human review rather than auto-approval.
Contributor
There was a problem hiding this comment.
I didn't find any bugs, but this PR makes substantive behavior changes across the core networking stack (usockets loop dispatch, WebSocket close state machine, h2 cork/uncork ownership, node:net errno surfacing) alongside the new fault-injection infrastructure, so it warrants a human review.
Extended reasoning...
Overview
This PR spans 29 files and has two intertwined parts: (1) a new socket-syscall fault-injection layer (new C in bsd.c/fault_inject.c, Rust FFI in uws_sys/socket_body.rs, build-config plumbing, and a JS control surface in bun:internal-for-testing) plus nine new test files, and (2) seven bug fixes the injector surfaced — touching loop.c SEMI_SOCKET dispatch, WebSocketProtocol.h close-code validation, websocket_client.rs close-frame sequencing + a new close_dispatch_pending state, WebSocket.cpp m_connectedWebSocketKind lifetime, h2_frame_parser.rs uncork()/on_native_writable semantics, NodeHTTPResponse.rs removing set_on_aborted_handler() post-upgrade, and socket_body.rs/net.ts changing how on_close errnos surface to JS.
Security risks
The fault-injection surface is gated behind bun:internal-for-testing (already restricted to debug/CI) and compiled out of release builds by default, so there's no new attack surface in shipped binaries. None of the bug fixes touch auth/crypto/permissions. No security concerns identified.
Level of scrutiny
High. While the fault-injection harness itself is test-only and compiled out of release, the seven bug fixes are production-path changes to the core event loop, WebSocket close handshake, HTTP/2 flush loop, and node:net error semantics. Several are subtle: the err > 2 heuristic in NewSocket::on_close to distinguish libus CloseCode enum values from real errnos, the deferred-close state machine in websocket_client.rs (new ref-counting/teardown ordering in handle_close), the self-scoping of uncork() plus the new flush-until-no-progress loop in on_native_writable, and the removal of set_on_aborted_handler() after WS upgrade. Each is well-explained and tested under the new harness, but they alter long-standing behavior in code paths that affect every TCP/TLS/HTTP/WS connection.
Other factors
The PR is exceptionally well-documented and ships extensive targeted tests, but its size and the number of independent behavior changes in production-critical networking code put it well outside the threshold for bot approval. There are no prior human reviews on the thread yet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@packages/bun-usockets/src/fault_inject.c`:
- Around line 37-42: us_fault_set currently publishes us_fault_state[sc].rule
with a plain struct write while us_fault_hit and other readers access the same
shared rule and counters concurrently, which can lead to torn/stale state. Fix
us_fault_set and the corresponding access path in us_fault_hit by putting rule
publication and the calls_seen/fired updates behind one synchronization scheme
that every accessor uses, such as a mutex around the shared state after the
us_fault_armed fast-path or an atomic snapshot/replace approach. Make sure the
same scheme is applied wherever us_fault_state[*].rule is read or written so the
shared fault state is updated and observed consistently.
In `@src/http_jsc/websocket_client.rs`:
- Around line 1448-1454: The pending-close path in websocket_client’s close
handling leaves proxied closes unfinished because the writable callback only
drains send_buffer and never completes the deferred dispatch. Update the
writable/drain flow in src/http_jsc/websocket_client.rs so that after
proxy_tunnel finishes flushing buffered close bytes, it calls
finish_pending_close() when send_buffer is empty and close_dispatch_pending is
set. Use the existing close_dispatch_pending and finish_pending_close symbols to
locate the teardown path and ensure the close event is dispatched and pending
state cleared for proxied WebSocket closes.
In `@src/js/internal-for-testing.ts`:
- Around line 311-316: The JSDoc for the TestingAPIs `available()` method is
inaccurate because it says socket fault injection is enabled by default for
debug builds, but `resolveConfig()` actually defaults it from
`partial.socketFaultInjection ?? asan`. Update the comment on `available()` in
`internal-for-testing` to describe the real default behavior and keep it aligned
with the `resolveConfig()` logic, so the documentation matches the runtime
behavior on non-ASan debug builds.
In `@src/runtime/socket/socket_body.rs`:
- Around line 4838-4888: `js_set_socket_fault()` currently accepts action
"short" for all syscalls and always succeeds, but only recv/send can actually
clamp bytes. Add a validation guard in `js_set_socket_fault` (near the `action`
parsing and `fi::UsFaultRule` construction) to reject `short` for unsupported
syscalls like `writev`, `sendmsg`, `recvmsg`, `accept`, and `connect`, returning
an error instead of arming a no-op fault. Use the existing syscall/action
symbols in this function to detect the target syscall before calling
`fi::us_fault_set`.
In `@test/js/bun/http/serve-syscall-fault.test.ts`:
- Around line 38-47: The subprocess assertions in the serve-syscall-fault tests
only verify the HTTP response and then await shutdown, so they can miss a
non-zero child exit after teardown. Update each affected test block around the
relevant fetch/assertion flow and proc handling to explicitly check the child
process result after waiting for proc.exited, including exitCode (and any
relevant stderr/output) in addition to signalCode, using the existing proc
variable and the test cases in serve-syscall-fault.test.ts.
- Around line 113-145: The test around spawnServer and the client abort loop
does not actually verify that every response reaches a terminal state because
the finished counter is never updated or asserted and the server is always
force-stopped in finally. Update the Bun.serve fixture’s fetch/readable stream
handling so it increments finished when each response definitively ends or is
closed, then assert the expected count in the parent test before calling
proc.kill; use the existing finished variable and the Promise.all client loop to
confirm all responses terminate, not just that the first read succeeds.
In `@test/js/first_party/ws/ws-syscall-fault.test.ts`:
- Around line 56-67: The websocket fault test reports success too early because
the `message` handler logs before the separate `pong` event has necessarily
fired, which can make `pong` appear false. Update the test fixture around the
`c.on("message")` and `c.on("pong")` handlers so it only emits the success JSON
after both events have been observed, using the existing `pong` flag (and any
additional local state if needed) to wait for the actual condition being
verified.
In `@test/js/node/http2/node-http2-syscall-fault.test.ts`:
- Around line 188-199: The fuzz loop in node-http2-syscall-fault.test.ts only
exercises short recv behavior because the syscall variable in the randomized
test is hard-coded in the test body; update the randomized short recv/send test
to vary the syscall between recv and send so both paths are covered. Use the
existing randomized loop around makePrng, fault.set, and http2.connect to choose
the syscall dynamically while keeping the body integrity assertion unchanged.
In `@test/js/web/fetch/fetch-syscall-fault.test.ts`:
- Around line 147-150: The send-side scenarios in runClientFetch do not exercise
the response path because it always performs a body-less GET, so syscalls
configured with "send" only affect outbound request bytes. Update the affected
fetch-syscall-fault tests to use "recv" when asserting response
streaming/cancellation behavior, or add a request body if the intent is to
verify the send path; use the runClientFetch helper and the abort/fuzz cases to
ensure the assertions actually cover the intended faulted I/O path.
🪄 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: 51b0d3d3-a98a-4050-ad89-62f54b00ecdc
📒 Files selected for processing (29)
Cargo.tomlpackages/bun-usockets/src/bsd.cpackages/bun-usockets/src/fault_inject.cpackages/bun-usockets/src/internal/fault_inject.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/WebSocketProtocol.hscripts/build.tsscripts/build/config.tsscripts/build/flags.tsscripts/build/rust.tssrc/http_jsc/websocket_client.rssrc/js/internal-for-testing.tssrc/js/node/net.tssrc/jsc/bindings/webcore/WebSocket.cppsrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/socket/mod.rssrc/runtime/socket/socket_body.rssrc/uws_sys/lib.rstest/js/bun/http/serve-syscall-fault.test.tstest/js/bun/util/socket-fault-injection.test.tstest/js/first_party/ws/ws-syscall-fault.test.tstest/js/node/http/node-http-syscall-fault.test.tstest/js/node/http2/node-http2-syscall-fault.test.tstest/js/node/net/net-syscall-fault.test.tstest/js/node/tls/tls-syscall-fault.test.tstest/js/web/fetch/fetch-syscall-fault.test.tstest/js/web/websocket/websocket-syscall-fault.test.tstest/no-validate-leaksan.txt
- fault_inject.c: guard all us_fault_state access with Bun__lock so a re-arm on the JS thread cannot tear the rule a faulting I/O thread is reading; the per-slot counters become plain ints under the lock - websocket_client.rs: handle_tunnel_writable now finishes a pending close once the send buffer drains, matching handle_writable. A close() issued while frames are queued under tunnel backpressure deferred its dispatch and then never fired the close event - socket_body.rs: reject rule.action "short" for syscalls that cannot clamp a byte count (only recv/send can), instead of arming a no-op - internal-for-testing.ts: available() doc said the default was debug builds; it is ASan builds - tests: assert subprocess exit codes after teardown, prove every response reaches a terminal state via graceful stop(), report the ws fixture only after both message and pong, randomize recv/send in the http2 fuzz loop, cover the recv-side cancel path in fetch, and add regression tests for the three fixes above
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/bun-usockets/src/fault_inject.c`:
- Around line 71-73: us_fault_hit currently indexes us_fault_state with sc
before validating it, so add the same bounds guard used by us_fault_set and
us_fault_clear before touching the slot. Check sc against the valid fault slot
range at the start of us_fault_hit, and return early or no-op on invalid values
so the process-global table is never indexed out of bounds.
In `@src/http_jsc/websocket_client.rs`:
- Around line 2091-2096: The SAFETY justification is currently shared above the
conditional, but each unsafe use in websocket_client’s close/send path needs its
own directly adjacent comment. Move or duplicate the SAFETY comments so they sit
immediately above each unsafe expression in the finish_pending_close and
send_buffer_out calls within the relevant branch, keeping the explanation tied
to each distinct unsafe block.
🪄 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: fac49afd-5a8b-4dd4-bb7a-839e008b53ae
📒 Files selected for processing (11)
packages/bun-usockets/src/fault_inject.cpackages/bun-usockets/src/internal/fault_inject.hsrc/http_jsc/websocket_client.rssrc/js/internal-for-testing.tssrc/runtime/socket/socket_body.rstest/js/bun/http/serve-syscall-fault.test.tstest/js/bun/util/socket-fault-injection.test.tstest/js/first_party/ws/ws-syscall-fault.test.tstest/js/node/http2/node-http2-syscall-fault.test.tstest/js/web/fetch/fetch-syscall-fault.test.tstest/js/web/websocket/websocket-syscall-fault.test.ts
The direct-socket and proxy-tunnel writable handlers had the same drain-then-finish-pending-close sequence duplicated; the tunnel copy is the one that had previously drifted and dropped the close dispatch. Folding it into one helper also leaves a single unsafe block in handle_tunnel_writable instead of three.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http_jsc/websocket_client.rs (1)
1387-1390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t skip peer close validation while a local close is pending.
This guard also runs for received Close frames because that path sets
close_receivedand callssend_close_with_body(...). Returning here bypasses the UTF-8 validation below and preserves the old pending local code/reason, so a peer close with invalid reason bytes or a different status can be reported as a normal local close.Suggested direction
- if self.close_dispatch_pending.is_some() { - // A close is already mid-flush (user-initiated ws.close() under - // backpressure); don't enqueue a second close frame on top of it. - return; - } if !self.has_tcp() { self.dispatch_abrupt_close(ErrorCode::Ended); self.clear_data(); return; }After the reason is decoded/validated and
dispatch_codeis known, skip only the duplicate write; if this was a peer Close (close_received), replace the pending dispatch tuple and deref the old reason.🤖 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/http_jsc/websocket_client.rs` around lines 1387 - 1390, Do not return early from the close-handling path in websocket_client::send_close_with_body just because close_dispatch_pending is already set, since that also blocks received Close frames from validating the peer reason and status. Instead, keep the UTF-8 decode/validation and dispatch_code selection in place, and only skip the duplicate frame write when a pending local close exists. In the close_received path, replace the existing pending dispatch tuple with the newly validated peer close and release the old reason so peer close semantics are preserved.
🤖 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/http_jsc/websocket_client.rs`:
- Around line 1387-1390: Do not return early from the close-handling path in
websocket_client::send_close_with_body just because close_dispatch_pending is
already set, since that also blocks received Close frames from validating the
peer reason and status. Instead, keep the UTF-8 decode/validation and
dispatch_code selection in place, and only skip the duplicate frame write when a
pending local close exists. In the close_received path, replace the existing
pending dispatch tuple with the newly validated peer close and release the old
reason so peer close semantics are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7e25cec-fc67-475f-af31-3fb3e1e54656
📒 Files selected for processing (1)
src/http_jsc/websocket_client.rs
Member
Author
|
@coderabbitai On the outside-diff note about |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
(・×・) ✏️ Learnings added
|
bsd_do_connect_raw returns errno when a fault fires, but the "zero"
action never sets errno, so arming {syscall:"connect", action:"zero"}
returned whatever stale value the thread had. Zero only means something
for the data syscalls (EOF on the read side, backpressure on the write
side), so the setter now rejects it elsewhere, like it already does for
"short". Also corrects two comments that called the disarmed fast-path
load "relaxed"; it is an acquire load.
- epoll forwarded the raw EPOLLERR bit (8) as a libus close code. The JS socket close path treats codes above 2 as real read errnos, so on Linux a paused or half-open socket whose peer sent RST surfaced a bogus "read ENOEXEC" error instead of closing cleanly. kqueue already normalizes EV_ERROR to 0/1; epoll now matches it - setSocketFault: require bytes > 0 for action "short" (a 0-byte clamp silently behaved like action "zero"), and stop accepting the socket, close and shutdown syscalls, which have enum slots but no bsd.c hooks and therefore could never fire
The epoll normalization in a2cd8c1 fixed EPOLLERR being misread as errno 8, but it also removed the only signal node:net had that such a socket died: the close code became a libus enum value, the JS layer filtered it out, and a socket with unflushable buffered writes was treated as a graceful close that never finishes. test-net-error-twice timed out on every Linux target because of this. Resolve the close code from getsockopt(SO_ERROR), with an ECONNRESET fallback, in the poll-error branch — the same way the connect-error path already does — so JS sees the real errno on both backends.
The JS close handler treats close codes 0..2 as libus CloseCode enum values and anything above as a real errno. The poll-error branch now clamps an SO_ERROR of 0..2 to ECONNRESET so it can never collide with that enum, and both comments name all three producers of the code. SO_ERROR cannot actually be EPERM or ENOENT for an established TCP socket, so this is defensive only.
robobun
added a commit
that referenced
this pull request
Jun 27, 2026
Resolve the conflict in src/http_jsc/websocket_client.rs. Main's #32681 refactored the close path (deferred close via close_dispatch_pending, plus shutdown_after_close_frame / finish_pending_close / drain_send_buffer_and_finish_close), while this branch threads the unsent backlog through dispatch_close so bufferedAmount does not reset to 0 on close. Kept main's deferred-close structure and snapshot the backlog before clear_data() at each dispatch_close call site: the handle_close pending block, the close() immediate-dispatch branch, and finish_pending_close. Adapt two tests to the new deferred-close semantics: close() may flush a few frames to the OS buffer before snapshotting (relax the retention assertion to within tolerance), and a server-initiated close with an undrainable backlog now defers until the transport dies (drop the connection after the Close frame).
This was referenced Jun 27, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 1, 2026
…#31977) Fixes #31976 ### Problem `bun_jsc::rare_data::ErasedBox` declared both fields `pub`: ```rust pub struct ErasedBox { pub ptr: NonNull<c_void>, pub dtor: unsafe fn(*mut c_void), } impl Drop for ErasedBox { fn drop(&mut self) { // SAFETY: `dtor` was supplied by the same high-tier code that allocated `ptr`. unsafe { (self.dtor)(self.ptr.as_ptr()) }; } } ``` A struct literal needs no `unsafe`, and naming an `unsafe fn` as a value needs no `unsafe` either, so fully safe code in any crate could forge an arbitrary pointer/destructor pair and the safe `Drop` would invoke it: undefined behavior with zero `unsafe` blocks on the caller's side. The SAFETY comment asserted an invariant no constructor enforced. No shipped behavior was affected: `ErasedBox` was never constructed anywhere in the tree. The `RareData.websocket_deflate` slot holding it was only ever written as `None`, and `websocket_deflate_slot()` had no callers. The lazy init it was reserved for never happened because the concrete payload type lives in `bun_http_jsc` (a dep cycle for `bun_jsc`), so `PerMessageDeflate` stores its libdeflate handles per connection instead. ### Fix Delete the dead code rather than keep an unsound reserved slot: - `ErasedBox`, the `websocket_deflate` field, and `websocket_deflate_slot()` in `src/jsc/rare_data.rs` - the discarded `rare_data: &mut JscRareData` parameter of `PerMessageDeflate::init` (its body was `let _ = rare_data;`) and the comments referencing the deleted slot in `src/http_jsc/websocket_client/WebSocketDeflate.rs`, plus the two call sites in `src/http_jsc/websocket_client.rs` (the `vm` binding in `init_with_tunnel` existed only for this argument) ### Test The unsoundness has no runtime-observable behavior (the type was dead code), so the guard is a source-scan test, `test/internal/unsound-erased-box.test.ts`, following the existing `ban-words` / `dead-code-escapes` pattern there. It scans tracked Rust sources and asserts: 1. `ErasedBox` stays deleted 2. no `pub dtor: unsafe fn` field exists anywhere (the forgeable-destructor shape, independent of the struct name) Both assertions fail on the unfixed tree (offender: `src/jsc/rare_data.rs`) and pass with this change. Verified `cargo check -p bun_jsc -p bun_http_jsc`, a full debug build, and the websocket permessage-deflate suites (`websocket-permessage-deflate*.test.ts`, 13 pass / 1 skip) since `PerMessageDeflate::init`'s signature changed. ### Rebase note Rebased onto current main and resolved a conflict in `websocket_client.rs`, where #32681 consolidated the two WebSocket `init` paths into a single `new_raw` helper (fields now wrapped in `Cell`/`RefCell`) that still carried the dead `vm.rare_data()` argument to `WebSocketDeflate::init`. Re-applied the argument removal on top of `new_raw`, dropping the now-unused `vm` binding and its stale borrow comment. `rare_data.rs` and `WebSocketDeflate.rs` applied without conflict. Re-verified: `cargo check -p bun_jsc -p bun_http_jsc`, full debug build, the source-scan test (fails with src stashed, passes with the fix), and `websocket-permessage-deflate*.test.ts` (15 pass / 1 skip).
cirospaciari
pushed a commit
that referenced
this pull request
Sep 10, 2026
… connecting (#40048) ### Problem - `http2.connect("http://127.0.0.1:<closed port>")` emits `connect ECONNRESET` (`code: "ECONNRESET"`) on bun 1.4.x. Node v26.3.0 and bun 1.3.14 emit `connect ECONNREFUSED`. With no `'error'` listener on the session, bun 1.4.x swallows the failure entirely, where Node raises it. - The cause is in `ClientHttp2Session`'s constructor (`src/js/node/http2.ts`). It handed `socket._handle` to `new H2FrameParser({ native })` while the socket's `connect()` was still in flight. The parser serializes the connection preface at construction and its auto flush wrote it to that handle, so the kernel got a `send()` on a connecting socket. Linux returns the pending connect error from that `send()` and clears `SO_ERROR`, so the connect completion read 0 and usockets reported ECONNRESET. ### Fix - The client constructor no longer passes `native` to the parser. `#onConnect` is now the one place that attaches the native socket (`setNativeSocket()` then `flush()`), and every construction path reaches it: the `'connect'`/`'secureConnect'` listener for a connecting socket, a next-tick call for a `createConnection` socket that is already connected. Until then the preface, and any frame written before connect, stays queued in the parser and goes out first, in order. This is the path a JS transport (`createConnection` returning a Duplex) already took. - Correct because nothing is written to a socket before Node would write to it: after `'connect'` for h2c, after `'secureConnect'` for h2. - Verified: new test in `test/js/node/http2/node-http2.test.js` runs `http2-connect-refused.fixture.js` under Bun and under Node.js and requires the same `{code, syscall, address, port, message}`. It fails on stock bun (`ECONNRESET`) and passes with this change; the fixture passes on Node v26.3.0. `node-http2.test.js` (380 pass), the other `test/js/node/http2/` files, and 107 vendored `test-http2-{client,connect,session,settings,goaway,ping,window,tls,...}` tests pass. ### Background - `H2FrameParser` (`src/runtime/api/bun/h2_frame_parser.rs`) is the native HTTP/2 framer behind `node:http2`. With a native socket attached it writes to the socket directly. Without one it hands bytes to the session's `write` handler, which returns -1 until the session is connected, and the parser keeps the bytes queued. - usockets tracks a client socket whose `connect()` returned EINPROGRESS as a half-open socket and reads the real connect errno with `getsockopt(SO_ERROR)` when the poll reports an error. A `send()` on that socket consumes the pending error first, so the completion is blind. <details><summary>Notes</summary> Syscall sequence on stock 1.4.x for a refused h2c connect (LD_PRELOAD trace of `connect`/`send`/`getsockopt`): ``` connect(fd=7) = -1 EINPROGRESS send(fd=7, PREFACE len=33) = -1 ECONNREFUSED send(fd=7, PREFACE len=33) = -1 EPIPE getsockopt(fd=7, SO_ERROR) -> 0 => connect ECONNRESET ``` With this change: ``` connect(fd=8) = -1 EINPROGRESS getsockopt(fd=8, SO_ERROR) -> 111 (Connection refused) => connect ECONNREFUSED ``` A successful h2c connect sends the preface once, after the connect completes, and a request round-trips. Why 1.3.14 reported ECONNREFUSED: the half-open dispatch in `loop.c` passed a boolean to `us_internal_socket_after_open`, and the error path labeled every connect failure ECONNREFUSED without reading `SO_ERROR`. #32681 made the loop report the kernel's real errno, which exposed the consumed error. The unobserved case: `ClientHttp2Session#onError` tears the session down quietly for an `ECONNRESET` that nobody listens for (an idle pooled connection dropped by the peer). The mislabeled refusal took that branch, so `http2.connect()` to a dead port with no `'error'` listener did nothing on 1.4.x. With the right code it raises, as Node does. An earlier revision of this PR made the usockets write entry points refuse to `send()` on a half-open socket. Per review, the write should not be issued in the first place, so that change was dropped and the fix lives at the call site. `test/js/third_party/grpc-js/test-client.test.ts` shows the same three debug-build timing failures (100 ms `waitForReady` deadline) with and without this change. </details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a socket syscall fault-injection layer so we can deterministically fuzz
node:net,node:tls, andnode:httpagainst transport-level conditions that are otherwise hard to reproduce in a unit test: partial reads/writes, mid-streamECONNRESET/EPIPE,EAGAINstorms, connect failures, and backpressure →on_drainracing a concurrent destroy.Architecture
bsd.cis the only file in the repo that calls the raw socket syscalls; uWS and the BoringSSL BIO both bottom out inus_socket_write*/ the loop'sbsd_recv. So all hooks live inbsd.c— no edits tosocket.c,loop.c,openssl.c, or uWS.Interceptor (
packages/bun-usockets/src/{fault_inject.c, internal/fault_inject.h}): a process-global rule table indexed byenum us_fault_syscall(recv/send/writev/sendmsg/recvmsg/connect/accept). Each rule says{action: errno|short|zero, errno_value, clamp_bytes, after_n_calls, repeat, target_fd}. The hot-path check isUS_FAULT_CHECK(sc, fd, out, clamp)— an acquire atomic load of one process-wideus_fault_armedint, then a rule-table lookup if armed. Rules are process-global (not thread-local) so a rule armed from JS also affects the HTTP-client thread that runsfetch(); per-socket isolation is provided bytarget_fd.Build gate (
scripts/build/{config,flags,rust}.ts,scripts/build.ts): newConfig.socketFaultInjectionfield, exposed as--socket-fault-injection=on|off. Drives both-DLIBUS_SOCKET_FAULT_INJECTION=1(C) and--cfg=socket_fault_injection(Rust) so the C symbol and the Rustexternare always either both present or both absent.Default =
asan. The flag is not tied todebugorci— it's compiled in exactly whenasanis, and can be overridden independently:asandefaultsocketFaultInjectionbun bdon Linuxbun bdon arm64 macOSbun bdon x64 macOS / Windowsrelease-asanrelease(shipped binary)--asan=on--socket-fault-injection=on--socket-fault-injection=offSo the shipped release binary never has the hooks; CI's asan lane and local debug on Linux/arm64-mac do; everything else doesn't unless explicitly enabled.
Runtime control (
src/uws_sys/lib.rs,src/runtime/socket/socket_body.rs,src/js/internal-for-testing.ts):Cost
bsd_recvhot-path overheadUS_FAULT_CHECKis#define ... 0, dead-code eliminated; object code identical to todayBug fixes
The injector surfaced seven bugs, all fixed in this PR:
src/runtime/server/NodeHTTPResponse.rs:467— heap-use-after-free inUpgradeCTX::preserve_web_socket_headers_if_neededwhen a WebSocket upgrade request spans multiplerecv()calls. uWS buffers partial bytes inHttpParser::fallback; afterraw_response.upgrade()adopts the socket that buffer is freed, but the post-upgradeset_on_aborted_handler()re-readsec-websocket-*from it. The call is provably dead at that point, so it is removed.src/js/node/net.tsSocketHandlers2.close— recv errno other thanECONNRESET(e.g.ETIMEDOUT,EHOSTUNREACH) was dropped to a clean'end'instead of surfacing as'error'. Now matches Node'sonStreamReadexactly (destroy(errnoException(nread, "read"))for any non-EOF read error). The earlier whitelist was a workaround forNewSocket::on_closetreating libusCloseCodeenum values (0/1/2) as errnos —_handle.close()→FastShutdown=2→ JS saw ENOENT. Fixed at the native boundary: onlyerr > 2is a real recv errno.src/http_jsc/websocket_client.rs+src/jsc/bindings/webcore/WebSocket.cpp—ws.close(code, reason)never delivered the close frame to the peer (server always saw 1006; reproduces on main with no fault).send_close_with_body()calledshutdown_read()before writing the close frame; on Linux SHUT_RD makes the socket immediately readable, sohandle_end → terminate → fail → cancel → tcp.close(CloseKind::Failure)RST-dropped the queued frame. Under partial writes the deferreddispatch_closewas additionally blocked by C++WebSocket::close()clearingm_connectedWebSocketKindearly. Fixed by deferring SHUT_RD/SHUT_WR until the frame is in the kernel buffer, finishing the pending close inhandle_end/handle_close, and leavingm_connectedWebSocketKindfordidClose()to clear.src/runtime/api/bun/h2_frame_parser.rson_native_writable—flush()ends inflush_stream_queue() → write() → cork(), leaving newly-serialized frames in the thread-local CORK_BUFFER instead of on the wire. Returning let loop.c seelast_write_failed==0and disarm WRITABLE, stranding the corked bytes — under partial sends the session hung. Now loopsflush()until backpressure or no progress.src/runtime/api/bun/h2_frame_parser.rsuncork()— operated on whatever parser owned the thread-localCORKED_H2slot, so with both client and server sessions in one process a client'sflush()would drain the server's corked bytes to the server's fd from inside the client's writable callback (and tear down the server's auto-flush). Now self-scoped and returns the byte count so the writable-loop exit condition is accurate.packages/bun-usockets/src/loop.c:386SEMI_SOCKET dispatch — a pre-connect partial write callsus_poll_change(R|W)on a socket whosepoll_typeis stillPOLL_TYPE_SEMI_SOCKET; the dispatch then failed the== LIBUS_SOCKET_WRITABLEequality, fell through to the listen-socket arm, and busy-spun onaccept4(connecting_fd)=EINVALforever — never promoting toPOLL_TYPE_SOCKET. Connecting semi-sockets poll WRITABLE (and may add READABLE); listen semi-sockets only poll READABLE; so testing the WRITABLE bit instead of exact equality fixes it. Exposed by http2 send-short at <128 bytes (preface corked + auto-flushed before connect completes).packages/bun-uws/src/WebSocketProtocol.hparseClosePayload— rejected close codes 1012-3999 as abnormal (returned 1006). Per RFC 6455 §7.4, 3000-3999 are IANA-registered for libraries/frameworks and 1012-1015 are now defined; only 1016-2999 are reserved. Updated the validation;ws.close(3001, ...)now round-trips.Tests (78 pass, 0 todo, 239 assertions across 9 files)
test/js/bun/util/socket-fault-injection.test.ts— control-surface validation, errno-required, all-13-names (10)test/js/node/net/net-syscall-fault.test.ts— recv ECONNRESET/ETIMEDOUT →'error', recv 0 →'end', byte-exact short recv/send, send EAGAIN buffer+flush, connect ECONNREFUSED, accept failure recovery, fd-targeting (both halves), multi-rule, 128 KB short-send,after:skip-count, torture loop, seeded fuzz (14)test/js/node/tls/tls-syscall-fault.test.ts— recv reset (session + handshake), recv 0 →'end', 1-byte/5-byte BIO reads/writes, handshake under 3-byte send, close_notify under 1-byte send, FIN-before-close_notify, seeded fuzz (10)test/js/node/http/node-http-syscall-fault.test.ts— TLS proxy pipe + destroy under backpressure; Bun.serve abort; close-tracking; seeded fuzz (4)test/js/web/fetch/fetch-syscall-fault.test.ts— http+https short recv/send (Content-Length + chunked), recv reset/zero, connect refused, body cancel, TLS handshake, 5-byte boundary, fuzz (11)test/js/bun/http/serve-syscall-fault.test.ts— fixed/streaming body short send, request body short recv, https, concurrent abort (5)test/js/web/websocket/websocket-syscall-fault.test.ts— ws/wss client short recv/send, 70 KB binary frame split, recv ECONNRESET/zero → 1006, connect refused, TLS handshake; server short send + writev-zero on 20 KB frame + close-tracking + close(code,reason) round-trip (no-fault and 1-byte send) (14)test/js/node/http2/node-http2-syscall-fault.test.ts— 1-byte and 9-byte (frame header boundary) recv reassembly, recv ECONNRESET → session error, https/2 over TLS, seeded fuzz (7: send-short now works at 8 bytes after the loop.c SEMI_SOCKET dispatch fix)test/js/first_party/ws/ws-syscall-fault.test.ts— recv/send short echo + ping/pong, recv ECONNRESET split-process (3)All tests
skipIf(!fault.available() || isWindows)— they run on the asan jobs and skip cleanly elsewhere.Patterns applied
malloc()fault injector, curltests/libtest/lib1560node:net(recv short at every i in 1..10),node:tls/fetchseeded short-I/O fuzzBUN_SOCKET_FUZZ_SEEDso CI failures are reproducibletesting/quick, libuvtest-fs-readdirseednode:net,node:tls,node:http,fetchnet/httpslowReader, NettyEmbeddedChannelfragmented decoder testsnode:net,node:tls(incl. 5-byte TLS-record-header boundary),fetch(Content-Length and chunked),WebSocketclient (text, 70 KB binary 8-byte ext-len header, FIN=0+continuation),Bun.serverequest body,wsshimon_writableturnsChannelOutboundBufferpartial-write tests, curlCURL_SOCKOPTshort-send harnessnode:net,node:tls(incl. handshake flights and close_notify),node:httpproxy pipe,Bun.serve(fixed, streaming, TLS),WebSocketclient (data and close(code,reason) frames),Bun.serveWS serveruv__iotest hooks, Gointernal/poll*TestHookDidWritevaccept(), prove the listening socket is still usabletest-emfile, Gonet.TestAcceptErrornode:neton_writableevery turn, then destroy the consumer; assert every server-side response reaches a terminal state and the process exits 0Http2StreamChannelreset-under-backpressure testsBun.serve(HTTP and WS),node:httpTLS proxy pipetarget_fdso client and server can share a processnode:netfd-targeting test (now asserts both halves)writevreturns 0, prove the remainder is buffered and drainedIovArraypartial-gather testsBun.serveWS server ≥16 KB frame (new)lib1915SSL-shutdown-under-EAGAINnode:tls(new ssl_release_spill test)Surfaces covered
node:netSocketnode:tlsTLSSocketnode:httpservernode:http2Bun.serveHTTPfetch()clientWebSocketclientBun.serveWS serverws(thirdparty shim)—= not applicable or intentionally deferred.How did you verify your code works?