Fix connections being closed after an unrelated SSL failure - #116
Merged
Conversation
SSL_get_error only describes the call it is given when the calling thread's OpenSSL error queue was empty beforehand. Nothing cleared that queue, so a failed operation left an entry behind and the next session to read on the same thread got SSL_ERROR_SSL instead of SSL_ERROR_WANT_READ. That session moved to SSLError, and callers branching on the state closed a connection that was working. The handshake path does not misreport, because OpenSSL's handshake machinery empties the queue itself. The clear goes before all five SSL I/O calls anyway: an empty queue going in is the documented contract, not a property to work out one call site at a time. Closes #115
SeanTAllen
added
the
changelog - fixed
ponylang-main
added
the
discuss during sync
ponylang-main
removed
the
discuss during sync
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.
SSL_get_erroronly describes the call it is asked about when the calling thread's OpenSSL error queue was empty before that call ran. Nothing cleared that queue, so one failed operation could put an unrelated healthy session intoSSLErrorand get its connection closed.@ERR_clear_error()now runs immediately before each of the five SSL I/O calls.Two tests. One checks that a session reading after an unrelated failed handshake stays
SSLReady; it also asserts the failure actually left an entry on the queue, because without that the test cannot fail whateverreaddoes. The other checks that a read whose record will not decrypt still reportsSSLError, so clearing the queue cannot cost a session its own errors — before this,_state = SSLErrorcould be deleted outright and the suite stayed green.Measured on every backend the package supports. With the fix removed, the regression test fails on OpenSSL 1.1.1w, OpenSSL 3.6.2, LibreSSL 3.9.2 and LibreSSL 4.2.1, and passes on OpenSSL 4.0.0, which does not have the bug. The full suite passes on all five with the fix.
Closes #115