Fixes a code defect reported in https://github.com/envoyproxy/envoy/issues/31710. In a very specific situation, OgHttp2Session can enter an infinite loop. This requires the following conditions: * a stream has both response body data and trailers to write; * the stream becomes blocked by stream-level flow control while writing the body; and * the connection still has flow control quota available. In this circumstance, OgHttp2Session::HasReadyStream() returns true because trailers_ready_ is not empty, but WriteForStream() cannot make progress. The fix is to remove the stream ID from trailers_ready_ in GetNextReadyStream(). Protected by not protected, does not affect the GFE binary. PiperOrigin-RevId: 597622882
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index d077051..6b570e2 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -5157,6 +5157,7 @@ send_result = adapter->Send(); EXPECT_EQ(0, send_result); + EXPECT_FALSE(adapter->want_write()); } else { // Even though the data source has not finished sending data, the library // will write the trailers anyway. @@ -5171,10 +5172,131 @@ send_result = adapter->Send(); EXPECT_EQ(0, send_result); EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::HEADERS})); + EXPECT_FALSE(adapter->want_write()); } } } +// Tests the case where the response body and trailers become blocked by flow +// control while the stream is writing. Regression test for +// https://github.com/envoyproxy/envoy/issues/31710 +TEST(OgHttp2AdapterTest, ServerSubmitsTrailersWithFlowControlBlockage) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options; + options.perspective = Perspective::kServer; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "POST"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/false) + .WindowUpdate(0, 2000) + .Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(0, 4, WINDOW_UPDATE, 0)); + EXPECT_CALL(visitor, OnWindowUpdate(0, 2000)); + + const int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + EXPECT_TRUE(adapter->want_write()); + + 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)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + visitor.Clear(); + + EXPECT_EQ(kInitialFlowControlWindowSize, adapter->GetStreamSendWindowSize(1)); + + const std::string kBody(60000, 'a'); + + // The body source must indicate that the end of the body is not the end of + // the stream. + auto body1 = std::make_unique<TestDataFrameSource>(visitor, false); + body1->AppendPayload(kBody); + auto* body1_ptr = body1.get(); + int submit_result = adapter->SubmitResponse( + 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), + std::move(body1)); + EXPECT_EQ(submit_result, 0); + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0)).Times(4); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), + EqualsFrames({SpdyFrameType::HEADERS, SpdyFrameType::DATA, + SpdyFrameType::DATA, SpdyFrameType::DATA, + SpdyFrameType::DATA})); + visitor.Clear(); + EXPECT_FALSE(adapter->want_write()); + + body1_ptr->AppendPayload(std::string(6000, 'b')); + // The next response body data payload is larger than the available stream + // flow control window. + EXPECT_LT(adapter->GetStreamSendWindowSize(1), 6000); + // There is more than enough connection flow control window. + EXPECT_GT(adapter->GetSendWindowSize(), 6000); + + adapter->ResumeStream(1); + int trailer_result = + adapter->SubmitTrailer(1, ToHeaders({{"final-status", "a-ok"}})); + ASSERT_EQ(trailer_result, 0); + + EXPECT_TRUE(adapter->want_write()); + // This will send data but not trailers, because the data source hasn't + // finished sending. + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0)); + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::DATA})); + visitor.Clear(); + + // Stream flow control window is exhausted. + EXPECT_EQ(adapter->GetStreamSendWindowSize(1), 0); + // Connection flow control window is available. + EXPECT_GT(adapter->GetSendWindowSize(), 0); + + // After a window update, the adapter will send the last data, followed by + // trailers. + EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0)); + EXPECT_CALL(visitor, OnWindowUpdate(1, 2000)); + adapter->ProcessBytes(TestFrameSequence().WindowUpdate(1, 2000).Serialize()); + + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, + END_STREAM_FLAG | END_HEADERS_FLAG)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, + END_STREAM_FLAG | END_HEADERS_FLAG, 0)); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), + EqualsFrames({SpdyFrameType::DATA, SpdyFrameType::HEADERS})); + EXPECT_FALSE(adapter->want_write()); +} + TEST(OgHttp2AdapterTest, ServerSubmitsTrailersWithDataEndStream) { DataSavingVisitor visitor; OgHttp2Adapter::Options options;
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 0242f40..ad67a18 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -632,6 +632,7 @@ const Http2StreamId stream_id = *trailers_ready_.begin(); // WriteForStream() will re-mark the stream as ready, if necessary. write_scheduler_.MarkStreamNotReady(stream_id); + trailers_ready_.erase(trailers_ready_.begin()); return stream_id; } return write_scheduler_.PopNextReadyStream();