Fix http2 content-length header check (#12747)

[floom4]

HEAD method with "Content-Length" header was returning 502 code because header differs from payload length as there is no payload in this case.

See https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.6

A response that is defined to have no payload, as described in [RFC7230], Section 3.3.2, can have a non-zero content-length header field, even though no content is included in DATA frames.

Fixes #12747

The change fixes the behavior by having payload_length_is_valid() return true if data_length is 0 meaning that the response has no payload, allowing for non-zero 'Content-Length' in this case as specified in the rfc.

[maskit]

Good catch. However, as the RFC says, "A response that is defined to have no payload" is the condition. It looks like your change allows arbitrarily response that have no payload.

[floom4]

My understanding of the RFC is that the condition defining a valid response without payload is the stream ending at header or continuation frame. Considering that data_length is only incremented inside rcv_data_frame, I assumed that having data_length == 0 means that there was no data frame.

However, if you prefer we could skip this check in rcv_header_frame and rcv_continuation_frame if and only if they are not trailing headers (since trailing headers implies that data frames where received)

Here is an alternative condition that could be used:

if (stream->receive_end_stream && stream->trailing_header_is_possible() && !stream->payload_length_is_valid()) {

In both:

• rcv_header_frame https://github.com/apache/trafficserver/blob/0e4287020548e7316156a4debfed49c0fedc7312/src/proxy/http2/Http2ConnectionState.cc#L505
• rcv_continuation_frame https://github.com/apache/trafficserver/blob/0e4287020548e7316156a4debfed49c0fedc7312/src/proxy/http2/Http2ConnectionState.cc#L1117

[maskit]

I don't think what frame carries END_STREAM flag matters. A DATA frame that carries just END_STREAM flag (no payload) is valid.

If a request is GET and the response has content-length: 123, data_length must be 123, right? Because a response for a GET request is not defined to have no payload. A response for a HEAD request is.

[floom4]

You're right I misunderstood the RFC, I went back to it and in my understanding, the only cases where a non-zero 'Content-Lenght' header without payload is allowed are:

• HEAD response
• 304 response to conditional GET

From Section 3.3.2 of [RFC7230]

A server MAY send a Content-Length header field in a response to a
   HEAD request (Section 4.3.2 of [RFC7231]); ...

A server MAY send a Content-Length header field in a 304 (Not
   Modified) response to a conditional GET request (Section 4.1 of
   [RFC7232]); ...

It indeed means that, while other types of request and response may have an empty payload (or data_length == 0), they must not have a Content-Length header with a value different from 0.

Here is a proposal to check that the current frame is part of an HEAD response that we could use in rcv_headers_frame and rcv_continuation_frame:

stream->is_outbound_connection() && stream->get_sm()->t_state.hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_HEAD

My understanding is that is_outbound_connection represents communication with origin which in case of receiving frame means response from origin I assumed that server_request field would be set since the previous condition ensure that we are at the response step and consequently, the request was created and sent.

I think the same fields can be used to know if the frame is part of a response to a conditional GET request, however to find the http status code seems trickier since the response headers are not parsed yet as we just received the frame and are still validating it.

Maybe we could only fix HEAD request for the moment, does the above conditional statement work for you ?

[maskit]

My understanding is that is_outbound_connection represents communication with origin which in case of receiving frame means response from origin I assumed that server_request field would be set since the previous condition ensure that we are at the response step and consequently, the request was created and sent.

Correct.

I think the same fields can be used to know if the frame is part of a response to a conditional GET request, however to find the http status code seems trickier since the response headers are not parsed yet as we just received the frame and are still validating it.

stream->decode_header_blocks is called before !stream->payload_length_is_valid() in rcv_headers_frame and rcv_continuation_frame. The status code should be available. But if I'm missing something and it's tricky, I'm fine with fixing just the HEAD request case.

The proposed condition looks fine. Since those are accessible from stream, I think you can use the condition in payload_length_is_valid. And then it should work for rcv_data_frame as well.