Skip to content

udp: do not let remote ICMP kill an unconnected socket - #35923

Closed
robobun wants to merge 4 commits into
mainfrom
farm/ab2d7d82/udp-icmp-not-fatal
Closed

robobun wants to merge 4 commits into
mainfrom
farm/ab2d7d82/udp-icmp-not-fatal

Conversation

@robobun

@robobun robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator

Repro

A docs-shape Bun.udpSocket reply server, no error handler:

const server = await Bun.udpSocket({
  port: 0, hostname: "127.0.0.1",
  socket: { data(socket, data, port, address) { socket.send(data, port, address); } },
});
// client: send one datagram then close before the reply arrives
import dgram from "node:dgram";
const c = dgram.createSocket("udp4");
c.send("query", server.port, "127.0.0.1", () => c.close());

On main the process dies with ECONNREFUSED errno: -111 as soon as the reply hits the closed port and the kernel answers with ICMP port unreachable. Node survives. The same happens to a node:dgram echo server with no 'error' listener.

Separately, a send() that already throws EMSGSIZE synchronously is redelivered a second time via the error queue, so a caught oversize send still kills the process.

Cause

Bun enables IP_RECVERR on every UDP socket so Linux queues the ICMP instead of dropping it. Three delivery paths turn that into an uncaught:

  • loop.c drains MSG_ERRQUEUE and calls on_recv_error(.., is_errqueue=1); udp_socket.rs routes that to uncaught_exception when there is no error handler.
  • The kernel also mirrors the queued ICMP into sk_err, so the next recvmmsg in the same dispatch (right after on_data sent the reply) fails with the same errno. That reaches on_recv_error(.., is_errqueue=0), which node:dgram's errqueue filter cannot drop, so 'error' is emitted on a socket with no listener.
  • ip_local_error() queues the EMSGSIZE from the failing sendmsg with SO_EE_ORIGIN_LOCAL; the drain loop never read ee_origin, so it redelivered an errno the syscall had already returned.

Fix

  • packages/bun-usockets/src/loop.c: factor the errqueue drain into a helper that reads ee_origin, skips SO_EE_ORIGIN_LOCAL entries, and reads SO_ERROR afterwards to clear the mirrored sk_err. When recvmmsg fails with a non-EAGAIN errno on Linux, drain the error queue first; if it has the entry, surface that (flagged) instead of a second un-flagged copy.
  • src/runtime/socket/udp_socket.rs: drop is_errqueue=1 errors on unconnected sockets. Node never enables IP_RECVERR, so the kernel drops these before they reach sk_err; dropping them here gives Bun.udpSocket and node:dgram the same semantics. Connected sockets keep the error.

Verification

New tests (all Linux-only, IP_RECVERR is Linux-specific), each fail on main and pass here, 5/5 runs:

  • test/js/bun/udp/udp_socket.test.ts: reply server with no error handler survives a vanished client and answers the next one; oversized send() throws EMSGSIZE once and is not redelivered
  • test/js/bun/udp/dgram.test.ts: node:dgram echo server with no 'error' listener survives a vanished client

test/js/bun/udp/udp_socket_recv_flags.test.ts now uses a connected socket to exercise the ECONNREFUSED path (unconnected sockets intentionally no longer surface remote ICMP).

262/262 in test/js/bun/udp/, 75/75 test/js/node/test/parallel/test-dgram-*.js, 52/52 in fetch-http3-client.test.ts (shares the UDP poll path).

Related: #29135 took the alternative of toggling IP_RECVERR on connect/disconnect; this PR keeps IP_RECVERR on and filters at dispatch instead, so the sk_err mirror on a reply server's own recvmmsg loop is handled and SO_EE_ORIGIN_LOCAL is distinguished.


no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/udp/dgram.test.ts test/js/bun/udp/udp_socket.test.ts test/js/bun/udp/udp_socket_recv_flags.test.ts

A Bun.udpSocket reply server (the shape the docs show, no error handler)
or a node:dgram echo server with no 'error' listener dies with an
uncaught ECONNREFUSED the first time a client sends a datagram and
disappears before the reply arrives. The reply hits a closed port, the
kernel answers with ICMP port unreachable, and because Bun enables
IP_RECVERR on every UDP socket that error is queued and then routed to
uncaught_exception.

The same oversize send that already throws EMSGSIZE synchronously is
also redelivered asynchronously: ip_local_error queues the errno with
SO_EE_ORIGIN_LOCAL and the drain loop surfaces it a second time.

Fix:
- loop.c: factor the MSG_ERRQUEUE drain into a helper that also reads
  ee_origin, skips SO_EE_ORIGIN_LOCAL entries (already reported
  synchronously), and reads SO_ERROR afterwards to clear the mirrored
  sk_err so EPOLLERR stops level-triggering with an empty queue.
- loop.c: when recvmmsg fails with a non-EAGAIN errno on Linux, drain
  the error queue first. If it has the entry, the recvmmsg errno was its
  sk_err mirror; surface the queue (is_errqueue=1) instead of a second
  un-flagged copy. This is the path a reply server hits when on_data
  sends to a port that closed between the datagram and the reply.
- udp_socket.rs: drop is_errqueue=1 errors on unconnected sockets. Node
  never enables IP_RECVERR so Linux drops these in the kernel; doing it
  here means both Bun.udpSocket and node:dgram inherit Node's semantics.
  Connected sockets keep the error (Node surfaces that via recvmsg too).

Tests:
- reply server with no error handler survives a vanished client and
  answers the next one (Bun.udpSocket)
- same for a node:dgram echo server with no 'error' listener
- oversized send throws EMSGSIZE once and is not redelivered async
- the connected-socket ECONNREFUSED test now connect()s (the previous
  unconnected variant no longer surfaces remote ICMP, by design)
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d478eed8-c22f-4352-8822-a1fc3b04b026

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and 70b5e14.

📒 Files selected for processing (6)
  • packages/bun-usockets/src/loop.c
  • src/js/node/dgram.ts
  • src/runtime/socket/udp_socket.rs
  • test/js/bun/udp/dgram.test.ts
  • test/js/bun/udp/udp_socket.test.ts
  • test/js/bun/udp/udp_socket_recv_flags.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:35 AM PT - Jul 26th, 2026

❌ @robobun, your commit 70b5e14 has 1 failures in Build #82408 (All Failures):

  • 📦 Binary size — 12 over 0.50 MB
  • targetthis build canary: main #79916
    sizeΔ
    ❌ bun-darwin-aarch6458.13 MB57.58 MB+564.9 KB
    ❌ bun-darwin-x6463.48 MB62.95 MB+544.5 KB
    ❌ bun-linux-aarch6470.98 MB70.42 MB+576.0 KB
    ❌ bun-linux-x6472.47 MB71.95 MB+528.0 KB
    ❌ bun-linux-aarch64-musl64.88 MB64.32 MB+576.0 KB
    ❌ bun-linux-x64-musl66.98 MB66.45 MB+544.0 KB
    ❌ bun-linux-aarch64-android78.47 MB77.97 MB+512.0 KB
    ❌ bun-linux-x64-android80.62 MB80.10 MB+529.2 KB
    ❌ bun-freebsd-x6483.07 MB82.56 MB+528.0 KB
    ❌ bun-freebsd-aarch6484.84 MB84.31 MB+544.0 KB
    ❌ bun-windows-x6480.26 MB79.70 MB+570.5 KB
    ❌ bun-windows-aarch6470.86 MB70.34 MB+532.5 KB

    Add [skip size check] to the commit message if this increase is intentional.


🧪   To try this PR locally:

bunx bun-pr 35923

That installs a local version of the PR into your bun-35923 executable, so you can run:

bun-35923 --bun

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. udp: stop setting IP_RECVERR on UDP sockets #35922 - Stops IP_RECVERR on unconnected sockets (toggle on connect/disconnect) and makes recv errors non-fatal by default
  2. udp: only enable IP_RECVERR on connected sockets #29135 - Only enables IP_RECVERR on connected sockets (same connect/disconnect toggle approach, predecessor to udp: stop setting IP_RECVERR on UDP sockets #35922)

🤖 Generated with Claude Code

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate of #35922. Same bug, different mechanism, and this PR covers two cases #35922 does not:

#35922 (toggle IP_RECVERR on connect) #35923 (this PR, filter at dispatch)
Unconnected reply server survives ICMP yes (kernel drops it) yes (dropped in on_recv_error)
node:dgram echo server survives yes yes
EMSGSIZE double report, unconnected yes (IP_RECVERR off, kernel never queues it) yes (SO_EE_ORIGIN_LOCAL skipped)
EMSGSIZE double report, connected no (IP_RECVERR on, ee_origin still unread) yes
sk_err mirror on connected socket with handler no (recvmmsg can surface it once, errqueue drain once more) yes (drain from the recvmmsg failure path, single delivery)
Bun.udpSocket no-handler recv errors dropped silently only errqueue-origin on unconnected dropped; anything else still reaches uncaught_exception

#29135 already exists for the toggle approach; this PR implements the dispatch-layer fix (read ee_origin, clear sk_err after drain, drain from the recvmmsg failure path, drop remote ICMP for unconnected at on_recv_error). Happy to close whichever is not wanted.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

All test lanes green. The only hard failure is 📦 binary-size, which is comparing against a stale canary baseline (main #79916) and showing ~+540KB across every target. That is not this diff (net ~+60 LoC); the same job fails on #35922 and other unrelated PRs in the 823xx range, and build 82330 on farm/c3caa878/binary-size-stale-baseline is the fix for it. complex-workspace.test.ts passed on retry (marked flaky by the helper).

Ready for review.

Comment thread test/js/bun/udp/udp_socket.test.ts Resolved
Comment thread src/runtime/socket/udp_socket.rs Resolved
Comment thread test/js/bun/udp/udp_socket.test.ts Resolved
…h a connected socket, match ASAN stderr convention

- src/js/node/dgram.ts: remove the errqueue/unconnected filter now that
  on_recv_error drops those natively; the only reader is gone
- src/runtime/socket/udp_socket.rs: stop attaching .errqueue to the JS
  error (its only reader was the dgram.ts filter above)
- test/js/bun/udp/udp_socket.test.ts: the EMSGSIZE double-report test
  now uses a connected socket so the SO_EE_ORIGIN_LOCAL skip is the only
  clause that prevents the async redelivery; filter the ASAN banner from
  subprocess stderr to match the rest of the file
- test/js/bun/udp/udp_socket_recv_flags.test.ts: retitle 'stays usable'
  to 'stays open' (Bun.udpSocket exposes no public reconnect to prove
  usability via a round-trip on the connected path; the reply-server
  test covers I/O-after-ICMP)
Comment thread src/runtime/socket/udp_socket.rs Resolved
Comment thread src/runtime/socket/udp_socket.rs Resolved
Comment thread src/runtime/socket/udp_socket.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four nits from my earlier pass are addressed (ASAN stderr filter, dead dgram.ts guard removed, EMSGSIZE test now connected, recv_flags title matches its assertion). I didn't find anything further, but leaving this for a maintainer: it changes low-level loop.c UDP dispatch semantics and competes with #35922/#29135's toggle-IP_RECVERR approach — someone should pick one.

Checked: the factored us_internal_udp_drain_errqueue re-checks u->closed around on_recv_error (which enters JS) and clears SO_ERROR only when still open; the second call site from the recvmmsg-failure path correctly breaks to avoid the un-flagged double delivery; call_error_handler invokes the JS error callback with a single err arg, matching the new tests' handler shape.

Extended reasoning...

Overview

The PR fixes a Linux-specific crash where an unconnected Bun.udpSocket / node:dgram reply server dies with an uncaught ECONNREFUSED when a client vanishes before the reply arrives (ICMP port unreachable → IP_RECVERR errqueue → on_recv_error → uncaught_exception). It also fixes a double-report of EMSGSIZE (synchronous sendmsg failure + async SO_EE_ORIGIN_LOCAL errqueue redelivery). Files: packages/bun-usockets/src/loop.c (factor errqueue drain into a helper, read ee_origin, skip SO_EE_ORIGIN_LOCAL, clear mirrored sk_err via SO_ERROR, drain from the recvmmsg-failure path), src/runtime/socket/udp_socket.rs (drop errqueue errors on unconnected sockets, remove now-unread .errqueue property write), src/js/node/dgram.ts (delete the now-dead JS-side errqueue guard), plus three Linux-only tests.

Security risks

None identified. The change narrows what reaches user JS (drops remote-origin ICMP on unconnected sockets, skips local-origin entries) rather than widening it. Untrusted input here is kernel-generated cmsg data, walked via the standard CMSG_FIRSTHDR/CMSG_NXTHDR macros with a fixed 512-byte control buffer; sock_extended_err fields are read only after the cmsg_level/cmsg_type match.

Level of scrutiny

High. loop.c's us_internal_dispatch_ready_poll is the core UDP poll dispatch shared with QUIC and node:dgram; the change adds a second call into user JS (on_recv_error) from inside the recvmmsg loop and depends on Linux kernel semantics (sk_err mirroring, SO_EE_ORIGIN_LOCAL, level-triggered EPOLLERR) that are hard to verify by reading. The refactor is largely a hoist of the pre-existing drain loop, and the !u->closed guards around and after the JS re-entry look correct, but this is exactly the kind of platform-specific event-loop change a maintainer should sign off on.

Other factors

  • Competing PRs. #35922 and #29135 solve the same headline bug by toggling IP_RECVERR on connect/disconnect instead of filtering at dispatch. The author's comparison table argues this PR additionally fixes the connected-socket EMSGSIZE double-report and the sk_err-mirror double-delivery that the toggle approach leaves. A maintainer should choose between the approaches (or merge this and close the others).
  • Prior review addressed. My four earlier inline nits were all applied in e83009c / e429ecd / 70b5e14: ASAN stderr filter on both new subprocess tests, dead dgram.ts errqueue guard and its .errqueue writer removed, EMSGSIZE test switched to a connected socket so SO_EE_ORIGIN_LOCAL is the load-bearing clause, recv_flags test retitled to match its weakened assertion (Bun.udpSocket has no public disconnect(), so the round-trip couldn't be restored there — reasonable).
  • Test coverage. Three new Linux-only subprocess tests each pin one clause of the fix; the author reports fail-before / pass-after on all three plus green on the full UDP, dgram-parallel, and HTTP/3 suites. CI is green except an unrelated binary-size baseline job.
  • Comment-cop bot. Fired three times on the on_recv_error doc comment; the author folded the rationale into the pre-existing function comment at the same length as main. That resolution looks fine — it's Node-compat semantics documentation, not a workaround justification.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants