Fix oghttp2 to handle the submission of trailers after sending data with END_STREAM.
This CL updates OgHttp2Session::WriteForStream() to gracefully handle the
scenario of having pending trailers to write after writing a DATA frame with
the END_STREAM flag, which locally half-closes the stream. This scenario can
arise if the user calls SubmitTrailers() for a stream that also has a
DataFrameSource with send_fin() true. Before, OgHttp2Session would infinite
loop. With this change, OgHttp2Session will instead close the stream with
INTERNAL_ERROR.
Note that the new oghttp2 behavior is still a functional diff from nghttp2
behavior, which appears to drop the data source (including pending body) when
trailers are submitted. OgHttp2Session prioritizes writing the body (and
trailers if submitted without signaling END_STREAM on the body) over writing
the trailers.
This change resolves a fuzzing bug with oghttp2.
This CL also adds trace logging for the sending of DATA frames.
PiperOrigin-RevId: 567320945
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc
index 464135a..0493c87 100644
--- a/quiche/http2/adapter/nghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -3879,6 +3879,156 @@
EXPECT_FALSE(adapter->want_write());
}
+TEST(NgHttp2AdapterTest, ServerSubmitsTrailersWithDataEndStream) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1, {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}})
+ .Data(1, "Example data, woohoo.")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(static_cast<size_t>(result), frames.size());
+
+ // Send a body that will end with the END_STREAM flag.
+ const absl::string_view kBody = "This is an example response body.";
+ auto body = std::make_unique<TestDataFrameSource>(visitor, /*has_fin=*/true);
+ body->AppendPayload(kBody);
+ body->EndData();
+
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}}), std::move(body));
+ ASSERT_EQ(submit_result, 0);
+
+ const std::vector<Header> trailers =
+ ToHeaders({{"extra-info", "Trailers are weird but good?"}});
+ submit_result = adapter->SubmitTrailer(1, trailers);
+ ASSERT_EQ(submit_result, 0);
+
+ // It looks like nghttp2 drops the response body altogether and goes straight
+ // to writing the trailers.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, ACK_FLAG, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, END_HEADERS_FLAG, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _,
+ END_HEADERS_FLAG | END_STREAM_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _,
+ END_HEADERS_FLAG | END_STREAM_FLAG, 0));
+
+ const int send_result = adapter->Send();
+ EXPECT_EQ(send_result, 0);
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS,
+ SpdyFrameType::HEADERS}));
+}
+
+TEST(NgHttp2AdapterTest, ServerSubmitsTrailersWithDataEndStreamAndDeferral) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1, {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}})
+ .Data(1, "Example data, woohoo.")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(static_cast<size_t>(result), frames.size());
+
+ // Send a body that will end with the END_STREAM flag. Don't end the body here
+ // so that more body can be added later.
+ const absl::string_view kBody = "This is an example response body.";
+ auto body = std::make_unique<TestDataFrameSource>(visitor, /*has_fin=*/true);
+ body->AppendPayload(kBody);
+ TestDataFrameSource& body_ref = *body;
+
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}}), std::move(body));
+ ASSERT_EQ(submit_result, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, ACK_FLAG, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, END_HEADERS_FLAG, 0));
+ EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS,
+ SpdyFrameType::DATA}));
+ visitor.Clear();
+
+ const std::vector<Header> trailers =
+ ToHeaders({{"extra-info", "Trailers are weird but good?"}});
+ submit_result = adapter->SubmitTrailer(1, trailers);
+ ASSERT_EQ(submit_result, 0);
+
+ // Add more body and signal the end of data. Resuming the stream should allow
+ // the new body to be sent, though nghttp2 does not send the body.
+ body_ref.AppendPayload(kBody);
+ body_ref.EndData();
+ adapter->ResumeStream(1);
+
+ // For some reason, nghttp2 drops the new body and goes straight to writing
+ // the trailers.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _,
+ END_HEADERS_FLAG | END_STREAM_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _,
+ END_HEADERS_FLAG | END_STREAM_FLAG, 0));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(send_result, 0);
+ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::HEADERS}));
+}
+
TEST(NgHttp2AdapterTest, ClientDisobeysConnectionFlowControl) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc
index 1d52d28..d077051 100644
--- a/quiche/http2/adapter/oghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -5175,6 +5175,160 @@
}
}
+TEST(OgHttp2AdapterTest, ServerSubmitsTrailersWithDataEndStream) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options;
+ options.perspective = Perspective::kServer;
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1, {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}})
+ .Data(1, "Example data, woohoo.")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(static_cast<size_t>(result), frames.size());
+
+ // Send a body that will end with the END_STREAM flag.
+ const absl::string_view kBody = "This is an example response body.";
+ auto body = std::make_unique<TestDataFrameSource>(visitor, /*has_fin=*/true);
+ body->AppendPayload(kBody);
+ body->EndData();
+
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}}), std::move(body));
+ ASSERT_EQ(submit_result, 0);
+
+ const std::vector<Header> trailers =
+ ToHeaders({{"extra-info", "Trailers are weird but good?"}});
+ submit_result = adapter->SubmitTrailer(1, trailers);
+ ASSERT_EQ(submit_result, 0);
+
+ // The data should be sent, but because it has END_STREAM, it would not be
+ // correct to send trailers afterward. The stream should be closed.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, ACK_FLAG, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, END_HEADERS_FLAG, 0));
+ EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, END_STREAM_FLAG, 0));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::INTERNAL_ERROR));
+
+ const int send_result = adapter->Send();
+ EXPECT_EQ(send_result, 0);
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS,
+ SpdyFrameType::HEADERS, SpdyFrameType::DATA}));
+}
+
+TEST(OgHttp2AdapterTest, ServerSubmitsTrailersWithDataEndStreamAndDeferral) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options;
+ options.perspective = Perspective::kServer;
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ const std::string frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1, {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}})
+ .Data(1, "Example data, woohoo.")
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ // Stream 1
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, _));
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(static_cast<size_t>(result), frames.size());
+
+ // Send a body that will end with the END_STREAM flag. Don't end the body here
+ // so that more body can be added later.
+ const absl::string_view kBody = "This is an example response body.";
+ auto body = std::make_unique<TestDataFrameSource>(visitor, /*has_fin=*/true);
+ body->AppendPayload(kBody);
+ TestDataFrameSource& body_ref = *body;
+
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}}), std::move(body));
+ ASSERT_EQ(submit_result, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, ACK_FLAG, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, END_HEADERS_FLAG));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, END_HEADERS_FLAG, 0));
+ EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(send_result, 0);
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS,
+ SpdyFrameType::HEADERS, SpdyFrameType::DATA}));
+ visitor.Clear();
+
+ const std::vector<Header> trailers =
+ ToHeaders({{"extra-info", "Trailers are weird but good?"}});
+ submit_result = adapter->SubmitTrailer(1, trailers);
+ ASSERT_EQ(submit_result, 0);
+
+ // Add more body and signal the end of data. Resuming the stream should allow
+ // the new body to be sent.
+ body_ref.AppendPayload(kBody);
+ body_ref.EndData();
+ adapter->ResumeStream(1);
+
+ // The new body should be sent, but because it has END_STREAM, it would not be
+ // correct to send trailers afterward. The stream should be closed.
+ EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, END_STREAM_FLAG, 0));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::INTERNAL_ERROR));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(send_result, 0);
+ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::DATA}));
+}
+
TEST(OgHttp2AdapterTest, ClientDisobeysConnectionFlowControl) {
DataSavingVisitor visitor;
OgHttp2Adapter::Options options;
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc
index cb27927..a2f4605 100644
--- a/quiche/http2/adapter/oghttp2_session.cc
+++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -802,6 +802,10 @@
auto block_ptr = std::move(state.trailers);
if (state.half_closed_local) {
QUICHE_LOG(ERROR) << "Sent fin; can't send trailers.";
+
+ // TODO(birenroy,diannahu): Consider queuing a RST_STREAM INTERNAL_ERROR
+ // instead.
+ CloseStream(stream_id, Http2ErrorCode::INTERNAL_ERROR);
} else {
SendTrailers(stream_id, std::move(*block_ptr));
}
@@ -826,7 +830,8 @@
state.data_deferred = true;
break;
} else if (length == DataFrameSource::kError) {
- // TODO(birenroy): Consider queuing a RST_STREAM INTERNAL_ERROR instead.
+ // TODO(birenroy,diannahu): Consider queuing a RST_STREAM INTERNAL_ERROR
+ // instead.
CloseStream(stream_id, Http2ErrorCode::INTERNAL_ERROR);
// No more work on the stream; it has been closed.
break;
@@ -840,6 +845,7 @@
spdy::SpdyFramer::SerializeDataFrameHeaderWithPaddingLengthField(
data);
QUICHE_DCHECK(buffered_data_.empty() && frames_.empty());
+ data.Visit(&send_logger_);
const bool success =
state.outbound_body->Send(absl::string_view(header), length);
if (!success) {
@@ -875,6 +881,12 @@
auto block_ptr = std::move(state.trailers);
if (fin) {
QUICHE_LOG(ERROR) << "Sent fin; can't send trailers.";
+
+ // TODO(birenroy,diannahu): Consider queuing a RST_STREAM
+ // INTERNAL_ERROR instead.
+ CloseStream(stream_id, Http2ErrorCode::INTERNAL_ERROR);
+ // No more work on this stream; it has been closed.
+ break;
} else {
SendTrailers(stream_id, std::move(*block_ptr));
}
