install(security-scanner): gate post-start() deref on Ok to avoid over-deref

[robobun]

Follow-up to #30875 (#30875 (comment) and #30875 (comment)).

What

security_scanner.rs over-deref on start() failure

#30875 made StaticPipeWriter::start() refcount-neutral on Err — it now releases its own +1 before returning (static_pipe_writer.rs:196 Windows / :210 POSIX) and leaves started == false. But SecurityScanSubprocess::finish_spawn was still unconditionally releasing the start() ref right after the call, which on the Err path is an over-deref:

step	site	refs
entry	json_writer field + writer_local	2
start()	ref_()	3
start() fails	self-deref (new in #30875)	2
caller	unconditional deref	1 ← over-deref
Err arm	writer_local.deref()	0 → freed
return Err	scopeguard: json_writer.take() → dangling	—
	source.detach()	UAF
	w.deref()	double-free

Fix: gate the deref (and the started = false write) on start_result.is_ok(). On Err, start() has already balanced its own ref and started is already false.

Reachability is limited to PosixBufferedWriter::start failing on a fresh pipe fd (epoll/kqueue ENOMEM/ENOSPC), so there's no practical repro, but the refcount trace is unambiguous.

SSLConfig.rs stale doc comments

Two function-level doc comments adjacent to the block comment rewritten in #30875 still said mi_free / "mimalloc allocation" after dupe_z / free_sensitive moved to bun_alloc::default_alloc. Updated to match.

Verification

cargo check -p bun_install -p bun_runtime clean; bun bd test bun-security-scanner-workspaces.test.ts passes (3/3).

[robobun]

^{Updated 3:17 PM PT - May 21st, 2026}

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

• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 25.04 x64-baseline
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 13 x64-baseline
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 25.04 x64
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 13 x64
• test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 11 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 31032

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

bun-31032 --bun

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6009e3da-7f7e-4d8f-a825-e6e0a28b4761

📥 Commits

Reviewing files that changed from the base of the PR and between 7e40844 and 4daf5f5.

📒 Files selected for processing (2)

• src/install/PackageManager/security_scanner.rs
• src/runtime/socket/SSLConfig.rs

---

Walkthrough

This PR contains two unrelated corrections to runtime and package manager code. The security scanner now guards unsafe refcount operations behind a success check, and the SSL configuration documentation clarifies the allocator contract for a helper function.

Changes

Safety and documentation fixes

Layer / File(s)	Summary
SecurityScanSubprocess refcount guard src/install/PackageManager/security_scanner.rs	In finish_spawn, the intrusive RefCount::<StaticPipeWriter>::deref(writer_ptr) call and started = false reset now occur conditionally only when StaticPipeWriter::start() returns Ok, preventing unsafe state manipulation on error paths.
SSLConfig allocator documentation src/runtime/socket/SSLConfig.rs	Documentation for the zbox_into_raw helper updated to reflect that bun_core::ZBox uses the global default allocator and must be freed with bun_core::free_sensitive, replacing previous mimalloc-specific wording.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main security fix in the changeset: gating a post-start() deref on Ok to prevent over-deref in the security_scanner component.
Description check	✅ Passed	The description comprehensively covers both changes with detailed explanation of the refcount bug fix and stale docs update. It includes verification steps and references to related PRs.
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.}

---

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

[robobun]

Closing per the no-test gate — this one genuinely can't be covered by a regression test:

• security_scanner.rs: the over-deref only fires when StaticPipeWriter::start() returns Err, which on POSIX reduces to PosixBufferedWriter::start → FilePoll::register_with_fd → epoll_ctl(ADD) / kevent failing on a freshly-created socketpair fd. The only real-world triggers are kernel ENOMEM / ENOSPC (max_user_watches); there's no fault-injection hook in the poll layer to force it, and the Ok path is byte-for-byte identical before and after the change so no happy-path test can discriminate.
• SSLConfig.rs: doc-comment-only change (stale mi_free / "mimalloc" references → free_sensitive / "default-allocator"), no runtime behavior.

The refcount trace in the PR body is the proof of correctness. Branch farm/5601c27f/security-scanner-over-deref is left pushed; feel free to reopen + merge.

[claude]

LGTM — verified start() self-derefs on Err (static_pipe_writer.rs:196/210), so gating the caller deref on is_ok() is correct and the Ok path is unchanged.

Extended reasoning...

Overview

Follow-up to #30875 addressing two inline review comments. In security_scanner.rs, wraps the post-start() RefCount::deref + started = false write in if start_result.is_ok(). In SSLConfig.rs, updates two stale doc-comment words (mi_free → free_sensitive, mimalloc → default-allocator) to match the already-updated block comment above them.

Verification

I checked src/spawn/static_pipe_writer.rs:175-228: start() does ref_() up front, and on the Err branch (both Windows :196 and POSIX :210) it self-deref()s and never sets started = true. So on Err the function is already refcount-neutral and started is already false — the caller's unconditional deref was indeed an over-deref, and the PR's refcount trace (2 → 3 → 2 → 1 → 0 → UAF/double-free via the scopeguard) is accurate. On Ok, the gated block runs exactly as before, so success-path behavior is byte-identical.

Security risks

None. No new inputs, no parsing, no auth/crypto surface. The change narrows when an unsafe deref runs; it cannot introduce a new unsafe path.

Level of scrutiny

This is intrusive-refcount code in unsafe Rust, which normally warrants care, but the diff is a 4-line conditional wrapper around existing code with no change to the common (Ok) path. Reachability of the fixed path is limited to PosixBufferedWriter::start failing on a fresh pipe fd (epoll/kqueue ENOMEM/ENOSPC), so risk of regression is very low. The SSLConfig changes are comment-only.

Other factors

This directly closes out two review-comment threads from #30875; cargo check and the security-scanner test suite pass per the description; no bugs were flagged by the bug-hunting pass.

[robobun]

Reopened + rebased onto main (aa018cd onto 832edbc) per Discord — re-verified against current main:

• StaticPipeWriter::start() (static_pipe_writer.rs:183/196/210) is still refcount-neutral on Err
• security_scanner.rs:1394 on main still derefs unconditionally → over-deref on the Err path (trace in PR body holds)
• Other callers already gate on started (subprocess.rs:220) or don't extra-deref (shell/subproc.rs:891)
• SSLConfig.rs: bun_core::dupe_z → default_alloc::malloc (util.rs:4162), so the mi_free/mimalloc comments were stale

No regression test: the Err path needs epoll_ctl(ADD)/kevent to fail on a fresh socketpair fd (kernel ENOMEM/ENOSPC only); no fault-injection hook in FilePoll::register_with_fd, and the Ok path is byte-identical before/after so no happy-path test can discriminate.

cargo check -p bun_install clean on the rebased commit.

[claude]

LGTM — verified StaticPipeWriter::start() self-derefs on Err (static_pipe_writer.rs:196/210), so gating the caller's deref on is_ok() is correct; Ok path is unchanged.

Extended reasoning...

Overview

Two changes, both follow-ups to review comments on #30875:

1. src/install/PackageManager/security_scanner.rs (finish_spawn): wraps the post-start() intrusive deref + started = false reset in if start_result.is_ok(). I verified against src/spawn/static_pipe_writer.rs:183/196/210 that start() takes a +1 at entry and self-releases it on the Err path (both Windows and POSIX), leaving started == false. So the unconditional caller-side deref was indeed an over-deref on Err, and the refcount trace in the PR body (→ UAF in the scopeguard, double-free) is accurate. The Ok path executes the exact same two statements as before, just inside the if — byte-for-byte identical happy-path behavior.
2. src/runtime/socket/SSLConfig.rs: two doc-comment words updated (mi_free → free_sensitive, mimalloc → default-allocator) to match the adjacent block comment that already references bun_core::free_sensitive / default_alloc. Zero runtime impact.

Security risks

None. The refcount fix only affects an error path that requires epoll_ctl(ADD)/kevent to fail with ENOMEM/ENOSPC on a freshly-created socketpair fd — not attacker-reachable, and the change strictly removes a UAF/double-free rather than introducing one. The SSLConfig change is comment-only.

Level of scrutiny

Low. The diff is +8/-4 effective lines, the Ok path is provably unchanged, and the Err-path reasoning is mechanically verifiable against the start() source I checked. No CODEOWNERS entries cover either file.

Other factors

• Bug-hunting system found no issues; CodeRabbit found no actionable comments.
• cargo check and the security-scanner workspace test pass per the PR body / rebase comment.
• The lack of a regression test is well-justified in the timeline (no fault-injection hook for the kernel error, and the Ok path can't discriminate before/after).
• Other StaticPipeWriter::start() callers were already checked by the author (subprocess.rs:220 gates on started, shell/subproc.rs:891 doesn't extra-deref), so this was the only stale call site.

[robobun]

This over-deref was found again independently while working on the POSIX pipe writers, so I re-checked this PR against current main (b555e06), which includes #37774's rework of how StaticPipeWriter releases its start() ref:

• StaticPipeWriter::start() still releases its own +1 and leaves started == false when the underlying writer fails to start (src/spawn/static_pipe_writer.rs:189 on POSIX, :175 on Windows).
• On POSIX that failure is PosixBufferedWriter::start returning the register_with_fd error directly (src/io/PipeWriter.rs:539); no on_error/on_close runs, so the json_writer field still holds its ref at that point.
• finish_spawn on main still derefs unconditionally after start() (src/install/PackageManager/security_scanner.rs:1323) and again in the Err arm (:1328), so the writer is freed while json_writer still points at it and the cleanup guard at :1308 runs source.detach() + deref() on the freed allocation. The trace in the PR body holds unchanged.
• Gating on start_result.is_ok() leaves the Ok path as it is today (which is balanced) and makes the Err path end at refcount 0 via the guard, so the fix is still the right one.

#37898 touches the same lines from the Windows side (it gates the scanner's release on the started token instead), so whichever of the two lands second needs a small rebase here.