diff --git a/quiche/quic/moqt/moqt_key_value_pair.cc b/quiche/quic/moqt/moqt_key_value_pair.cc index 3abeaf0c8..62ca0daab 100644 --- a/quiche/quic/moqt/moqt_key_value_pair.cc +++ b/quiche/quic/moqt/moqt_key_value_pair.cc @@ -5,6 +5,7 @@ #include "quiche/quic/moqt/moqt_key_value_pair.h" #include +#include #include #include #include @@ -19,6 +20,20 @@ namespace moqt { +namespace { + +// Converts a peer-supplied millisecond count to a QuicTimeDelta. Values that +// don't fit in int64 microseconds collapse to an infinite delta. +quic::QuicTimeDelta MillisecondsToTimeDelta(uint64_t ms) { + if (ms > static_cast(std::numeric_limits::max())) { + return quic::QuicTimeDelta::Infinite(); + } + return quic::QuicTimeDelta::TryFromMilliseconds(static_cast(ms)) + .value_or(quic::QuicTimeDelta::Infinite()); +} + +} // namespace + void KeyValuePairList::insert(uint64_t key, std::variant value) { QUICHE_DCHECK( @@ -158,20 +173,21 @@ quic::QuicTimeDelta TrackProperties::object_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kObjectDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::FromMilliseconds(*value) + ? MillisecondsToTimeDelta(*value) : kDefaultDeliveryTimeout; } quic::QuicTimeDelta TrackProperties::max_cache_duration() const { std::optional value = GetValueIfExactlyOne(PropertyType::kMaxCacheDuration); - return value.has_value() ? quic::QuicTimeDelta::FromMilliseconds(*value) - : kDefaultMaxCacheDuration; + return value.has_value() + ? MillisecondsToTimeDelta(*value) + : kDefaultMaxCacheDuration; } quic::QuicTimeDelta TrackProperties::subgroup_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kSubgroupDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::FromMilliseconds(*value) + ? MillisecondsToTimeDelta(*value) : kDefaultDeliveryTimeout; } absl::string_view TrackProperties::immutable_properties() const { diff --git a/quiche/quic/moqt/moqt_key_value_pair_test.cc b/quiche/quic/moqt/moqt_key_value_pair_test.cc index 5bc849526..e3af2ae87 100644 --- a/quiche/quic/moqt/moqt_key_value_pair_test.cc +++ b/quiche/quic/moqt/moqt_key_value_pair_test.cc @@ -210,6 +210,32 @@ TEST_F(TrackPropertiesTest, ExplicitDefaults) { EXPECT_TRUE(properties.immutable_properties().empty()); } +TEST_F(TrackPropertiesTest, OversizedTimeoutsDoNotOverflow) { + // Timeout properties carry a peer-supplied millisecond value that is turned + // into microseconds by a multiply by 1000. Values whose conversion would + // overflow int64 must collapse to an infinite delta rather than trigger + // signed overflow. Values above INT64_MAX must not wrap to a negative + // delta. Validate() places no upper bound on these keys. + for (uint64_t overflowing_ms : {uint64_t{1} << 62, UINT64_MAX}) { + TrackProperties properties; + properties.insert( + static_cast(PropertyType::kObjectDeliveryTimeout), + overflowing_ms); + properties.insert(static_cast(PropertyType::kMaxCacheDuration), + overflowing_ms); + properties.insert( + static_cast(PropertyType::kSubgroupDeliveryTimeout), + overflowing_ms); + EXPECT_TRUE(properties.Validate()); + EXPECT_EQ(properties.object_delivery_timeout(), + quic::QuicTimeDelta::Infinite()); + EXPECT_EQ(properties.max_cache_duration(), + quic::QuicTimeDelta::Infinite()); + EXPECT_EQ(properties.subgroup_delivery_timeout(), + quic::QuicTimeDelta::Infinite()); + } +} + TEST_F(TrackPropertiesTest, Validate) { TrackProperties properties; // Unknown property. diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc index 451e7074d..ba83ca6d5 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc @@ -756,7 +756,12 @@ absl::StatusOr MoqtControlMessageParser::ProcessRequestError( (raw_interval == 0) ? std::nullopt : std::make_optional( - quic::QuicTimeDelta::FromMilliseconds(raw_interval - 1)); + (raw_interval - 1 > + static_cast(std::numeric_limits::max())) + ? quic::QuicTimeDelta::Infinite() + : quic::QuicTimeDelta::TryFromMilliseconds( + static_cast(raw_interval - 1)) + .value_or(quic::QuicTimeDelta::Infinite())); if (request_error.error_code == RequestErrorCode::kRedirect) { request_error.redirect.emplace(); if (!reader.ReadStringMoqVarInt(request_error.redirect->connect_uri)) { diff --git a/quiche/quic/moqt/moqt_parser_test.cc b/quiche/quic/moqt/moqt_parser_test.cc index cc35e3d9c..046997cbe 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc @@ -2403,4 +2403,45 @@ TEST_F(MoqtMessageSpecificTest, RequestErrorRedirect) { HasSubstr("Control message has excess data"))); } +TEST_F(MoqtMessageSpecificTest, RequestErrorOversizedRetryInterval) { + // retry_interval is a peer-supplied millisecond varint that is converted to + // microseconds (a multiply by 1000). A value whose conversion overflows + // int64 must collapse to an infinite delta rather than trigger signed + // overflow. + char request_error[] = { + 0x05, 0x00, 0x0b, // type = REQUEST_ERROR, size + 0x11, // error_code (non-redirect) + 0xff, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, // retry_interval = 2^60 as a + 0x00, 0x00, // 9-byte MoqVarInt + 0x00, // reason_phrase = "" + }; + absl::StatusOr> parsed = ParseAllMessages( + absl::string_view(request_error, sizeof(request_error)), + kDefaultMoqtVersion, kWebTrans, quic::Perspective::IS_CLIENT); + ASSERT_TRUE(parsed.ok()) << parsed.status(); + ASSERT_EQ(parsed->size(), 1u); + const MoqtRequestError& error = std::get((*parsed)[0]); + ASSERT_TRUE(error.retry_interval.has_value()); + EXPECT_EQ(*error.retry_interval, quic::QuicTimeDelta::Infinite()); +} + +TEST_F(MoqtMessageSpecificTest, RequestErrorMaxRetryIntervalDoesNotWrap) { + // A retry_interval above INT64_MAX must not wrap to a negative delta. + char request_error[] = { + 0x05, 0x00, 0x0b, // type = REQUEST_ERROR, size + 0x11, // error_code (non-redirect) + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, // retry_interval = UINT64_MAX + 0xff, 0xff, // as a 9-byte MoqVarInt + 0x00, // reason_phrase = "" + }; + absl::StatusOr> parsed = ParseAllMessages( + absl::string_view(request_error, sizeof(request_error)), + kDefaultMoqtVersion, kWebTrans, quic::Perspective::IS_CLIENT); + ASSERT_TRUE(parsed.ok()) << parsed.status(); + ASSERT_EQ(parsed->size(), 1u); + const MoqtRequestError& error = std::get((*parsed)[0]); + ASSERT_TRUE(error.retry_interval.has_value()); + EXPECT_EQ(*error.retry_interval, quic::QuicTimeDelta::Infinite()); +} + } // namespace moqt::test