net: throw on an invalid socket handler config instead of aborting#31817
net: throw on an invalid socket handler config instead of aborting#31817alii wants to merge 6 commits into
Conversation
|
@robobun adopt |
|
Updated 8:40 PM PT - Jun 16th, 2026
❌ @robobun, your commit fca1fae has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31817That installs a local version of the PR into your bun-31817 --bun |
WalkthroughSocket handler initialization now validates callback fields before constructing Handlers, requiring at least one of ChangesSocket handler validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
🤖 Adopted and verified on a debug build:
Merge conflict with #31155 resolved in fca1fae: folded the four new handler callbacks ( CI on fca1fae (build 63052): 284 pass, 2 fail — neither touches this PR:
|
|
This PR may be a duplicate of:
🤖 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 — validation is hoisted before construction so the error path no longer drops a never-protected Handlers; behavior on valid configs is unchanged and there's a subprocess regression test.
Extended reasoning...
Overview
This PR fixes a debug-build abort in Handlers::from_generated (src/runtime/socket/Handlers.rs). Previously, the Handlers struct was constructed first and then callbacks were validated via assign_callback!; an invalid (non-callable, non-null) handler returned Err after the struct existed, so Drop → unprotect() ran with protection_count == 0 and tripped a debug_assert!. The fix hoists all validation (per-callback type check + the "at least data or drain" requirement) above the struct literal, then assigns callable handlers afterwards. A subprocess regression test is added to test/js/bun/net/socket.test.ts.
The new "at least data or drain" check (!generated.on_data.is_callable() && !generated.on_writable.is_callable()) is equivalent to the old result.on_data.is_empty() && result.on_writable.is_empty() because the old code only assigned a field when is_callable() held. is_callable() is a pure type query, so calling it twice (once in validate, once in assign) has no side effects.
Security risks
None. This is input-validation ordering for user-supplied socket handler callbacks; no auth, crypto, filesystem, or network surface changes. The error messages and accepted inputs are byte-identical to before.
Level of scrutiny
Low–medium. ~30 lines moved within a single function with no semantic change on the happy path, plus a 25-line test. The transformation is mechanical (validate-then-construct instead of construct-then-validate) and the PR description explains the failure mode precisely. No CODEOWNERS coverage for these paths.
Other factors
The one inline finding is explicitly a pre-existing, cosmetic nit (internal handler names like onWritable in error strings instead of user-facing drain) carried verbatim from the old macro — non-blocking and orthogonal to the abort fix. CI build is in progress; no outstanding human reviewer comments.
…dler regression test
There was a problem hiding this comment.
LGTM — clean validate-before-construct reordering; my earlier nit was non-blocking and the rationale for keeping error text byte-identical is sound.
Extended reasoning...
Overview
This PR fixes a debug-build abort (and release-build unbalanced GC unprotect) when Bun.listen()/Bun.connect() is given an invalid socket handler config. The fix is a pure reordering inside Handlers::from_generated in src/runtime/socket/Handlers.rs: the per-callback type validation and the "at least data or drain" check are hoisted to run before the Handlers struct is constructed, so an early-return error never leaves a half-built Handlers for Drop → unprotect() to trip over. The assign_callback! macro is correspondingly simplified to assign-if-callable. A subprocess regression test in test/js/bun/net/socket.test.ts covers both error paths.
Security risks
None. This is input-validation ordering on a config object; no auth, crypto, permissions, or untrusted-data parsing is involved. If anything, it removes an unbalanced unprotect() on the release path, which is strictly safer.
Level of scrutiny
Moderate — it touches the JSValue protect/unprotect lifecycle in core socket setup, but the change is mechanical: identical validation predicates (is_undefined_or_null, is_callable) and identical error strings, just evaluated earlier. The hoisted "at least one of data/drain" check (!generated.on_data.is_callable() && !generated.on_writable.is_callable()) is equivalent to the old post-assign is_empty() check given the per-field validation above has already passed. Valid-config behavior is unchanged, which robobun verified on a debug build along with confirming the regression test fails pre-fix and passes post-fix.
Other factors
- No CODEOWNERS cover the modified paths.
- The bug-hunting system found no issues.
- My only prior comment was a cosmetic nit about internal vs. user-facing handler names in error messages; the author declined with a reasonable rationale (keep byte-identical to released/Zig behavior, out of scope for a crash fix) and the thread is resolved.
- A possible duplicate (#31360) was flagged by automation, but that is a merge-coordination question, not a correctness concern for this diff.
There was a problem hiding this comment.
LGTM — clean validate-before-construct reorder; success path is unchanged and the error path now throws instead of tripping the Drop assert.
Extended reasoning...
Overview
This PR touches one function, Handlers::from_generated in src/runtime/socket/Handlers.rs, plus a subprocess regression test in test/js/bun/net/socket.test.ts. The change hoists handler-callback validation (each field is undefined/null or callable; at least one of data/drain is present) to before the Handlers struct is constructed, so an invalid config returns ERR_INVALID_ARG_TYPE while there is nothing to drop. Previously the error was returned after construction, so Drop → unprotect() ran on a never-protect()ed Handlers, hitting debug_assert!(protection_count > 0) (debug abort) and unbalancing GC protect/unprotect in release.
Behavioral equivalence
For valid configs the success path is byte-identical: the same fields are assigned (is_callable() on a value already known to be undefined/null-or-callable is equivalent to the old !is_undefined_or_null() && is_callable() branch), then with_async_context_if_needed() and protect() run as before. For invalid configs the same error messages are produced (the literals were carried over verbatim — my earlier nit about internal vs user-facing names was reasonably declined as out of scope, and that thread is resolved). The "at least data or drain" check on !generated.on_*.is_callable() is equivalent to the old result.on_*.is_empty() since validation has already excluded non-callable non-nullish values by that point.
Security risks
None. This is input-validation reordering on a user-facing config object; no auth, crypto, permissions, or trust-boundary changes. The net effect is replacing a process abort with the intended catchable TypeError.
Level of scrutiny
Low–medium. The change is mechanical ("validate early, allocate late"), confined to a single constructor, and covered by a subprocess regression test that fails pre-fix and passes post-fix on a debug build (verified in the adoption comment). No CODEOWNERS entries match these paths. CI failures on this build are unrelated infrastructure issues (musl LTO data-layout link error, macOS x64 build script), not caused by this diff.
Other factors
The bug-hunting system found nothing. The only prior review feedback (mine) was a pre-existing cosmetic nit, now resolved. The extended test (99e7ea3) covers both error paths. This supersedes #31360 with the same approach plus broader test coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/bun/net/socket.test.ts`:
- Around line 1589-1594: The test currently destructures proc.stdout,
proc.stderr, and proc.exited into stdout, stderr, and exitCode but never asserts
stderr; add an explicit assertion to ensure no abort output by asserting stderr
is empty (e.g., expect(stderr).toBe("") ) before asserting exitCode, updating
the test around the proc destructuring and the expectations for stdout/exitCode
(variables: proc, stdout, stderr, exitCode) to make the no-abort contract
explicit.
🪄 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: 90a9e0d1-2b59-44bd-ac93-765a812dd8de
📒 Files selected for processing (1)
test/js/bun/net/socket.test.ts
There was a problem hiding this comment.
LGTM — validation moved before construction, error paths byte-identical, valid path unchanged; my earlier nit was non-blocking and the keep-as-is rationale is sound.
Extended reasoning...
Overview
This PR touches one function, Handlers::from_generated in src/runtime/socket/Handlers.rs, plus a regression test in test/js/bun/net/socket.test.ts. The change hoists the per-callback type checks and the "at least data or drain" check to before the Handlers struct is constructed, so an invalid config returns Err while there is nothing to Drop. Previously the error returned after construction, so Drop → unprotect() ran on a Handlers that protect() had never rooted, tripping debug_assert!(protection_count > 0) (debug abort) and doing an unbalanced GC unprotect in release.
I verified the new logic is equivalent to the old on every path: the validation predicate (!is_undefined_or_null() && !is_callable()) is unchanged; the "at least one of" check now tests !generated.on_*.is_callable() instead of result.on_*.is_empty(), which is equivalent because after validation a non-undefined/null value is callable; and assign_callback! now gates on is_callable(), which is equivalent to the old !is_undefined_or_null() once validation has passed. Error message text is byte-identical to before. The valid-config path (with_async_context_if_needed → protect() → Ok) is untouched.
Security risks
None. This is input validation ordering for a user-facing config object; no auth, crypto, permissions, or untrusted-data parsing is involved. The change strictly removes an unbalanced GC unprotect on the error path.
Level of scrutiny
Medium — it sits in GC-adjacent socket runtime code, but the change is a pure reordering that makes the error path strictly safer (nothing constructed → nothing to clean up). The diff is ~30 net lines in a single function, mechanical, and well-commented. A subprocess regression test covers both error paths and would catch a re-introduction of the abort.
Other factors
- Bug-hunting system found nothing.
- My prior inline comment was an explicitly non-blocking, pre-existing nit about internal vs. user-facing names in the error strings; robobun's reply (keep byte-identical with the Zig reference, rename is a separate user-visible change) is reasonable and the thread is resolved.
- CI red is
test/cli/install/bunx.test.tsfailing simultaneously on all 15 lanes (network-dependent, reproduced on unpatched bun) and a Windows baseline-verify setup failure — neither touches theBun.listen/Bun.connecthandler-validation path;socket.test.tspasses everywhere. - No CODEOWNERS entry covers
src/runtime/socket/Handlers.rs.
There was a problem hiding this comment.
LGTM — clean validate-before-construct reordering with a regression test; error text unchanged.
Extended reasoning...
Overview
This PR reorders Handlers::from_generated in src/runtime/socket/Handlers.rs so that all socket-handler callbacks are validated before the Handlers struct is constructed. Previously, validation errors returned after the struct existed, so Drop ran unprotect() on a Handlers that protect() had never rooted — tripping debug_assert!(protection_count > 0) (debug abort) and doing an unbalanced GC unprotect in release. The fix splits the old assign_callback! macro into a pre-construction validate_callback! pass and a post-construction assign_callback! pass, and moves the "at least data or drain" check up as well. A subprocess regression test is added to test/js/bun/net/socket.test.ts.
Security risks
None. This is input-validation reordering inside the Bun.listen/Bun.connect config path. No new surface is exposed; the same inputs that threw before still throw with byte-identical messages, and valid configs are unchanged. No auth, crypto, filesystem, or network-trust logic is touched.
Level of scrutiny
Low–medium. The diff is small and mechanical: the validation predicates (is_undefined_or_null, is_callable) and error strings are carried over verbatim from the removed assign_callback! body, and the "at least data/drain" check is the equivalent !is_callable() && !is_callable() form of the old is_empty() && is_empty() check (equivalent because, after validation, a field is either undefined/null or callable). Once result is constructed there is no remaining fallible path before protect() + Ok(result), which is exactly the invariant the comment describes.
Other factors
- Bug hunter found nothing. My only prior comment was a non-blocking nit about user-facing vs internal handler names in the error text, which was deliberately kept as-is to stay byte-identical with the Zig reference; that thread is resolved.
- CodeRabbit's stderr-assertion suggestion was declined per the established
void stderrconvention in this file; resolved. - robobun verified on a debug build that both invalid-config paths now throw catchable
ERR_INVALID_ARG_TYPE(pre-fix: abort), and that the new test fails pre-fix / passes post-fix. The two red CI groups were analysed as main-wide / network-cert flake unrelated to this change. - No CODEOWNERS match for the touched paths.
Resolved conflict in src/runtime/socket/Handlers.rs: #31155 added four new handler callbacks (session, keylog, serverName, alpnCallback). Folded them into the validate-before-construct scheme so they are type-checked before the Handlers struct exists and assigned via the infallible single-arg macro, keeping the error path abort-free.
There was a problem hiding this comment.
LGTM — clean reorder of validation before construction; happy path is byte-identical and the error path no longer drops a never-protected Handlers.
Extended reasoning...
Overview
This PR touches one function, Handlers::from_generated in src/runtime/socket/Handlers.rs, plus a new subprocess regression test in test/js/bun/net/socket.test.ts. The change hoists all callback type validation (the per-field "must be a function" check and the "at least data or drain" check) to before the Handlers struct is constructed, so an invalid config returns Err while there is nothing to Drop. Previously the error returned after construction, so Drop → unprotect() ran on a struct that protect() had never rooted — tripping debug_assert!(protection_count > 0) in debug and doing an unbalanced GC unprotect in release.
Correctness
I verified the happy path is behaviorally identical: the new assign_callback! assigns when is_callable(), which is equivalent to the old "not undefined/null and callable" given validation already rejected non-null-non-callable values. The "at least one of" check moved from result.on_*.is_empty() to !generated.on_*.is_callable(), which is equivalent under the same precondition. After construction there are no remaining fallible paths before protect(), so a constructed Handlers is always protected before it can drop. The merge with #31155 correctly folded the four new TLS callbacks (session, keylog, serverName, alpnCallback) into both the validate and assign blocks.
Security risks
None. This is input-validation reordering with no change to what is accepted or rejected; error messages are byte-identical to released behavior.
Level of scrutiny
Moderate — it touches GC protect/unprotect bookkeeping, but the change strictly removes an unbalanced-unprotect path rather than adding new GC interaction. The diff is small (~70 lines in one function), mechanical, and well-commented.
Other factors
All inline review threads are resolved: my prior nit about internal vs. user-facing names in the error strings was deliberately deferred as out-of-scope (keeps the Rust port diffable against the Zig reference), and CodeRabbit's stderr-assertion suggestion was correctly declined per the file's established void stderr pattern for abort-regression subprocess tests. The regression test covers both validation error paths and would fail on pre-fix main. No bugs were found by the bug-hunting system.
2 participants
Bun.listen()/Bun.connect()with an invalid socket-handler config (a non-function handler, e.g.socket: { data: 123 }) aborted the debug process instead of throwing: validation returned an error after theHandlerswas constructed, so itsDropranunprotect()on aHandlersthatprotect()had never rooted, trippingdebug_assert!(protection_count > 0). (In release the assert is gone, but the same path still unprotects callbacks that were never protected — an unbalanced GC unprotect.)Validate every handler before constructing the
Handlers, so an invalid config returnsERR_INVALID_ARG_TYPEwhile there is nothing to drop or unprotect ("validate early, allocate late"). Valid configs are unchanged.Verified on a debug build: a bad handler now throws a catchable
ERR_INVALID_ARG_TYPE(no abort), a valid listener still opens/accepts. Adds a regression test that runs in a subprocess, so the pre-fix abort surfaces as a non-zero exit (the test fails on pre-fixmain, passes after).