dns: keep pending exceptions out of coalesced lookup promise resolves - #37004
dns: keep pending exceptions out of coalesced lookup promise resolves#37004robobun wants to merge 3 commits into
Conversation
The DNS completion callbacks converted the c-ares/getaddrinfo result to a JS array with the error swallowed (unwrap_or(ZERO)) and then resolved the promise anyway. A failed conversion left its exception pending on the VM and resolved with an empty JSValue, and the pending-host drain loops settle several coalesced promises in one task, so a pending exception from one settle flowed into the next JSPromise::resolve, which asserts in JSObjectInlines.h when it reads "then" off the resolution value while an exception is pending. Found by fuzzing. Settle through one helper: an empty result rejects the promise with the pending exception, a settle that leaves an exception pending reports it as unhandled instead of leaking it into the next one, and the drain loops re-convert the result after a failure instead of reusing the empty sentinel (drain_pending_host_native's unwrap() on that path could also panic). A getaddrinfo success with a null result list now rejects with DNS_ENOTFOUND instead of resolving with an empty value.
WalkthroughDNS lookup conversion now preserves JavaScript exceptions and rejects promises through shared settlement logic. Native and c-ares pending drains re-convert results after failed settlement or global-object changes. A subprocess test covers concurrent lookups across all DNS backends. ChangesDNS exception propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Settle a DNS request promise with the converted JS result. An empty | ||
| /// `result` means the conversion threw and left the exception pending on the | ||
| /// VM; reject with that exception instead of resolving. If settling itself | ||
| /// leaves an exception pending, report it so it cannot leak into the next | ||
| /// promise settled in the same task (the drain loops settle several in a row, | ||
| /// and `JSPromise::resolve` must not be entered with an exception pending). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // node is a valid c-ares hostent for the callback's duration. | ||
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Conversion threw; the empty sentinel makes | ||
| // `settle_lookup_promise` reject with the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Re-convert after a failure: the previous settle consumed the | ||
| // pending exception, so the empty sentinel must not be reused. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Re-convert after a failure: the previous settle consumed the | ||
| // pending exception, so the empty sentinel must not be reused. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Conversion threw; the empty sentinel makes | ||
| // `settle_lookup_promise` reject with the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Re-convert after a failure: the previous settle consumed the | ||
| // pending exception, so the empty sentinel must not be reused. | ||
| // `Ok(None)` is unreachable here (the head already classified | ||
| // `result` as non-null), so it folds into the empty sentinel. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Re-convert after a failure: the previous settle consumed the | ||
| // pending exception, so the empty sentinel must not be reused. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Empty on conversion failure; `settle_lookup_promise` rejects with | ||
| // the pending exception. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Re-convert after a failure: the previous settle consumed the | ||
| // pending exception, so the empty sentinel must not be reused. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Replaces the empty-JSValue sentinel that stood in for "conversion threw" with the conversion's JsResult, so the settle helper rejects through JSPromise::reject's per-variant handling (Thrown takes the pending exception, OutOfMemory builds an OOM error without requiring one, Terminated leaves the promise unsettled). Removes the per-site comments the sentinel needed and the unwrap_or(None) conflation of a thrown conversion with a null addrinfo list.
|
The comment-cop flags were pointing at a real smell: every conversion site needed a comment because the empty-JSValue sentinel carried the "conversion threw" state out of band. c9125ce removes the sentinel and passes the conversion's JsResult straight to the settle helper, which also lets JSPromise::reject handle each error variant properly (an OutOfMemory conversion error no longer assumes a JS exception is pending). Net 47 lines smaller and the per-site comments are gone. |
| /// Resolve a DNS request promise with the converted result, or reject it with | ||
| /// the conversion error. An exception left pending by the settle is reported | ||
| /// as unhandled so the drain loops never carry it into the next settle. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Consume the request and move `head` out by value; | ||
| // `ptr::read` + `heap::take` would double-Drop `DNSLookup`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
The two remaining flags are on the settle helper's doc comment and on the pre-existing SAFETY comment that moved inside the let-else in drain_pending_host_native. The first is the fn contract (the report-as-unhandled behavior is the one non-obvious part of the fix), the second is the standard SAFETY annotation every unsafe block in this file carries, so I'm leaving both as they are. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this reworks exception handling across five unsafe drain loops in the DNS resolver with no verified fail-before repro, so it's worth a human look.
What was reviewed:
settle_lookup_promiseroutesErrthroughJSPromise::reject(handles OOM/Terminated/Thrown distinctly) and reports post-settle exceptions via the samereport_active_exception_as_unhandledidiom used inevent_loop.rs/napi/postgres.- The
array.is_err()re-convert guard in each drain loop — required becauseJSPromise::rejecttakes the pending exception, so a reusedErr(Thrown)would panic intry_take_exception. .transpose().unwrap()indrain_pending_host_native—result_any_to_jsreturnsNoneonly for a nullAddrinfo, which the head already proved non-null.on_complete_native's newNonebranch mirrors the existingENOTFOUNDreject-then-destroy pattern inprocess_get_addr_info.
Extended reasoning...
Overview
The PR replaces unwrap_or(JSValue::ZERO) + let _ = promise.resolve_task(...) at every DNS result-conversion site with a shared settle_lookup_promise helper that carries the conversion's JsResult<JSValue> through to the promise: Ok resolves, Err rejects via JSPromise::reject (which properly branches on Thrown/OutOfMemory/Terminated), and any exception left pending after the settle is reported as unhandled so it can't leak into the next settle in the same drain loop. Five drain loops (drain_pending_cares, drain_pending_host_cares, drain_pending_host_native, drain_pending_addr_cares, drain_pending_name_info_cares) and four single-request on_complete sites are updated. on_complete_native additionally routes a null getaddrinfo list to a DNS_ENOTFOUND rejection instead of resolving with an empty value.
Security risks
None identified. This is error-path plumbing in DNS result delivery; no new untrusted-input parsing, no auth/crypto, no allocation sizing changes.
Level of scrutiny
High. Every touched function is unsafe with raw-pointer dispatch and manual lifetime management (heap::take, intrusive linked lists, ensure_still_alive GC pinning). The change alters control flow on error paths — the reject arm now uses promise.swap().reject(...) rather than the previous resolve_task, which means the error path no longer enters the _task event-loop scope; that's likely fine (the previous behavior was resolving with an empty JSValue, which is UB territory), but it is a scheduling-visible difference a maintainer should sanity-check. The array.is_err() re-convert condition and the .transpose().unwrap() in drain_pending_host_native both rely on non-local invariants that check out on inspection but are exactly the kind of thing a second pair of eyes should confirm.
Other factors
The PR description is candid that the original fuzzer assertion did not reproduce standalone (0/180 direct, 0/300 REPRL), so the new subprocess test is a best-effort guard on the coalesced-drain path rather than a proven fail-before repro. JsError is Copy, so JsResult<JSValue> is Copy and the by-value passes of array across the loop are sound. The two remaining comment-cop flags are on the helper's doc comment and a pre-existing SAFETY block that moved indentation; the author's explanation for keeping them is reasonable. Given the unsafe-code surface, the lack of a verified fail-before, and the subtle exception-scope semantics involved, this should get a human review rather than an auto-approval.
|
On the scheduling difference the review mentions: the reject arm never had a _task variant to preserve, since the old code resolved with an empty JSValue on this path (which is its own debug assert). The direct reject matches the existing reject idiom in this file: Strong::reject and the ErrorDeferred::reject task do not open an event loop scope either. The scope's only effect is draining microtasks when the enter count returns to zero, and every path that reaches these settles (task dispatch, or synchronously inside the lookup host call) already holds the count at 1 or higher, so the pair is a no-op there and rejection reactions drain at the next tick boundary either way. CI is green on c9125ce (196/196, build 89385). |
…or TLS session/keylog dispatch fails (#37067) ### What Fuzzing (Fuzzilli) keeps hitting this debug assertion in processes exercising sockets: ``` ASSERTION FAILED: !scope.exception() || vm.hasPendingTerminationException() || !hasProperty JavaScriptCore/JSObjectInlines.h(137) : JSValue JSC::JSObject::get(JSGlobalObject *, PropertyName) ``` It fires when any native `JSObject::get` runs while a non-termination JS exception is pending. Several socket dispatch sites could leave the VM in exactly that state: they call a fallible operation whose `Err` means "a JS exception is pending", discard the `Err`, and return to the uSockets/libuv event loop. The next native JSC call in the same tick then runs with the stale exception. In debug/ASAN builds that trips the assert (`JSObject::get`, or `assertNoExceptionExceptTermination` at the microtask-drain entry); in release the exception gets attributed to whatever runs next. The sites, all in `src/runtime/socket/`: - `handle_connect_error` swallowed a failed promise reject with `handlers.reject_promise(err_val).unwrap_or(true)` and returned `Ok` with the exception still set (a `// TODO: properly propagate exception upwards` site), and its other two settle sites (`reject`, `reject_as_handled`) returned `Err` into callers that all discarded it: `connect()`'s synchronous `do_connect()` failure branch, the `on_connect_error` event-loop dispatch, the Windows named-pipe `on_error`/`fail_and_release`, and the duplex TLS upgrade teardown. - `on_open` swallowed a failed resolve of the connect promise (the other `// TODO: properly propagate exception upwards` site) and returned to the loop with the exception pending, and did the same `unwrap_or(true)` swallow when rejecting with an error returned from the `open` callback. - `us_dispatch_session` / `us_dispatch_keylog` (and the duplex and named-pipe equivalents) discarded `on_session` / `on_keylog` results; those return `Err` when allocating the session/keylog `Buffer` throws. ### Fix The pending exception has to be handled at the failure site, before the dispatch's scope guards drop: `ScopeExit` runs a microtask checkpoint on scope exit, and entering that checkpoint with a non-termination exception pending is itself the asserted condition, so an `Err` propagated or discarded past the guard is already too late. And the event-loop dispatch callers have no JS frame to propagate into in the first place. So each failed settle now reports its exception as unhandled right where it fails, via `report_active_exception_as_unhandled`: the same idiom the socket error handler, ipc, h2, fs-watcher, and dns (#37004) completions use. That clears and reports a real throw, and leaves a termination exception pending, which is what the event loop expects (`assertNoExceptionExceptTermination`). With every failure handled internally, `handle_connect_error`, `on_session`, and `on_keylog` become infallible, and `on_handshake`/`on_close` (whose `Err` paths were structurally dead: `verify_error_to_js` cannot fail) are made infallible with them, so no dispatch call site can discard an `Err` from this family again; every `let _ =` at those call sites is gone. ### Verification The promise-settle failures (`JSPromise::resolve/reject`) only occur on JSC heap OOM or termination mid-call, so those arms have no deterministic reproduction; they are covered by the behavior-level tests below plus the analysis above. The session/keylog `Buffer` allocation has the same trigger, but for that one this PR adds a `session_buffer` fault-injection rule (mirroring the existing `ssl_loop_buffer` rule for the one other allocation whose failure is unreachable without injection), which makes the fix deterministically testable: - `socket-session-oom-fixture.ts` arms the rule, completes a TLS handshake with a `session` handler, and asserts the injected allocation failure surfaces as an `uncaughtException` while the event loop survives (a TCP round trip completes afterwards and the process exits 0). With the hook but without the fix, the same fixture dies on `ASSERTION FAILED: ... !exception() || m_vm.hasPendingTerminationException()` in `ExceptionScope::assertNoExceptionExceptTermination`, the fuzzer's failure class. - New behavior tests cover every touched dispatch path: TLS `session`/`keylog` delivery (previously untested), throws from `session`/`keylog`/`open` handlers reaching the socket's (or listener's) `error` handler while `connect()` still resolves, a synchronous unix connect failure rejecting with `ENOENT` (run in a child with a relative socket path so macOS's 104-byte `sun_path` limit cannot clobber the errno), and a throw from `connectError` becoming the `connect()` rejection. - The Windows named-pipe paths were exercised directly (named-pipe client lifecycle tests, including the failed-connect branch this PR touches, and the Windows connect error-code tests): all pass on a Windows debug build. - The existing fault-injection suites (`socket-syscall-fault`, `tls-syscall-fault`) pass with the new rule added. - `test/js/bun/net/`, `test/js/node/net/`, and the node-tls connect suites show the identical failure set as a baseline build in this sandboxed environment (local DNS/IPv6 limitations), with no new failures. `cargo check` passes on all 10 platform targets. Related but distinct open work: #36808 gates these dispatch sites against an already-pending termination (entry-side), while this PR fixes exceptions the dispatch itself creates (exit-side); #35221 changes which errno the DNS-path `connectError` reports, not the exception discipline. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts <!-- robobun:evidence:end -->
|
Closing: superseded by the Outcome refactor that landed in #37075. Main's version covers everything this PR did and improves on it, taking the exception off the VM at conversion time (Outcome::of) so no pending state can ride the drain loops at all, which also removes the need for the re-convert-on-failure logic this PR added to the waiter loops. The dns.rs swallow sites and the empty-sentinel resolve are all gone on main. One small note for anyone touching this area: the coalesced-lookup test from this branch (hostile Object.prototype.then accessor across all three backends, test/js/bun/dns/resolve-dns.test.ts) pins the drain loop's settlement behavior under prototype pollution and is free to lift, and Outcome::settle still discards its own settle result, which only matters if a resolve throws internally under OOM. |
1 participant
What
Fuzzing (Fuzzilli) hit a flaky debug assertion in a process that was hammering
Bun.dns.lookup:That assertion fires when
JSObject::getis entered while a JS exception is already pending and the property exists. The only native caller of thatgeton this workload isJSPromise::resolvePromisereadingthenoff the resolution value, reached fromJSC__JSPromise__resolve, which has no entry exception check.The DNS completion code is the path that drives those resolves with an exception left pending:
unwrap_or(JSValue::ZERO)and a// TODO: properly propagate exception upwards, then resolved anyway. That leaves the conversion's exception pending on the VM and resolves with an empty JSValue (itself anASSERT(!target.isEmpty())in debug builds).drain_pending_*loops settle several coalesced promises in one native task, each withlet _ = promise.resolve_task(...). The generatedcheck_slowwrapper surfaces a pending exception asErrbut does not clear it, so a discarded error from one settle stays on the VM and the next settle in the same loop entersJSPromise::resolvewith it, which is exactly the asserted invariant. In release builds the stale exception instead gets misattributed:resolvePromisecatches it and rejects an unrelated lookup's promise with it.drain_pending_host_nativeadditionally conflatedErr(conversion threw) withOk(None)(null addrinfo) at the head, and had a bare.unwrap()on the re-conversion in its waiter loop.Fix
All five settle sites go through one helper,
settle_lookup_promise:report_active_exception_as_unhandled, the same idiom the socket/ipc/h2/watcher completions use) so it cannot leak into the next promise settled in the same task; termination exceptions stay pending as the event loop expects,.unwrap()panic path),on_complete_nativeroutes a null getaddrinfo result list to aDNS_ENOTFOUNDrejection, matching the c-ares path, instead of resolving with an empty value.Verification
The fuzzer crash is timing and process-history dependent (it needs an exception, e.g. OOM under memory pressure, to land between two settles in one drain, plus a reachable
thenon the result's prototype chain left by an earlier script in the reused fuzzer process). It did not reproduce standalone here in 180 direct runs, nor in 300 iterations of the script replayed through a REPRL harness in one process, before or after the fix, so there is no fail-before repro for the assertion itself.What was verified with the debug (ASAN, assertions on) build:
thenaccessors onObject.prototype, throwing microtasks between coalesced settles,Error.prepareStackTracehooks, Bun-object getter sweeps) run clean before and after,Object.prototype.thenaccessor, so every resolve in a drain throws while fetchingthen; each promise must reject with that error and the process must exit cleanly,test/js/bun/dns/and the non-networktest/js/node/dns/tests pass, with the same set of network-dependent failures as a baseline build in this sandboxed environment (29, identical list, no external DNS available).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/dns/resolve-dns.test.ts