Fix content-length validation for trailers. This CL fixes a bug in content-length validation when trailers end the stream: 1. End of headers, stores content-length for validation 2. End of data, does not end stream and does not match content-length 3. End of trailers, ends stream and stores content-length for validation (!) The re-storing of content-length at the end of trailers erases the previous validation at the end of data. This CL fixes Step 3 to avoid storing content-length for trailers. MultiplexedIntegrationTest.InconsistentContentLength: http://sponge2/e572060c-8ff8-45a8-b3b3-25994345b2bb (demonstrates RST_STREAM) PiperOrigin-RevId: 422904538
diff --git a/http2/adapter/header_validator.cc b/http2/adapter/header_validator.cc index d256156..d5ae7a2 100644 --- a/http2/adapter/header_validator.cc +++ b/http2/adapter/header_validator.cc
@@ -136,6 +136,7 @@ return false; } + // TODO(diannahu): Do the same for 1xx responses. if (status_ == "204" && value != "0") { // There should be no body in a "204 No Content" response. return false;
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 0bf7295..206d8e2 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -4402,6 +4402,16 @@ {":path", "/this/is/request/four"}, {"content-length", "2"}}, /*fin=*/true) + .Headers(7, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/four"}, + {"content-length", "2"}}, + /*fin=*/false) + .Data(7, "h", /*fin=*/false) + .Headers(7, {{"extra-info", "Trailers with content-length mismatch"}}, + /*fin=*/true) .Serialize(); // Client preface (empty SETTINGS) @@ -4438,6 +4448,22 @@ OnInvalidFrame( 5, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); + // Stream 7: content-length is invalid and trailers end the stream + // When the stream ends with trailers, nghttp2 invokes OnInvalidFrame(). + EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(7)); + EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)).Times(5); + EXPECT_CALL(visitor, OnEndHeadersForStream(7)); + EXPECT_CALL(visitor, OnFrameHeader(7, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(7, 1)); + EXPECT_CALL(visitor, OnDataForStream(7, "h")); + EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(7)); + EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)); + EXPECT_CALL(visitor, + OnInvalidFrame( + 7, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); + const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -4458,6 +4484,11 @@ OnFrameSent(RST_STREAM, 5, _, 0x0, static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); EXPECT_CALL(visitor, OnCloseStream(5, Http2ErrorCode::PROTOCOL_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 7, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 7, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(7, Http2ErrorCode::PROTOCOL_ERROR)); EXPECT_TRUE(adapter->want_write()); int result = adapter->Send(); @@ -4465,6 +4496,7 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::RST_STREAM, spdy::SpdyFrameType::RST_STREAM, + spdy::SpdyFrameType::RST_STREAM, spdy::SpdyFrameType::RST_STREAM})); }
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 3660d20..d3f235d 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -4564,6 +4564,16 @@ {":path", "/this/is/request/four"}, {"content-length", "2"}}, /*fin=*/true) + .Headers(7, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/four"}, + {"content-length", "2"}}, + /*fin=*/false) + .Data(7, "h", /*fin=*/false) + .Headers(7, {{"extra-info", "Trailers with content-length mismatch"}}, + /*fin=*/true) .Serialize(); // Client preface (empty SETTINGS) @@ -4600,6 +4610,21 @@ EXPECT_CALL(visitor, OnHeaderForStream(5, _, _)).Times(5); EXPECT_CALL(visitor, OnEndHeadersForStream(5)); + // Stream 7: content-length is invalid and trailers end the stream + // Only oghttp2 invokes OnEndHeadersForStream(). Only nghttp2 invokes + // OnInvalidFrame(). + EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(7)); + EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)).Times(5); + EXPECT_CALL(visitor, OnEndHeadersForStream(7)); + EXPECT_CALL(visitor, OnFrameHeader(7, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(7, 1)); + EXPECT_CALL(visitor, OnDataForStream(7, "h")); + EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(7)); + EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)); + EXPECT_CALL(visitor, OnEndHeadersForStream(7)); + const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -4622,15 +4647,21 @@ OnFrameSent(RST_STREAM, 5, _, 0x0, static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); EXPECT_CALL(visitor, OnCloseStream(5, Http2ErrorCode::HTTP2_NO_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 7, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 7, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(7, Http2ErrorCode::HTTP2_NO_ERROR)); EXPECT_TRUE(adapter->want_write()); int result = adapter->Send(); EXPECT_EQ(0, result); - EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, - spdy::SpdyFrameType::SETTINGS, - spdy::SpdyFrameType::RST_STREAM, - spdy::SpdyFrameType::RST_STREAM, - spdy::SpdyFrameType::RST_STREAM})); + EXPECT_THAT( + visitor.data(), + EqualsFrames( + {spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM, spdy::SpdyFrameType::RST_STREAM, + spdy::SpdyFrameType::RST_STREAM, spdy::SpdyFrameType::RST_STREAM})); } } // namespace
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 827e80b..4c14b1b 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -6,6 +6,7 @@ #include "absl/memory/memory.h" #include "absl/strings/escaping.h" +#include "http2/adapter/header_validator.h" #include "http2/adapter/http2_protocol.h" #include "http2/adapter/http2_util.h" #include "http2/adapter/http2_visitor_interface.h" @@ -1117,7 +1118,10 @@ } else { it->second.received_header_type = headers_handler_.header_type(); } - it->second.remaining_content_length = headers_handler_.content_length(); + if (headers_handler_.header_type() == HeaderType::REQUEST || + headers_handler_.header_type() == HeaderType::RESPONSE) { + it->second.remaining_content_length = headers_handler_.content_length(); + } headers_handler_.set_stream_id(0); } }