Fix panic when socket handler validation fails in Bun.listen/Bun.connect - #31819
Fix panic when socket handler validation fails in Bun.listen/Bun.connect#31819robobun wants to merge 1 commit into
Conversation
Handlers::from_generated returned errors (non-callable callback, missing data/drain callback) after constructing the Handlers value but before protect(). Dropping that value ran unprotect() on callbacks that were never protected: debug builds panic on the protection_count assertion, and release builds unbalance the JSC GC protection of any callback object protected elsewhere. Read and validate the callbacks into locals first so a Handlers only exists once it can be protected.
|
Warning Review limit reached
More reviews will be available in 100 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:17 PM PT - Jun 4th, 2026
❌ @robobun, your commit 747df35 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31819That installs a local version of the PR into your bun-31819 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #31817, which was opened earlier and covers both validation error paths with an equivalent fix and a subprocess-based regression test. Closing in favor of that PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean reorder so validation errors return before Handlers is constructed and droppable; behavior and error messages unchanged, with a regression test.
Extended reasoning...
Overview
This PR fixes a debug-build panic (and a release-build GC protection imbalance) in Handlers::from_generated (src/runtime/socket/Handlers.rs). The old code constructed a Handlers value with zeroed callback fields, then ran a validation macro that could return Err(...) — at which point Rust's Drop for Handlers would call unprotect() on values that were never protect()ed. The fix reads/validates each callback into a local first (read_callback!), checks the data/drain requirement, and only then constructs Handlers and immediately calls with_async_context_if_needed + protect(). After construction there are no remaining fallible steps before protect(), so the Drop invariant (protection_count > 0) always holds.
A regression test in test/js/bun/net/socket.test.ts exercises both error paths (missing data/drain, non-callable callback) for both Bun.listen and Bun.connect.
Security risks
None. This is a pure control-flow reorder within a constructor; no new inputs are accepted, no parsing changes, and error messages are byte-identical to before. The release-mode side effect being fixed (spurious JSValue::unprotect()) was itself a latent memory-safety hazard, so this is strictly a hardening change.
Level of scrutiny
Low-to-moderate. While socket/GC lifecycle code is sensitive in general, this specific diff is a mechanical refactor: the macro body is identical except it evaluates to a value instead of assigning into result.$field, and the data/drain check uses the same .is_empty() predicate on the same values. No semantic change on the success path; the only behavioral change is that the error path no longer drops a half-built Handlers.
Other factors
- The PR description includes an audit of other
Handlerslifecycle sites confirming this was the only unprotected-drop path. with_async_context_if_needed(the only call between construction andprotect()) is infallible (FFI returningJSValue, notJsResult), so no new gap is introduced.- The bug-hunting system found no issues.
- No CODEOWNERS coverage for this path.
- CI build is in progress; tests look correct and self-contained.
1 participant
What
Fixes a fuzzer-found panic (
assertion failed: self.protection_count > 0) in debug builds:Why
Handlers::from_generatedreturned validation errors (non-callable callback, missingdata/draincallback) after constructing theHandlersvalue but before callingprotect(). Unlike the Zig original (which simply abandons the stack value on the error path), the Rust port gaveHandlersaDropimpl that unconditionally callsunprotect(), so those error returns dropped a never-protectedHandlers:protection_countassertion and panicJSValue::unprotect()on callbacks that were never protected, which unbalances JSC GC protection for any callback object that is protected elsewhere (e.g. the same handler function also used by a live socket)Fix
Read and validate the callbacks from the generated config into locals first, and only construct the
Handlersonce no fallible step remains — so every droppableHandlersis protected. Error messages are unchanged.Audited the other
Handlerslifecycle sites (listen/connect moves,reload,upgradeTLS,mark_inactive, listener finalizer): this was the only path that drops an unprotected instance.Test
Added a regression test to
test/js/bun/net/socket.test.tscovering both error paths forBun.listenandBun.connect; previously the debug build aborted, now they throw the expectedTypeErrors.