Fix oghttp2 to reject 204 responses with a body, even with no content-length header.
This CL updates OgHttp2Session::CanReceiveBody() to return false for responses
with :status 204. This change aligns oghttp2 further with nghttp2 and GFE2:
http://google3/third_party/quiche/common/balsa/balsa_headers.cc;l=1093;rcl=436357765.
PiperOrigin-RevId: 436844954
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index fe4e32c..645575e 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -579,8 +579,19 @@
const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
ASSERT_GT(stream_id1, 0);
+ const std::vector<Header> headers2 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}});
+
+ const int32_t stream_id2 = adapter->SubmitRequest(headers2, nullptr, nullptr);
+ ASSERT_GT(stream_id2, stream_id1);
+
EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id2, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id2, _, 0x5, 0));
int result = adapter->Send();
EXPECT_EQ(0, result);
@@ -592,6 +603,8 @@
.Headers(1, {{":status", "204"}, {"content-length", "2"}},
/*fin=*/false)
.Data(1, "hi")
+ .Headers(3, {{":status", "204"}}, /*fin=*/false)
+ .Data(3, "hi")
.Serialize();
// Server preface (empty SETTINGS)
@@ -609,6 +622,12 @@
EXPECT_CALL(
visitor,
OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
+ EXPECT_CALL(visitor, OnHeaderForStream(3, ":status", "204"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(3));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(3, 2));
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
@@ -620,12 +639,18 @@
OnFrameSent(RST_STREAM, 1, _, 0x0,
static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::PROTOCOL_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::PROTOCOL_ERROR));
EXPECT_TRUE(adapter->want_write());
result = adapter->Send();
EXPECT_EQ(0, result);
- EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS,
- SpdyFrameType::RST_STREAM}));
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM}));
}
TEST(NgHttp2AdapterTest, ClientHandles304WithContent) {
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 2aa0255..775609a 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -671,10 +671,21 @@
const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
ASSERT_GT(stream_id1, 0);
+ const std::vector<Header> headers2 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}});
+
+ const int32_t stream_id2 = adapter->SubmitRequest(headers2, nullptr, nullptr);
+ ASSERT_GT(stream_id2, stream_id1);
+
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id2, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id2, _, 0x5, 0));
int result = adapter->Send();
EXPECT_EQ(0, result);
@@ -686,6 +697,8 @@
.Headers(1, {{":status", "204"}, {"content-length", "2"}},
/*fin=*/false)
.Data(1, "hi")
+ .Headers(3, {{":status", "204"}}, /*fin=*/false)
+ .Data(3, "hi")
.Serialize();
// Server preface (empty SETTINGS)
@@ -699,6 +712,12 @@
EXPECT_CALL(
visitor,
OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
+ EXPECT_CALL(visitor, OnHeaderForStream(3, ":status", "204"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(3));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(3, 2));
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
@@ -710,12 +729,18 @@
OnFrameSent(RST_STREAM, 1, _, 0x0,
static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR));
EXPECT_TRUE(adapter->want_write());
result = adapter->Send();
EXPECT_EQ(0, result);
- EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS,
- SpdyFrameType::RST_STREAM}));
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM}));
}
TEST(OgHttp2AdapterTest, ClientHandles304WithContent) {
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 25cd0e9..d8431dd 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -284,7 +284,7 @@
frame_contains_fin_ = false;
}
-// TODO(diannahu): Add checks for other response codes and request methods.
+// TODO(diannahu): Add checks for request methods.
bool OgHttp2Session::PassthroughHeadersHandler::CanReceiveBody() const {
switch (header_type()) {
case HeaderType::REQUEST_TRAILER:
@@ -294,7 +294,9 @@
case HeaderType::RESPONSE:
// 304 responses should not have a body:
// https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.2
- return status_header() != "304";
+ // Neither should 204 responses:
+ // https://httpwg.org/specs/rfc7231.html#rfc.section.6.3.5
+ return status_header() != "304" && status_header() != "204";
case HeaderType::REQUEST:
return true;
}
