server: release StringImpl ref for 'unix' option in ServerConfig by robobun · Pull Request #32287 · oven-sh/bun

robobun · 2026-06-15T09:00:33Z

What

get_stringish() returns a bun_core::String carrying a +1 WTFStringImpl ref (via to_bun_string). bun_core::String is #[derive(Copy)] and has no Drop impl by design; callers must wrap it in OwnedString for scope-exit deref().

The hostname branch in ServerConfig::from_js already does this. The unix branch did not, so every Bun.serve({ unix: ... }) and server.reload({ unix: ... }) leaked one WTFStringImpl. The Zig reference (ServerConfig.zig:810) has the explicit defer unix.deref().

Repro

Drive the hostname+unix error path so no server is actually created:

const base = Buffer.alloc(256 * 1024, "x").toString();
for (let i = 0; i < 1000; i++) {
  try {
    Bun.serve({ hostname: "127.0.0.1", unix: "/tmp/" + i + base, fetch: () => new Response("ok") });
  } catch {}
}

RSS growth scales 1:1 with the unix string size per call.

Fix

Wrap the returned string in bun_core::OwnedString, matching the hostname branch directly above.

Verification

	RSS delta (1000 iters, 256KB string)
before	~258 MB
after	~22 MB (flat, independent of string size)

bun-serve-args.test.ts: 47 pass, 1 todo (pre-existing).

get_stringish() returns a bun_core::String carrying a +1 WTFStringImpl ref. The 'hostname' branch wraps it in OwnedString for RAII deref, but the 'unix' branch did not, leaking one StringImpl per Bun.serve({unix}) or server.reload({unix}) call. Matches the Zig reference's 'defer unix.deref()'.

robobun · 2026-06-15T09:00:38Z

Repro: RSS grows ~1:1 with unix string size per Bun.serve call (~500MB over 1000 iters with 512KB strings). After fix: ~22MB, flat regardless of string size.

PR: #32287

The diff is green: 282 CI jobs passed across Linux x64/aarch64/musl/ASAN, Windows, macOS x64, FreeBSD. The new test passed on every lane that ran it, including x64-asan.

Remaining red is unrelated infrastructure:

build 62425: test/bake/deinitialization.test.ts ASAN heap-use-after-free in the bake dev server (unrelated to ServerConfig)
build 62486: bun install on darwin-26-aarch64 got HTTP 5xx from api.github.com/repos/oven-sh/bun.report/tarball/912ca63 (GitHub tarball endpoint server error); earlier darwin-aarch64 test jobs expired waiting for agents

Ready for a maintainer to merge.

coderabbitai · 2026-06-15T09:00:42Z

Warning

Review limit reached

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

More reviews will be available in 7 minutes and 14 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c12d706-dcdd-4445-8bc0-3c1a29f054fb

📥 Commits

Reviewing files that changed from the base of the PR and between e0acad3 and d44ac3f.

📒 Files selected for processing (2)

src/runtime/server/ServerConfig.rs
test/js/bun/http/bun-serve-args.test.ts

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

robobun · 2026-06-15T09:00:45Z

^{Updated 6:47 AM PT - Jun 15th, 2026}

❌ @robobun, your commit d44ac3f has 2 failures in Build #62486 (All Failures):

test/cli/install/bun-install-lifecycle-scripts.test.ts - code 1 on 🍎 26 aarch64
package.json - code 1 on 🍎 26 aarch64
package.json - code 1 on 🍎 14 aarch64

🧪 To try this PR locally:

bunx bun-pr 32287

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

bun-32287 --bun

claude

LGTM — one-line fix mirroring the hostname branch directly above; the ASAN-threshold nit on the test is worth addressing but doesn't block.

Extended reasoning...

Overview

This PR adds a single line to ServerConfig::from_js in src/runtime/server/ServerConfig.rs: wrapping the bun_core::String returned by get_stringish(global, "unix") in bun_core::OwnedString so its Drop impl releases the +1 WTFStringImpl ref at scope exit. It also adds an RSS-delta leak regression test to bun-serve-args.test.ts.

The fix is mechanically identical to the hostname branch ~15 lines above (let host = bun_core::OwnedString::new(host);). I verified that OwnedString is #[repr(transparent)] over String with Deref<Target = String> (so unix.to_utf8() still resolves) and Drop calling self.0.deref() — exactly the RAII the PR description claims. The shadowing let unix = ... is intentional and matches house style.

Security risks

None. This is a refcount-release fix on an argument-parsing path; no auth, crypto, or permission surface is touched. The only behavioral change is that a WTFStringImpl is now freed instead of leaked.

Level of scrutiny

Low. A one-line change applying an established in-file pattern, with a clear repro and before/after RSS measurements in the PR description. The Rust change cannot introduce a use-after-free because the bytes are duped into a ZBox before unix drops, and OwnedString's Deref keeps to_utf8() working unchanged.

Other factors

The bug-hunting system flagged one nit: the new test's expect(deltaMB).toBeLessThan(64) lacks the isASAN ? … : 64 widening used by sibling leak tests in the same directory. That's a fair point about potential ASAN-lane flakiness and is posted inline; it's test-only and doesn't affect the correctness of the fix, so I'm approving while leaving the nit for the author to address.

Match house convention for RSS-delta leak tests in this directory. Bumped string size to 512KB so the leak signature (~500MB) stays well above the widened ASAN threshold (192MB).

claude

LGTM — the one-line OwnedString wrap mirrors the hostname branch exactly; the remaining ASAN-threshold nit is a test-tuning follow-up, not a blocker.

Extended reasoning...

Overview

One-line production fix in src/runtime/server/ServerConfig.rs: wrap the bun_core::String returned by get_stringish(global, "unix") in bun_core::OwnedString::new() so its WTFStringImpl ref is released on scope exit. This is a copy of the pattern used by the hostname branch ~15 lines above and matches the Zig reference's defer unix.deref(). The accompanying test in bun-serve-args.test.ts drives the hostname+unix error path with a 512KB string and asserts bounded RSS growth.

Security risks

None. No new inputs, parsing, auth, or external surface. The change only adds an RAII wrapper around an existing local; control flow and the value passed to ZBox::from_bytes are unchanged.

Level of scrutiny

Low for the Rust change: it is mechanical, pattern-matched against adjacent code, and verified by before/after RSS measurements (~258MB → ~22MB). Low-medium for the test: RSS-threshold leak tests are inherently tunable and the ASAN constant may need one more bump, but that is a CI-calibration concern, not a correctness concern.

Other factors

I previously flagged the missing ASAN guard; the author addressed it in a30f693 by adding isASAN ? 192 : 64. The new inline nit observes that 192 may still sit below the quarantine-fill ceiling for 512KB strings and suggests 400 (matching neighboring tests) or quarantine_size_mb=0. That's a reasonable follow-up but speculative (no observed CI failure yet) and trivially fixable if it flakes — not worth holding a one-line leak fix over. Approving with the nit attached.

claude

LGTM — one-line OwnedString wrap matching the hostname branch directly above; the ASAN threshold discussion is resolved by the empirical x64-asan pass.

Extended reasoning...

Overview

The production change is a single line in src/runtime/server/ServerConfig.rs: wrapping the bun_core::String returned by get_stringish(global, "unix") in bun_core::OwnedString so its Drop releases the +1 WTFStringImpl ref. This is a verbatim copy of the pattern used ~15 lines above for the hostname branch (let host = bun_core::OwnedString::new(host)), and mirrors the Zig reference's defer unix.deref(). The accompanying change in bun-serve-args.test.ts adds a subprocess RSS-delta regression test plus the harness imports it needs.

Security risks

None. The change adds RAII cleanup for a string ref on an option-parsing path; it does not touch auth, crypto, permissions, request handling, or any externally-reachable surface beyond what already existed. The only behavioral effect is that a previously-leaked refcount is now released.

Level of scrutiny

Low. The fix is mechanical and pattern-matched against adjacent, already-shipping code in the same function. OwnedString is the documented owner wrapper for the Copy/no-Drop bun_core::String; there is no plausible way for this wrap to introduce a use-after-free since unix_str (the to_utf8() borrow) and the ZBox::from_bytes copy both complete before unix drops at scope end. The test follows the repo's standard RSS-leak-test shape (subprocess via bunExe()/bunEnv, warmup, GC, delta assertion with an isASAN widen).

Other factors

I previously raised two nits about the ASAN threshold; both threads are resolved. On the second, the author rebutted my quarantine arithmetic with the correct observation that WTFStringImpl goes through bmalloc/libpas (not ASAN-intercepted malloc), measured ~22MB under ASAN across 5 local runs, and confirmed the test passed on the x64-asan CI lane in build 62425 with the 192MB threshold — that's empirical evidence over my theoretical model, and 192MB is ~8× headroom over observed while staying well below the ~500MB leak signature. The musl LTO link failures in build 62412 are an unrelated bitcode/data-layout infrastructure issue (the change adds no symbols and cannot affect linking); CI was retriggered. The bug-hunting system found nothing. No CODEOWNERS cover these paths.

robobun · 2026-08-24T06:50:06Z

Closing: #40238 made bun_core::String release its WTF ref on Drop and removed OwnedString. In src/runtime/server/ServerConfig.rs, arg.get_stringish(global, "unix") now returns an owned String that drops at scope exit, including on the "Cannot specify both hostname and unix" error path. The Fixes list in that PR names this site ("Bun.serve({ unix })").

Verified: the "unix option does not leak the string" test from this branch fails on a debug build from before #40238 (RSS delta 516 MB) and passes on current main (4441116) with no change to src/.

github-actions Bot added the claude label Jun 15, 2026

claude Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread test/js/bun/http/bun-serve-args.test.ts Outdated

test: widen ASAN RSS threshold for unix leak test

a30f693

Match house convention for RSS-delta leak tests in this directory. Bumped string size to 512KB so the leak signature (~500MB) stays well above the widened ASAN threshold (192MB).

claude Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread test/js/bun/http/bun-serve-args.test.ts

ci: retrigger

d44ac3f

claude Bot reviewed Jun 15, 2026

View reviewed changes

robobun mentioned this pull request Jul 25, 2026

Bun.serve: percent-encode unix socket paths in server.url #35806

Open

robobun closed this Aug 24, 2026

robobun mentioned this pull request Aug 24, 2026

Add leak tests for Bun.serve unix option and Transpiler reject path #40297

Open

Navigation Menu

Repository navigation

server: release StringImpl ref for 'unix' option in ServerConfig - #32287

server: release StringImpl ref for 'unix' option in ServerConfig#32287
robobun wants to merge 3 commits into
mainfrom
farm/976546d0/fix-unix-string-leak

robobun commented Jun 15, 2026

Uh oh!

robobun commented Jun 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

Review limit reached

Uh oh!

robobun commented Jun 15, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Aug 24, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented Jun 15, 2026

What

Repro

Fix

Verification

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Review limit reached

Uh oh!

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

Uh oh!

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

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Aug 24, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jun 15, 2026 •

edited

Loading

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

robobun commented Jun 15, 2026 •

edited

Loading