gfe-relnote: Close connection with H3_CLOSED_CRITICAL_STREAM if peer closes a critical stream. Protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27. Note that write (send) unidirectional streams can only receive STOP_SENDING, and read (received) unidirectional streams can only receive RESET_STREAM. The wrong kind of frame would not reach the stream object from the transport layer. PiperOrigin-RevId: 300733944 Change-Id: I2780dc928b6f9ed093854329378c12dcd94ecb0a
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index d87ec83..f77bdea 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -160,10 +160,9 @@ void QuicReceiveControlStream::OnStreamReset( const QuicRstStreamFrame& /*frame*/) { - // TODO(renjietang) Change the error code to H/3 specific - // HTTP_CLOSED_CRITICAL_STREAM. - stream_delegate()->OnStreamError(QUIC_INVALID_STREAM_ID, - "Attempt to reset receive control stream"); + stream_delegate()->OnStreamError( + QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "RESET_STREAM received for receive control stream"); } void QuicReceiveControlStream::OnDataAvailable() {
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 0997fc0..8bbf789 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -147,7 +147,8 @@ QuicRstStreamFrame rst_frame(kInvalidControlFrameId, receive_control_stream_->id(), QUIC_STREAM_CANCELLED, 1234); - EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)); + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, _, _)); receive_control_stream_->OnStreamReset(rst_frame); }
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc index 0d8616c..01c03dd 100644 --- a/quic/core/http/quic_send_control_stream.cc +++ b/quic/core/http/quic_send_control_stream.cc
@@ -32,10 +32,14 @@ max_inbound_header_list_size_(max_inbound_header_list_size) {} void QuicSendControlStream::OnStreamReset(const QuicRstStreamFrame& /*frame*/) { - // TODO(renjietang) Change the error code to H/3 specific - // HTTP_CLOSED_CRITICAL_STREAM. - stream_delegate()->OnStreamError(QUIC_INVALID_STREAM_ID, - "Attempt to reset send control stream"); + QUIC_BUG << "OnStreamReset() called for write unidirectional stream."; +} + +bool QuicSendControlStream::OnStopSending(uint16_t /* code */) { + stream_delegate()->OnStreamError( + QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "STOP_SENDING received for send control stream"); + return false; } void QuicSendControlStream::MaybeSendSettingsFrame() {
diff --git a/quic/core/http/quic_send_control_stream.h b/quic/core/http/quic_send_control_stream.h index 0095317..899abb9 100644 --- a/quic/core/http/quic_send_control_stream.h +++ b/quic/core/http/quic_send_control_stream.h
@@ -30,9 +30,10 @@ QuicSendControlStream& operator=(const QuicSendControlStream&) = delete; ~QuicSendControlStream() override = default; - // Overriding QuicStream::OnStreamReset to make sure control stream is never + // Overriding QuicStream::OnStopSending() to make sure control stream is never // closed before connection. void OnStreamReset(const QuicRstStreamFrame& frame) override; + bool OnStopSending(uint16_t code) override; // Send SETTINGS frame if it hasn't been sent yet. Settings frame must be the // first frame sent on this stream.
diff --git a/quic/core/http/quic_send_control_stream_test.cc b/quic/core/http/quic_send_control_stream_test.cc index a5ff591..8332cec 100644 --- a/quic/core/http/quic_send_control_stream_test.cc +++ b/quic/core/http/quic_send_control_stream_test.cc
@@ -185,13 +185,11 @@ send_control_stream_->WritePriorityUpdate(frame); } -TEST_P(QuicSendControlStreamTest, ResetControlStream) { +TEST_P(QuicSendControlStreamTest, CloseControlStream) { Initialize(); - QuicRstStreamFrame rst_frame(kInvalidControlFrameId, - send_control_stream_->id(), - QUIC_STREAM_CANCELLED, 1234); - EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)); - send_control_stream_->OnStreamReset(rst_frame); + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, _, _)); + send_control_stream_->OnStopSending(QUIC_STREAM_CANCELLED); } TEST_P(QuicSendControlStreamTest, ReceiveDataOnSendControlStream) {
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 0412a76..408b0fa 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -1187,6 +1187,7 @@ TEST_P(QuicSpdySessionTestServer, OnRstStreamStaticStreamId) { QuicStreamId id; + QuicErrorCode expected_error; std::string error_message; // Initialize HTTP/3 control stream. if (VersionUsesHttp3(transport_version())) { @@ -1195,9 +1196,11 @@ QuicStreamFrame data1(id, false, 0, quiche::QuicheStringPiece(type, 1)); session_.OnStreamFrame(data1); - error_message = "Attempt to reset receive control stream"; + expected_error = QUIC_HTTP_CLOSED_CRITICAL_STREAM; + error_message = "RESET_STREAM received for receive control stream"; } else { id = QuicUtils::GetHeadersStreamId(transport_version()); + expected_error = QUIC_INVALID_STREAM_ID; error_message = "Attempt to reset headers stream"; } @@ -1206,7 +1209,7 @@ QUIC_ERROR_PROCESSING_STREAM, 0); EXPECT_CALL( *connection_, - CloseConnection(QUIC_INVALID_STREAM_ID, error_message, + CloseConnection(expected_error, error_message, ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET)); session_.OnRstStream(rst1); } @@ -2879,6 +2882,82 @@ session_.OnStreamFrame(data); } +TEST_P(QuicSpdySessionTestServer, PeerClosesCriticalReceiveStream) { + if (!VersionUsesHttp3(transport_version())) { + return; + } + + struct { + char type; + const char* error_details; + } kTestData[] = { + {kControlStream, "RESET_STREAM received for receive control stream"}, + {kQpackEncoderStream, "RESET_STREAM received for QPACK receive stream"}, + {kQpackDecoderStream, "RESET_STREAM received for QPACK receive stream"}, + }; + for (size_t i = 0; i < QUICHE_ARRAYSIZE(kTestData); ++i) { + QuicStreamId stream_id = + GetNthClientInitiatedUnidirectionalStreamId(transport_version(), i + 1); + const QuicByteCount data_length = 1; + QuicStreamFrame data( + stream_id, false, 0, + quiche::QuicheStringPiece(&kTestData[i].type, data_length)); + session_.OnStreamFrame(data); + + EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, + kTestData[i].error_details, _)); + + QuicRstStreamFrame rst(kInvalidControlFrameId, stream_id, + QUIC_STREAM_CANCELLED, data_length); + session_.OnRstStream(rst); + } +} + +TEST_P(QuicSpdySessionTestServer, PeerClosesCriticalSendStream) { + if (!VersionUsesHttp3(transport_version())) { + return; + } + + QuicSendControlStream* control_stream = + QuicSpdySessionPeer::GetSendControlStream(&session_); + ASSERT_TRUE(control_stream); + + QuicStopSendingFrame stop_sending_control_stream( + kInvalidControlFrameId, control_stream->id(), + static_cast<QuicApplicationErrorCode>(QUIC_STREAM_CANCELLED)); + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "STOP_SENDING received for send control stream", _)); + session_.OnStopSendingFrame(stop_sending_control_stream); + + QpackSendStream* decoder_stream = + QuicSpdySessionPeer::GetQpackDecoderSendStream(&session_); + ASSERT_TRUE(decoder_stream); + + QuicStopSendingFrame stop_sending_decoder_stream( + kInvalidControlFrameId, decoder_stream->id(), + static_cast<QuicApplicationErrorCode>(QUIC_STREAM_CANCELLED)); + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "STOP_SENDING received for QPACK send stream", _)); + session_.OnStopSendingFrame(stop_sending_decoder_stream); + + QpackSendStream* encoder_stream = + QuicSpdySessionPeer::GetQpackEncoderSendStream(&session_); + ASSERT_TRUE(encoder_stream); + + QuicStopSendingFrame stop_sending_encoder_stream( + kInvalidControlFrameId, encoder_stream->id(), + static_cast<QuicApplicationErrorCode>(QUIC_STREAM_CANCELLED)); + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "STOP_SENDING received for QPACK send stream", _)); + session_.OnStopSendingFrame(stop_sending_encoder_stream); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/qpack/qpack_receive_stream.cc b/quic/core/qpack/qpack_receive_stream.cc index f896ca4..d2fc88b 100644 --- a/quic/core/qpack/qpack_receive_stream.cc +++ b/quic/core/qpack/qpack_receive_stream.cc
@@ -14,10 +14,9 @@ receiver_(receiver) {} void QpackReceiveStream::OnStreamReset(const QuicRstStreamFrame& /*frame*/) { - // TODO(renjietang) Change the error code to H/3 specific - // HTTP_CLOSED_CRITICAL_STREAM. - stream_delegate()->OnStreamError(QUIC_INVALID_STREAM_ID, - "Attempt to reset Qpack receive stream"); + stream_delegate()->OnStreamError( + QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "RESET_STREAM received for QPACK receive stream"); } void QpackReceiveStream::OnDataAvailable() {
diff --git a/quic/core/qpack/qpack_receive_stream_test.cc b/quic/core/qpack/qpack_receive_stream_test.cc index 6aeeeae..bc47d9d 100644 --- a/quic/core/qpack/qpack_receive_stream_test.cc +++ b/quic/core/qpack/qpack_receive_stream_test.cc
@@ -86,7 +86,8 @@ QuicRstStreamFrame rst_frame(kInvalidControlFrameId, qpack_receive_stream_->id(), QUIC_STREAM_CANCELLED, 1234); - EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)); + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, _, _)); qpack_receive_stream_->OnStreamReset(rst_frame); }
diff --git a/quic/core/qpack/qpack_send_stream.cc b/quic/core/qpack/qpack_send_stream.cc index 57493e3..15525b5 100644 --- a/quic/core/qpack/qpack_send_stream.cc +++ b/quic/core/qpack/qpack_send_stream.cc
@@ -17,10 +17,14 @@ stream_type_sent_(false) {} void QpackSendStream::OnStreamReset(const QuicRstStreamFrame& /*frame*/) { - // TODO(renjietang) Change the error code to H/3 specific - // HTTP_CLOSED_CRITICAL_STREAM. - stream_delegate()->OnStreamError(QUIC_INVALID_STREAM_ID, - "Attempt to reset qpack send stream"); + QUIC_BUG << "OnStreamReset() called for write unidirectional stream."; +} + +bool QpackSendStream::OnStopSending(uint16_t /* code */) { + stream_delegate()->OnStreamError( + QUIC_HTTP_CLOSED_CRITICAL_STREAM, + "STOP_SENDING received for QPACK send stream"); + return false; } void QpackSendStream::WriteStreamData(quiche::QuicheStringPiece data) {
diff --git a/quic/core/qpack/qpack_send_stream.h b/quic/core/qpack/qpack_send_stream.h index bc2d9c0..50d2808 100644 --- a/quic/core/qpack/qpack_send_stream.h +++ b/quic/core/qpack/qpack_send_stream.h
@@ -30,9 +30,10 @@ QpackSendStream& operator=(const QpackSendStream&) = delete; ~QpackSendStream() override = default; - // Overriding QuicStream::OnStreamReset to make sure QPACK stream is - // never closed before connection. + // Overriding QuicStream::OnStopSending() to make sure QPACK stream is never + // closed before connection. void OnStreamReset(const QuicRstStreamFrame& frame) override; + bool OnStopSending(uint16_t code) override; // The send QPACK stream is write unidirectional, so this method // should never be called.
diff --git a/quic/core/qpack/qpack_send_stream_test.cc b/quic/core/qpack/qpack_send_stream_test.cc index d6a4df7..c77a621 100644 --- a/quic/core/qpack/qpack_send_stream_test.cc +++ b/quic/core/qpack/qpack_send_stream_test.cc
@@ -106,11 +106,10 @@ qpack_send_stream_->MaybeSendStreamType(); } -TEST_P(QpackSendStreamTest, ResetQpackStream) { - QuicRstStreamFrame rst_frame(kInvalidControlFrameId, qpack_send_stream_->id(), - QUIC_STREAM_CANCELLED, 1234); - EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)); - qpack_send_stream_->OnStreamReset(rst_frame); +TEST_P(QpackSendStreamTest, StopSendingQpackStream) { + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_CLOSED_CRITICAL_STREAM, _, _)); + qpack_send_stream_->OnStopSending(QUIC_STREAM_CANCELLED); } TEST_P(QpackSendStreamTest, ReceiveDataOnSendStream) {
diff --git a/quic/core/quic_error_codes.cc b/quic/core/quic_error_codes.cc index e24e839..d897aac 100644 --- a/quic/core/quic_error_codes.cc +++ b/quic/core/quic_error_codes.cc
@@ -174,6 +174,7 @@ RETURN_STRING_LITERAL(QUIC_HTTP_DUPLICATE_UNIDIRECTIONAL_STREAM); RETURN_STRING_LITERAL(QUIC_HTTP_SERVER_INITIATED_BIDIRECTIONAL_STREAM); RETURN_STRING_LITERAL(QUIC_HTTP_STREAM_WRONG_DIRECTION); + RETURN_STRING_LITERAL(QUIC_HTTP_CLOSED_CRITICAL_STREAM); RETURN_STRING_LITERAL(QUIC_HPACK_INDEX_VARINT_ERROR); RETURN_STRING_LITERAL(QUIC_HPACK_NAME_LENGTH_VARINT_ERROR); RETURN_STRING_LITERAL(QUIC_HPACK_VALUE_LENGTH_VARINT_ERROR);
diff --git a/quic/core/quic_error_codes.h b/quic/core/quic_error_codes.h index 3ef34a5..dc575d3 100644 --- a/quic/core/quic_error_codes.h +++ b/quic/core/quic_error_codes.h
@@ -373,6 +373,9 @@ // Server opens stream with stream ID corresponding to client-initiated // stream or vice versa. QUIC_HTTP_STREAM_WRONG_DIRECTION = 155, + // Peer closes one of the six critical unidirectional streams (control, QPACK + // encoder or decoder, in either direction). + QUIC_HTTP_CLOSED_CRITICAL_STREAM = 156, // HPACK header block decoding errors. // Index varint beyond implementation limit. @@ -409,7 +412,7 @@ QUIC_HPACK_COMPRESSED_HEADER_SIZE_EXCEEDS_LIMIT = 150, // No error. Used as bound while iterating. - QUIC_LAST_ERROR = 156, + QUIC_LAST_ERROR = 157, }; // QuicErrorCodes is encoded as four octets on-the-wire when doing Google QUIC, // or a varint62 when doing IETF QUIC. Ensure that its value does not exceed
diff --git a/quic/core/quic_types.cc b/quic/core/quic_types.cc index 5198eec..6282358 100644 --- a/quic/core/quic_types.cc +++ b/quic/core/quic_types.cc
@@ -489,6 +489,10 @@ QuicHttp3ErrorCode::IETF_QUIC_HTTP3_STREAM_CREATION_ERROR)}}; case QUIC_HTTP_STREAM_WRONG_DIRECTION: return {true, {static_cast<uint64_t>(STREAM_STATE_ERROR)}}; + case QUIC_HTTP_CLOSED_CRITICAL_STREAM: + return {false, + {static_cast<uint64_t>( + QuicHttp3ErrorCode::IETF_QUIC_HTTP3_CLOSED_CRITICAL_STREAM)}}; case QUIC_HPACK_INDEX_VARINT_ERROR: return {false, {static_cast<uint64_t>(QUIC_HPACK_INDEX_VARINT_ERROR)}}; case QUIC_HPACK_NAME_LENGTH_VARINT_ERROR: