Fix H2 origin payload handling - #13363
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes HTTP/2 origin-side edge cases in payload handling by preserving the outbound request method for response validation (e.g., HEAD with non-zero Content-Length but no body) and by capping outbound DATA frame payloads to the remaining write VIO byte count. It also extends the existing H2 origin replay gold tests to cover these regressions.
Changes:
- Track the sent outbound request method on an H2 stream and use it when deciding whether
Content-Lengthvs. received payload-length mismatches are valid for no-body responses. - Limit outbound H2 DATA frame payload length to the remaining write VIO bytes while still properly emitting
END_STREAMwhen the transaction body is complete. - Extend H2 origin replay coverage (HEAD no-body w/ non-zero
Content-Length, large PUT + response) and update expected metric outputs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/proxy/http2/Http2ConnectionState.cc |
Caps DATA frame payload by remaining write VIO bytes; records the outbound request method when sending request headers. |
include/proxy/http2/Http2Stream.h |
Adds write_vio_ntodo() accessor; uses stored sent request method for payload-length validation logic. |
tests/gold_tests/h2/replay_h2origin/h2-origin.yaml |
Adds new replay scenarios for HEAD (non-zero Content-Length, no body) and large PUT request/response. |
tests/gold_tests/h2/h2origin.test.py |
Updates squid log expectations for the new UUID ranges. |
tests/gold_tests/h2/gold/http-request-method-metrics.gold |
Updates the expected PUT request metric count to reflect the new PUT test. |
Comments suppressed due to low confidence (1)
include/proxy/http2/Http2Stream.h:429
- Typo in the Warning message key: "data_legnth" should be "data_length" to make the log output searchable/consistent.
if (content_length != 0 && !is_payload_precluded && content_length != data_length) {
Warning("Bad payload length content_length=%d data_legnth=%d session_id=%" PRId64, content_length,
static_cast<int>(data_length), _proxy_ssn->connection_id());
c99484e to
4d912fa
Compare
There was a problem hiding this comment.
when a buffer reader has more bytes available than should be sent on the stream
I wonder how that happens. Isn't that the root cause that needs to be fixed? The change on this PR look fine as a bandaid fix though. I'm not going to push this back, but want to confirm if I understand the issue correctly.
| (_send_header.method_get_wksidx() == HTTP_WKSIDX_GET && _send_header.presence(mask) && | ||
| _receive_header.status_get() == HTTPStatus::NOT_MODIFIED)); | ||
| this->is_outbound_connection() && | ||
| (_sent_request_method == HTTP_WKSIDX_HEAD || (_sent_request_method == HTTP_WKSIDX_GET && _send_header.presence(mask) && |
There was a problem hiding this comment.
Note for future analysis: This could use _sm->t_state.hdr_info.server_request instead, but I'm not sure if we want to access it from here.
HTTP/2 origin responses can legally carry a non-zero Content-Length when no payload is sent, such as responses to HEAD requests. ATS discarded the outbound request method after encoding the H2 HEADERS frame and could therefore reject a valid no-body response as a payload-length error. An H2 DATA sender also treated every byte visible through its IOBufferReader as eligible for the current write. Reader availability is independent of the finite VIO operation: VIO::ntodo() is the authoritative boundary, and the ordinary network VConnection already caps writes to it. Without that cap, the regression sent 327,675 bytes for a 300,000-byte PUT and the H2 origin returned GOAWAY with PROTOCOL_ERROR. This retains the outbound request method on the H2 stream for response validation. It also caps DATA payloads to the remaining write VIO bytes, still setting END_STREAM when the final authorized bytes are sent, and extends the H2 origin replay coverage with HEAD and large PUT cases.
4d912fa to
70adfea
Compare
Thanks for asking. The original wording made this sound like an unexpected buffer state, so I updated the commit message and PR description to clarify it. |
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
Thanks for the clarification. The change makes sense now.
HTTP/2 origin responses can legally carry a non-zero Content-Length when no payload is sent, such as responses to HEAD requests. ATS discarded the outbound request method after encoding the H2 HEADERS frame and could therefore reject a valid no-body response as a payload-length error. An H2 DATA sender also treated every byte visible through its IOBufferReader as eligible for the current write. Reader availability is independent of the finite VIO operation: VIO::ntodo() is the authoritative boundary, and the ordinary network VConnection already caps writes to it. Without that cap, the regression sent 327,675 bytes for a 300,000-byte PUT and the H2 origin returned GOAWAY with PROTOCOL_ERROR. This retains the outbound request method on the H2 stream for response validation. It also caps DATA payloads to the remaining write VIO bytes, still setting END_STREAM when the final authorized bytes are sent, and extends the H2 origin replay coverage with HEAD and large PUT cases.
|
The following PR contains the 10.2.x version of this: |
HTTP/2 origin responses can legally carry a non-zero Content-Length when no payload is sent, such as responses to HEAD requests. ATS discarded the outbound request method after encoding the H2 HEADERS frame and could therefore reject a valid no-body response as a payload-length error. An H2 DATA sender also treated every byte visible through its IOBufferReader as eligible for the current write. Reader availability is independent of the finite VIO operation: VIO::ntodo() is the authoritative boundary, and the ordinary network VConnection already caps writes to it. Without that cap, the regression sent 327,675 bytes for a 300,000-byte PUT and the H2 origin returned GOAWAY with PROTOCOL_ERROR. This retains the outbound request method on the H2 stream for response validation. It also caps DATA payloads to the remaining write VIO bytes, still setting END_STREAM when the final authorized bytes are sent, and extends the H2 origin replay coverage with HEAD and large PUT cases. (cherry picked from commit dcb1850)
|
Cherry-picked to the 10.2.x branch as 7b31a6c for the 10.2.0 release. |
HTTP/2 origin responses can legally carry a non-zero Content-Length when no payload is sent, such as responses to HEAD requests. ATS discarded the outbound request method after encoding the H2 HEADERS frame and could therefore reject a valid no-body response as a payload-length error. An H2 DATA sender also treated every byte visible through its IOBufferReader as eligible for the current write. Reader availability is independent of the finite VIO operation: VIO::ntodo() is the authoritative boundary, and the ordinary network VConnection already caps writes to it. Without that cap, the regression sent 327,675 bytes for a 300,000-byte PUT and the H2 origin returned GOAWAY with PROTOCOL_ERROR. This retains the outbound request method on the H2 stream for response validation. It also caps DATA payloads to the remaining write VIO bytes, still setting END_STREAM when the final authorized bytes are sent, and extends the H2 origin replay coverage with HEAD and large PUT cases.
This breaks out HTTP/2 work from this otherwise HTTP/3 focused PR:
#13213
HTTP/2 origin responses can legally carry a non-zero Content-Length
when no payload is sent, such as responses to HEAD requests. ATS
discarded the outbound request method after encoding the H2 HEADERS
frame and could therefore reject a valid no-body response as a
payload-length error.
An H2 DATA sender also treated every byte visible through its
IOBufferReader as eligible for the current write. Reader availability is
independent of the finite VIO operation: VIO::ntodo() is the
authoritative boundary, and the ordinary network VConnection already
caps writes to it. Without that cap, the regression sent 327,675 bytes
for a 300,000-byte PUT and the H2 origin returned GOAWAY with
PROTOCOL_ERROR.
This retains the outbound request method on the H2 stream for response
validation. It also caps DATA payloads to the remaining write VIO bytes,
still setting END_STREAM when the final authorized bytes are sent, and
extends the H2 origin replay coverage with HEAD and large PUT cases.