Update GOAWAY format to MOQT draft-18. This is just the parser/framer changes; handling of new fields will be in the next CL. PiperOrigin-RevId: 990510694
diff --git a/quiche/quic/moqt/moqt_framer.cc b/quiche/quic/moqt/moqt_framer.cc index aa4d9f1..663835a 100644 --- a/quiche/quic/moqt/moqt_framer.cc +++ b/quiche/quic/moqt/moqt_framer.cc
@@ -20,6 +20,7 @@ #include "absl/strings/string_view.h" #include "absl/types/span.h" #include "quiche/quic/core/quic_time.h" +#include "quiche/quic/core/quic_types.h" #include "quiche/quic/moqt/moqt_error.h" #include "quiche/quic/moqt/moqt_key_value_pair.h" #include "quiche/quic/moqt/moqt_messages.h" @@ -582,9 +583,22 @@ } quiche::QuicheBuffer MoqtFramer::SerializeGoAway(const MoqtGoAway& message) { + if (perspective_ == quic::Perspective::IS_CLIENT && + !message.new_session_uri.empty()) { + QUICHE_BUG(QUICHE_BUG_serialize_go_away_01) + << "New session URI must be empty from client"; + return quiche::QuicheBuffer(); + } + if (message.new_session_uri.length() > kMaxNewSessionUriLength) { + QUICHE_BUG(QUICHE_BUG_serialize_go_away_02) + << "New session URI is too long"; + return quiche::QuicheBuffer(); + } return SerializeControlMessage( MoqtMessageType::kGoAway, - WireStringWithMoqVarIntLength(message.new_session_uri)); + WireStringWithMoqVarIntLength(message.new_session_uri), + WireMoqVarInt(message.timeout.ToMilliseconds()), + WireOptional<WireMoqVarInt>(message.request_id)); } quiche::QuicheBuffer MoqtFramer::SerializeSubscribeNamespace(
diff --git a/quiche/quic/moqt/moqt_framer_test.cc b/quiche/quic/moqt/moqt_framer_test.cc index 0be2932..54e53d1 100644 --- a/quiche/quic/moqt/moqt_framer_test.cc +++ b/quiche/quic/moqt/moqt_framer_test.cc
@@ -13,6 +13,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/moqt/moqt_key_value_pair.h" #include "quiche/quic/moqt/moqt_messages.h" @@ -69,6 +70,11 @@ message_type, uses_web_transport, perspective)); } } + } else if (message_type == MoqtMessageType::kGoAway) { + // GoAwayMessage has a non-empty new_session_uri, which is only valid + // when sent by the server. + params.push_back(MoqtFramerTestParams(message_type, true, + quic::Perspective::IS_SERVER)); } else { // All other types are processed the same for either perspective or // transport. @@ -434,4 +440,36 @@ EXPECT_EQ(buffer.AsStringView(), message.PacketSample()); } +TEST_F(MoqtFramerSimpleTest, GoAwayNewSessionUriTooLong) { + MoqtGoAway goaway = { + /*new_session_uri=*/std::string(kMaxNewSessionUriLength, 'a'), + /*timeout=*/quic::QuicTimeDelta::FromMilliseconds(3), + /*request_id=*/std::nullopt, + }; + quiche::QuicheBuffer buffer = framer_.SerializeGoAway(goaway); + EXPECT_GT(buffer.size(), 0); + + goaway.new_session_uri = std::string(kMaxNewSessionUriLength + 1, 'a'); + EXPECT_QUIC_BUG(buffer = framer_.SerializeGoAway(goaway), + "New session URI is too long"); + EXPECT_EQ(buffer.size(), 0); +} + +TEST_F(MoqtFramerSimpleTest, GoAwayNewSessionUriFromClient) { + MoqtFramer client_framer(/*using_webtrans=*/true, + quic::Perspective::IS_CLIENT); + MoqtGoAway goaway = { + /*new_session_uri=*/"", + /*timeout=*/quic::QuicTimeDelta::FromMilliseconds(3), + /*request_id=*/std::nullopt, + }; + quiche::QuicheBuffer buffer = client_framer.SerializeGoAway(goaway); + EXPECT_GT(buffer.size(), 0); + + goaway.new_session_uri = "foo"; + EXPECT_QUIC_BUG(buffer = client_framer.SerializeGoAway(goaway), + "New session URI must be empty from client"); + EXPECT_EQ(buffer.size(), 0); +} + } // namespace moqt::test
diff --git a/quiche/quic/moqt/moqt_messages.h b/quiche/quic/moqt/moqt_messages.h index a64a33d..149fa69 100644 --- a/quiche/quic/moqt/moqt_messages.h +++ b/quiche/quic/moqt/moqt_messages.h
@@ -35,7 +35,8 @@ // The maximum length of a message, excluding any OBJECT payload. This prevents // DoS attack via forcing the parser to buffer a large message (OBJECT payloads // are not buffered by the parser). -inline constexpr size_t kMaxMessageHeaderSize = 2048; +inline constexpr size_t kMaxMessageHeaderSize = 16384; +static constexpr size_t kMaxNewSessionUriLength = 8192; inline constexpr uint64_t kPaddingStreamType = 0x132B3E28; inline constexpr uint64_t kPaddingDatagramType = 0x132B3E29; @@ -406,6 +407,8 @@ struct QUICHE_EXPORT MoqtGoAway { std::string new_session_uri; + quic::QuicTimeDelta timeout = quic::QuicTimeDelta::Zero(); + std::optional<uint64_t> request_id; }; struct QUICHE_EXPORT MoqtSubscribeNamespace {
diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc index 72dc57a..a9518a8 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc
@@ -789,9 +789,30 @@ absl::string_view data) const { quic::QuicDataReader reader(data); MoqtGoAway goaway; + uint64_t raw_timeout; if (!reader.ReadStringMoqVarInt(goaway.new_session_uri)) { return absl::InvalidArgumentError("Missing new session URI"); } + if (perspective_ == quic::Perspective::IS_SERVER && + !goaway.new_session_uri.empty()) { + return absl::InvalidArgumentError( + "New session URI must be empty from client"); + } + if (goaway.new_session_uri.length() > kMaxNewSessionUriLength) { + return absl::InvalidArgumentError("New session URI too long"); + } + if (!reader.ReadMoqVarInt(&raw_timeout)) { + return absl::InvalidArgumentError("Missing GOAWAY timeout"); + } + goaway.timeout = quic::QuicTimeDelta::TryFromMilliseconds(raw_timeout) + .value_or(quic::QuicTimeDelta::Infinite()); + if (!reader.IsDoneReading()) { + uint64_t request_id; + if (!reader.ReadMoqVarInt(&request_id)) { + return absl::InvalidArgumentError("Invalid GOAWAY request ID"); + } + goaway.request_id = request_id; + } QUICHE_RETURN_IF_ERROR(CheckForTrailingData(reader)); return goaway; }
diff --git a/quiche/quic/moqt/moqt_parser_test.cc b/quiche/quic/moqt/moqt_parser_test.cc index 95d0f33..fda2c12 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc
@@ -100,6 +100,11 @@ message_type, uses_web_transport, perspective)); } } + } else if (message_type == MoqtMessageType::kGoAway) { + // GoAwayMessage has a non-empty new_session_uri, which is only valid + // when received by the client. + params.push_back(MoqtParserTestParams(message_type, true, + quic::Perspective::IS_CLIENT)); } else { // All other types are processed the same for either perspective or // transport. @@ -408,6 +413,10 @@ } std::unique_ptr<TestMessageBase> message = MakeMessage(); message->DecreasePayloadLengthByOne(); + if (std::get<MoqtMessageType>(message_type_) == MoqtMessageType::kGoAway) { + // GOAWAY has a variant that is one byte shorter, so decrease by another. + message->DecreasePayloadLengthByOne(); + } ProcessData(message->PacketSample(), false); EXPECT_EQ(messages_received(), 0); EXPECT_TRUE(parsing_error().has_value()); @@ -1285,11 +1294,11 @@ std::string buffer; for (MoqtMessageType type : kMessageTypes) { std::unique_ptr<TestMessageBase> message = - CreateTestMessage(type, kRawQuic); + CreateTestMessage(type, kRawQuic, quic::Perspective::IS_SERVER); buffer += message->PacketSample(); } - absl::StatusOr<std::vector<AnyMoqtControlMessage>> parsed = - ParseAllMessages(buffer, kDefaultMoqtVersion, kRawQuic); + absl::StatusOr<std::vector<AnyMoqtControlMessage>> parsed = ParseAllMessages( + buffer, kDefaultMoqtVersion, kRawQuic, quic::Perspective::IS_CLIENT); ASSERT_TRUE(parsed.ok()); } @@ -1877,4 +1886,62 @@ StatusIs(absl::StatusCode::kInternal, HasSubstr("moved-from parser"))); } +TEST_F(MoqtMessageSpecificTest, ParseGoAwayWithoutRequestId) { + GoAwayMessage goaway_message; + goaway_message.RequestStreamMessage(); + absl::StatusOr<std::vector<AnyMoqtControlMessage>> parsed = + ParseAllMessages(goaway_message.PacketSample(), kDefaultMoqtVersion, + kRawQuic, quic::Perspective::IS_CLIENT); + ASSERT_TRUE(parsed.ok()); + ASSERT_EQ(parsed->size(), 1); + EXPECT_TRUE( + goaway_message.EqualFieldValues(std::get<MoqtGoAway>((*parsed)[0]))); +} + +TEST_F(MoqtMessageSpecificTest, GoAwayNewSessionUriFromClient) { + GoAwayMessage goaway_message; + absl::StatusOr<std::vector<AnyMoqtControlMessage>> parsed = + ParseAllMessages(goaway_message.PacketSample(), kDefaultMoqtVersion, + kWebTrans, quic::Perspective::IS_SERVER); + EXPECT_THAT(parsed, + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("New session URI must be empty from client"))); +} + +TEST_F(MoqtMessageSpecificTest, GoAwayNewSessionUriTooLong) { + auto make_goaway_packet = [](size_t uri_length) { + std::string uri(uri_length, 'a'); + std::string buffer(kMaxMessageHeaderSize, '\0'); + quic::QuicDataWriter writer(buffer.size(), buffer.data()); + QUICHE_CHECK( + writer.WriteMoqVarInt(static_cast<uint64_t>(MoqtMessageType::kGoAway))); + // Payload is: 2-byte varint length (8192 or 8193) + URI bytes + 1-byte + // timeout. + uint16_t payload_length = 2 + uri.length() + 1; + QUICHE_CHECK(writer.WriteUInt16(payload_length)); + QUICHE_CHECK(writer.WriteMoqVarInt(uri.length())); + QUICHE_CHECK(writer.WriteStringPiece(uri)); + QUICHE_CHECK(writer.WriteMoqVarInt(0)); + buffer.resize(writer.length()); + return buffer; + }; + + // Exactly kMaxNewSessionUriLength succeeds. + std::string valid_packet = make_goaway_packet(kMaxNewSessionUriLength); + absl::StatusOr<std::vector<AnyMoqtControlMessage>> parsed = + ParseAllMessages(valid_packet, kDefaultMoqtVersion, kWebTrans, + quic::Perspective::IS_CLIENT); + ASSERT_TRUE(parsed.ok()); + ASSERT_EQ(parsed->size(), 1); + EXPECT_EQ(std::get<MoqtGoAway>((*parsed)[0]).new_session_uri.length(), + kMaxNewSessionUriLength); + + // kMaxNewSessionUriLength + 1 fails with "New session URI too long". + std::string invalid_packet = make_goaway_packet(kMaxNewSessionUriLength + 1); + parsed = ParseAllMessages(invalid_packet, kDefaultMoqtVersion, kWebTrans, + quic::Perspective::IS_CLIENT); + EXPECT_THAT(parsed, StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("New session URI too long"))); +} + } // namespace moqt::test
diff --git a/quiche/quic/moqt/moqt_session_test.cc b/quiche/quic/moqt/moqt_session_test.cc index 9cf0a33..73a9b5d 100644 --- a/quiche/quic/moqt/moqt_session_test.cc +++ b/quiche/quic/moqt/moqt_session_test.cc
@@ -2309,8 +2309,9 @@ TEST_F(MoqtSessionTest, ReceiveGoAwayEnforcement) { bidi_wrapper_ = MoqtSessionPeer::CreateControlStream(&session_, &mock_bidi_stream_); - EXPECT_CALL(session_callbacks_.goaway_received_callback, Call("foo")); - bidi_wrapper_->ReceiveMessage(MoqtGoAway("foo")); + EXPECT_CALL(session_callbacks_.goaway_received_callback, Call("")); + bidi_wrapper_->ReceiveMessage(MoqtGoAway("", quic::QuicTimeDelta::Zero(), + /*request_id=*/0)); // New requests not allowed. EXPECT_CALL(mock_bidi_stream_, Writev).Times(0); MessageParameters parameters = SubscribeForTest(); @@ -2336,12 +2337,12 @@ "Received multiple GOAWAY messages")) .Times(1); bool reported_error = false; - EXPECT_CALL(session_callbacks_.session_terminated_callback, Call(_)) + EXPECT_CALL(session_callbacks_.session_terminated_callback, Call) .WillOnce([&](absl::string_view error_message) { reported_error = true; EXPECT_EQ(error_message, "Received multiple GOAWAY messages"); }); - bidi_wrapper_->ReceiveMessage(MoqtGoAway("foo")); + bidi_wrapper_->ReceiveMessage(MoqtGoAway("")); } TEST_F(MoqtSessionTest, SendGoAwayEnforcement) { @@ -2415,30 +2416,6 @@ session_.GoAway("foo"); } -TEST_F(MoqtSessionTest, ServerCannotReceiveNewSessionUri) { - webtransport::test::MockSession mock_session; - MoqtSession session(&mock_session, - MoqtSessionParameters(quic::Perspective::IS_SERVER), - std::make_unique<quic::test::TestAlarmFactory>(), - session_callbacks_.AsSessionCallbacks()); - bidi_wrapper_ = - MoqtSessionPeer::CreateControlStream(&session, &mock_bidi_stream_); - EXPECT_CALL( - mock_session, - CloseSession(static_cast<uint64_t>(MoqtError::kProtocolViolation), - "Received GOAWAY with new_session_uri on the server")) - .Times(1); - bool reported_error = false; - EXPECT_CALL(session_callbacks_.session_terminated_callback, Call(_)) - .WillOnce([&](absl::string_view error_message) { - reported_error = true; - EXPECT_EQ(error_message, - "Received GOAWAY with new_session_uri on the server"); - }); - bidi_wrapper_->ReceiveMessage(MoqtGoAway("foo")); - EXPECT_TRUE(reported_error); -} - TEST_F(MoqtSessionTest, IncomingTrackStatusBeforeSetup) { MoqtSessionParameters session_parameters(quic::Perspective::IS_SERVER); MoqtSession server_session(&mock_session_, session_parameters,
diff --git a/quiche/quic/moqt/test_tools/moqt_test_message.h b/quiche/quic/moqt/test_tools/moqt_test_message.h index 750e1f8..51dc7fe 100644 --- a/quiche/quic/moqt/test_tools/moqt_test_message.h +++ b/quiche/quic/moqt/test_tools/moqt_test_message.h
@@ -1142,25 +1142,41 @@ bool EqualFieldValues(const MessageStructuredData& values) const override { auto cast = std::get<MoqtGoAway>(values); if (cast.new_session_uri != goaway_.new_session_uri) { - QUIC_LOG(INFO) << "GOAWAY full track name mismatch"; + QUIC_LOG(INFO) << "GOAWAY new session URI mismatch"; + return false; + } + if (cast.timeout != goaway_.timeout) { + QUIC_LOG(INFO) << "GOAWAY timeout mismatch"; + return false; + } + if (cast.request_id != goaway_.request_id) { + QUIC_LOG(INFO) << "GOAWAY request ID mismatch"; return false; } return true; } - void ExpandVarints() override { ExpandVarintsImpl("v---"); } + void RequestStreamMessage() { // Remove the request ID. + raw_packet_[2] = 0x05; + SetWireImage(raw_packet_, sizeof(raw_packet_) - 1); + goaway_.request_id = std::nullopt; + } + + void ExpandVarints() override { ExpandVarintsImpl("v---vv"); } MessageStructuredData structured_data() const override { return TestMessageBase::MessageStructuredData(goaway_); } private: - uint8_t raw_packet_[7] = { - 0x10, 0x00, 0x04, 0x03, 0x66, 0x6f, 0x6f, + uint8_t raw_packet_[9] = { + 0x10, 0x00, 0x06, 0x03, 0x66, 0x6f, 0x6f, 0x03, 0x40, }; MoqtGoAway goaway_ = { /*new_session_uri=*/"foo", + /*timeout=*/quic::QuicTimeDelta::FromMilliseconds(3), + /*request_id=*/64, }; };