Fix data race in SSLSocketStream causing TLS session corruption in WebSocket - #2550
Open
Hyukya wants to merge 2 commits into
Open
Fix data race in SSLSocketStream causing TLS session corruption in WebSocket#2550Hyukya wants to merge 2 commits into
Hyukya wants to merge 2 commits into
Conversation
Protect TLS session operations in SSLSocketStream with a mutex. This prevents races when WebSocket send and receive paths concurrently access the same TLS session, including pending checks, peer-close detection, reads, and writes.
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
SSLSocketStreamcan be shared across threads, allowing concurrent access to the same TLS session.The WebSocket ping thread, the application's
send(), andclose()'s wait-for-response read all enter the same session.The write path also reads (
wait_writable() -> is_peer_closed() -> SSL_peek), soSSL_readandSSL_peekcollide on the same record layer buffer, causing a buffer overflow.This only affects
wss://.ws://doesn't use a TLS session, so it's unaffected.Reproduction
Added
test_websocket_thread_safety.cc. One thread loopsread()while another callssend()/close().CloseWhileAnotherThreadReads(macOS, ASan): heap-buffer-overflow viaWebSocketClient::close() -> read_websocket_frame() -> SSLSocketStream::read() -> SSL_read()(during OpenSSL GCM cipher param handling).SendWhileAnotherThreadReads(macOS/Linux):sentfalls far short of the expected 2000 (macOS: 7, Linux: 19).On Linux,
frames_read == 0also failed.No crash — messages are silently dropped.
Being a race, the exact failure point varies per run; a single pass doesn't
prove safety.
Fix
Add one mutex per
SSLSocketStream, held around every call that enters thesession:
tls::pending,tls::read,tls::write,tls::is_peer_closed.select_read/select_writestay outside the lock since they're socketoperations, not session operations. Locking is not applied at the WebSocket
layer, so an idle reader doesn't block a sender for the whole read timeout.
Out of scope
I/O.
std::mutexdoesn't guarantee fairness, so send starvation underheavy read load is still possible. A single I/O owner with an outbound
queue would be a better long-term structure.
identical internal corruption on other backends.