Repository navigation
Read OpenSSL-buffered TLS data before polling - #413
Conversation
A peer may send a TLS record with more plaintext than the 10 KiB buffer the server passes to SSL_read. OpenSSL then keeps the rest of the record decrypted in its own buffer, where poll(2) on the socket cannot see it. The HTTP/2 server receive loop stops reading once a stream ends or its time budget runs out, so it can stop partway through such a record. The next poll_for_headers call then waited on a socket the peer, having sent everything, never made readable again, and a request whose END_STREAM sat in that tail never got a response. A proxy that coalesces many small DATA frames under load hits this. TLSSession::poll_for_headers now reports data ready when SSL_pending shows decrypted bytes, and the HTTP/2 server path polls through it. A new Uranium test sends two HTTP/2 requests in one 12 KiB TLS record, the second ending past the read buffer, and checks that both are answered.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Run tools/format.sh (yapf 0.43.0) over the new Uranium test so the Formatting check passes.
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Thanks, this looks right to me. receive_nghttp2_request stops as soon as one stream ends, so the rest of a large record can stay decrypted inside OpenSSL while poll(2) waits on an idle socket. Checking OpenSSL's buffer before the socket poll fixes that. The client-side receive_nghttp2_data calls SSL_read before it polls, so it doesn't have this problem.
One side effect to note: because the fix is in TLSSession::poll_for_headers, HTTP/1-over-TLS server sessions also get the buffered-data check. I think that's an improvement (for example, a pipelined request in the same record), but it's a behavior change beyond HTTP/2 and might deserve a sentence in the PR description.
I left one optional suggestion inline. The Uranium test is well built: sending nothing after the batch is what makes the stall show up reliably, and the verifier-client run works as both a positive control and the ordering gate for the server.
| swoc::Rv<int> | ||
| TLSSession::poll_for_headers(chrono::milliseconds timeout) | ||
| { | ||
| if (_ssl != nullptr && SSL_pending(_ssl) > 0) { |
There was a problem hiding this comment.
Optional: consider SSL_has_pending(_ssl) here. SSL_pending only counts processed plaintext from the record currently being read. SSL_has_pending also returns true when OpenSSL has already pulled further, unprocessed record bytes off the socket (for example with read-ahead or kTLS), which poll(2) also can't see. With the current defaults, SSL_pending covers the case in this PR, so this is a hardening change, not a fix. A false positive is harmless: SSL_read returns WANT_READ and we fall into poll_for_data_on_ssl_socket as before. TLSSession::is_idle uses SSL_pending too, so if you change it here you could change it there as well to stay consistent.
The HTTP/2 server receive loop reads TLS into a 10 KiB buffer, then waits
in poll(2) for more. When a peer sends a TLS record with more plaintext
than that buffer, OpenSSL keeps the rest of the record decrypted in its
own buffer, where poll(2) on the socket cannot see it. If the loop stops
partway through such a record, and the tail holds a request's END_STREAM,
the server waits on a socket the peer has no reason to make readable
again, and that request never gets a response. A proxy that coalesces
many small DATA frames into one large record under load hits this. ATS's
http2_flow_control test fails intermittently in CI this way: ATS forwards
the whole POST to the verifier-server and the response never comes.
This patch makes TLSSession::poll_for_headers report data ready when
SSL_pending shows decrypted bytes, and routes the HTTP/2 server path
through it. A new Uranium test, http2_large_tls_record, sends two HTTP/2
requests in one 12 KiB TLS record, the second ending past the read
buffer, and checks that both are answered.
Testing: the new test fails 20 of 20 runs against v3.2.2 binaries and
passes 20 of 20 with this patch. With ATS's http2_flow_control Uranium
test under CPU load (63 parallel containers plus 32 CPU hogs, all arms at
once), the run stalled 13 of 651 times with v3.2.0, 8 of 651 with
v3.2.2, and 0 of 651 with this patch. Every stall was a POST the server
held until the session closed.
Once this is in, a v3.2.3 release would let ATS move its pinned verifier
version forward and drop the flake from its CI.