Update goaway behavior. protected by gfe2_reloadable_flag_quic_http3_goaway_new_behavior. Updating behavior to https://github.com/quicwg/base-drafts/pull/3129. If client receives GOAWAY with Stream ID that is not client-initiated bidirectional stream ID, it now closes connection with application error H3_ID_ERROR instead of transport error PROTOCOL_VIOLATION (draft-04 behavior). If server receives GOAWAY, it ignores it instead of closing connection with H3_FRAME_UNEXPECTED (draft-27 behavior). Stream ID/push ID carried by GOAWAY frame is ignored in all cases. This updates behavior to draft-28. While we are still supporting draft-27, this should be okay as the new behavior is more permissive. PiperOrigin-RevId: 323358652 Change-Id: I17919364a259a38a110284bab0023779b194094e
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index e0e51a7..82d1fe1 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -10,6 +10,8 @@ #include "net/third_party/quiche/src/quic/core/http/http_decoder.h" #include "net/third_party/quiche/src/quic/core/http/quic_spdy_session.h" #include "net/third_party/quiche/src/quic/core/quic_types.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_flag_utils.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" #include "net/third_party/quiche/src/common/platform/api/quiche_text_utils.h" @@ -105,7 +107,9 @@ return false; } - if (spdy_session()->perspective() == Perspective::IS_SERVER) { + if (GetQuicReloadableFlag(quic_http3_goaway_new_behavior)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_http3_goaway_new_behavior); + } else if (spdy_session()->perspective() == Perspective::IS_SERVER) { OnWrongFrame("Go Away"); return false; }
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 91e09f2..6e677d7 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -294,14 +294,16 @@ EXPECT_CALL(debug_visitor, OnGoAwayFrameReceived(goaway)); - if (perspective() == Perspective::IS_SERVER) { + if (!GetQuicReloadableFlag(quic_http3_goaway_new_behavior) && + perspective() == Perspective::IS_SERVER) { EXPECT_CALL( *connection_, CloseConnection(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, _, _)); } receive_control_stream_->OnStreamFrame(frame); - if (perspective() == Perspective::IS_CLIENT) { + if (GetQuicReloadableFlag(quic_http3_goaway_new_behavior) || + perspective() == Perspective::IS_CLIENT) { EXPECT_TRUE(session_.http3_goaway_received()); } }
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 63b6b87..e3f6dcf 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -404,7 +404,6 @@ server_push_enabled_(true), ietf_server_push_enabled_( GetQuicFlag(FLAGS_quic_enable_http3_server_push)), - http3_goaway_received_(false), http3_goaway_sent_(false), http3_max_push_id_sent_(false) { h2_deframer_.set_visitor(spdy_framer_visitor_.get()); @@ -657,6 +656,41 @@ } void QuicSpdySession::OnHttp3GoAway(uint64_t id) { + if (GetQuicReloadableFlag(quic_http3_goaway_new_behavior)) { + if (last_received_http3_goaway_id_.has_value() && + id > last_received_http3_goaway_id_.value()) { + CloseConnectionWithDetails( + QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS, + quiche::QuicheStrCat("GOAWAY received with ID ", id, + " greater than previously received ID ", + last_received_http3_goaway_id_.value())); + return; + } + last_received_http3_goaway_id_ = id; + + if (perspective() == Perspective::IS_SERVER) { + // TODO(b/151749109): Cancel server pushes with push ID larger than |id|. + return; + } + + // QuicStreamId is uint32_t. Casting to this narrower type is well-defined + // and preserves the lower 32 bits. Both IsBidirectionalStreamId() and + // IsIncomingStream() give correct results, because their return value is + // determined by the least significant two bits. + QuicStreamId stream_id = static_cast<QuicStreamId>(id); + if (!QuicUtils::IsBidirectionalStreamId(stream_id, version()) || + IsIncomingStream(stream_id)) { + CloseConnectionWithDetails(QUIC_HTTP_GOAWAY_INVALID_STREAM_ID, + "GOAWAY with invalid stream ID"); + return; + } + + // TODO(b/161252736): Cancel client requests with ID larger than |id|. + // If |id| is larger than numeric_limits<QuicStreamId>::max(), then use + // max() instead of downcast value. + return; + } + DCHECK_EQ(perspective(), Perspective::IS_CLIENT); // QuicStreamId is uint32_t. Casting to this narrower type is well-defined @@ -671,7 +705,7 @@ "GOAWAY's last stream id has to point to a request stream"); return; } - http3_goaway_received_ = true; + last_received_http3_goaway_id_ = id; } bool QuicSpdySession::OnStreamsBlockedFrame(
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h index 9342da2..6dc65f3 100644 --- a/quic/core/http/quic_spdy_session.h +++ b/quic/core/http/quic_spdy_session.h
@@ -351,7 +351,9 @@ Http3DebugVisitor* debug_visitor() { return debug_visitor_; } - bool http3_goaway_received() const { return http3_goaway_received_; } + bool http3_goaway_received() const { + return last_received_http3_goaway_id_.has_value(); + } bool http3_goaway_sent() const { return http3_goaway_sent_; } @@ -577,8 +579,9 @@ // Server push is enabled for a client by calling SetMaxPushId(). bool ietf_server_push_enabled_; - // If the endpoint has received HTTP/3 GOAWAY frame. - bool http3_goaway_received_; + // The identifier in the most recently received GOAWAY frame. Unset if no + // GOAWAY frame has been received yet. + quiche::QuicheOptional<uint64_t> last_received_http3_goaway_id_; // If the endpoint has sent HTTP/3 GOAWAY frame. bool http3_goaway_sent_;
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index df83890..18b3951 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -1114,6 +1114,29 @@ session_.OnGoAway(go_away); } +TEST_P(QuicSpdySessionTestServer, Http3GoAwayLargerIdThanBefore) { + if (!VersionUsesHttp3(transport_version())) { + return; + } + if (!GetQuicReloadableFlag(quic_http3_goaway_new_behavior)) { + return; + } + + EXPECT_FALSE(session_.http3_goaway_received()); + PushId push_id1 = 0; + session_.OnHttp3GoAway(push_id1); + EXPECT_TRUE(session_.http3_goaway_received()); + + EXPECT_CALL( + *connection_, + CloseConnection( + QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS, + "GOAWAY received with ID 1 greater than previously received ID 0", + _)); + PushId push_id2 = 1; + session_.OnHttp3GoAway(push_id2); +} + // Test that server session will send a connectivity probe in response to a // connectivity probe on the same path. TEST_P(QuicSpdySessionTestServer, ServerReplyToConnecitivityProbe) { @@ -2831,16 +2854,47 @@ if (!VersionUsesHttp3(transport_version())) { return; } - EXPECT_CALL( - *connection_, - CloseConnection( - QUIC_INVALID_STREAM_ID, - "GOAWAY's last stream id has to point to a request stream", _)); + if (GetQuicReloadableFlag(quic_http3_goaway_new_behavior)) { + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_GOAWAY_INVALID_STREAM_ID, + "GOAWAY with invalid stream ID", _)); + } else { + EXPECT_CALL( + *connection_, + CloseConnection( + QUIC_INVALID_STREAM_ID, + "GOAWAY's last stream id has to point to a request stream", _)); + } QuicStreamId stream_id = GetNthServerInitiatedUnidirectionalStreamId(transport_version(), 0); session_.OnHttp3GoAway(stream_id); } +TEST_P(QuicSpdySessionTestClient, Http3GoAwayLargerIdThanBefore) { + if (!VersionUsesHttp3(transport_version())) { + return; + } + if (!GetQuicReloadableFlag(quic_http3_goaway_new_behavior)) { + return; + } + + EXPECT_FALSE(session_.http3_goaway_received()); + QuicStreamId stream_id1 = + GetNthClientInitiatedBidirectionalStreamId(transport_version(), 0); + session_.OnHttp3GoAway(stream_id1); + EXPECT_TRUE(session_.http3_goaway_received()); + + EXPECT_CALL( + *connection_, + CloseConnection( + QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS, + "GOAWAY received with ID 4 greater than previously received ID 0", + _)); + QuicStreamId stream_id2 = + GetNthClientInitiatedBidirectionalStreamId(transport_version(), 1); + session_.OnHttp3GoAway(stream_id2); +} + // Test that receipt of CANCEL_PUSH frame does not result in closing the // connection. // TODO(b/151841240): Handle CANCEL_PUSH frames instead of ignoring them.
diff --git a/quic/core/quic_error_codes.cc b/quic/core/quic_error_codes.cc index 1c0984a..7065363 100644 --- a/quic/core/quic_error_codes.cc +++ b/quic/core/quic_error_codes.cc
@@ -205,6 +205,8 @@ RETURN_STRING_LITERAL(QUIC_HTTP_STREAM_LIMIT_TOO_LOW); RETURN_STRING_LITERAL(QUIC_HTTP_ZERO_RTT_RESUMPTION_SETTINGS_MISMATCH); RETURN_STRING_LITERAL(QUIC_HTTP_ZERO_RTT_REJECTION_SETTINGS_MISMATCH); + RETURN_STRING_LITERAL(QUIC_HTTP_GOAWAY_INVALID_STREAM_ID); + RETURN_STRING_LITERAL(QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS); 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); @@ -576,6 +578,10 @@ return {false, static_cast<uint64_t>(QuicHttp3ErrorCode::SETTINGS_ERROR)}; case QUIC_HTTP_ZERO_RTT_REJECTION_SETTINGS_MISMATCH: return {true, static_cast<uint64_t>(INTERNAL_ERROR)}; + case QUIC_HTTP_GOAWAY_INVALID_STREAM_ID: + return {false, static_cast<uint64_t>(QuicHttp3ErrorCode::ID_ERROR)}; + case QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS: + return {false, static_cast<uint64_t>(QuicHttp3ErrorCode::ID_ERROR)}; case QUIC_HPACK_INDEX_VARINT_ERROR: return {true, static_cast<uint64_t>(INTERNAL_ERROR)}; case QUIC_HPACK_NAME_LENGTH_VARINT_ERROR:
diff --git a/quic/core/quic_error_codes.h b/quic/core/quic_error_codes.h index 4c13f2c..0c990c7 100644 --- a/quic/core/quic_error_codes.h +++ b/quic/core/quic_error_codes.h
@@ -437,6 +437,11 @@ // Received mismatched SETTINGS frame from HTTP/3 connection where early data // is rejected. Our implementation currently doesn't support it. QUIC_HTTP_ZERO_RTT_REJECTION_SETTINGS_MISMATCH = 165, + // Client received GOAWAY frame with stream ID that is not for a + // client-initiated bidirectional stream. + QUIC_HTTP_GOAWAY_INVALID_STREAM_ID = 166, + // Received GOAWAY frame with ID that is greater than previously received ID. + QUIC_HTTP_GOAWAY_ID_LARGER_THAN_PREVIOUS = 167, // HPACK header block decoding errors. // Index varint beyond implementation limit. @@ -485,7 +490,7 @@ QUIC_ZERO_RTT_RESUMPTION_LIMIT_REDUCED = 163, // No error. Used as bound while iterating. - QUIC_LAST_ERROR = 166, + QUIC_LAST_ERROR = 168, }; // 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