Fix misread of spec and uninitialized value test flake. SERVER_SETUP buffer sizing was wrong, leading to extra bytes at the end. Verified vs.provided repro PiperOrigin-RevId: 578968834
diff --git a/quiche/quic/moqt/moqt_framer.cc b/quiche/quic/moqt/moqt_framer.cc index b03293f..1e00c85 100644 --- a/quiche/quic/moqt/moqt_framer.cc +++ b/quiche/quic/moqt/moqt_framer.cc
@@ -187,6 +187,7 @@ static_cast<uint64_t>(MoqtSetupParameter::kPath), message.path.value()); } + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -198,8 +199,7 @@ if (message.role.has_value()) { num_params++; buffer_size += - ParameterLen(static_cast<uint64_t>(MoqtSetupParameter::kRole), - static_cast<uint64_t>(message.role.value())); + ParameterLen(static_cast<uint64_t>(MoqtSetupParameter::kRole), 1); } buffer_size += NeededVarIntLen(num_params); quiche::QuicheBuffer buffer(allocator_, buffer_size); @@ -212,6 +212,7 @@ static_cast<uint64_t>(MoqtSetupParameter::kRole), static_cast<uint64_t>(message.role.value())); } + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -227,7 +228,8 @@ return quiche::QuicheBuffer(); } size_t buffer_size = NeededVarIntLen(MoqtMessageType::kSubscribeRequest) + - LengthPrefixedStringLength(message.full_track_name) + + LengthPrefixedStringLength(message.track_namespace) + + LengthPrefixedStringLength(message.track_name) + LocationLength(message.start_group) + LocationLength(message.start_object) + LocationLength(message.end_group) + @@ -244,7 +246,8 @@ quic::QuicDataWriter writer(buffer.size(), buffer.data()); writer.WriteVarInt62( static_cast<uint64_t>(MoqtMessageType::kSubscribeRequest)); - writer.WriteStringPieceVarInt62(message.full_track_name); + writer.WriteStringPieceVarInt62(message.track_namespace); + writer.WriteStringPieceVarInt62(message.track_name); WriteLocation(writer, message.start_group); WriteLocation(writer, message.start_object); WriteLocation(writer, message.end_group); @@ -256,6 +259,7 @@ static_cast<uint64_t>(MoqtTrackRequestParameter::kAuthorizationInfo), message.authorization_info.value()); } + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -274,6 +278,7 @@ writer.WriteStringPieceVarInt62(message.track_name); writer.WriteVarInt62(message.track_id); writer.WriteVarInt62(message.expires.ToMilliseconds()); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -292,6 +297,7 @@ writer.WriteStringPieceVarInt62(message.track_name); writer.WriteVarInt62(message.error_code); writer.WriteStringPieceVarInt62(message.reason_phrase); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -306,6 +312,7 @@ writer.WriteVarInt62(static_cast<uint64_t>(MoqtMessageType::kUnsubscribe)); writer.WriteStringPieceVarInt62(message.track_namespace); writer.WriteStringPieceVarInt62(message.track_name); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -324,6 +331,7 @@ writer.WriteStringPieceVarInt62(message.track_name); writer.WriteVarInt62(message.final_group); writer.WriteVarInt62(message.final_object); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -346,6 +354,7 @@ writer.WriteStringPieceVarInt62(message.reason_phrase); writer.WriteVarInt62(message.final_group); writer.WriteVarInt62(message.final_object); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -373,6 +382,7 @@ static_cast<uint64_t>(MoqtTrackRequestParameter::kAuthorizationInfo), message.authorization_info.value()); } + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -385,6 +395,7 @@ quic::QuicDataWriter writer(buffer.size(), buffer.data()); writer.WriteVarInt62(static_cast<uint64_t>(MoqtMessageType::kAnnounceOk)); writer.WriteStringPieceVarInt62(message.track_namespace); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -401,6 +412,7 @@ writer.WriteStringPieceVarInt62(message.track_namespace); writer.WriteVarInt62(message.error_code); writer.WriteStringPieceVarInt62(message.reason_phrase); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -413,6 +425,7 @@ quic::QuicDataWriter writer(buffer.size(), buffer.data()); writer.WriteVarInt62(static_cast<uint64_t>(MoqtMessageType::kUnannounce)); writer.WriteStringPieceVarInt62(message.track_namespace); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; } @@ -424,6 +437,7 @@ quic::QuicDataWriter writer(buffer.size(), buffer.data()); writer.WriteVarInt62(static_cast<uint64_t>(MoqtMessageType::kGoAway)); writer.WriteStringPieceVarInt62(message.new_session_uri); + QUICHE_DCHECK(writer.remaining() == 0); return buffer; }
diff --git a/quiche/quic/moqt/moqt_messages.h b/quiche/quic/moqt/moqt_messages.h index 7203fb3..7be848b 100644 --- a/quiche/quic/moqt/moqt_messages.h +++ b/quiche/quic/moqt/moqt_messages.h
@@ -123,7 +123,8 @@ }; struct QUICHE_EXPORT MoqtSubscribeRequest { - absl::string_view full_track_name; + absl::string_view track_namespace; + absl::string_view track_name; // If the mode is kNone, the these are absl::nullopt. absl::optional<MoqtSubscribeLocation> start_group; absl::optional<MoqtSubscribeLocation> start_object;
diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc index 12050af..2c76502 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc
@@ -304,7 +304,10 @@ size_t MoqtParser::ProcessSubscribeRequest(quic::QuicDataReader& reader) { MoqtSubscribeRequest subscribe_request; - if (!reader.ReadStringPieceVarInt62(&subscribe_request.full_track_name)) { + if (!reader.ReadStringPieceVarInt62(&subscribe_request.track_namespace)) { + return 0; + } + if (!reader.ReadStringPieceVarInt62(&subscribe_request.track_name)) { return 0; } if (!ReadLocation(reader, subscribe_request.start_group)) {
diff --git a/quiche/quic/moqt/moqt_parser_test.cc b/quiche/quic/moqt/moqt_parser_test.cc index ae6d545..b91768e 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc
@@ -114,11 +114,13 @@ end_of_message_ = true; messages_received_++; MoqtSubscribeRequest subscribe_request = message; - string0_ = std::string(subscribe_request.full_track_name); - subscribe_request.full_track_name = absl::string_view(string0_); + string0_ = std::string(subscribe_request.track_namespace); + subscribe_request.track_namespace = absl::string_view(string0_); + string1_ = std::string(subscribe_request.track_name); + subscribe_request.track_name = absl::string_view(string1_); if (subscribe_request.authorization_info.has_value()) { - string1_ = std::string(subscribe_request.authorization_info.value()); - subscribe_request.authorization_info = absl::string_view(string1_); + string2_ = std::string(subscribe_request.authorization_info.value()); + subscribe_request.authorization_info = absl::string_view(string2_); } last_message_ = TestMessageBase::MessageStructuredData(subscribe_request); } @@ -593,7 +595,8 @@ TEST_F(MoqtMessageSpecificTest, SubscribeRequestAuthorizationInfoTwice) { MoqtParser parser(kWebTrans, visitor_); char subscribe_request[] = { - 0x03, 0x03, 0x66, 0x6f, 0x6f, // full_track_name = "foo" + 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_namespace = "foo" + 0x04, 0x61, 0x62, 0x63, 0x64, // track_name = "abcd" 0x02, 0x04, // start_group = 4 (relative previous) 0x01, 0x01, // start_object = 1 (absolute) 0x00, // end_group = none @@ -690,6 +693,7 @@ MoqtParser parser(kRawQuic, visitor_); char subscribe_request[] = { 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_name = "foo" + 0x04, 0x61, 0x62, 0x63, 0x64, // track_name = "abcd" 0x00, // start_group = none 0x01, 0x01, // start_object = 1 (absolute) 0x00, // end_group = none @@ -709,6 +713,7 @@ MoqtParser parser(kRawQuic, visitor_); char subscribe_request[] = { 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_name = "foo" + 0x04, 0x61, 0x62, 0x63, 0x64, // track_name = "abcd" 0x02, 0x04, // start_group = 4 (relative previous) 0x00, // start_object = none 0x00, // end_group = none @@ -728,6 +733,7 @@ MoqtParser parser(kRawQuic, visitor_); char subscribe_request[] = { 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_name = "foo" + 0x04, 0x61, 0x62, 0x63, 0x64, // track_name = "abcd" 0x02, 0x04, // start_group = 4 (relative previous) 0x01, 0x01, // start_object = 1 (absolute) 0x00, // end_group = none
diff --git a/quiche/quic/moqt/test_tools/moqt_test_message.h b/quiche/quic/moqt/test_tools/moqt_test_message.h index b5d1020..b25c998 100644 --- a/quiche/quic/moqt/test_tools/moqt_test_message.h +++ b/quiche/quic/moqt/test_tools/moqt_test_message.h
@@ -334,8 +334,12 @@ bool EqualFieldValues(MessageStructuredData& values) const override { auto cast = std::get<MoqtSubscribeRequest>(values); - if (cast.full_track_name != subscribe_request_.full_track_name) { - QUIC_LOG(INFO) << "SUBSCRIBE REQUEST full track name mismatch"; + if (cast.track_namespace != subscribe_request_.track_namespace) { + QUIC_LOG(INFO) << "SUBSCRIBE REQUEST track namespace mismatch"; + return false; + } + if (cast.track_name != subscribe_request_.track_name) { + QUIC_LOG(INFO) << "SUBSCRIBE REQUEST track name mismatch"; return false; } if (cast.start_group != subscribe_request_.start_group) { @@ -361,15 +365,16 @@ return true; } - void ExpandVarints() override { ExpandVarintsImpl("vv---vvvvvvvvv---"); } + void ExpandVarints() override { ExpandVarintsImpl("vv---v----vvvvvvvvv---"); } MessageStructuredData structured_data() const override { return TestMessageBase::MessageStructuredData(subscribe_request_); } private: - uint8_t raw_packet_[17] = { - 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_name = "foo" + uint8_t raw_packet_[22] = { + 0x03, 0x03, 0x66, 0x6f, 0x6f, // track_namespace = "foo" + 0x04, 0x61, 0x62, 0x63, 0x64, // track_name = "abcd" 0x02, 0x04, // start_group = 4 (relative previous) 0x01, 0x01, // start_object = 1 (absolute) 0x00, // end_group = none @@ -379,7 +384,8 @@ }; MoqtSubscribeRequest subscribe_request_ = { - /*full_track_name=*/"foo", + /*track_namespace=*/"foo", + /*track_name=*/"abcd", /*start_group=*/MoqtSubscribeLocation(false, (int64_t)(-4)), /*start_object=*/MoqtSubscribeLocation(true, (uint64_t)1), /*end_group=*/absl::nullopt,