Fix callback exception handling to be per-Connection instead of per-Context - #1535
Merged
Merged
Conversation
Exceptions raised inside verify, ALPN selection, OCSP and DTLS cookie callbacks cannot be raised from inside the callback, so they were deferred on a _CallbackExceptionHelper and re-raised by Connection._raise_ssl_error on the next send/recv/do_handshake. For callbacks registered on a Context, that helper (and its FIFO of pending exceptions) was shared by every Connection created from the Context. Because cffi releases the GIL around the SSL_* calls, a server that drives connections from several threads could have one connection pop and raise an exception that another connection's callback produced, aborting an otherwise healthy connection with a misattributed error, while the connection that actually failed reported a generic SSL error (or an IndexError from the unsynchronised check-then-pop). Record the pending exceptions on the Connection whose callback raised them instead, and have _raise_ssl_error and DTLSv1_listen only raise that connection's own. With no shared state left, the _CallbackExceptionHelper base class has nothing to do and is removed. This also fixes exceptions raised from a verify callback installed via Connection.set_verify being silently dropped: _raise_ssl_error only consulted the Context's verify helper, but the callback OpenSSL actually invokes for a connection is the one copied at SSL_new time or the one installed by Connection.set_verify. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm
…ption _raise_callback_problem drained the thread's OpenSSL error queue by calling _raise_current_error() and swallowing the resulting Error. That relies on _raise_current_error() raising even when the queue is empty, and obscures the intent. _raise_ssl_error already uses ERR_clear_error() for the same purpose a few lines further down; do the same here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm
alex
approved these changes
Sep 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
3 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.
Summary
This PR fixes a critical race condition in callback exception handling where exceptions raised by verify, ALPN selection, OCSP, or DTLS cookie callbacks for one
Connectioncould be incorrectly raised on an unrelatedConnectioncreated from the sameContextwhen used concurrently from multiple threads.Key Changes
Removed
_CallbackExceptionHelperbase class: This class stored exceptions in a shared list at the callback wrapper level, which was context-wide and shared across all connections using that context.Moved exception storage to
Connectioninstances: EachConnectionnow has its own_callback_problemslist to track exceptions that occurred during callbacks specific to that connection.Updated all callback helpers: Modified
_VerifyHelper,_ALPNSelectHelper,_OCSPServerCallbackHelper,_OCSPClientCallbackHelper,_CookieGenerateCallbackHelper, and_CookieVerifyCallbackHelperto:_CallbackExceptionHelperconnection._callback_problems.append(e)Centralized exception raising: Added
_raise_callback_problem()method toConnectionthat:_raise_ssl_error()andDTLSv1_listen()Added error handling to ALPN callback: The ALPN callback wrapper now specifies
error=_lib.SSL_TLSEXT_ERR_ALERT_FATALto cffi to ensure proper error handling if an exception somehow escapes the wrapper.Added test coverage: New tests verify that:
Implementation Details
The fix ensures thread-safety by storing callback exceptions on the connection object rather than on the shared callback wrapper. This way, when multiple threads are driving different connections from the same context concurrently, each connection only sees its own exceptions. The connection lookup from the SSL object happens before the try block to ensure we always have the correct connection reference for storing exceptions.
https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm