socket: don't assert in Handlers::unprotect when never protected#30948
socket: don't assert in Handlers::unprotect when never protected#30948robobun wants to merge 2 commits into
Conversation
Handlers::from_generated returns early with an error when the socket handlers object has no data/drain callback or a non-callable handler. At that point protect() has not been called, so protection_count is 0. Unlike the Zig original where the stack value is simply abandoned on error, Rust runs Drop, which calls unprotect() and trips the debug_assert!(protection_count > 0). unprotect() on JSValue::ZERO (and on unprotected cells) is already a no-op on the C++ side, so release builds were unaffected. Skip the unprotect body when protection_count is 0 in debug builds instead of asserting.
|
Updated 7:15 PM PT - May 17th, 2026
❌ @robobun, your commit 4d87604 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30948That installs a local version of the PR into your bun-30948 --bun |
|
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 (2)
WalkthroughThis PR adds a defensive safeguard to socket handler unprotection and introduces validation tests for socket handler arguments. The runtime fix prevents calling ChangesSocket Handler Safety and Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal debug-only fix for the Drop-before-protect assertion, with regression tests for both error paths.
Extended reasoning...
Overview
This PR fixes a debug-assertion panic in Handlers::unprotect() (src/runtime/socket/Handlers.rs) that fires when Bun.listen/Bun.connect is called with an invalid socket handlers object. The Rust port introduced a Drop impl that runs unprotect() even on the early-error returns in from_generated (before protect() is called), tripping debug_assert!(self.protection_count > 0). The fix replaces the assertion with an early return when protection_count == 0, and adds two tests covering both validation-error paths (no data/drain callback; non-callable handler).
Security risks
None. This is a debug-assertions-only control-flow change in an error path. Release builds are entirely unaffected (the modified block is gated by #[cfg(debug_assertions)]). No auth, crypto, untrusted input parsing, or memory-safety surface is touched.
Level of scrutiny
Low. The change is 4 lines in a #[cfg(debug_assertions)] block plus two straightforward expect().toThrow() tests. I verified that Handlers::unprotect() is private and only called from Drop for Handlers (Handlers.rs:443), so the PR's claim that the double-unprotect case the assertion guarded against cannot arise is correct — Drop runs at most once. When protection_count == 0, all callback fields are either JSValue::ZERO or never-protected values, so skipping the per-field unprotect() calls is correct (and release builds already rely on JSValue::unprotect() being a no-op for these, which this PR does not change).
Other factors
The PR description clearly explains the Zig→Rust porting divergence (stack value abandoned vs. Drop running). The new tests directly exercise the repro from the Fuzzilli finding. No CODEOWNERS apply to these paths, no outstanding reviewer comments, and the bug-hunting system found nothing.
There was a problem hiding this comment.
LGTM — minimal debug-only fix with regression tests; the unrelated spawn-stdout-iterate-leak CI failure looks like flake.
Extended reasoning...
Overview
This PR touches two files: a 4-line change in src/runtime/socket/Handlers.rs replacing a debug_assert!(self.protection_count > 0) with an early return when the count is zero, and two new regression tests in test/js/bun/net/socket.test.ts that exercise the early-error paths in Handlers::from_generated (empty handlers object, non-callable handler).
The root cause is well-explained: when from_generated returns Err before calling protect(), Rust still runs Drop for Handlers on the partially-built result, which calls unprotect() with protection_count == 0 and trips the assertion. The Zig original simply abandoned the stack value with no destructor.
Security risks
None. The modified code is inside a #[cfg(debug_assertions)] block, so release builds are byte-for-byte unchanged. In debug builds the early return skips JSValue::unprotect() calls that were already no-ops (the values are either JSValue::ZERO or never-protected cells, both of which JSC's gcUnprotect handles as no-ops). No GC-rooting imbalance is introduced.
Level of scrutiny
Low. The change is mechanical and tightly scoped. unprotect() is a private method called only from Drop, which runs exactly once per instance, so the double-unprotect scenario the original assertion guarded against cannot occur — protection_count == 0 at Drop time can only mean protect() was never called. The early return is the correct behavior. There is a minor debug/release asymmetry (debug returns early, release falls through to no-op unprotect calls), but both are semantically equivalent.
Other factors
The new tests follow the established pattern in the same file (e.g., the adjacent "empty hostname" / "empty unix path" tests use the same synchronous expect(() => Bun.connect(...)).toThrow() form for validation errors). Error message strings match the source exactly. The single CI failure (spawn-stdout-iterate-leak.test.ts on x64-asan) is in an unrelated subsystem and not touched by this PR. No CODEOWNERS cover these paths, and no outstanding reviewer comments exist.
|
CI status on build #55599 (re-roll): all 73 individual jobs passed, including The aggregate This change is debug-assertions-only in socket |
1 participant
Fuzzilli found a debug-assertion crash in
Bun.listen/Bun.connectwhen thesockethandlers object fails validation.Repro
Root cause
Handlers::from_generatedreturns early with an error when the socket handlers object has nodata/draincallback or contains a non-callable handler. At that pointprotect()has not been called, soprotection_countis still 0.In the Zig original, the stack
resultis simply abandoned on the early error return. In Rust,Drop for Handlersruns →unprotect()→debug_assert!(self.protection_count > 0)fails.Release builds were unaffected: the assertion is
#[cfg(debug_assertions)]-only, andJSValue::unprotect()onZERO/ unprotected cells is a no-op on the C++ side.Fix
Skip the unprotect body when
protection_count == 0in debug builds. There is nothing to unprotect, andDropcan only run once so the double-unprotect case the assertion guarded against cannot arise.