proxy-io: Fix theoretical Connection::onDisconnect bugs - #361
Open
ryanofsky wants to merge 3 commits into
Open
Conversation
Two Connection callback-registration methods had names that did not match when they actually run: - onDisconnect() only ran its handler on a *remote* disconnect (it is canceled when the connection is disconnected locally), so rename it to onRemoteDisconnect(), and rename its backing TaskSet m_on_disconnect to m_on_remote_disconnect. - addSyncCleanup()/removeSyncCleanup() registered a function that runs on *any* disconnect (it is invoked from the connection teardown path), so rename them to onDisconnect()/cancelOnDisconnect(). Pure rename, no behavior change. The honest names make the following commits easier to follow: the next commit moves a listener bookkeeping callback from onRemoteDisconnect() to onDisconnect() so it runs on local disconnects too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A ListenConnections listener that reached its max-connection limit would stop accepting new connections permanently if one of its connections was closed locally instead of by a remote disconnect. Closing a connection locally (e.g. erasing it from m_incoming_connections) left the listener's active-connection count stuck at the limit, so it never resumed accepting. The count is decremented by the on_disconnect callback, which ran from the _Serve onRemoteDisconnect handler. That handler only fires on a remote disconnect and is canceled when a connection is closed locally, so the decrement was skipped on local closes. Register on_disconnect with onDisconnect() instead, so it runs on the connection teardown path for both remote and local disconnects, and keep only the list erase on onRemoteDisconnect(). Add a regression test that closes a connection locally and checks the listener resumes accepting; it fails before this change (the listener never accepts the waiting client) and passes after. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix a use-after-free, possible since the destroy_connection option was added in 2019 (c685fa9): a Connection's disconnect handler could run after the Connection had already been destroyed, deleting it a second time and crashing. Reported by enirox001 in bitcoin-core#335 (comment) Give each Connection a shared_ptr "alive" token that disconnect handlers hold a weak_ptr to and check before running, so a handler is skipped once its Connection is gone. Having this check also enables the simplifications described below. Previously each Connection kept its disconnect handlers in its own kj::TaskSet, and when the network disconnected it moved a handler onto the shared event loop TaskSet with kj::evalLater. Destroying the Connection destroyed that per-connection TaskSet, canceling a still-pending handler -- but a handler already moved onto the shared TaskSet was no longer canceled and could run after the Connection was gone. (The evalLater step existed only to avoid a "promise callback destroyed itself" error when a handler deletes its own Connection, which the per-connection TaskSet made possible.) With the token doing the cancellation, neither the per-connection TaskSet nor the evalLater step is needed, and both are removed. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-09-04 20:24:53 |
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
2 participants
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.
Two bugs in
Connection::onDisconnectwere pointed out in #335 review by @enirox001:evalLatercould allow an onDisconnect handler to run after a Connection object was destroyed.This PR fixes each issue in a separate commit and also includes a renaming commit to name disconnect callback methods more clearly.
Both bugs are "theoretical" just in the sense that they haven't been seen in practice and were found in code review. The first ListenConnections bug can't currently happen in bitcoin core because it doesn't disconnect IPC clients except when it is shutting down, and it would not make sense to accept new connections. The second race condition bug has just existed for many years and not been seen previously.