Repository navigation
finish short writes in write_websocket_frame and open_stream_ - #2642
Merged
Merged
Conversation
write_websocket_frame and the request-body write in open_stream_ called Stream::write once and only failed on a negative return, so a short write (one record per mbedtls_ssl_write call, or SO_SNDTIMEO expiring after part of the data went out) dropped the rest of the frame or body. Route both through write_data, as the other body writers do.
Owner
|
Thanks! |
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.
Short writes truncate WebSocket frames and streamed request bodies
Problem:
detail::write_websocket_frameand the request-body write inClientImpl::open_stream_callstrm.write()once per buffer and only treat a negative return as failure, so a short write is taken as success and the rest of the data is dropped.Cause: a short count is normal in two cases. Under the Mbed TLS backend,
mbedtls_ssl_write()writes at most one record (16 KiB by default) per call, so any unmasked frame larger than that loses its tail. On a plain socket,send()returns a short count whenSO_SNDTIMEOexpires after part of the data has gone out to a slow reader. The frame header still announces the full length, so the receiver takes the next frame's header and payload as the rest of this one, then parses a frame header from the middle of a later payload. Whoever controls the content of a later message can therefore place a complete frame there (a Text or a Close), and it is delivered as a message of its own. With a stream capped at 16384 bytes per write, a 20000-byte binary message followed by a crafted text message arrives as three frames, the last two of them forged. On the client side, the streamed body falls short of itsContent-Lengthand the server answers 400 once it gives up waiting.Fix: both sites now go through
detail::write_data, which the other body writers already use, so a short count is resumed and only a real error fails the send.Tests:
WebSocketPartialWriteTest.FrameIsWrittenWholeandOpenStreamPartialWriteTest.BodyIsWrittenWholeshrink the socket buffers, let the write timeout expire partway through a 4 MiB write while the peer sleeps, then check that everything arrives whole. Both fail on master (the server sees 294912 of 4194304 body bytes and answers 400, and the WebSocket client never reads the message). Both pass with the fix. They are skipped on Windows, where a timed-out blockingsend()leaves the socket unusable rather than returning a short count.Reviewer note: I could only run the suite without TLS here, so the Mbed TLS case rests on the documented
mbedtls_ssl_write()behaviour rather than a run on that backend. It is probably worth a glance at that CI job.