Skip to content

usockets: bound basename copy in Linux long-unix-path workaround - #34078

Merged
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/74f5be80/fix-unix-socket-oob-read
Jul 13, 2026
Merged

Jarred-Sumner merged 2 commits into
mainfrom
claude/farm/74f5be80/fix-unix-socket-oob-read

Conversation

@robobun

@robobun robobun commented Jul 13, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

bsd_create_unix_socket_address() takes the caller's path as (const char *path, size_t path_len) and, on Linux, works around sun_path's 108-byte limit by opening the parent directory and binding to /proc/self/fd/<dirfd>/<basename> instead. The basename was being copied with

snprintf(sun_path, sizeof sun_path, "/proc/self/fd/%d/%s", fd, path + dirname_len);

but path is a ptr+len pair coming from a Rust &[u8] with no NUL terminator. %s walks past the end of the allocation. On ASan builds this aborts with heap-buffer-overflow; on release builds sun_path is assembled from whatever heap bytes follow the path buffer, so the kernel sees an address built from out-of-bounds memory (sometimes the right one, sometimes EINVAL, sometimes something else).

The trigger window is any pathname unix socket with 108 <= path_len whose basename still fits inside /proc/self/fd/N/, reachable from net.createServer().listen(path), net.connect(path), Bun.listen({unix}) and Bun.connect({unix}). Node binds a full 108-byte sun_path here, so this is also a parity break at exactly length 108.

Fix: use %.*s with (int)(path_len - dirname_len) so the copy is bounded by the known basename length.

Repro

import * as net from "node:net";
import * as fs from "node:fs";
const dir = fs.mkdtempSync("/tmp/sun108-");
const path = dir + "/" + "l".repeat(108 - dir.length - 1); // exactly 108 bytes
net.createServer().listen(path, () => { console.log("LISTENING"); process.exit(0); });

Before (debug/ASan):

==510==ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 90 at 0x7339f260062c thread T0
    #0 ... in printf_common
    #2 ... in snprintf
    #3 ... in bsd_create_unix_socket_address packages/bun-usockets/src/bsd.c

After: LISTENING, exit 0.

How did you verify your code works?

bun bd test test/js/bun/net/unix-socket-long-path.test.ts passes (4/4). With the packages/ change stashed out, all four cases fail with the ASan heap-buffer-overflow header in the subprocess stderr.


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/unix-socket-long-path.test.ts

bsd_create_unix_socket_address() takes a ptr+len path and, for paths
>= sizeof(sun_path) on Linux, opens the parent directory and rewrites
the address as /proc/self/fd/<dirfd>/<basename>. The basename was
copied via snprintf("%s", path + dirname_len), which reads until a
NUL byte, but the Rust callers pass a bare byte slice with no
terminator. Any pathname unix socket with 108 <= len < dirname_len+107
read past the allocation (ASan heap-buffer-overflow abort) and, on
release builds, assembled sun_path from whatever bytes followed on the
heap. This is reachable from net.createServer().listen(path),
net.connect(path), Bun.listen({unix}) and Bun.connect({unix}).

Switch to %.*s with the known basename length so exactly
path_len - dirname_len bytes are copied.
@robobun

robobun commented Jul 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:12 PM PT - Jul 13th, 2026

❌ @autofix-ci[bot], your commit 7578548 has 1 failures in Build #72492 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34078

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

bun-34078 --bun

@coderabbitai

coderabbitai Bot commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 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: 4ded1d56-05a2-4601-ab20-8b7f4df98de4

📥 Commits

Reviewing files that changed from the base of the PR and between 4048fc5 and 7578548.

📒 Files selected for processing (1)
  • test/js/bun/net/unix-socket-long-path.test.ts

Walkthrough

Changes

The Linux Unix socket address workaround now bounds basename reads by the supplied path length. A Linux-only regression suite tests exact 108-byte and 150-byte paths using Node and Bun echo fixtures, checking output, exit status, and ASan diagnostics.

Unix socket long-path handling

Layer / File(s) Summary
Bound basename copying
packages/bun-usockets/src/bsd.c
Linux socket address construction formats the basename using its explicit length rather than an unbounded string read.
Long-path regression coverage
test/js/bun/net/unix-socket-long-path.test.ts
Linux-only tests generate exact-length paths and validate Node and Bun Unix socket echo round trips at 108 and 150 bytes, including exit and ASan checks.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: bounding the basename copy in the Linux long-unix-path workaround.
Description check ✅ Passed The description follows the template and includes both the change summary and verification details.
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.

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

@coderabbitai coderabbitai 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.

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/bun/net/unix-socket-long-path.test.ts`:
- Around line 1-109: Move the long Unix socket path tests from the standalone
suite into the existing net/socket test suite. Preserve the
`describe.skipIf(!isLinux)` coverage, helper functions, test cases, and
assertions unchanged while integrating them into `socket.test.ts` and removing
the new test file.
🪄 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: 8659747b-74a7-4213-b94c-1b53dc8aa419

📥 Commits

Reviewing files that changed from the base of the PR and between a59a9c3 and 4048fc5.

📒 Files selected for processing (2)
  • packages/bun-usockets/src/bsd.c
  • test/js/bun/net/unix-socket-long-path.test.ts

Comment thread test/js/bun/net/unix-socket-long-path.test.ts

@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.

LGTM — one-line %s → %.*s bounding fix for a real ASan heap-buffer-overflow in the Linux long-unix-path workaround.

Checked that (int)(path_len - dirname_len) is safe: the earlier (path_len - dirname_len + 1) >= sizeof(sun_path) guard already caps it below 108, and dirname_len <= path_len so it can't underflow. Verified the sibling macOS branch and the short-path fallback both already use bounded memcpy, so this was the only unbounded read of path. Test constructs exact-length paths, covers both the 108-byte boundary and a longer path, exercises both net.* and Bun.* entry points, and drains subprocess pipes concurrently.

Extended reasoning...

Overview

One-line change in packages/bun-usockets/src/bsd.c inside bsd_create_unix_socket_address()'s Linux-only /proc/self/fd/<dirfd>/<basename> workaround: the snprintf format changes from %s to %.*s with an explicit precision of (int)(path_len - dirname_len). The path argument is a ptr+len pair from Rust and is not NUL-terminated, so the old %s read past the allocation. A new Linux-only test file spawns subprocesses that bind/connect over 108- and 150-byte unix socket paths via both node:net and Bun.listen/Bun.connect, asserting a clean echo round-trip and no ASan header in stderr.

Security risks

The fix removes an out-of-bounds heap read on a user-controlled path buffer. The precision value cannot overflow: dirname_len is derived by walking back from path_len (so path_len - dirname_len >= 0), and the preceding (path_len - dirname_len + 1) >= sizeof(sun_path) early-return already bounds it below 108, well within int. No new attack surface is introduced.

Level of scrutiny

Low-to-medium. The native change is a single format-string precision addition — a textbook fix for passing non-terminated buffers to printf-family functions. I audited the other two branches in the same function that copy from path (the macOS __pthread_fchdir workaround and the short-path fallback) and both already use length-bounded memcpy, so there are no sibling sites with the same bug. The snprintf return-value check on the following line remains valid with %.*s.

Other factors

The test follows harness conventions (tempDir, bunEnv, bunExe, concurrent pipe draining, using for cleanup, test.concurrent) and asserts on a combined {stdout, asan, exitCode} object so failures show all three. The standalone-file placement was raised by CodeRabbit and reasonably justified by the author against the unix-socket-unlink.test.ts precedent (the thread is resolved). The ASAN_OPTIONS symbolize=0 tweak with its comment is a sensible guard against symbolizer-induced timeouts on the pre-fix failure path. No CODEOWNERS cover these paths.

@robobun

robobun commented Jul 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI on build #72492 finished: 285/286 jobs passed. The new test unix-socket-long-path.test.ts passes on every lane.

The one hard-failed job is debian 13 x64-asan on test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (JSC getOwnPropertyDescriptor assertion during worker termination), which the annotation tooling tags as pre-existing on main. The remaining annotations (bun-install-registry, 30205, napi, no-orphans, bun-run-dir, net-mongodb-pattern-leak, test-fs-promises-file-handle-readFile, bun-add, fetch-leak) are all tagged flaky and passed on retry.

This change is a one-line %s → %.*s inside the #if defined(__linux__) branch of packages/bun-usockets/src/bsd.c; none of the above touch that code path. Ready for review.

@Jarred-Sumner
Jarred-Sumner merged commit df791a7 into main Jul 13, 2026
76 of 77 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/farm/74f5be80/fix-unix-socket-oob-read branch July 13, 2026 22:54
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