node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport

[robobun]

Problem

tls.connect({ socket: duplex }) / new TLSSocket(duplex) over a user-supplied Duplex: when the duplex's _write throws synchronously during a TLS write, the TLSSocket's 'error' listener receives the engine-internal JSC::Exception wrapper cell instead of the thrown Error:

• v instanceof Error is false, v.message is undefined, Bun.inspect(v) prints [native code: Exception]
• Object.prototype.toString.call(v) aborts the process (SIGABRT) on release builds
• debug builds assert isSymbol() in JSValue::synthesizePrototype on the first property read, before the listener even runs (node:events reads err.code in its emit path)

Node delivers the Error (as a write EPROTO plus an uncaughtException); it never crashes.

Cause

UpgradedDuplex::call_write_or_end forwarded global.take_exception(err) to handlers.on_error. take_exception returns the JSC::Exception* cell itself, not Exception::value(); that cell flowed unchanged through TLSSocket::handle_error into JS. take_error is the unwrapping variant used by the other call_error_handler feeds in socket_body.rs.

Fix

Use global.take_error(err) at every site in these files that fed a raw exception cell to an error handler:

• the three on_error sites in UpgradedDuplex::call_write_or_end
• TLSSocket::on_data's binary-type conversion error branch
• the ALPN selection path in socket_body.rs (review catch): when ToString of the alpnCallback return value throws, the cell reached the socket's error handler the same way

udp_socket.rs has two same-shape take_exception feeds, but its call_error_handler keys its termination guard on the Exception cell itself (err.is_termination_exception(), udp_socket.rs:818), so the swap is not a drop-in change there and is left out of this PR.

Verification

New test test/js/node/tls/node-tls-duplex-write-throw-error-value.test.ts wires two TLSSockets over an in-memory Duplex pair whose client _write throws after the handshake, and asserts the 'error' listener sees instanceof Error, [object Error], and the original message.

• without the fix: subprocess dies with SIGABRT (exit 134) at Object.prototype.toString.call
• with the fix: prints error:true:[object Error]:boom-in-transport-_write, exits 0

New test in test/js/bun/net/socket.test.ts covers the ALPN path: a Bun.listen server whose alpnCallback returns a Symbol. Without the fix the subprocess aborts the same way; with it the error handler receives a real TypeError.

Also ran node-tls-duplex-close-throw-uaf.test.ts (exercises the end()-throws path now going through take_error), node-tls-upgrade.test.ts, and node-tls-connect.test.ts: all pass.

---

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/net/socket.test.ts

[coderabbitai]

Walkthrough

Changes

Socket error handling

Layer / File(s)	Summary
Runtime socket error extraction src/runtime/socket/UpgradedDuplex.rs, src/runtime/socket/socket_body.rs	Socket write, end, and binary conversion failures now use take_error before error dispatch.
TLS Duplex failure regression test test/js/node/tls/node-tls-duplex-write-throw-error-value.test.ts	The test verifies synchronous transport write failures emit the expected Error and produce clean stderr and exit status.

Possibly related PRs

• oven-sh/bun#33191: Related TLS error-handling coverage in different code paths and APIs.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary change: passing the thrown Error instead of the internal exception wrapper over Duplex TLS transports.
Description check	✅ Passed	The description explains the problem, cause, fix, affected paths, and verification results, although it uses headings different from the repository template.

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/node/tls/node-tls-duplex-write-throw-error-value.test.ts`:
- Around line 12-77: The TLS Duplex regression test currently covers only the
_write() throw path. Add a sibling case in the fixture that invokes a
user-supplied Duplex end() implementation which throws, exercising
UpgradedDuplex::call_write_or_end’s end dispatch, and assert the client error
listener receives the original Error with its type, string representation, and
message.

🪄 Autofix

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: 2268bdab-aaae-44c3-b278-8234f6026b3c

📥 Commits

Reviewing files that changed from the base of the PR and between ace8f42 and d575dae.

📒 Files selected for processing (3)

• src/runtime/socket/UpgradedDuplex.rs
• src/runtime/socket/socket_body.rs
• test/js/node/tls/node-tls-duplex-write-throw-error-value.test.ts

[claude]

LGTM — mechanical take_exception → take_error swap at four sites so the thrown Error (not the JSC::Exception cell) reaches JS.

What was reviewed:

• Confirmed take_error = take_exception + .to_error() unwrap in JSGlobalObject.rs; the four changed sites feed straight into handle_error/on_error with no later unwrap, unlike the remaining take_exception callers in socket_body.rs which all follow with .to_error() or discard/rethrow.
• Checked sibling paths for the same bug class: WindowsNamedPipe.rs has no take_exception calls; udp_socket.rs unwraps via err.to_error().unwrap_or(err) inside its own call_error_handler.
• Test copies the exact ASAN_OPTIONS + stderr: "" pattern from node-tls-duplex-close-throw-uaf.test.ts; allow_user_segv_handler=1 suppresses the ASAN/JSC signal-handler warning so the stderr assertion is safe.
• CodeRabbit's request for an end()-throws case was withdrawn after robobun showed that path's error never reaches a JS listener.

Extended reasoning...

Overview

Four one-token substitutions in src/runtime/socket/UpgradedDuplex.rs and src/runtime/socket/socket_body.rs, replacing global.take_exception(err) with global.take_error(err) at the sites that feed a caught JS exception directly into the TLSSocket on_error handler, plus a new subprocess-based regression test.

I read take_exception and take_error in src/jsc/JSGlobalObject.rs:1006-1041: both clear the pending exception; take_error additionally calls .to_error() to unwrap the JSC::Exception cell to the value the user actually threw. The four changed sites were the only ones in the socket layer that passed the raw cell into JS without a subsequent .to_error() — every other take_exception in socket_body.rs (lines 156, 181, 1203, 1512, 1797, 1826, 1841, 1924, 1971) either immediately calls .to_error() on the result, discards it, or rethrows via throw_value. udp_socket.rs's two take_exception sites route through its own call_error_handler, which unwraps at line 813. WindowsNamedPipe.rs (the sibling transport) has no take_exception calls. So the whole bug class in this subsystem is covered.

Security risks

None. This is a value-unwrapping fix in the error-delivery path; it does not touch cert validation, handshake policy, or any trust decision. If anything it removes a user-reachable SIGABRT (Object.prototype.toString.call on the exception cell aborted release builds).

Level of scrutiny

Low-to-moderate. The native change is a mechanical function swap with well-defined semantics, and the surrounding call sites already establish take_error as the correct feed for call_error_handler. The one remaining take_exception in UpgradedDuplex.rs:238 is a deliberate discard (drop(...)) of a best-effort probe and is not affected.

Other factors

The test follows the harness conventions precisely (tempDir, bunEnv/bunExe, concurrent pipe drain, single combined-object assertion) and mirrors the neighboring node-tls-duplex-close-throw-uaf.test.ts for its ASAN_OPTIONS override — including allow_user_segv_handler=1, which is what keeps stderr clean on debug+ASAN. The assertion uses arrayContaining because the server side may independently surface a protocol error as an uncaughtException; the comment explains this and the load-bearing check (client's 'error' listener sees instanceof Error, [object Error], and the original message) is exact. The PR description documents fail-before (SIGABRT / exit 134) and pass-after, plus green runs of the three related TLS test files. The only review thread (CodeRabbit asking for an end()-throws variant) was resolved after robobun showed that path's error never reaches a JS listener because DuplexUpgradeContext::on_error short-circuits on the nulled TLS pointer post-close.

[robobun]

Cross-reference: #37664 (client-side new TLSSocket(socket) actually upgrading the socket) carries the same three take_error substitutions in UpgradedDuplex::call_write_or_end, kept byte-identical to this PR's hunk so the two merge cleanly in either order. It does not include the on_data site or this PR's dedicated test, so this PR is still useful on its own.

[robobun]

^{Updated 7:44 PM PT - Aug 16th, 2026}

✅ @robobun, your commit 168fc5dc2a3e007648ce043b561e0204d49d82bc passed in Build #99742! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36909

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

bun-36909 --bun

[claude]

LGTM — the ALPN ToString-throw site I flagged earlier is now fixed and covered by a test, and the udp_socket.rs exclusion is documented with a valid reason (its call_error_handler keys the termination guard on the Exception cell at udp_socket.rs:818).

What was reviewed:

• Verified take_error (JSGlobalObject.rs:1026) is take_exception(...).to_error(), so the swap unwraps JSC::Exception::value() as intended.
• Re-checked every remaining take_exception in src/runtime/socket/: all either .to_error() before reaching a JS handler (socket_body.rs:171/1536/etc.), are drop()ed (UpgradedDuplex.rs:234), or feed a handler that inspects the cell (udp_socket.rs).
• Both new tests are subprocess crash-repros with fail-without/pass-with evidence in the PR body.

Extended reasoning...

Overview

Five one-token substitutions (take_exception → take_error) at sites that feed a caught JS exception into a user-visible error handler: three in UpgradedDuplex::call_write_or_end, one in NewSocket::on_data's binary-type conversion branch, and one in select_alpn_callback's ToString-throw branch. take_exception returns the raw JSC::Exception* wrapper cell; take_error unwraps it to the thrown value. Passing the wrapper cell to JS causes instanceof Error to be false and Object.prototype.toString.call to abort the process. Two new subprocess-based crash-regression tests cover the Duplex-write and ALPN paths.

Security risks

None. This changes which JSValue reaches a user's error listener (the thrown Error instead of an internal wrapper); it does not touch TLS handshake logic, certificate validation, or any security decision.

Level of scrutiny

Low-to-medium. The fix is mechanical and mirrors the pattern already used at every other call_error_handler feed in socket_body.rs (verified: sites at 171, 1221, 1536, 1836, 1866, 1977, 2020 all unwrap via .to_error() before dispatch). The PR body includes fail-without/pass-with evidence on both ASAN-debug and release builds.

Other factors

My prior review (2026-08-17) flagged the ALPN ToString-throw site at socket_body.rs:189 as a same-class miss and asked about udp_socket.rs. The author addressed both: fixed line 189 with a dedicated test in socket.test.ts, and documented that udp_socket.rs's call_error_handler gates on err.is_termination_exception() (line 818), which needs the Exception cell — so the swap there is not drop-in and is intentionally out of scope. CodeRabbit's earlier request for an end()-throw test was withdrawn after the author showed that path is not observable from JS (the TLS context is already cleared when end() runs); the existing node-tls-duplex-close-throw-uaf.test.ts covers its no-crash guarantee. The remaining drop(global.take_exception(err)) at UpgradedDuplex.rs:234 is a best-effort exception clear on the writableEnded probe, not a handler feed, so it correctly stays as-is.