socket: set Handlers.mode=.client for Windows named-pipe Bun.connect by robobun · Pull Request #30150 · oven-sh/bun

robobun · 2026-05-03T00:12:16Z

Repro

Windows only:

await Bun.connect({
  unix: '\\\\.\\pipe\\x',
  socket: { data() {}, open() {}, close() {} },
});
// then close (or fail) the connection

On close, Handlers.markInactive() hits active_connections == 0 with .mode == .server and does @fieldParentPtr("handlers", this) expecting an enclosing Listener — but the handlers live in a standalone allocator.create(Handlers) block, so reading listen_socket.listener falls past the allocation. Under ASAN that's a heap-buffer-overflow; on release it reads garbage and — because the .client branch is skipped — leaks the block.

Cause

connectInner() calls SocketConfig.fromJS(vm, opts, globalObject, true) at Listener.zig:564. The last argument is is_server, which feeds handlers.mode. It was false until 4a06991 (#23755) flipped it during a bindings-generator refactor.

The non-pipe path at :797 has always had an explicit handlers_ptr.mode = .client after copying into the heap block (it was handlers_ptr.is_server = false before #26539), which masked the flip everywhere except the Windows named-pipe early-return at :655–656, which never had one.

is_server is only used to set handlers.mode; nothing else in SocketConfig.fromGenerated / Handlers.fromGenerated branches on it.

Fix

Restore is_server=false at the connectInner call site (this is the client connect path).
Add the same defensive handlers_ptr.mode = .client on the named-pipe branch to mirror the non-pipe branch, so the two copies into a standalone Handlers block look the same.

Audited the other standalone allocator.create(Handlers) sites:

socket.zig:1557 — sourced from Handlers.fromJS(..., false), already .client.
socket.zig:2062 — explicit .mode = if (is_server) .duplex_server else .client.

Verification

bun run zig:check-all passes (all targets, including both Windows arches).

New Windows-only tests in test/js/bun/net/socket.test.ts:

Listen on a named pipe, Bun.connect to it, close → clean exit.
Bun.connect to a non-existent pipe → rejects, clean exit.

Both are spawned in a subprocess so an ASAN crash surfaces as a non-zero exit instead of killing the test runner. Skipped on non-Windows (the if (Environment.isWindows) branch is unreachable there, and the non-pipe path's :797 override already covers it).

coderabbitai · 2026-05-03T00:12:21Z

Warning

Rate limit exceeded

@robobun has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 13 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 801c6a89-4ea9-4998-9187-b8bc0868b6b1

📥 Commits

Reviewing files that changed from the base of the PR and between 28be36b and 7dd9d40.

📒 Files selected for processing (1)

src/bun.js/api/bun/socket/Listener.zig

Walkthrough

connectInner() in Listener.zig now builds SocketConfig with is_server=false for client connections instead of true, with clarifying comments on allocator and handler lifecycle implications. A new Windows-only test suite validates that client handlers clean up correctly on both successful and failed connection flows.

Changes

Client Socket Handler Lifecycle Fix

Layer / File(s)	Summary
Core Implementation `src/bun.js/api/bun/socket/Listener.zig`	`connectInner()` switches `SocketConfig` initialization from `is_server=true` to `is_server=false`, with comments explaining client-side allocator ownership and `markInactive()` behavior differences from server/listener layouts.
Windows Integration Tests `test/js/bun/net/socket.test.ts`	New Windows-only test suite verifies client handlers lifecycle: open→close flow cleans up without buffer overruns, and failed connections to non-existent pipes properly reject and clean up. Tests use subprocess spawning and GC verification.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: setting Handlers.mode to .client for Windows named-pipe Bun.connect, which is the core fix in this PR.
Description check	✅ Passed	The description comprehensively covers the template requirements with detailed Repro, Cause, Fix, and Verification sections that exceed minimal template expectations.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Review rate limit: 0/5 reviews remaining, refill in 2 minutes and 13 seconds.}

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

robobun · 2026-05-03T00:12:26Z

^{Updated 3:05 AM PT - May 3rd, 2026}

❌ @robobun, your commit 7dd9d40 has 1 failures in Build #50501 (All Failures):

🧪 To try this PR locally:

bunx bun-pr 30150

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

bun-30150 --bun

github-actions · 2026-05-03T00:15:28Z

Found 2 issues this PR may fix:

http.request with socketPath connects to localhost on Windows #18653 - http.request with socketPath on Windows connects to localhost instead of the named pipe (//./pipe/docker_engine); this PR fixes the Bun.connect() path for Windows named pipes where handlers.mode was incorrectly set to .server.
Playwright chromium.launch() times out on Windows under Bun, but works under Node #27977 - Playwright chromium.launch() times out on Windows; Playwright uses a named pipe (--remote-debugging-pipe) to communicate with Chrome, and the handlers.mode = .server bug in Bun.connect() would cause the connection to crash or leak, hanging the launch handshake.

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #18653
Fixes #27977

🤖 Generated with Claude Code

claude

LGTM — targeted regression fix restoring is_server=false for the client connect path, with Windows named-pipe regression tests.

Extended reasoning...

Overview

This PR fixes a regression in Listener.connectInner() where SocketConfig.fromJS was called with is_server=true instead of false (flipped accidentally in #23755). The only effect of that argument is setting handlers.mode, and on the Windows named-pipe early-return branch the resulting .server mode caused Handlers.markInactive() to do @fieldParentPtr("handlers", this) against a standalone heap-allocated Handlers block — a heap-buffer-overflow under ASAN and a leak in release. The non-pipe branch was unaffected because it already had an explicit handlers_ptr.mode = .client override.

The fix is two lines of production code: restore the false argument, and add the same defensive handlers_ptr.mode = .client to the named-pipe branch to mirror the non-pipe branch. Two Windows-only subprocess regression tests are added.

Security risks

None. This is a memory-safety/lifecycle fix on the client connect path; no auth, crypto, input parsing, or trust-boundary changes. If anything it removes an out-of-bounds read.

Level of scrutiny

Low-to-moderate. I verified in Handlers.zig that is_server is consumed only at mode = if (is_server) .server else .client (line 136) and nowhere else in SocketConfig.fromGenerated / Handlers.fromGenerated, so the flip has no other side effects. markInactive (lines 80–97) confirms the .server branch does @fieldParentPtr and the .client branch does allocator.destroy, exactly as the PR description states. The change restores pre-#23755 behavior.

Other factors

The PR description includes a thorough audit of all other standalone allocator.create(Handlers) sites confirming they're already correct. Tests are isolated in subprocesses so an ASAN crash surfaces as a non-zero exit rather than killing the runner, and are gated to Windows where the affected branch is reachable. No outstanding reviewer comments; bug-hunting system found nothing.

Jarred-Sumner · 2026-05-03T02:17:29Z

@robobun fix conflicts

connectInner() passed is_server=true to SocketConfig.fromJS (regressed in 4a06991; it was false before), so the handlers start with .mode=.server. The non-pipe path overrides that to .client right after copying into the standalone allocator.create(Handlers) block, but the Windows named-pipe branch did not. With .mode left as .server, Handlers.markInactive() on close/fail takes the server branch and does @fieldParentPtr("handlers", this) to reach an enclosing Listener — but the block is a bare Handlers, so reading listen_socket.listener falls past the allocation (heap-buffer-overflow under ASAN) and the .client branch that frees the block is skipped (leak). Restore is_server=false at the connectInner call site and mirror the defensive .mode=.client override on the named-pipe branch to match the non-pipe path.

claude

LGTM — minimal, well-justified restoration of the pre-#23755 is_server=false value plus a defensive .mode = .client to mirror the non-pipe branch. The flagged connection.unix leak in the non-TLS named-pipe arm is pre-existing and minor; fine to address separately.

Extended reasoning...

Overview

Two-line Zig fix in src/bun.js/api/bun/socket/Listener.zig:

SocketConfig.fromJS(vm, opts, globalObject, true) → false in connectInner() — restores the value that was accidentally flipped in #23755. Verified that is_server flows only into Handlers.fromGenerated where it sets .mode = if (is_server) .server else .client (Handlers.zig:136); nothing else branches on it.
Adds handlers_ptr.mode = .client after the standalone allocator.create(Handlers) in the Windows named-pipe branch, mirroring the identical assignment that already exists on the non-pipe path a few lines down. Redundant after fix (1), but keeps the two heap-Handlers copies symmetric.

Plus two new Windows-only subprocess tests in test/js/bun/net/socket.test.ts exercising named-pipe connect open→close and connect-failure cleanup.

Security risks

None. No user-controlled input handling changes; this is a lifecycle/cleanup correctness flag. The fix removes a heap-buffer-overflow (@fieldParentPtr past a standalone allocation) rather than introducing one.

Level of scrutiny

Low-to-medium. Socket lifecycle code is sensitive, but this change is a targeted regression fix: a single boolean restored to its documented pre-refactor value, plus a one-line assignment copied from the adjacent branch. The PR description traces the regression to a specific commit, audits the other allocator.create(Handlers) sites, and adds regression tests. markInactive() (Handlers.zig:80–97) confirms the .server/.client semantics described.

Other factors

The one inline finding (non-TLS named-pipe arm leaks the connection.unix slice) is explicitly pre-existing — .connection = null in that arm predates this PR. It's a small per-connect string leak on Windows only and does not block this fix.
The 3 CI failures (bake/dev-and-prod, cli/hot, bun-install-lifecycle-scripts) are in unrelated test files, not the new socket tests.
Jarred's "fix conflicts" comment appears to have been addressed (diff applies cleanly to current main).

The non-TLS Windows named-pipe arm in connectInner() was the only construction path that left tcp.connection = null instead of adopting the heap-owned connection.unix slice (the TLS arm right above and the non-pipe arm below both adopt it). On the success return the errdefer doesn't fire, so the duped pipe-path bytes leaked per Bun.connect({unix: pipe}) on Windows. Mirror the TLS arm: deinit any old connection on the reuse path and assign connection so TCPSocket deinit frees it.

claude

LGTM — the connection adoption in 7dd9d40 addresses the pre-existing leak I flagged, and the non-TLS arm now mirrors the TLS/non-pipe arms.

Extended reasoning...

Overview

This PR fixes a heap-buffer-overflow / leak in the Windows named-pipe Bun.connect() path. Three changes in src/bun.js/api/bun/socket/Listener.zig:

SocketConfig.fromJS(..., true) → false in connectInner(), restoring the pre-#23755 value.
Defensive handlers_ptr.mode = .client on the named-pipe branch, mirroring the existing override on the non-pipe branch.
Adopt connection into the non-TLS named-pipe TCPSocket (addressing the pre-existing leak I flagged on the previous revision).

Plus two Windows-only subprocess tests in test/js/bun/net/socket.test.ts.

Verification

I confirmed via Handlers.zig:136 that is_server is only used to set .mode = if (is_server) .server else .client — nothing else in SocketConfig.fromGenerated / Handlers.fromGenerated branches on it. The non-pipe path already had an explicit handlers_ptr.mode = .client override (line ~810), so flipping the upstream flag is a no-op everywhere except the Windows named-pipe early-return — which is exactly what's being fixed. markInactive() at Handlers.zig:84-95 confirms the described .server → @fieldParentPtr vs .client → allocator.destroy divergence.

For the connection adoption: socket.zig:1400-1403 shows the socket's cleanup path does connection.deinit(), so ownership transfer is sound. The new code is byte-for-byte the same pattern as the TLS arm directly above it and the non-pipe arm below.

Security risks

None. This is a memory-lifecycle correctness fix (preventing an OOB read and a leak); no new attack surface, parsing, or auth logic.

Level of scrutiny

Medium — socket lifecycle code in Zig with manual memory management. However, every change here either restores prior behavior (the boolean flip) or copies an existing proven pattern from a sibling arm in the same function. The PR description includes a thorough root-cause analysis with commit references, and zig:check-all passes for all targets including both Windows arches.

Other factors

My previous review flagged the connection leak in the non-TLS arm; commit 7dd9d40 addresses it exactly as suggested (mirror the TLS arm).
The bug-hunting system found no issues on this revision.
The robobun CI failure (test-http-should-emit-close-when-connection-is-aborted.ts timeout) is on the earlier commit 28be36b, in an unrelated HTTP test that doesn't go through the named-pipe branch; the non-pipe connectInner path is provably unchanged in behavior.
New tests are properly gated with describe.skipIf(!isWindows) and use subprocesses so ASAN crashes surface as test failures.

…ven-sh#30150) ## Repro Windows only: ```js await Bun.connect({ unix: '\\\\.\\pipe\\x', socket: { data() {}, open() {}, close() {} }, }); // then close (or fail) the connection ``` On close, `Handlers.markInactive()` hits `active_connections == 0` with `.mode == .server` and does `@fieldParentPtr("handlers", this)` expecting an enclosing `Listener` — but the handlers live in a standalone `allocator.create(Handlers)` block, so reading `listen_socket.listener` falls past the allocation. Under ASAN that's a heap-buffer-overflow; on release it reads garbage and — because the `.client` branch is skipped — leaks the block. ## Cause `connectInner()` calls `SocketConfig.fromJS(vm, opts, globalObject, true)` at `Listener.zig:564`. The last argument is `is_server`, which feeds `handlers.mode`. It was `false` until 4a06991 (oven-sh#23755) flipped it during a bindings-generator refactor. The non-pipe path at :797 has always had an explicit `handlers_ptr.mode = .client` after copying into the heap block (it was `handlers_ptr.is_server = false` before oven-sh#26539), which masked the flip everywhere except the Windows named-pipe early-return at :655–656, which never had one. `is_server` is only used to set `handlers.mode`; nothing else in `SocketConfig.fromGenerated` / `Handlers.fromGenerated` branches on it. ## Fix - Restore `is_server=false` at the `connectInner` call site (this is the client connect path). - Add the same defensive `handlers_ptr.mode = .client` on the named-pipe branch to mirror the non-pipe branch, so the two copies into a standalone `Handlers` block look the same. Audited the other standalone `allocator.create(Handlers)` sites: - `socket.zig:1557` — sourced from `Handlers.fromJS(..., false)`, already `.client`. - `socket.zig:2062` — explicit `.mode = if (is_server) .duplex_server else .client`. ## Verification `bun run zig:check-all` passes (all targets, including both Windows arches). New Windows-only tests in `test/js/bun/net/socket.test.ts`: - Listen on a named pipe, `Bun.connect` to it, close → clean exit. - `Bun.connect` to a non-existent pipe → rejects, clean exit. Both are spawned in a subprocess so an ASAN crash surfaces as a non-zero exit instead of killing the test runner. Skipped on non-Windows (the `if (Environment.isWindows)` branch is unreachable there, and the non-pipe path's :797 override already covers it). --------- Co-authored-by: robobun <robobun@users.noreply.github.com>

github-actions Bot added the claude label May 3, 2026

claude Bot reviewed May 3, 2026

View reviewed changes

robobun force-pushed the farm/a745049e/named-pipe-client-handlers-mode branch from ed4eaae to 28be36b Compare May 3, 2026 02:43

claude Bot reviewed May 3, 2026

View reviewed changes

Comment thread src/bun.js/api/bun/socket/Listener.zig

claude Bot reviewed May 3, 2026

View reviewed changes

robobun mentioned this pull request May 4, 2026

image: preserve ICC profile through WebP decode/encode #30211

Merged

Jarred-Sumner merged commit bab007c into main May 4, 2026
72 of 77 checks passed

Jarred-Sumner deleted the farm/a745049e/named-pipe-client-handlers-mode branch May 4, 2026 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

socket: set Handlers.mode=.client for Windows named-pipe Bun.connect#30150

socket: set Handlers.mode=.client for Windows named-pipe Bun.connect#30150
Jarred-Sumner merged 2 commits into
mainfrom
farm/a745049e/named-pipe-client-handlers-mode

robobun commented May 3, 2026

Uh oh!

coderabbitai Bot commented May 3, 2026 •

edited

Loading

Rate limit exceeded

Uh oh!

robobun commented May 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 3, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Jarred-Sumner commented May 3, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented May 3, 2026

Repro

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Uh oh!

robobun commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 3, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Jarred-Sumner commented May 3, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Verification

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 3, 2026 •

edited

Loading

robobun commented May 3, 2026 •

edited

Loading