test: run the full ported Node suite leak-clean under the ASAN runner and fix the bugs it surfaces — subprocess fd double-close, console/Strong-handle teardown UAFs, SAN/scope-function/valkey leaks, post-exit child_process streams, runner tmpdir isolation (2322 tests green)#31833
test: run the full ported Node suite leak-clean under the ASAN runner and fix the bugs it surfaces — subprocess fd double-close, console/Strong-handle teardown UAFs, SAN/scope-function/valkey leaks, post-exit child_process streams, runner tmpdir isolation (2322 tests green)#31833cirospaciari wants to merge 13 commits into
Conversation
…e local ASAN runner Fixes seven native bugs surfaced by running all 2322 ported Node tests with LeakSanitizer + BUN_DESTRUCT_VM_ON_EXIT: a subprocess stdio fd double-close, ConsoleObject and Strong-handle teardown UAFs, SAN ASN1 / scope-function / valkey URL string leaks, and a post-exit child_process stream crash. Adds runner tmpdir isolation and verified leak suppressions for OS/JSC exit-time false positives.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR bundles several independent changes: converting URL-related FFI functions and callers from ChangesString to OwnedString URL Ownership Refactor
VM/Worker Teardown Handle Release and Console Ownership
child_process Ownership Transfer via takeStdio
Test Infrastructure Updates
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:25 PM PT - Jul 9th, 2026
❌ @alii, your commit d538d79 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31833That installs a local version of the PR into your bun-31833 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Heads up on an overlap: #31990 (fixing the Sentry worker-shutdown segfaults BUN-3DSK/BUN-3END in |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Add one extern for BoringSSL's GENERAL_NAMES_free and delete the whole hand-rolled sk_GENERAL_NAME_pop_free + type-punned shim machinery — the shim exists only to work around a mistyped callback signature that shouldn't survive this PR.
BoringSSL exports it: vendor/boringssl/include/openssl/x509.h:308 typedef STACK_OF(GENERAL_NAME) GENERAL_NAMES; and :2087 OPENSSL_EXPORT void GENERAL_NAMES_free(GENERAL_NAMES *gens); (deep free via IMPLEMENT_ASN1_FUNCTIONS_const(GENERAL_NAMES) at crypto/x509/v3_genn.cc:73). BoringSSL uses it for exactly this — freeing SAN results — at crypto/x509/x_x509.cc:81, and its own Rust bindings at rust/bssl-x509/src/certificates.rs:153 use it in Drop. Root cause the PR does NOT fix: src/boringssl_sys/boringssl.rs:444 declares sk_GENERAL_NAME_free_func = unsafe extern "C" fn(*mut struct_stack_st_GENERAL_NAME) — wrong; the C header stack.h:402 generates typedef void (*sk_GENERAL_NAME_free_func)(GENERAL_NAME *) (element, not stack). That mistyped signature is why GENERAL_NAME_free couldn't be passed directly and why the original code passed the container-free (leaking). The PR's sk_GENERAL_NAME_element_free (boringssl.rs:480, boringssl.zig:2974) casts *mut struct_stack_st_GENERAL_NAME → *mut GENERAL_NAME inside the callback to satisfy the wrong type — a shim over the bug. Call-site census: sk_GENERAL_NAME_pop_free/sk_GENERAL_NAME_free/sk_GENERAL_NAME_call_free_func/sk_GENERAL_NAME_free_func/sk_GENERAL_NAME_element_free are used ONLY at the 4 sites this PR touches (src/boringssl/lib.rs:363, src/boringssl/boringssl.zig:180,202) — all become `GENERAL_NAMES_free(nam
Fix: Replace the fix with: (1) add pub fn GENERAL_NAMES_free(gens: *mut struct_stack_st_GENERAL_NAME); to the extern block in src/boringssl_sys/boringssl.rs and the equivalent pub extern fn GENERAL_NAMES_free(gens: ?*struct_stack_st_GENERAL_NAME) void; in boringssl.zig; (2) call sites become defer boring.GENERAL_NAMES_free(names) (Zig) and scopeguard::guard(names, |n| boring::GENERAL_NAMES_free(n)) (Rust); (3) delete sk_GENERAL_NAME_element_free, sk_GENERAL_NAME_pop_free, sk_GENERAL_NAME_call_free_func, sk_GENERAL_NAME_free, sk_GENERAL_NAME_free_func and the GENERAL_NAME_free extern from both boringssl_sys files — none have other callers. Net: ~2 lines added, ~40 lines deleted
The valkey OwnedString wrap is the third independent per-call-site fix for the same +1-BunString footgun; the URL getters themselves should return OwnedString so the type system enforces the deref, and there are live unfixed leaks of the identical class right now.
Root cause at binding layer: src/jsc/URL.rs:18-34,92-151 and src/url/lib.rs:140-184 — 12 getters (protocol/href/username/password/search/host/hostname/pathname/hash/fragment_identifier) + 5 free fns all return bun_core::String (Copy, no Drop). C++ side src/jsc/bindings/BunString.cpp:286,294 does wtfString.impl()->ref() — always +1. bun_core/string/mod.rs:1249-1253 documents String is deliberately Copy-no-Drop for FFI; OwnedString (line 1259) is the RAII wrapper and Derefs to String (line 1291-1297), so returning it is near-transparent to callers.
Footgun already hit and half-fixed in 3 files independently: (a) fetch.rs:257-259 has a comment "href_from_js returns a +1 (Bun::toStringRef)" then wraps in OwnedString; (b) hosted_git_info.rs wraps 7 sites (993,1007,1382,1443,1498,1563,1640) in OwnedString but leaves 7 sibling sites bare; (c) this PR wraps 5 more in js_valkey.rs and adds ANOTHER per-call-site comment (js_valkey.rs:559-560) documenting the API contract.
LIVE UNFIXED LEAKS of the identical class this PR misses: (1) src/runtime/socket/SocketAddress.rs:221 let host: BunString = url.host(); — never deref'd (grep confirms no host.deref() in file), used via .latin1() then dropped. (2) src/install/hosted_git_info.rs:292,322,1351,1419,1477,1531,1616 — url.pathname().to_owned_slice() / .fragment_identifier().to_owned_slice(); to_owned_slice (mod.rs:762) takes &self, c
Fix: Change src/jsc/URL.rs getters (protocol/href/username/password/search/host/hostname/pathname/hash/fragment_identifier) and the free functions (href_from_string/href_from_js/join/file_url_from_string/path_from_file_url) to return bun_core::OwnedString instead of bun_core::String; do the same in the duplicate src/url/lib.rs::whatwg impl block. OwnedString Derefs to String so most call sites need no change; the ones that do (e.g. Request.rs:977 storing the value) call .into_inner()/take. Delete the per-call-site OwnedString::new() wraps in js_valkey.rs, fetch.rs, hosted_git_info.rs. This also fixes the live leaks at SocketAddress.rs:221 and hosted_git_info.rs:292,322,1351,1419,1477,1531,1616 wi
The double-close fix mutates ownership inside a public, uncached, readonly getter — trading child_process's loud fd-UAF for a silent fd leak on the bare Bun.spawn().stdio API.
subprocess.rs:832 #[bun_jsc::host_fn(getter)] pub fn get_stdio now ends (869-874) by downgrading every ExtraPipe::OwnedFd → UnownedFd on read. The getter is NOT cached (BunObject.classes.ts:123 stdio: { getter: "getStdio" } — no cache: true, unlike terminal at :127). It is documented public API (bun.d.ts:7217 readonly stdio: [null, null, null, ...(number | null)[]]; doc at :7210 states no ownership-transfer contract) and is read directly in Bun-API tests (spawn.test.ts:832/863/885). OwnedFd is created for extra "pipe" slots via socketpair (spawn_process.rs:933 extra_fds.push(ExtraPipe::OwnedFd(fds[0]))). finalize_streams closes only OwnedFd (subprocess.rs:1215). So Bun.spawn({stdio:[0,1,2,"pipe"]}); console.log(proc.stdio) — before PR: finalize closes the socketpair end; after PR: read downgrades it, finalize skips it, fd leaks. The mutation exists solely to serve child_process.ts:1253 NetModule.connect({ fd }), a single internal consumer. A prior PR review comment already flagged this exact leak-for-UAF trade.
Fix: Move the ownership transfer to the layer that actually takes ownership. Cheapest: in child_process.ts:1253, dup() the fd before NetModule.connect({ fd: dupd }) so the Socket owns its own copy and native finalize_streams still closes the original — both sides have exactly-once close semantics and the getter stays pure. Alternative: expose an explicit method (e.g. takeStdioFd(i) or a takeStdio array-returning method) that child_process.ts calls once, keeping get_stdio a pure getter; or transfer at spawn time via the lazy: true / child_process-specific spawn option so the fd is born UnownedFd only when the caller has committed to wrapping it. At minimum, if the getter mutation s
The Strong-handle UAF fix is a hand-picked list of the 5 handles the Node suite happened to exercise; at least one more member of the same class — VirtualMachine.overridden_main, deinit()'d in destroy() at line 4464 — is left with the identical UAF, and the fix shape guarantees drift.
The bug class per the PR body: any Strong dropped in destroy() (post-destructOnExit) UAFs in Bun__StrongRef__delete. destroy() runs at src/jsc/VirtualMachine.rs:1605, after destructOnExit at :1594. The PR pre-releases rare.s3_default_client (:1586) and 4 sql_rare handles (src/runtime/jsc_hooks.rs:1458-1470). But destroy() itself (VirtualMachine.rs:4419-4476) still contains self.overridden_main.deinit() at :4464 — a strong::Optional (declared :164) populated by Bun.main = value (src/runtime/api/BunObject.rs:977-985). Under BUN_DESTRUCT_VM_ON_EXIT=1 (the mode this PR targets, VirtualMachine.rs:1374-1376), a script that assigns Bun.main and exits hits the identical Bun__StrongRef__delete UAF. Not caught because Bun.main is Bun-specific — no Node test writes it. Contrast: the many other StrongOptionals in the probe (UpgradedDuplex.rs:29-41, server/mod.rs:297 on_clienterror, NodeHTTPResponse.rs:47, napi_body.rs:2444, zlib) are NOT in this class — they live on JSC-heap objects finalized by lastChanceToFinalize() inside destructOnExit while the HandleSet is live. The at-risk set is exactly "Strongs reached from destroy()": RareData (s3 only, :290 — handled), RuntimeState (sql_rare only — handled), and VirtualMachine direct fields (overridden_main :164 — MISSED; entry_point_result.value :295 — never deinit'd in destroy() so leak-not-UAF). CHILD_SINGLETON (node_cluster_binding.rs:33
Fix: (1) Move self.overridden_main.deinit() from destroy():4464 into the pre-destructOnExit block alongside s3_default_client (or add it there and make the destroy() call idempotent-redundant). (2) Collapse the enumeration into one named method per owner — RareData::release_js_handles(), VirtualMachine::release_direct_js_handles(), plus the existing release_runtime_state_js_handles hook — all called from one contiguous block before destructOnExit, with a comment on each struct that any new Strong field must be added there. (3) Decide whether entry_point_result.value and CHILD_SINGLETON should be released in the same block for leak-cleanliness (they don't UAF, but the PR's goal is leak-cle
The 2-element-stdio post-exit crash fix ships with zero committed test — the "stdio-shape probe matrix" the PR body cites as verification is not in the diff, and no existing Bun test exercises the stdioCount < 3 lazy-load path.
PR file list (gh pr view 31833 --json files): only test/ changes are test/expectations.txt (+6, worker flake entry) and test/leaksan.supp (+50) — no *.test.* file. The fix is at src/js/node/child_process.ts:1221-1230 (constructNativeReadable → newStreamReadableFromReadableStream). Trigger predicate is src/js/node/child_process.ts:1350 const hasSocketsToEagerlyLoad = stdioCount >= 3; — only false when the user passes a ≤2-element stdio array. Grep of test/js/node/child_process/ for stdio: arrays: every occurrence is 3- or 4-element (child_process.test.ts:146,234,420,431; child_process-node.test.js:112,155,258; child-process-stdio.test.js:12,32,58,88; fixtures/ipc_fixture.js:11) — none exercises hasSocketsToEagerlyLoad = false. PR body Testing section: "a stdio-shape probe matrix (2- vs 3-element arrays × inherit/ignore/pipe) for the child_process adapter change" — described but not committed.
Fix: Commit the probe matrix the PR body already describes as a ~20-line test in test/js/node/child_process/child-process-stdio.test.js: test.each([["pipe","pipe"], ["ignore","pipe"], ["inherit","pipe"]]) → spawn a fast-exit child (e.g. bunExe(), ["-e", "1"]) with that 2-element stdio, await once(child, "exit") FIRST (guarantees post-exit), then read child.stdio[1]/child.stdout and assert it's a Readable that emits "close" (or reads empty) rather than throwing. Verify it fails with USE_SYSTEM_BUN=1 (or against a build with the one-line revert) and passes with bun bd test. Also add the 3-element control row to prove the eager-load path is unchanged.
The #[cfg(not(windows))] gate leaves Windows with the same double-close: get_stdio hands the pipe HANDLE to net.connect({ fd }) (which adopts it into a second uv_pipe_t), yet finalize_streams still uv_closes the original Buffer entry.
child_process.ts:1251-1255 — extra-pipe "pipe" case does NetModule.connect({ fd: handle.stdio[i] }) with NO platform gate. subprocess.rs:840-849 (Windows get_stdio) exposes buffer.fd() (HANDLE via uv_fileno). subprocess.rs:1202-1210 (Windows finalize_streams) still does Box::leak(buffer).close(on_pipe_close) on every StdioResult::Buffer. Listener.rs:1020-1040 → WindowsNamedPipe.rs:814-822 — net.connect({fd}) on Windows reinterprets the number as a HANDLE, make_lib_uv_owned() → uv_pipe_open, creating a second owner. The cfg gate exists only because WindowsStdioResult (spawn/process.rs:1485-1490) has no Unowned variant to flip to — not because Windows is safe. Compare js_bun_spawn_bindings.rs:1448-1476: the IPC-channel path already neutralizes the Windows slot via mem::take → Unavailable with the exact comment "so finalizeStreams can't double-close it" — the same neutralization get_stdio needs. PR body confirms testing was macOS-ASAN only.
Fix: On Windows, neutralize the exposed slots in get_stdio the same way the IPC-channel path already does at js_bun_spawn_bindings.rs:1461-1476: mem::take each StdioResult::Buffer → Unavailable after pushing its fd() (and Box::leak the pipe so JS's net.Socket becomes the sole owner via its own uv_pipe_t), OR add a WindowsStdioResult::UnownedBuffer variant that finalize_streams skips. If Windows is deliberately deferred, drop the #[cfg] silence: state in the PR body that Windows extra-pipe ownership is unfixed and open a tracked follow-up — the current diff reads as "Windows is fine" when it isn't.
The PR's own body names the root cause — hasSocketsToEagerlyLoad reads the raw pre-padding stdio.length — then fixes the downstream assertion instead; a one-line change at line 1349 fixes the whole class and keeps the direct native path.
src/js/node/child_process.ts:1349 const stdioCount = stdio.length reads the RAW user array; bunStdio (line 1333) is already padded to ≥3 by normalizeStdio (lines 1750-1753). With stdio: ["pipe","pipe"], stdioCount=2 so the guard is false, but bunStdio=["pipe","pipe","pipe"] — three real pipes exist. The guard is false ONLY for 0/1/2-element user arrays, all of which get padded with "pipe". Root-cause fix: const stdioCount = bunStdio.length (or delete the guard — bunStdio.length≥3 always). Then line 1425-1428 eager-loads synchronously right after Bun.spawn() returns, $bunNativePtr is guaranteed present, and the original direct constructNativeReadable call at line 1228 never asserts. The adapter path (newStreamReadableFromReadableStream, webstreams_adapters.ts:513-538) adds $inheritsReadableStream + validateObject + Buffer.isEncoding + validateBoolean on EVERY child_process stdout/stderr construction (hot path — line 1228 is reached from get stdout()/get stderr() at 1304-1310, not just #handleOnExit). The wrong guard also gates two OTHER behaviors the PR leaves broken for short arrays: line 1425-1428 .ref() on stdio items (keepalive semantics) and line 1380-1385 onExit nextTick eager-load — so stdio:["pipe","pipe"] still diverges from stdio:["pipe","pipe","pipe"] in ref behavior after this PR.
Fix: Change line 1349 to const stdioCount = bunStdio.length; (or drop the guard and unconditionally eager-load, since bunStdio.length is always ≥3), then revert line 1228 to the direct constructNativeReadable(value, { encoding }) call. If defense-in-depth is wanted for a hypothetical late access, add an inline if (!value.$bunNativePtr) { …destroyed Readable… } branch alongside the existing if (!value) guard at line 1216-1222 rather than routing the hot path through the full adapter's validation layer.
leak:9selectors suppresses Bun's own 6.5K-line CSS selector parser (bun_css::selectors), not just the third-party crate — the mangled substring collides across two unrelated codebases.
test/leaksan.supp:+leak:9selectors with comment "selectors crate global caches (hashbrown tables) — process-lifetime statics". LSan leak: is substring-match against every stack frame (scripts/runner.node.mjs:717 wires it via LSAN_OPTIONS suppressions=). nm build/debug/bun-debug | grep -c 9selectors → 2720 symbols; grep -c 7bun_css9selectors → 1069 of those are Bun's own bun_css::selectors module (src/css/lib.rs:65 pub mod selectors; src/css/selectors/{parser.rs 4400 LOC, selector.rs 1863 LOC, builder.rs}); the remainder are Servo's crates.io selectors reached via lol_html (Cargo.lock:2929-2941 lol_html→selectors). Sample colliding symbol: __RINvCs..._8smallvec10deallocateINtNtNtCs..._7bun_css9selectors6parser15GenericSelector.... The comment's justification is also inaccurate: Servo selectors 0.33.0 has NO lazily-allocated global caches — its only statics are compile-time constants (attr.rs:144 SELECTOR_WHITESPACE: &[char], matching.rs:31 usize, build.rs:23 phf::Set); the FxHashMaps in nth_index_cache.rs/relative_selector/*.rs are per-instance struct fields, not statics. Contrast leak:8once_box: only 13 symbols, all OnceBox<pal::Mutex|Condvar> in private std::sys::sync — that one is as narrow as it can be and only ever hides a fixed-size pthread struct. leak:bun_jsc8debugger: 161 symbols, all in one dev-only module — module-wide but bounded.
Fix: Narrow leak:9selectors to the frame that actually appears in the leak report — almost certainly lol_html's compiled-selector storage: leak:12selectors_vm (lol_html's selectors_vm module; 2133 symbols in nm, zero overlap with bun_css) or leak:8lol_html if the retaining root is lol_html itself. Re-run the failing test to capture the real allocating frame and update the comment to name it — "global caches" is wrong. 8once_box can stay as-is (private std internals, bounded to pthread structs). bun_jsc8debugger optionally narrows to 8Debugger6create per its own comment, but that's a nit.
get_stdio's OwnedFd→UnownedFd downgrade sits on the PUBLIC Bun.spawn().stdio getter, but the "JS wraps it in net.Socket" justification only holds for one internal caller — a Bun.spawn user who reads .stdio without wrapping now leaks the pipe fd.
subprocess.rs:832-833 #[bun_jsc::host_fn(getter)] pub fn get_stdio bound at BunObject.classes.ts:123-125 as stdio: { getter: "getStdio" } (no cache: true) — this IS the public Bun.spawn().stdio getter. Public contract at packages/bun-types/bun.d.ts:7210-7217: readonly stdio: [null, null, null, ...(number | null)[]] — "Entries beyond index 2 are number for \"pipe\" slots" with NO ownership-transfer language. The PR comment (subprocess.rs:864-866) cites only child_process.ts's "pipe" case; grep confirms handle.stdio is read at exactly one src/js/ site (child_process.ts:1253) but the getter is public API. spawn_process.rs:912-933 shows PosixStdio::Buffer (from JS "pipe" string, stdio.rs:281,414-415) creates a socketpair and stores parent side as ExtraPipe::OwnedFd. finalize_streams (subprocess.rs:1215-1216) closes OwnedFd but skips UnownedFd — so post-PR, Bun.spawn({stdio:[..., "pipe"]}); proc.stdio; await proc.exited; leaks the fd (pre-PR it was closed). Tests at spawn.test.ts:832,863 and bun-types/fixture/spawn.ts:71-74 demonstrate the read-for-inspection pattern (they use caller-owned fds so happen not to leak, but establish the usage shape).
Fix: Move the ownership transfer to the actual transfer point: add an internal method (e.g. a $-prefixed takeExtraStdioFd(i) on Subprocess, or a private symbol) that returns the fd AND downgrades that single slot, and call it from child_process.ts:1253 instead of handle.stdio[i]. Leave the public get_stdio getter side-effect-free so finalize_streams still closes Bun-created pipes for direct Bun.spawn users. Alternatively, if the intent is that reading .stdio DOES transfer ownership for "pipe" slots, that must be documented in bun.d.ts:7210 and covered by a Bun.spawn-native test asserting the fd is NOT closed by the subprocess after a .stdio read.
release_runtime_state_js_handles (and rare.s3_default_client.deinit()) is only called from global_exit() — Worker teardown reaches ~VM via WebWorker__teardownJSCVM without ever releasing these Strong handles, so a Worker that used Bun.sql or Bun.s3 hits the same Bun__StrongRef__delete UAF this PR fixes for the main VM.
Hook call site: src/jsc/VirtualMachine.rs:1585-1591 — inside global_exit() (defined :1506), the ONLY caller of release_runtime_state_js_handles and rare.s3_default_client.deinit(). Worker path: src/jsc/web_worker.rs:1229-1252 (step 2 pre-JSC cleanup) calls cron_clear_all_teardown / cancel_all_timers / close_all_socket_groups but NOT the new hook nor s3_default_client.deinit(); :1261 (step 3) WebWorker__teardownJSCVM → src/jsc/bindings/webcore/Worker.cpp:598 vm.derefSuppressingSaferCPPChecking() runs ~VM (comment :594 "~VM runs here"), freeing the HandleSet; :1299 (step 5) (*vm_ptr).destroy() → src/jsc/VirtualMachine.rs:4444 drop(rare) drops RareData.s3_default_client:Strong (src/jsc/rare_data.rs:290) and :4473 deinit_runtime_state → src/runtime/jsc_hooks.rs:592 drop(RuntimeState) drops sql_rare's 4× StrongOptional (src/sql_jsc/mysql/MySQLContext.rs:6-7, src/sql_jsc/postgres/PostgresSQLContext.rs:10-11) → src/jsc/Strong.rs:178-184 Drop → :239 Bun__StrongRef__delete against freed HandleSet. RuntimeState is per-thread/per-Worker (src/runtime/jsc_hooks.rs:110-115 thread_local RUNTIME_STATE), so a Worker using Bun.sql populates its own on_query_resolve_fn. rg global_exit src/jsc/web_worker.rs — no hits (only comments referring to the parent's global_exit).
Fix: Mirror the two new pre-teardown releases into web_worker.rs step 2 (right after close_all_socket_groups at :1251, before WebWorker__teardownJSCVM at :1261): if let Some(rare) = vm.rare_data.as_deref_mut() { rare.s3_default_client.deinit(); } and unsafe { (hooks.release_runtime_state_js_handles)(vm_ptr) } inside the existing if let Some(hooks) block. Alternatively, move both releases into a shared helper called from both global_exit() and worker step 2 so the two "pre-JSC-teardown Strong release" lists cannot drift again. Add a test that spawns a Worker, runs one Bun.sql query (or accesses Bun.s3 defaults), and terminate()s it under BUN_DESTRUCT_VM_ON_EXIT=1 + ASAN.
All 17 new leaksan.supp entries omit the # test/... first-seen anchor that the file's own header comment (line 113) documents as the convention for this section — the PR body says each was "verified against a live repro", so the anchors exist and were simply not written down.
test/leaksan.supp:113 declares the section convention: # file comments below are where it was first seen, not an exhaustive list. git blame shows that header and the 4 pre-existing entries below it (create_ssl_context_from_bun_options, jsc.Debugger.startJSDebuggerThread, jsSQLStatementOpenStatementFunction, WTF::RunLoop::dispatchAfter) each carry a # test/… line, contributed across 3 separate commits by 2 separate authors — an established convention, not one contributor's habit. git diff origin/main -- test/leaksan.supp shows 21 new leak: lines and 0 new # test/ lines. PR body: "17 entries, each verified against a live repro before adding". The WebCore::EventNames::operator new entry is flagged in-file and in the PR body as a probable real leak needing a "ThreadGlobalData teardown follow-up" — yet names no test that surfaces it.
Fix: Prefix each new entry in the documented section with the repro test path, matching the four entries above them — e.g. # test/js/node/test/parallel/test-crypto-subtle-... above leak:WebCore::SubtleCrypto::create. The WTF::ParkingLot::parkConditionallyImpl line (currently the only new entry with neither rationale NOR repro) needs both. The parseImportDeclaration addition in the top uncommented block can stay as-is since that block has no anchor convention.
child_process piped stdout now eagerly evaluates webstreams_adapters + Writable + Duplex (~2040 LOC) on the common path, when the fix only needs the fallback branch that is rare by the diff's own comment.
child_process.ts:1228 swaps require("internal/streams/native-readable") for require("internal/webstreams_adapters"). Tracing top-level requires: (a) child_process.ts:2-13 loads only events/os/shared/validators — no streams. (b) OLD dep native-readable.ts:8-10 loads only readable+destroy. (c) NEW dep webstreams_adapters.ts:9-18 top-level loads primordials, writable, readable, duplex, destroy, utils, shared, validators, end-of-stream. (d) readable.ts:3-28 does NOT load writable or duplex (its adapters require at :1630 is lazy webStreamsAdapters ??= require(...) — same lazy pattern in writable.ts:1099 and duplex.ts:135). Net-new module evaluation on first piped-stdout access = webstreams_adapters (765L) + writable (1122L) + duplex (153L). The call site is reached on every default spawn(): child_process.ts:1115-1122 unconditionally calls #getBunSpawnIo(1/2, …, true) in the exit handler even if the user never touched .stdout. The fast path still ends up loading native-readable anyway via webstreams_adapters.ts:37 tryTransferToNativeReadable when $bunNativePtr is present — so the adapter is pure overhead on the common path. The diff comment itself frames the missing-$bunNativePtr case as the rare one ("after the child exits (lazy spawn)").
Fix: Inline the guard at the call site so the fast path is unchanged and only the rare fallback pays for the adapter: const ptr = value.$bunNativePtr; const pipe = (ptr && ptr !== -1) ? require("internal/streams/native-readable").constructNativeReadable(value, { encoding }) : require("internal/webstreams_adapters").newStreamReadableFromReadableStream(value, { encoding });. Alternatively, relax native-readable's $assert(typeof bunNativePtr === "object") to a return-undefined and keep a single require of native-readable with a fallback — but the inline check is the minimal diff and matches the existing lazy-adapter pattern at readable.ts:1630 / writable.ts:1099 / duplex.ts:135.
leak:bun_jsc8debugger blanket-suppresses the entire 1146-line bun_jsc::debugger module — including per-timer, per-test, and per-connection paths — when a function-specific suppression already exists in the binary and a one-token narrowing is available.
test/leaksan.supp (added line): leak:bun_jsc8debugger with comment scoping intent to "Inspector/debugger server thread (bun_jsc::debugger::Debugger::create) still parked at exit". src/jsc/Debugger.rs is 1146 lines and the module contains, beyond the Debugger struct: BunFrontendDevServerAgent (line 50), free fn did_connect (762), AsyncTaskTracker (782) + free fns did_schedule_async_call/did_cancel_async_call/did_dispatch_async_call/will_dispatch_async_call (870-890), TestReporterAgent (895), LifecycleAgent (1072). did_schedule_async_call is invoked per-setTimeout from src/runtime/timer/mod.rs:170 and src/runtime/timer/timer_object_internals.rs:244 — an unbounded per-operation path whose mangled frame contains bun_jsc8debugger and is now suppressed. src/bun_bin/lib.rs:88 documents "LSAN matches by substring on a symbolized frame". src/bun_bin/lib.rs:131 already ships the narrow, correct entry baked into the binary: "leak:bun_jsc::debugger::Debugger>::start_js_debugger_thread\n" — the new module-wide entry is both broader AND redundant with it. File convention (src/bun_bin/lib.rs:112-115) requires per-entry justification and forbids blanket silencing; every neighbouring new entry in the same PR (WebCore::EventNames::operator new, WebCore::SubtleCrypto::create, JSC::JSONAtomStringCache) is function-specific.
Fix: Narrow to the Debugger struct's inherent methods — leak:bun_jsc8debugger8Debugger (mangled) or, matching the existing built-in convention at src/bun_bin/lib.rs:131, add the specific demangled frame(s) (bun_jsc::debugger::Debugger>::create and, if the built-in start_js_debugger_thread entry isn't matching under the mangled form, its mangled twin 24start_js_debugger_thread). Either keeps AsyncTaskTracker, TestReporterAgent, LifecycleAgent, BunFrontendDevServerAgent, and the free did_*_async_call fns observable. If the broader entry was added because the existing narrow built-in entry (lib.rs:131) stopped matching, say so in the comment and fix the narrow entry rather than w
The unscoped [ FLAKY ] on test-worker-terminate-http2-respond-with-file.js removes it from RELEASE CI too, but the described failure is #[cfg(debug_assertions)]-only — release loses the coverage for a panic it can never hit.
(1) src/runtime/dispatch.rs:600-609 — the "JavaScript functions were called outside of the microtask queue without draining microtasks" panic named in the PR's comment is gated by #[cfg(debug_assertions)]; release builds compile it out. (2) scripts/runner.node.mjs:2004-2019 — every expectations.txt entry becomes a hard SKIP: the filter at 2006-2007 destructures expectations but only tests modifiers, then 2011-2017 splices matching tests out of availableTests. [ FLAKY ] is not retry-to-green; it is do-not-run. (3) scripts/runner.node.mjs:383-420 + .buildkite/ci.mjs:62-74 — modifiers derive from the exec basename; release profile appends no suffix (profile !== "release" guard), so release lanes have no ASAN modifier and match the unscoped entry. (4) The test was added in #18299 "add all already-passing tests" and is picked up by isNodeTest (runner.node.mjs:1745-1759), so release CI runs it today. (5) scripts/build/profiles.ts:178-188 — the ASAN profile sets assertions: true, so [ ASAN ] is the scope where the panic can actually fire. (6) Precedent on origin/main: line 28 [ LINUX-X64-MUSL ] … [ FLAKY ] and line 149 [ DARWIN ] … [ FLAKY ] — build-specific flake is already scoped elsewhere in the same file.
Fix: Prefix the entry with [ ASAN ] to match lines 21-24 (and the origin/main [ LINUX-X64-MUSL ]/[ DARWIN ] precedent for scoped FLAKY). If the flake also bites local bun bd debug runs and the author wants that documented, keep the comment as-is — but CI's only assertions-enabled lane is ASAN, so [ ASAN ] is the correct CI scope. If the runner later grows a DEBUG modifier, that would be the more precise choice; today it does not exist for CI release lanes.
sk_GENERAL_NAME_free is now dead code whose only historical use was the bug this PR fixes, yet it stays pub with a signature that type-checks straight back into the leaking callback slot — CLAUDE.md and the file's own header both say delete it.
Zero callers post-PR: rg -n '\bsk_GENERAL_NAME_free\b' --type rust --type zig --type cpp --type c -g '!**/vendor/**' returns only the two definitions (boringssl.rs:488, boringssl.zig:2966) and three doc-comment mentions — no call sites. The PR removed its only two former callers (boringssl.zig:180/199 and lib.rs:360). Signature is byte-identical to the callback type: sk_GENERAL_NAME_free_func = unsafe extern "C" fn(*mut struct_stack_st_GENERAL_NAME) (boringssl.rs:444) vs pub unsafe extern "C" fn sk_GENERAL_NAME_free(sk: *mut struct_stack_st_GENERAL_NAME) (boringssl.rs:488), so sk_GENERAL_NAME_pop_free(n, sk_GENERAL_NAME_free) still compiles. It is the ONLY sk_*_free container-free wrapper in the entire file (rg 'sk_.*_free\b' boringssl.rs | grep -v '_func\|_element\|call_free\|pop_free' → line 488 alone), so no family-completeness defense. File header (boringssl.rs:1-6): "Hand-rolled BoringSSL FFI surface... exposes only the subset of symbols Bun's Rust crates actually consume — it is not a full bindgen dump." No #[deprecated] attribute present. Recent precedent: #31254 "Restrict crate-internal items to pub(crate) and remove dead code it exposes" touched this exact file.
Fix: Delete sk_GENERAL_NAME_free from src/boringssl_sys/boringssl.rs:487-493 and src/boringssl_sys/boringssl.zig:2966-2969 in this PR (name the deletion in the PR description per CLAUDE.md). If a maintainer insists on keeping the upstream-mirroring wrapper, add #[deprecated(note = "footgun: type-matches sk_GENERAL_NAME_free_func but leaks every element when passed to pop_free — use sk_GENERAL_NAME_element_free")] on the Rust side (and @compileError or a doc-warning on the Zig side) so re-misuse produces a compiler diagnostic rather than a silent ASAN leak.
release_runtime_state_js_handles hard-codes bun_sql_jsc's internal field layout in jsc_hooks.rs; the enumeration belongs on impl RareData in the crate that defines those fields — nothing in the crate graph prevents it.
src/runtime/jsc_hooks.rs:1458-1469 enumerates four dotted paths (state.sql_rare.{mysql,postgresql}_context.on_query_{resolve,reject}_fn.deinit()). The crate graph permits the method to live on the owning type: src/runtime/Cargo.toml:72 has bun_sql_jsc.workspace = true, and src/runtime/jsc_hooks.rs:75 already names bun_sql_jsc::jsc::RareData directly (and constructs it inline at :317). On the other side, src/sql_jsc/Cargo.toml:30 has bun_jsc.workspace = true and src/sql_jsc/jsc.rs:33 already imports StrongOptional, so deinit() is callable there — no cycle. grep -rn "impl RareData" src/sql_jsc/ returns nothing: no such method exists today. The fields being torn down are defined at src/sql_jsc/mysql/MySQLContext.rs:6-7 and src/sql_jsc/postgres/PostgresSQLContext.rs:10-11 — a different crate from where they're enumerated. src/sql_jsc/Cargo.toml also shows bun_runtime is a commented-out (not active) dep, confirming the dependency edge is one-way runtime→sql_jsc.
Fix: Add impl RareData { pub fn release_js_handles(&mut self) { … } } at src/sql_jsc/jsc.rs (next to the struct at :270), optionally cascading through per-context release_js_handles on MySQLContext/PostgresSQLContext so each struct enumerates its own Strongs. Then jsc_hooks.rs:1458-1469 collapses to state.sql_rare.release_js_handles();. Net: one edit site per new SQL backend, in the same file that defines the field.
DYLD_LIBRARY_PATH is hard-set (clobbering any parent value) on all macOS builds, but the repo's existing fix for the identical asan-dyld-shim problem uses DYLD_FALLBACK_LIBRARY_PATH — the actual fallback var — and the shim only exists on ASAN builds anyway.
scripts/runner.node.mjs:702-707 gates only on isMacOS and assigns env.DYLD_LIBRARY_PATH = dirname(realpathSync(execPath)); at :1274-1301 bunEnv = {...process.env, ...} then Object.assign(bunEnv, env) — so any inherited DYLD_LIBRARY_PATH is overwritten. Contrast: test/bundler/compile-sourcemap-internal.test.ts:51 solves the SAME "copied exe can't find asan-dyld-shim.dylib" problem with DYLD_FALLBACK_LIBRARY_PATH: dirname(bunExe()). Node's vendored convention at test/js/node/test/common/shared-lib-util.js:26-27 prepends (existing + ':' + new) rather than overwriting. scripts/build/shims.ts:225 emits the shim only when cfg.darwin && cfg.asan, so non-ASAN macOS runs set the var for a dylib that isn't there. The line-706 comment calling DYLD_LIBRARY_PATH "dyld's documented fallback" is wrong — per man dyld, DYLD_LIBRARY_PATH is searched FIRST (override); DYLD_FALLBACK_LIBRARY_PATH is the last-resort fallback.
Fix: Use DYLD_FALLBACK_LIBRARY_PATH instead (matches test/bundler/compile-sourcemap-internal.test.ts:51, cannot shadow system libs, and the "fallback" wording in the comment then becomes correct). If DYLD_LIBRARY_PATH must stay, prepend to any existing value ([dir, process.env.DYLD_LIBRARY_PATH].filter(Boolean).join(':')) and gate on basename(execPath).includes("asan") like the neighboring ASAN_OPTIONS/LSAN_OPTIONS blocks at :709/:713 so non-ASAN release runs are untouched.
TEST_THREAD_ID=testIndex works for isolation, but crashed/aborted tests leave .tmp.N in the repo checkout — the runner's own per-test tmpdir (already crash-proof-cleaned) could carry this via NODE_TEST_DIR instead, making TEST_THREAD_ID unnecessary.
test/js/node/test/common/tmpdir.js:24-31 — testRoot = NODE_TEST_DIR or __dirname/.. (repo checkout test/js/node/test/), tmpPath = ${testRoot}/.tmp.${TEST_THREAD_ID}. tmpdir.js:42-44 registers process.on('exit', onexit) so normal-exit tests DO self-clean (probe's "never cleaned" premise is wrong). Only ~208/2322 node tests require('../common/tmpdir') (grep count), not 2322. scripts/runner.node.mjs:715 sets ASAN_OPTIONS=…abort_on_error=1 → any ASAN detection is SIGABRT → exit handler skipped → .tmp.N survives in-repo (gitignored at test/js/node/test/.gitignore:8). scripts/runner.node.mjs:1271+1409 — spawnBun already creates a per-test mkdtempSync(tmpdir(), "buntmp-") and rm-rf's it in finally (survives crash/kill), and at :1290 sets TEST_TMPDIR: tmpdirPath with the comment "Used in Node.js tests" — but nothing in test/js/node/test/** reads TEST_TMPDIR; tmpdir.js reads NODE_TEST_DIR. testIndex is unstable across runs (getRelevantTests:1960-2070 applies --include/--exclude/expectations/shard/smoke-random/modified-file-sort) but that is irrelevant to within-run uniqueness, which is all isolation needs.
Fix: In spawnBun's bunEnv (scripts/runner.node.mjs:1290), change the dead TEST_TMPDIR: tmpdirPath to NODE_TEST_DIR: tmpdirPath (the var common/tmpdir.js actually reads). Each test then gets ${buntmp-XXXXXX}/.tmp.0, already unique per test, and the existing finally { rmSync(tmpdirPath) } at :1409 sweeps it even on SIGABRT/SIGKILL. TEST_THREAD_ID: String(testIndex) becomes unnecessary and can be dropped. If any test turns out to depend on tmpdir being a sibling of fixtures/, keep TEST_THREAD_ID but add a rmSync(join(testsPath,'js/node/test'), {glob '.tmp.*'}) sweep after the Promise.all.
The Zig checkX509ServerIdentity the PR body calls "the still-live Zig original" is dead porting-reference source that no build compiles — its fix has zero coverage by definition, and the two .zig hunks touch files already deleted on main.
No build.zig exists; scripts/build/rust.ts:5-6 builds the whole runtime as libbun_rust.a via cargo build -p bun_bin alone. Both src/boringssl/boringssl.zig:61 and src/boringssl/lib.rs:209 export extern "C" fn OPENSSL_memory_alloc — a duplicate-symbol link error if both were ever linked, so only Rust is. Merged PR #32621 (d451445, 2026-06-25, "Remove the .zig porting-reference sources") deleted src/boringssl/boringssl.zig and src/boringssl_sys/boringssl.zig from main and states in its body: "the 1,270 .zig files … are not built (build.zig is gone, no Cargo build.rs or include_str! reads them)". git merge-base --is-ancestor d4514457e83 HEAD on the PR branch returns false — PR #31833 predates the removal, so its two .zig hunks edit files that no longer exist on main. The only reachable caller of the identity check is src/http/lib.rs:1510 → check_x509_server_identity; every checkX509ServerIdentity caller (src/http/http.zig:139, src/http_jsc/websocket_client.zig:211, src/sql_jsc/{mysql,postgres}/*.zig, src/runtime/valkey_jsc/js_valkey.zig) sits in the same never-compiled reference tree. PR body line 14 nonetheless claims the fix was applied to "the still-live Zig original".
Fix: Drop the src/boringssl/boringssl.zig and src/boringssl_sys/boringssl.zig hunks entirely and rebase on main (post-#32621). Keep src/boringssl/lib.rs and src/boringssl_sys/boringssl.rs (the GENERAL_NAME_free binding + sk_GENERAL_NAME_element_free shim). Reword the PR body's SAN-leak bullet to say the fix is in the Rust check_x509_server_identity only — remove "still-live Zig original".
Replaces the type-punned sk_GENERAL_NAME_pop_free trampoline with BoringSSL's exported deep-free. Deletes ~70 lines of transmute/wrapper machinery (sk_GENERAL_NAME_free_func, sk_GENERAL_NAME_element_free, sk_GENERAL_NAME_free, sk_GENERAL_NAME_call_free_func, sk_GENERAL_NAME_pop_free, sk_pop_free_ex, OPENSSL_sk_free_func). Also reverts the .zig porting-reference edits — those files are removed on main and not built.
Every URL__* extern returns +1 (Bun::toStringRef); with a bare bun_core::String return type each caller had to remember an OwnedString::new() wrap or the StringImpl leaked. Three files had independently discovered and half-fixed this. Declaring the extern returns as OwnedString (repr(transparent), ABI-identical) makes Drop handle the deref everywhere. Drops the per-site OwnedString::new() wraps and the manual .deref()s that would now double-free (url::URL::from_string, Request::ensure_url). Also fixes previously-unwrapped leaks at SocketAddress::parse, hosted_git_info to_owned_slice sites, and hosted_git_info_jsc.
hasSocketsToEagerlyLoad was keyed on the raw options.stdio.length, so 2-element arrays skipped eager-load and post-exit stdout access hit the native-readable $bunNativePtr assertion. bunStdio is already padded to >=3; read that instead and drop the webstreams-adapters fallback. The extra-pipe fd double-close is fixed by moving ownership transfer out of the public .stdio getter into a takeStdio() method that child_process.ts calls once at spawn time. get_stdio stays a pure read so Bun.spawn() users who inspect .stdio don't leak fds.
…path Collapses the s3_default_client + sql_rare enumeration into release_strong_refs_before_teardown() and adds the two Strongs the Node suite couldn't reach: overridden_main (Bun.main setter) and entry_point_result.value. The same UAF class hit Worker teardown (WebWorker__teardownJSCVM frees the HandleSet before vm.destroy() drops these); call the helper there too. sql_rare's field enumeration moves onto RareData in bun_sql_jsc so new SQL backends add their handles next to the fields, not in jsc_hooks.
…env fixes - leaksan.supp: 9selectors → 8lol_html (was matching bun_css::selectors, 1069 unrelated symbols); bun_jsc8debugger → 7bun_jsc8debugger8Debugger (was matching per-timer AsyncTaskTracker paths). Add # test/... repro anchors per file convention. - expectations.txt: scope test-worker-terminate-http2-respond-with-file to [ ASAN ] — the panic is #[cfg(debug_assertions)]-only. - runner: DYLD_FALLBACK_LIBRARY_PATH (last-resort, prepended) instead of clobbering DYLD_LIBRARY_PATH on all macOS builds; gate on ASAN. Set NODE_TEST_DIR to the per-test tmpdir so aborted-test .tmp.N dirs are swept by the finally-rmSync.
|
Pushed ab7a099..b340e97 addressing all 19 items — URL getters return OwnedString (fixes SocketAddress + hosted_git_info leaks), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sourcemap_jsc/JSSourceMap.rs (1)
43-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the old URL string before reassignment.
bun_core::StringisCopyand has noDrop, so thefile://branch drops the originalfrom_js(...)ref whensource_url_stringis overwritten.dupe_ref()only adds another ref forpath_from_file_url; it does not release the old one.🤖 Prompt for 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. In `@src/sourcemap_jsc/JSSourceMap.rs` around lines 43 - 70, The file URL handling in JSSourceMap::from_js is leaking the original bun_string_jsc::from_js reference when source_url_string is reassigned in the file:// branch. Before overwriting source_url_string with path.into_inner(), explicitly release the old string/reference held by source_url_string, then continue using the new value from path_from_file_url; keep the fix localized to the source_url_string/source_url_slice flow in JSSourceMap.
🤖 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 `@src/js/node/child_process.ts`:
- Around line 1250-1251: The stdio wrapping logic in ChildProcess’s native stdio
handling is treating fd 0 as missing because of a falsy check, which prevents
wrapping transferred descriptors. Update the check in the code that reads from
this.#nativeStdio so it only returns null when the descriptor is actually
absent, and allow fd 0 to continue into NetModule.connect({ fd }) after
takeStdio() has transferred ownership.
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Line 1068: The console client ownership initialization in ZigGlobalObject
should use the same unique-pointer helper as the rest of the file. Update the
m_ownedConsoleClient assignment in the ZigGlobalObject setup path to construct
the Bun::ConsoleObject through makeUnique instead of a raw new wrapped in
std::unique_ptr, keeping the ownership style consistent with the other
unique-ownership initializations in ZigGlobalObject.
In `@src/jsc/web_worker.rs`:
- Line 1253: Update the shutdown-ordering comment in web_worker teardown flow to
include the new required barrier. In the code around vm.onExit(), resource
cleanup, and teardownJSCVM(), revise the phase list so it explicitly mentions
vm.release_strong_refs_before_teardown() as the step between exit
handlers/cleanup and JSC VM teardown, keeping the documented order aligned with
the actual shutdown sequence.
In `@src/runtime/api/bun/subprocess.rs`:
- Around line 856-859: The Windows stdio adoption path in subprocess handling is
leaving `take` unused, so `finalize_streams()` can still close a handle that JS
has already adopted. Update the extra-stdio branch in
`takeStdio`/`StdioResult::Buffer` handling to neutralize ownership on Windows
the same way the IPC path does, or gate this wrapping behind a Windows-specific
fallback until transfer is implemented. Make the fix symmetrical in
`subprocess.rs` so native and JS do not both own the same pipe.
In `@src/runtime/api/BunObject.classes.ts`:
- Around line 126-131: The `takeStdio` entry in `BunObject.classes.ts` is
currently user-reachable on `Bun.spawn()` results, so it must not remain a
public prototype method. Move this ownership-transfer hook behind an unforgeable
internal token or private symbol, and update the native binding so only
`child_process.ts` can call it. Keep the existing internal behavior for fd
adoption, but make sure the public API surface no longer exposes `takeStdio` to
userland.
In `@test/js/bun/util/bun-main.test.ts`:
- Around line 31-35: The ASAN regression test currently checks stdout before
validating sanitizer output, which can mask the real failure signal. In
bun-main.test.ts, update the assertion order around the proc
stdout/stderr/exitCode handling so the AddressSanitizer stderr check happens
first, then stdout, then signalCode and exitCode; keep the focus on the proc
Promise.all result and ensure sanitizer reporting is asserted before any stdout
expectation.
In `@test/js/node/child_process/child-process-stdio.test.js`:
- Around line 133-137: In the child_process stdio tests around spawn(...) and
once(child, "close"), the 2-element stdio cases are not draining child.stderr
even though fd 2 may still be piped by normalization/defaults. Update the test
setup to read or resume child.stderr alongside child.stdout before awaiting
close, using the existing spawn/once flow so any diagnostic output cannot block
the child.
---
Outside diff comments:
In `@src/sourcemap_jsc/JSSourceMap.rs`:
- Around line 43-70: The file URL handling in JSSourceMap::from_js is leaking
the original bun_string_jsc::from_js reference when source_url_string is
reassigned in the file:// branch. Before overwriting source_url_string with
path.into_inner(), explicitly release the old string/reference held by
source_url_string, then continue using the new value from path_from_file_url;
keep the fix localized to the source_url_string/source_url_slice flow in
JSSourceMap.
🪄 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: 33e44f5a-dcc4-4e29-94b1-08ebbdd117ac
📒 Files selected for processing (28)
scripts/runner.node.mjssrc/boringssl/lib.rssrc/boringssl_sys/boringssl.rssrc/http/lib.rssrc/install/NetworkTask.rssrc/install/hosted_git_info.rssrc/js/node/child_process.tssrc/jsc/URL.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/ConsoleObject.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/web_worker.rssrc/runtime/api/BunObject.classes.tssrc/runtime/api/BunObject.rssrc/runtime/api/bun/subprocess.rssrc/runtime/jsc_hooks.rssrc/runtime/socket/SocketAddress.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/fetch.rssrc/sourcemap_jsc/JSSourceMap.rssrc/sql_jsc/jsc.rssrc/url/lib.rstest/expectations.txttest/js/bun/util/bun-main.test.tstest/js/node/child_process/child-process-stdio.test.jstest/leaksan.supp
Drops the takeStdio ownership-transfer hook: main's socket-fd stdio type (ExtraPipe::UnownedFd) already solves the same double-close, so the subprocess.rs/BunObject.classes.ts/#nativeStdio additions are removed and child_process.ts uses main's path unchanged. The always-true hasSocketsToEagerlyLoad guard is dropped in the same pass. runner.node.mjs keeps this branch's DYLD_FALLBACK_LIBRARY_PATH and NODE_TEST_DIR additions on top of main's runOneTest refactor; the TEST_THREAD_ID hunk is dropped in favor of main's TEST_SERIAL_ID. New OwnedString::new() wraps main added around bun_url/jsc::URL calls in fetch.rs (proxy paths) and Response.rs (redirect) are unwrapped to match the OwnedString return type.
|
Pushed 1b7f0fb — merges main (both CI-failing test files were stale-merge failures already fixed there: OCSP-URI assert in
|
|
Outstanding before merge: 1 unresolved review thread(s). |
The ASAN sweep motivation only applies to POSIX lanes; relocating common/tmpdir.js's testRoot to realpath(%TEMP%) on Windows regressed test-child-process-fork-exec-path.js, test-module-circular-symlinks.js, and test-child-process-execsync.js.
| **Build once, then drive `./build/debug/bun-debug` directly** — don't | ||
| re-run `bun bd` for every probe (a no-op rebuild still costs seconds | ||
| of cargo dep-checking). | ||
|
|
||
| ```bash | ||
| bun bd --revision # build; prints version+hash on success | ||
| ./build/debug/bun-debug -e '<snippet>' # drive it |
There was a problem hiding this comment.
🟡 The new skill's headline instruction — "Build once, then drive ./build/debug/bun-debug directly" (repeated at lines 13 and 20) — contradicts root CLAUDE.md:18, which says "you never invoke ./build/debug/bun-debug directly." Both are agent-facing instruction docs loaded into context, so an agent following one violates the other. Either soften CLAUDE.md's "never" to allow post-build direct invocation for rapid probing, or have the skill use bun bd -e '<snippet>' (which per CLAUDE.md suppresses build output when exec args are present, addressing the same "don't wait on cargo dep-check" concern).
Extended reasoning...
What the finding is
The new .claude/skills/verify/SKILL.md opens with:
Build once, then drive
./build/debug/bun-debugdirectly — don't re-runbun bdfor every probe (a no-op rebuild still costs seconds of cargo dep-checking).
and shows ./build/debug/bun-debug -e '<snippet>' as the canonical probe command at line 13, then again at line 20 (./build/debug/bun-debug -e 'console.log(new Request("https://x").url)').
Root CLAUDE.md (Build Commands section, line 18) says the opposite:
All build scripts support build-then-exec. Any
bun run build*command (andbun bd) accepts trailing args which are passed to the built executable after building — you never invoke./build/debug/bun-debugdirectly.
Both files are agent-facing instruction documents checked into the repo. CLAUDE.md is loaded into every session with the harness preamble "IMPORTANT: These instructions OVERRIDE any default behavior and you MUST follow them exactly as written"; SKILL.md is loaded when /verify is invoked. An agent that reads both is given directly contradictory guidance.
Why the repo's own rules flag this
CLAUDE.md's docs-consistency rule (referenced under Docs, types, and comments in .claude/docs/landing-prs.md) requires: "Sweep the same PR for everything describing the old state. When behavior, names, or contracts change… update or delete: comments beyond the hunk, sibling/mirror implementations, JSDoc, 'see above' cross-references, READMEs, CLAUDE.md." This PR adds a new agent instruction that conflicts with an existing one without updating the existing one.
Step-by-step proof
- Agent invokes
/verifyafter making a runtime change. SKILL.md:7is loaded and instructs: "drive./build/debug/bun-debugdirectly".CLAUDE.md:18is already in context and instructs: "you never invoke./build/debug/bun-debugdirectly".- The agent must either violate the skill's core instruction or violate the root instructions marked as OVERRIDING.
Impact
Documentation-only; zero runtime impact. This is why it's nit severity — the SKILL.md's rationale (avoiding seconds of cargo dep-checking on repeated no-op rebuilds) is legitimate and the guidance is operationally useful; it just conflicts textually with a pre-existing absolute "never" in CLAUDE.md.
Fix
Two options, either is fine:
- Soften
CLAUDE.md:18to allow direct invocation after an initial build, e.g. "preferbun bd <args>— it builds then execs; after the first build in a session you can also drive./build/debug/bun-debugdirectly for rapid probing." - Change SKILL.md to recommend
bun bd -e '<snippet>'instead — perCLAUDE.md"When exec args are present, build output is suppressed unless the build fails", so the only remaining cost is the cargo dep-check the skill wants to avoid. If that overhead is the real concern, the first option is the honest fix.
On the refutations
Both verifier refutations were deduplication votes ("duplicate of bug_001" / "duplicate of bug_003") rather than disagreements on substance — every verifier that evaluated the content confirmed the contradiction is real. The synthesis agent has already merged the duplicates into a single report.
3 participants
Makes the full ported Node.js test suite pass leak-clean under the local ASAN debug runner (LeakSanitizer +
BUN_DESTRUCT_VM_ON_EXIT=1) and fixes the native bugs it surfaces.Started from 1851 failing under the leak-checked runner; every remaining failure was driven to a root cause — seven native bugs fixed, the rest verified one-by-one as OS/JSC exit-time false positives and suppressed with a documented rationale each.
Bug fixes
get_stdiohanded outOwnedFds for stdio whose ownership had already transferred to the JS streams, so both sides closed the fd (EBADF/SIGTRAP under the debug fd guard across ~150 cluster/child_process tests). The getter now downgrades to unowned descriptors on access.Bun::ConsoleObjectand never freed it. Now owned by the global viaunique_ptr, withWTF_OVERRIDE_DELETE_FOR_CHECKED_PTRredeclared on the subclass:ConsoleClientisCanMakeThreadSafeCheckedPtr, and theWTF_DEPRECATED_MAKE_FAST_ALLOCATEDoperator delete otherwise shadows the checked-deletion protocol and trips them_didBeginDeletionassert (JSC's ownJSGlobalObjectConsoleClientdeclares both).RareData.s3_default_clientand the SQL contexts'on_query_resolve/rejectStrongOptionals were dropped duringdestroy(), afterdestructOnExithad already freed the HandleSet — ASAN UAF inBun__StrongRef__delete. The S3 handle is released explicitly inglobal_exitand a newrelease_runtime_state_js_handlesruntime hook releases the SQL handles, both before JSC teardown.check_x509_server_identity(fixed in both the Rust port and the still-live Zig original):sk_GENERAL_NAME_pop_freewas passed the container destructor (sk_GENERAL_NAME_free) as the per-element callback, so eachGENERAL_NAME's nested ASN1 strings leaked on every TLS server-identity check. BindsGENERAL_NAME_freeand passes it through an element-typed shim.ScopeFunctionscloned the nameBunStringfor a borrowing callee and never deref'd the clone — one leaked StringImpl perdescribe/testscope.protocol/username/password/host/pathnamecome back from the URL bindings as +1BunStrings with noDrop; all five are now wrapped inOwnedString.stdioarray,hasSocketsToEagerlyLoad(keyed off the raw user array length, pre-padding) skips the eager load, so a stream first touched in#handleOnExitreachesconstructNativeReadableafter the child exited — when the native stream no longer carries$bunNativePtr— and trips its assert. The construction now goes through the guardednewStreamReadableFromReadableStreamadapter, which transfers natively when possible and falls back toReadableFromWeb.Runner / harness
TEST_THREAD_ID:common/tmpdir.jsderives its directory from it; without it every node test under--parallelshared.tmp.0and raced one test'stmpdir.refresh()(rm -rf) against another's opens — the source of a 49-test "fs cluster" of phantom failures.DYLD_LIBRARY_PATH(macOS): tests that copyprocess.execPath(fork-exec-path, stdin-from-file-spawn) lose the@rpathanchor forasan-dyld-shim.dylib; the runner now exports the build dir as dyld's documented fallback.test-require-builtinstimeout: requires every builtin module, ~60s alone under a local ASAN debug build — right at the old 60s limit; bumped to 120s alongside the existing per-test entries.Leak suppressions (
test/leaksan.supp)17 entries, each verified against a live repro before adding: macOS framework exit-time TLS/caches (ParkingLot, CoreAnalytics XPC, libsystem_info, FSEvents thread, Security.framework keychain via the once-cached system root CAs), JSC VM-lifetime structures pinned at destruct-on-exit (parser-arena identifiers,
JSONAtomStringCache,StructureMemoryManagerbookkeeping, theSubtleCryptowrapper ref-cycle), Rust stdOnceBoxmutex storage,backtrace_symbolsbuffers inside the debug fd-UAF warning dump, and per-workerWebCore::EventNames(flagged in-file as a possible real teardown leak — see follow-ups).Known limitations / follow-ups
test-worker-terminate-http2-respond-with-fileis marked[ FLAKY ]intest/expectations.txt: worker entry-point load racingterminate()panics the debug invariant "JavaScript functions were called outside of the microtask queue without draining microtasks" (tick_queue_with_count, reached fromwait_for_promise_with_termination). Reproduces on an unmodified main binary; timing-dependent — fails reliably on a quiet machine, passes under load.WebCore::EventNamesis not reclaimed when a Worker thread exits. Suppressed for now (bounded by live worker count at exit); wants a ThreadGlobalData teardown pass.--parallelsweeps still surface a rotating handful of load flakes (port/timing-sensitive tests) that all pass serially; CI runs this suite serially.Testing
--node-testssweep (2322 tests, runner driven by a from-source node v26.3.0 build) green serially; parallel sweeps green modulo the load flakes above.test-assert-checktag(bun-test mode) for both Strong UAFs, the TLS SAN tests forGENERAL_NAME_free, and a stdio-shape probe matrix (2- vs 3-element arrays × inherit/ignore/pipe) for the child_process adapter change.