Repository navigation
Fix assertion failure in Bun.dns.setServers with non-int32 values - #28926
Conversation
|
Updated 3:35 PM PT - Apr 6th, 2026
❌ @robobun, your commit a3b774b has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28926That installs a local version of the PR into your bun-28926 --bun |
WalkthroughReplaces direct integer conversion with Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/dns/resolve-dns.test.ts`:
- Around line 126-140: Update the two tests that expect throws to assert
TypeError specifically: in the tests "triple with non-int32 family (double)
should not crash" and "triple with missing family (undefined) should not crash",
replace the generic expect(...).toThrow() with expect(...).toThrow(TypeError)
when invoking dns.setServers, and consider renaming the "missing family" test
title to reflect that it should throw a TypeError; keep the "missing port" test
unchanged (dns.setServers referenced in all three tests).
🪄 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: b5faadcd-5331-478c-ab8d-a83f6ce6d4d3
📒 Files selected for processing (2)
src/bun.js/api/bun/dns.zigtest/js/bun/dns/resolve-dns.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, correct fix for the Fuzzilli-found assertion crash.
Extended reasoning...
Overview\nThis PR modifies two files: (2 lines changed) and (22 lines added). The fix replaces with for reading the and fields out of each DNS server triple, preventing an assertion crash when non-int32 JS values (doubles, undefined) are passed.\n\n### Security risks\nNone. The change narrows the crash surface and does not affect any auth, permission, or data exposure path. Invalid families still get rejected with a TypeError.\n\n### Level of scrutiny\nLow. This is a two-line targeted fix in a DNS utility function, found by Fuzzilli and directly addressing the described assertion failure. The existing guard remains unchanged and still enforces valid input. The tests clearly exercise the repro case and the surrounding edge cases.\n\n### Other factors\nTwo nit-level issues were flagged: (1) test global state is mutated without cleanup — low practical risk since test files run in isolated processes and the mutating tests are last in the file; (2) port is coerced before family is validated, meaning a doubly-invalid call (bad family + throwing port) gets the wrong error — this requires two simultaneous bad inputs and the call still fails either way. Neither issue affects correctness of the crash fix. The PR is narrow, well-tested, and follows established patterns.
| // undefined port coerces to 0, which is a valid int32 | ||
| // @ts-expect-error | ||
| expect(() => dns.setServers([[4, "8.8.8.8"]])).not.toThrow(); | ||
| }); | ||
|
|
||
| test("triple with missing family (undefined) throws TypeError", () => { | ||
| // @ts-expect-error | ||
| expect(() => dns.setServers([["8.8.8.8"]])).toThrow(TypeError); | ||
| }); | ||
|
|
||
| test("valid triple should succeed", () => { | ||
| expect(() => dns.setServers([[4, "8.8.8.8", 53]])).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Two tests in the new setServers describe block call dns.setServers() successfully and permanently mutate global DNS resolver state without any cleanup. A simple beforeAll/afterAll to save and restore the original servers would prevent future tests added after this block from silently using 8.8.8.8 instead of the system resolver.
Extended reasoning...
What the bug is and how it manifests
Two tests in the setServers describe block actually change the global DNS resolver configuration without restoring it:
'triple with missing port (undefined) should not crash'callsdns.setServers([[4, '8.8.8.8']])— sets the resolver to 8.8.8.8 with port 0'valid triple should succeed'callsdns.setServers([[4, '8.8.8.8', 53]])— sets the resolver to 8.8.8.8:53
Neither test restores the original servers, and there is no afterAll/afterEach hook in the setServers describe block.
The specific code path that triggers it
At test/js/bun/dns/resolve-dns.test.ts lines 134–135 and 143, the calls to dns.setServers() succeed (the tests assert .not.toThrow()), which internally calls the Zig setServers function that writes to the global Resolver singleton. There is no corresponding call to restore the original state.
Why existing code doesn't prevent it
dns.getServers() exists (dns.zig:3198) and could be used to capture the original state before the block runs. The test file simply does not use it. The two tests that call setServers without throwing were designed to verify the crash fix, but the side effect on global state was not addressed.
What the impact would be
Currently the impact is minimal: Bun's test runner isolates each test file in its own process, and the setServers describe block is placed last in the file — after all the DNS lookup tests. So neither cross-file contamination nor within-file contamination occurs today.
However, any test added after line 144 (or inside the setServers describe block after the mutating tests) would silently use 8.8.8.8 as the DNS resolver instead of the system resolver, potentially causing flaky or incorrect results in environments where 8.8.8.8 is unreachable or returns different answers.
How to fix it
Add a beforeAll/afterAll pair inside the setServers describe block:
describe('setServers', () => {
let originalServers: string[];
beforeAll(() => { originalServers = dns.getServers(); });
afterAll(() => { dns.setServers(originalServers); });
// ... tests ...
});Step-by-step proof
- Test runner starts the file in a fresh process; system DNS resolver is active (e.g. 192.168.1.1).
- All
lookup()tests (lines 12–123) run successfully against the system resolver. 'triple with missing port'runs —dns.setServers([[4, '8.8.8.8']])succeeds; global resolver is now 8.8.8.8:0.'valid triple should succeed'runs —dns.setServers([[4, '8.8.8.8', 53]])succeeds; global resolver is now 8.8.8.8:53.- No cleanup runs. If a developer later adds a
test('resolves example.com')after line 144, it will query 8.8.8.8 instead of the system resolver, with no indication in the test output of why.
| const family = try (try triple.getIndex(globalThis, 0)).coerceToInt32(globalThis); | ||
| const port = try (try triple.getIndex(globalThis, 2)).coerceToInt32(globalThis); | ||
|
|
||
| if (family != 4 and family != 6) { | ||
| return globalThis.throwInvalidArguments("Invalid address family", .{}); |
There was a problem hiding this comment.
🟡 Port is coerced at line 3294 before family is validated at lines 3296–3298; if an invalid family is passed alongside a port value that throws during coerceToInt32 (e.g., a BigInt like 53n or an object with a throwing valueOf), the port exception fires instead of the expected 'Invalid address family' TypeError. The fix is to validate family immediately after coercing it, before touching the port. Separately (pre-existing, unrelated to this PR): no port range check exists, so values like -1 or 70000 silently reach c-ares and get cast to uint16_t.
Extended reasoning...
Ordering issue (new, nit)
The PR changed both family and port from toInt32() to coerceToInt32(). The key difference is that coerceToInt32 can propagate JavaScript exceptions (e.g., if the value is a BigInt or an object whose valueOf getter throws), whereas the old toInt32() only worked on values already confirmed to be isInt32.
In the updated code, the reads are:
const family = try (try triple.getIndex(globalThis, 0)).coerceToInt32(globalThis); // line 3293
const port = try (try triple.getIndex(globalThis, 2)).coerceToInt32(globalThis); // line 3294 ← port coerced HERE
// ... then:
if (family != 4 and family != 6) { // line 3296
return globalThis.throwInvalidArguments("Invalid address family", .{}); // line 3297
}Step-by-step proof of incorrect error:
- Caller invokes
dns.setServers([[999, 'addr', 53n]])— family999is invalid, port is a BigInt. - Line 3293 coerces family
999→999(succeeds, no exception). - Line 3294 coerces port
53n(BigInt) →coerceToInt32throws aTypeError: Cannot convert a BigInt value to a number. - The error propagates; lines 3296-3298 are never reached.
- The caller receives a BigInt coercion error instead of
'Invalid address family'.
Why existing code doesn't prevent it: Before the PR, toInt32() would assert/crash on non-int32 port values, so the ordering was irrelevant—both invalid inputs caused an abort. Now that coerceToInt32 can throw cleanly, the ordering matters for which error the caller sees.
Impact: Low. Requires two simultaneous invalid inputs (bad family + exotic port), and the call still fails with an error either way. Normal usage (integer ports) is unaffected. This is a nit-level ordering issue.
Fix: Move the family validation block to immediately after line 3293 (family coercion), before port coercion:
const family = try (try triple.getIndex(globalThis, 0)).coerceToInt32(globalThis);
if (family != 4 and family != 6) {
return globalThis.throwInvalidArguments("Invalid address family", .{});
}
const port = try (try triple.getIndex(globalThis, 2)).coerceToInt32(globalThis);Addressing the refutation
One verifier noted the scenario is contrived (bad family + throwing port simultaneously). This is correct and is why the severity is nit rather than normal. In all realistic DNS configuration, port is a plain integer. However, the ordering issue is now externally observable (the old crash is gone), so it's worth a one-line fix.
Port range validation (pre-existing, unrelated)
The struct fields udp_port/tcp_port in struct_ares_addr_port_node are c_int, so values like -1 or 70000 pass through to c-ares without validation. c-ares then casts them to uint16_t internally, silently wrapping -1 to 65535. This lack of range validation existed before this PR (the old toInt32() also had no range check). The PR's change from toInt32() to coerceToInt32() marginally expands the set of values that reach this path (e.g., large doubles now coerce instead of crashing), but the root issue is pre-existing. Port range validation (0 <= port <= 65535) would be a separate improvement.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/dns/resolve-dns.test.ts`:
- Around line 131-135: Rename the test title to accurately reflect the assertion
behavior and avoid misleading wording: change the test description for the case
that calls dns.setServers([[4, "8.8.8.8"]]) from "triple with missing port
(undefined) should not crash" to "triple with missing port (undefined) should
not throw"; keep the existing assertion (.not.toThrow()) and the test body using
dns.setServers, and if port-range validation is desired later, open a follow-up
to add explicit checks (e.g., reject port 0) in the DNS parsing code rather than
modifying this test now.
🪄 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: 3f51b4bd-af84-495e-a06d-e93affe81b95
📒 Files selected for processing (1)
test/js/bun/dns/resolve-dns.test.ts
| test("triple with missing port (undefined) should not crash", () => { | ||
| // undefined port coerces to 0, which is a valid int32 | ||
| // @ts-expect-error | ||
| expect(() => dns.setServers([[4, "8.8.8.8"]])).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Test title and validation gap: distinguish "crash" from "throw" and consider port validation.
Two concerns:
-
Minor: Title inconsistency. The test title says "should not crash" but the assertion is
.not.toThrow(). A crash (assertion failure/process abort) is different from a controlled exception. Consider renaming to "triple with missing port (undefined) should not throw" or similar to match the assertion. -
Major: Port 0 is silently accepted. The comment notes that
undefinedcoerces to0, which is a valid int32—but port0is not a valid DNS server port. From the context snippets indns.zig, the coerced port value is stored directly inudp_port/tcp_portwithout range validation, unlikefamilywhich has explicit 4/6 checks. This test correctly documents current behavior, but that behavior itself is questionable: accepting port 0 could lead to runtime issues downstream.
Suggested fix for title
- test("triple with missing port (undefined) should not crash", () => {
+ test("triple with missing port (undefined) should not throw", () => {Note: The PR scope is fixing the assertion crash, not adding port validation. If port validation is intended, consider opening a follow-up issue to validate port ranges (e.g., 1-65535) and reject port 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/dns/resolve-dns.test.ts` around lines 131 - 135, Rename the test
title to accurately reflect the assertion behavior and avoid misleading wording:
change the test description for the case that calls dns.setServers([[4,
"8.8.8.8"]]) from "triple with missing port (undefined) should not crash" to
"triple with missing port (undefined) should not throw"; keep the existing
assertion (.not.toThrow()) and the test body using dns.setServers, and if
port-range validation is desired later, open a follow-up to add explicit checks
(e.g., reject port 0) in the DNS parsing code rather than modifying this test
now.
| test("valid triple should succeed", () => { | ||
| expect(() => dns.setServers([[4, "8.8.8.8", 53]])).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🟡 The 'valid triple should succeed' test at line 143 calls dns.setServers([[4, "8.8.8.8", 53]]) without a // @ts-expect-error directive, but setServers is not declared in the Bun DNS namespace TypeScript types. The other three tests in the same describe("setServers") block all have // @ts-expect-error, making this test inconsistent and causing tsc --noEmit to fail with TS2339. Add // @ts-expect-error on the line immediately before the expect(...) call to match the surrounding pattern.
Extended reasoning...
What the bug is
The test at line 143 calls dns.setServers([[4, "8.8.8.8", 53]]) directly, but setServers is absent from the Bun DNS namespace TypeScript types (packages/bun-types/bun.d.ts only declares lookup, prefetch, getCacheStats, and the ADDRCONFIG/ALL/V4MAPPED constants). TypeScript resolves dns as typeof Bun.dns, which does not include setServers, so the call produces TS2339: Property 'setServers' does not exist on type 'typeof dns'.
The specific code path that triggers it
The test file imports { dns } from "bun" and the test tsconfig.json uses "types": ["../packages/bun-types"]. When tsc --noEmit is run on the test suite (via the typecheck script in both root and test/package.json), the compiler resolves dns against the bun-types declarations. Because setServers has no declaration there, line 143 is flagged as a type error.
Why existing code doesn't prevent it
The three other tests in the same block — for the non-int32 family, the missing port, and the missing family cases — all correctly suppress the error with // @ts-expect-error. The fourth test, added to verify that a valid triple succeeds, was simply written without the directive. There is no lint rule or other mechanism that would automatically enforce the directive's presence.
What the impact would be
Running bun run typecheck (or tsc --noEmit) on the test directory will exit with a non-zero status, failing any CI job that performs a type-check pass. Runtime test execution is unaffected because Bun strips types without checking them.
How to fix it
Add // @ts-expect-error on the line immediately before expect(() => dns.setServers(...)) in the 'valid triple should succeed' test, exactly as done in the three preceding tests.
Step-by-step proof
dnsis typed astypeof Bun.dnsfrompackages/bun-types/bun.d.ts.bun.d.tsnamespacednsdeclares onlylookup,prefetch,getCacheStats, and the flag constants — nosetServers.tscseesdns.setServers(...)at line 143 and emits:error TS2339: Property 'setServers' does not exist on type 'typeof dns'.- The three tests at lines 127, 133, 138 each have
// @ts-expect-errordirectly above thedns.setServerscall, suppressing TS2339 for those lines. - Line 143 has no such directive, so the type error is unhandled and
tsc --noEmitfails.
| test("triple with missing family (undefined) throws TypeError", () => { | ||
| // @ts-expect-error | ||
| expect(() => dns.setServers([["8.8.8.8"]])).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
🟡 The test named "triple with missing family (undefined) throws TypeError" (line 137) is misnamed: the triple passed is ["8.8.8.8"], so index 0 (the family slot) holds the string "8.8.8.8", not undefined. The test exercises the "string in family position" path, not the "missing/undefined family" path. Rename the test and/or add a separate test with [[undefined, "8.8.8.8", 53]] to actually cover the undefined-family case.
Extended reasoning...
The test at line 137 is:
test("triple with missing family (undefined) throws TypeError", () => {
// @ts-expect-error
expect(() => dns.setServers([["8.8.8.8"]])).toThrow(TypeError);
});What the bug is. The triple format for setServers is [family, address, port]. The call dns.setServers([["8.8.8.8"]]) creates the array ["8.8.8.8"]. At index 0 (the family slot), getIndex returns the string "8.8.8.8" — not undefined and not missing. The test name incorrectly claims the family is "missing (undefined)".
The specific code path that triggers it. coerceToInt32("8.8.8.8") converts the string through JS ToNumber("8.8.8.8") → NaN, then NaN|0 → 0. The family value 0 is then rejected by the family != 4 and family != 6 check in dns.zig, throwing TypeError: Invalid address family. So the test passes — but for the wrong documented reason.
Why the name matters. A developer reading the test suite would conclude that the "undefined family" edge case is covered. It is not. A truly missing/undefined family would require a triple like [[undefined, "8.8.8.8", 53]] — where index 0 is explicitly undefined. That case goes through coerceToInt32(undefined) → ToNumber(undefined) → NaN → 0, which is also rejected. The behavior is the same, but the test does not prove that path is exercised.
What the impact would be. The test gives a false sense of coverage. If a future refactor handled undefined specially (e.g., returning an early error before the int32 coercion), the "string in family" path would still pass but the "undefined family" path would have no coverage, and no one would notice because the test name implies it is covered.
How to fix it. Rename the test to something like "triple with string in family slot throws TypeError" to accurately describe what is being tested. Optionally add a separate test with [[undefined, "8.8.8.8", 53]] to genuinely cover the undefined-family path.
Step-by-step proof:
- dns.setServers([["8.8.8.8"]]) is called.
- The outer array has one element: ["8.8.8.8"] — this is triple.
- triple.getIndex(globalThis, 0) returns the string "8.8.8.8".
- coerceToInt32("8.8.8.8") → NaN|0 → 0.
- family = 0, which is neither 4 nor 6 → throwInvalidArguments("Invalid address family").
- The test passes, but triple[0] was "8.8.8.8", not undefined. The test name is factually wrong.
|
CI failures are unrelated flakes on linux-x64-asan:
Neither test touches |
|
All 4 CI failures are pre-existing flakes. Same pattern ( |
…en-sh#28926) `Bun.dns.setServers` called `toInt32()` on the family and port elements of each server triple. The internal `toInt32()` helper falls through to `JSC__JSValue__toInt32`, which calls `JSValue::asInt32()` and asserts `isInt32()`. Passing a double (e.g. `-9007199254740991`) or leaving the port out (so the index returns `undefined`) tripped the assertion: ``` ASSERTION FAILED: isInt32() JavaScriptCore/JSCJSValue.h(994) : int32_t JSC::JSValue::asInt32() const ``` Repro: ```js Bun.dns.setServers([[-9007199254740991, 15473]]); ``` Switch to `coerceToInt32`, which uses JSC's full numeric coercion. The existing `family != 4 and family != 6` check still rejects invalid families with a proper `TypeError`. Found by Fuzzilli.
…en-sh#28926) `Bun.dns.setServers` called `toInt32()` on the family and port elements of each server triple. The internal `toInt32()` helper falls through to `JSC__JSValue__toInt32`, which calls `JSValue::asInt32()` and asserts `isInt32()`. Passing a double (e.g. `-9007199254740991`) or leaving the port out (so the index returns `undefined`) tripped the assertion: ``` ASSERTION FAILED: isInt32() JavaScriptCore/JSCJSValue.h(994) : int32_t JSC::JSValue::asInt32() const ``` Repro: ```js Bun.dns.setServers([[-9007199254740991, 15473]]); ``` Switch to `coerceToInt32`, which uses JSC's full numeric coercion. The existing `family != 4 and family != 6` check still rejects invalid families with a proper `TypeError`. Found by Fuzzilli.
2 participants
Bun.dns.setServerscalledtoInt32()on the family and port elements of each server triple. The internaltoInt32()helper falls through toJSC__JSValue__toInt32, which callsJSValue::asInt32()and assertsisInt32(). Passing a double (e.g.-9007199254740991) or leaving the port out (so the index returnsundefined) tripped the assertion:Repro:
Switch to
coerceToInt32, which uses JSC's full numeric coercion. The existingfamily != 4 and family != 6check still rejects invalid families with a properTypeError.Found by Fuzzilli.