Do not send empty frame if stream is reset.
If a QUIC version other than v99 is used (that is, Google QUIC), and an empty header list (or a too large header list that becomes empty by the way QuicHeaderList enforces size limit) with FIN is received by QuicSpdyStream::OnStreamHeaderList(), then the stream is reset by OnHeadersTooLarge(), then OnInitialHeadersComplete() sends an empty frame to OnStreamFrame(). If the write side of the stream is already closed, then this triggers the following DCHECK:
quic_stream.cc:300 Check failed: !(read_side_closed_ && write_side_closed_)
This CL prevents that frame write if the stream is already reset.
The same issue does not arise with trailers, because if QuicHeaderList is empty, then SpdyUtils::CopyAndValidateTrailers() called from QuicSpdyStream::OnTrailingHeadersComplete() fails before the empty QuicStreamFrame is sent out.
I locally verified that the added test crashes without the fix.
gfe-relnote: Prevent DCHECK from firing. TODO Does this need to be flag protected?
PiperOrigin-RevId: 253256678
Change-Id: Ib5e6249fb850388637fd684f97f1ea93dc02e980
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 392ddc4..fc1ae0a 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -1668,6 +1668,35 @@
&session_, GetNthClientInitiatedBidirectionalId(0)));
}
+// Regression test for b/130740258 and https://crbug.com/971779.
+// If headers that are too large or empty are received (these cases are handled
+// the same way, as QuicHeaderList clears itself when headers exceed the limit),
+// then the stream is reset. No more frames must be sent in this case.
+TEST_P(QuicSpdySessionTestClient, TooLargeHeadersMustNotCauseWriteAfterReset) {
+ // In IETF QUIC, HEADERS do not carry FIN flag, and OnStreamHeaderList() is
+ // never called after an error, including too large headers.
+ if (VersionUsesQpack(transport_version())) {
+ return;
+ }
+
+ SetQuicReloadableFlag(quic_avoid_empty_frame_after_empty_headers, true);
+
+ TestStream* stream = session_.CreateOutgoingBidirectionalStream();
+
+ // Write headers with FIN set to close write side of stream.
+ // Header block does not matter.
+ stream->WriteHeaders(SpdyHeaderBlock(), /* fin = */ true, nullptr);
+
+ // Receive headers that are too large or empty, with FIN set.
+ // This causes the stream to be reset. No frames must be written after this.
+ QuicHeaderList headers;
+ EXPECT_CALL(*connection_, SendControlFrame(_));
+ EXPECT_CALL(*connection_,
+ OnStreamReset(stream->id(), QUIC_HEADERS_TOO_LARGE));
+ stream->OnStreamHeaderList(/* fin = */ true,
+ headers.uncompressed_header_bytes(), headers);
+}
+
TEST_P(QuicSpdySessionTestClient, RecordFinAfterReadSideClosed) {
// Verify that an incoming FIN is recorded in a stream object even if the read
// side has been closed. This prevents an entry from being made in
