On the wire, MOQT Objects cannot carry status and have properties or payloads. If they are sent to the framer with properties or payloads and kEndOfGroup status, there MUST be encoding that allows the last object to have the end_of_group flag. PiperOrigin-RevId: 996724770
diff --git a/quiche/quic/moqt/moqt_framer.cc b/quiche/quic/moqt/moqt_framer.cc index c6b4e16..92087bc 100644 --- a/quiche/quic/moqt/moqt_framer.cc +++ b/quiche/quic/moqt/moqt_framer.cc
@@ -598,7 +598,7 @@ quiche::QuicheBuffer MoqtFramer::SerializeObjectHeader( const MoqtObject& message, MoqtDataStreamType message_type, std::optional<PublishedObjectMetadata>& previous_object_in_stream) { - if (!ValidateObjectMetadata(message)) { + if (!ValidateObjectMetadata(message, message_type.EndOfGroupInStream())) { QUICHE_BUG(QUICHE_BUG_serialize_object_header_01) << "Object metadata is invalid"; return quiche::QuicheBuffer(); @@ -705,7 +705,9 @@ quiche::QuicheBuffer MoqtFramer::SerializeObjectDatagram( const MoqtObject& message, absl::string_view payload, MoqtPriority default_priority) { - if (!ValidateObjectMetadata(message) || message.subgroup_id.has_value()) { + // Any datagram can support an alternate end-of-group encoding. + if (!ValidateObjectMetadata(message, true) || + message.subgroup_id.has_value()) { QUICHE_BUG(QUICHE_BUG_serialize_object_datagram_01) << "Object metadata is invalid"; return quiche::QuicheBuffer(); @@ -731,9 +733,10 @@ ? std::optional<absl::string_view>(message.properties) : std::nullopt; std::optional<uint64_t> object_status = - payload.empty() ? std::optional<uint64_t>( - static_cast<uint64_t>(message.object_status)) - : std::nullopt; + datagram_type.has_status() + ? std::optional<uint64_t>( + static_cast<uint64_t>(message.object_status)) + : std::nullopt; std::optional<absl::string_view> raw_payload = payload.empty() ? std::nullopt : std::optional<absl::string_view>(payload); @@ -986,10 +989,20 @@ } // static -bool MoqtFramer::ValidateObjectMetadata(const MoqtObject& object) { - return (object.object_status == MoqtObjectStatus::kNormal || - object.object_status == MoqtObjectStatus::kEndOfGroup || - object.payload_length == 0); +bool MoqtFramer::ValidateObjectMetadata(const MoqtObject& object, + bool alternate_end_of_group_encoding) { + switch (object.object_status) { + case MoqtObjectStatus::kNormal: + return true; + case MoqtObjectStatus::kEndOfGroup: + return alternate_end_of_group_encoding || + (object.payload_length == 0 && object.properties.empty()); + case MoqtObjectStatus::kEndOfTrack: + return object.payload_length == 0 && object.properties.empty(); + default: + QUICHE_NOTREACHED(); + return false; + } } } // namespace moqt
diff --git a/quiche/quic/moqt/moqt_framer.h b/quiche/quic/moqt/moqt_framer.h index b742280..e43719c 100644 --- a/quiche/quic/moqt/moqt_framer.h +++ b/quiche/quic/moqt/moqt_framer.h
@@ -75,8 +75,11 @@ // Returns true if the parameters are valid for the message type. bool FillAndValidateSetupOptions(const SetupOptions& options, KeyValuePairList& out); - // Returns true if the metadata is internally consistent. - static bool ValidateObjectMetadata(const MoqtObject& object); + // Returns true if the metadata is internally consistent. If + // |alternate_end_of_group_encoding| is true, then the stream/datagram can + // encode EndOfGroup without using the explicit status field. + static bool ValidateObjectMetadata(const MoqtObject& object, + bool alternate_end_of_group_encoding); const bool using_webtrans_; const quic::Perspective perspective_; };
diff --git a/quiche/quic/moqt/moqt_framer_test.cc b/quiche/quic/moqt/moqt_framer_test.cc index 19cebc5..3afe5b8 100644 --- a/quiche/quic/moqt/moqt_framer_test.cc +++ b/quiche/quic/moqt/moqt_framer_test.cc
@@ -20,6 +20,7 @@ #include "quiche/quic/moqt/moqt_messages.h" #include "quiche/quic/moqt/moqt_names.h" #include "quiche/quic/moqt/moqt_object.h" +#include "quiche/quic/moqt/moqt_priority.h" #include "quiche/quic/moqt/moqt_types.h" #include "quiche/quic/moqt/test_tools/moqt_test_message.h" #include "quiche/quic/platform/api/quic_expect_bug.h" @@ -328,6 +329,36 @@ previous), "Object metadata is invalid"); EXPECT_TRUE(buffer.empty()); + // kEndOfTrack with empty payload and non-empty properties is still invalid, + // even when EndOfGroupInStream is set. + object.payload_length = 0; + EXPECT_QUIC_BUG( + buffer = framer_.SerializeObjectHeader( + object, MoqtDataStreamType::Subgroup(8, 0, false, false, true, true), + previous), + "Object metadata is invalid"); + EXPECT_TRUE(buffer.empty()); + // kEndOfGroup with payload or properties is disallowed unless + // EndOfGroupInStream is set. + object.object_status = MoqtObjectStatus::kEndOfGroup; + EXPECT_QUIC_BUG( + buffer = framer_.SerializeObjectHeader( + object, MoqtDataStreamType::Subgroup(8, 0, false, false, true, false), + previous), + "Object metadata is invalid"); + EXPECT_TRUE(buffer.empty()); + // If EndOfGroupInStream is set, kEndOfGroup is allowed with non-empty + // payload and properties. + buffer = framer_.SerializeObjectHeader( + object, MoqtDataStreamType::Subgroup(8, 0, false, false, true, true), + previous); + EXPECT_FALSE(buffer.empty()); + previous.reset(); + object.payload_length = 3; + buffer = framer_.SerializeObjectHeader( + object, MoqtDataStreamType::Subgroup(8, 0, false, false, true, true), + previous); + EXPECT_FALSE(buffer.empty()); } TEST_F(MoqtFramerSimpleTest, BadDatagramInput) { @@ -350,6 +381,19 @@ object, "foo", kDefaultPublisherPriority), "Object metadata is invalid"); EXPECT_TRUE(buffer.empty()); + // kEndOfTrack with empty payload and non-empty properties is invalid. + object.payload_length = 0; + EXPECT_QUIC_BUG(buffer = framer_.SerializeObjectDatagram( + object, "", kDefaultPublisherPriority), + "Object metadata is invalid"); + EXPECT_TRUE(buffer.empty()); + // kEndOfGroup with empty payload and non-empty properties is valid via the + // datagram kEndOfGroup flag. + object.object_status = MoqtObjectStatus::kEndOfGroup; + buffer = + framer_.SerializeObjectDatagram(object, "", kDefaultPublisherPriority); + EXPECT_FALSE(buffer.empty()); + object.payload_length = 3; object.object_status = MoqtObjectStatus::kNormal; object.subgroup_id = 8;
diff --git a/quiche/quic/moqt/moqt_messages.h b/quiche/quic/moqt/moqt_messages.h index 872eaae..46a6dd4 100644 --- a/quiche/quic/moqt/moqt_messages.h +++ b/quiche/quic/moqt/moqt_messages.h
@@ -148,34 +148,19 @@ MoqtDatagramType(bool payload, bool properties, bool end_of_group, bool default_priority, bool zero_object_id) : value_(0) { - // Avoid illegal types. Status cannot coexist with the zero-object-id flag - // or the end-of-group flag. - if (!payload && !end_of_group) { - // The only way to express non-normal, non-end-of-group with no payload is - // with an explicit status, so we cannot utilize object ID compression. - zero_object_id = false; - } else if (zero_object_id) { - // zero-object-id saves a byte; no-payload does not. - payload = true; - } else if (!payload) { - // If it's an empty end-of-group object, use the explict status because - // it's more readable. - end_of_group = false; - } if (properties) { value_ |= kProperties; } - if (end_of_group) { - value_ |= kEndOfGroup; - } if (zero_object_id) { value_ |= kZeroObjectId; } if (default_priority) { value_ |= kDefaultPriority; } - if (!payload) { + if (!payload && !properties) { value_ |= kStatus; + } else if (end_of_group) { + value_ |= kEndOfGroup; } } static std::optional<MoqtDatagramType> FromValue(uint64_t value) { @@ -250,6 +235,11 @@ // The data contained in every Object message, although the message type // implies some of the values. +// On the wire, objects cannot carry status and payload/properties. However, all +// objects can implicity have Normal status and payload/properties. Furthermore, +// when passed in internal APIs, an EndOfGroup status can indicate that a +// datagram, or last object in a stream, was encoded as being the last object in +// its group. struct QUICHE_EXPORT MoqtObject { uint64_t track_alias; // For FETCH, this is the subscribe ID. uint64_t group_id;
diff --git a/quiche/quic/moqt/moqt_messages_test.cc b/quiche/quic/moqt/moqt_messages_test.cc index d73198d..03544fb 100644 --- a/quiche/quic/moqt/moqt_messages_test.cc +++ b/quiche/quic/moqt/moqt_messages_test.cc
@@ -126,13 +126,12 @@ for (bool zero_object_id : {false, true}) { MoqtDatagramType type(payload, properties, end_of_group, default_priority, zero_object_id); - EXPECT_EQ(type.has_status(), - !payload && (!end_of_group || !zero_object_id)); + EXPECT_EQ(type.has_status(), !payload && !properties); EXPECT_EQ(type.has_properties(), properties); EXPECT_EQ(type.end_of_group(), - end_of_group && (payload || zero_object_id)); - EXPECT_EQ(type.has_object_id(), - !zero_object_id || (!payload && !end_of_group)); + end_of_group && (payload || properties)); + EXPECT_EQ(type.has_object_id(), !zero_object_id); + EXPECT_EQ(type.has_default_priority(), default_priority); // The constructor should always produce a valid value. std::optional<MoqtDatagramType> from_value = MoqtDatagramType::FromValue(type.value());
diff --git a/quiche/quic/moqt/moqt_object.h b/quiche/quic/moqt/moqt_object.h index 11fc220..8d5b0bf 100644 --- a/quiche/quic/moqt/moqt_object.h +++ b/quiche/quic/moqt/moqt_object.h
@@ -23,6 +23,8 @@ namespace moqt { +// See the "MoqtObject" comment in moqt_messages.h for how status can interact +// with payload and priority. struct PublishedObjectMetadata { Location location; std::optional<uint64_t> subgroup; // nullopt for datagrams.
diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc index aabb1a8..a348194 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc
@@ -1191,6 +1191,10 @@ return absl::InvalidArgumentError("Failed to parse datagram"); } object_metadata.object_status = IntegerToObjectStatus(object_status_raw); + if (datagram_type->has_properties() && + object_metadata.object_status != MoqtObjectStatus::kNormal) { + return absl::InvalidArgumentError("Datagram has status and properties"); + } return ""; } absl::string_view payload = reader.ReadRemainingPayload(); @@ -1538,7 +1542,11 @@ ParseError("Invalid object status provided"); return; } - + if (metadata_.object_status != MoqtObjectStatus::kNormal && + !metadata_.properties.empty()) { + ParseError("Object has status and properties"); + return; + } ++num_objects_read_; // TODO(martinduke): If contains_end_of_group_ && fin_read, the track is // malformed. There is no API to signal this to the session yet, but the
diff --git a/quiche/quic/moqt/moqt_parser_test.cc b/quiche/quic/moqt/moqt_parser_test.cc index 89cd2a3..825c249 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc
@@ -1868,6 +1868,34 @@ EXPECT_EQ(visitor_.parsing_error(), "Duplicate object ID in FETCH stream"); } +TEST_F(MoqtDataParserStateMachineTest, SubgroupObjectHasStatusAndProperties) { + MoqtDataStreamType type = + MoqtDataStreamType::Subgroup(0, 1, /*no_properties=*/false, + /*default_priority=*/false, + /*has_first_object=*/true); + // StreamHeaderSubgroupMessage with payload_length = 0 followed by status = + // kEndOfGroup (0x03) while properties are non-empty. + StreamHeaderSubgroupMessage header(type); + ASSERT_TRUE(header.SetPayloadLength(0)); + stream_.Receive(header.PacketSample()); + stream_.Receive("\x03", /*fin=*/true); + parser_.ReadAllData(); + EXPECT_EQ(visitor_.parsing_error(), "Object has status and properties"); +} + +TEST_F(MoqtMessageSpecificTest, DatagramHasStatusAndProperties) { + // Type 0x21 (kStatus | kProperties), track_alias = 4, group_id = 5, + // object_id = 6, priority = 7, 1-byte property blob, status = kEndOfTrack + // (0x04). + constexpr absl::string_view kBadDatagram( + "\x21\x04\x05\x06\x07\x02\x02\x00\x04", 9); + MoqtObject object; + bool use_default_priority = false; + EXPECT_THAT(ParseDatagram(kBadDatagram, object, use_default_priority), + StatusIs(absl::StatusCode::kInvalidArgument, + "Datagram has status and properties")); +} + TEST_F(MoqtMessageSpecificTest, StreamTypeParserToControlStream) { webtransport::test::InMemoryStream stream(/*stream_id=*/0); MoqtStreamTypeParser type_parser(&stream);
diff --git a/quiche/quic/moqt/test_tools/moqt_test_message.h b/quiche/quic/moqt/test_tools/moqt_test_message.h index 7bdb17e..cf17829 100644 --- a/quiche/quic/moqt/test_tools/moqt_test_message.h +++ b/quiche/quic/moqt/test_tools/moqt_test_message.h
@@ -31,7 +31,6 @@ #include "quiche/common/moq_varint.h" #include "quiche/common/platform/api/quiche_export.h" #include "quiche/common/platform/api/quiche_logging.h" -#include "quiche/common/quiche_endian.h" namespace moqt::test { @@ -297,7 +296,9 @@ : ObjectMessage(), datagram_type_(datagram_type) { // Update ObjectMessage::object_ to match the datagram type. if (datagram_type.has_status()) { - object_.object_status = MoqtObjectStatus::kEndOfTrack; + object_.object_status = datagram_type.has_properties() + ? MoqtObjectStatus::kNormal + : MoqtObjectStatus::kEndOfTrack; object_.payload_length = 0; } else { object_.object_status = datagram_type.end_of_group()