Repository navigation
Conversation
…hread When a thread other than the event loop thread calls EventLoop::sync() and the callback throws, sync() now rethrows the exception in the calling thread, the same as when the callback runs inline on the event loop thread. Before, EventLoop::loop() logged "EventLoop: m_sync_fn threw" and sync() returned as if the callback had succeeded, so callers continued with half-finished setup: - clientInvoke waited forever for a response if building the request threw, because the request was never sent. - MakeStream returned a null stream and ListenConnections returned as if it were listening if wrapping the socket threw, and ConnectStream built a client on a null Connection if creating the Connection threw. - An async server method ran without its cancellation callback if installing the callback threw. The log line is dropped because the exception now reaches the caller, which reports or handles it the way it handles any other exception. The async server method setup in PassField catches the exception inside its sync() callback and rethrows it after the KJ_DEFER cleanup is in place, instead of letting sync() rethrow it right away, so the cancellation callback and the self reference are still cleared on the event loop thread. First reported in https://ipcbugs.ofsky.org/audits/ar-14/ (request build hang), https://ipcbugs.ofsky.org/audits/ar-57/ (socket setup) and https://ipcbugs.ofsky.org/audits/ar-60/ (cancellation callback). This change was written with Claude Opus 5.5 (1M context).
MakeStream and ListenConnections now close the socket if allocating the KJ object that takes ownership of it throws std::bad_alloc. Before, the socket leaked, because nothing owned it until the KJ object was constructed. Other failures are left to KJ: once its wrapper object starts constructing, it owns the socket and closes it if a later step fails, so closing it here as well could close an unrelated descriptor (see the WrapSocket comment). First reported in https://ipcbugs.ofsky.org/audits/ar-57/. This change was written with Claude Opus 5.5 (1M context).
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Nov 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: When a callback that
EventLoop::sync()runs on the event loop thread for another thread throws, the error is only logged ("EventLoop: m_sync_fn threw") and the calling thread carries on as if the callback succeeded. Depending on the caller:MakeStreamreturns a null stream andListenConnectionsreturns as if it were listening. If creating theConnectionfails,ConnectStreambuilds a client on a nullConnectionand crashes.Separately, a socket leaks if allocating the KJ object that wraps it fails.
None of this happens in normal operation. The triggers are allocation failures, epoll or kqueue failing to register a socket, and oversized arguments. The problems were reported privately in security reviews: the request build hang twice (first report, second report), the socket setup failures and leak (report), and the cancellation callback (report).
Solution:
sync()now rethrows the callback's exception in the calling thread, as it already did when called on the event loop thread, so every caller sees the failure. Two call sites need more than that:PassFieldcatches the exception inside its callback and rethrows it after its cleanup is in place, so the cancellation callback and self reference are still released on the event loop thread.MakeStreamandListenConnectionsclose the socket if allocating its wrapper fails. They close it only onstd::bad_alloc, since KJ closes the socket itself when a later step of constructing the wrapper fails.Tests cover each case, using the existing
testing_hook_mischook, which is now called from three more places.For reviewers: callbacks passed to
sync()from destructors and from the server thread's result delivery inProxyServer<Thread>::post()now terminate the process if they throw, instead of logging. Nothing in those callbacks is expected to throw.