From 95b0ba0d70788b6d345c8b21b8e49b4e36f4d312 Mon Sep 17 00:00:00 2001 From: neeraj netrakar Date: Wed, 14 Oct 2026 22:12:42 +0530 Subject: [PATCH 1/2] moqt: avoid int64 overflow converting peer millisecond varints to QuicTimeDelta --- quiche/quic/moqt/moqt_key_value_pair.cc | 12 +++++++---- quiche/quic/moqt/moqt_key_value_pair_test.cc | 22 ++++++++++++++++++++ quiche/quic/moqt/moqt_parser.cc | 3 ++- quiche/quic/moqt/moqt_parser_test.cc | 22 ++++++++++++++++++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/quiche/quic/moqt/moqt_key_value_pair.cc b/quiche/quic/moqt/moqt_key_value_pair.cc index 3abeaf0c8..cca57b25d 100644 --- a/quiche/quic/moqt/moqt_key_value_pair.cc +++ b/quiche/quic/moqt/moqt_key_value_pair.cc @@ -158,20 +158,24 @@ quic::QuicTimeDelta TrackProperties::object_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kObjectDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::FromMilliseconds(*value) + ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( + quic::QuicTimeDelta::Infinite()) : 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() + ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( + quic::QuicTimeDelta::Infinite()) + : kDefaultMaxCacheDuration; } quic::QuicTimeDelta TrackProperties::subgroup_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kSubgroupDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::FromMilliseconds(*value) + ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( + quic::QuicTimeDelta::Infinite()) : 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..e08ae990d 100644 --- a/quiche/quic/moqt/moqt_key_value_pair_test.cc +++ b/quiche/quic/moqt/moqt_key_value_pair_test.cc @@ -210,6 +210,28 @@ 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. Validate() places no upper bound on these keys. + constexpr uint64_t kOverflowingMs = uint64_t{1} << 62; + TrackProperties properties; + properties.insert(static_cast(PropertyType::kObjectDeliveryTimeout), + kOverflowingMs); + properties.insert(static_cast(PropertyType::kMaxCacheDuration), + kOverflowingMs); + properties.insert( + static_cast(PropertyType::kSubgroupDeliveryTimeout), + kOverflowingMs); + 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..d5f711365 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc @@ -756,7 +756,8 @@ absl::StatusOr MoqtControlMessageParser::ProcessRequestError( (raw_interval == 0) ? std::nullopt : std::make_optional( - quic::QuicTimeDelta::FromMilliseconds(raw_interval - 1)); + quic::QuicTimeDelta::TryFromMilliseconds(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..1c9d37816 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc @@ -2403,4 +2403,26 @@ 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()); +} + } // namespace moqt::test From ccbd16369833be8bb2b719d183c652dd4d184d72 Mon Sep 17 00:00:00 2001 From: neeraj netrakar Date: Sun, 18 Oct 2026 15:48:33 +0530 Subject: [PATCH 2/2] moqt: treat millisecond values above INT64_MAX as infinite The uint64 varint was implicitly narrowed to int64 when passed to TryFromMilliseconds, so values above INT64_MAX wrapped to a negative delta and skipped the overflow check. --- quiche/quic/moqt/moqt_key_value_pair.cc | 24 +++++++++---- quiche/quic/moqt/moqt_key_value_pair_test.cc | 36 +++++++++++--------- quiche/quic/moqt/moqt_parser.cc | 8 +++-- quiche/quic/moqt/moqt_parser_test.cc | 19 +++++++++++ 4 files changed, 63 insertions(+), 24 deletions(-) diff --git a/quiche/quic/moqt/moqt_key_value_pair.cc b/quiche/quic/moqt/moqt_key_value_pair.cc index cca57b25d..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,24 +173,21 @@ quic::QuicTimeDelta TrackProperties::object_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kObjectDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( - quic::QuicTimeDelta::Infinite()) + ? MillisecondsToTimeDelta(*value) : kDefaultDeliveryTimeout; } quic::QuicTimeDelta TrackProperties::max_cache_duration() const { std::optional value = GetValueIfExactlyOne(PropertyType::kMaxCacheDuration); return value.has_value() - ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( - quic::QuicTimeDelta::Infinite()) + ? MillisecondsToTimeDelta(*value) : kDefaultMaxCacheDuration; } quic::QuicTimeDelta TrackProperties::subgroup_delivery_timeout() const { std::optional value = GetValueIfExactlyOne(PropertyType::kSubgroupDeliveryTimeout); return (value.has_value() && *value > 0) - ? quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or( - quic::QuicTimeDelta::Infinite()) + ? 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 e08ae990d..e3af2ae87 100644 --- a/quiche/quic/moqt/moqt_key_value_pair_test.cc +++ b/quiche/quic/moqt/moqt_key_value_pair_test.cc @@ -214,22 +214,26 @@ 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. Validate() places no upper bound on these keys. - constexpr uint64_t kOverflowingMs = uint64_t{1} << 62; - TrackProperties properties; - properties.insert(static_cast(PropertyType::kObjectDeliveryTimeout), - kOverflowingMs); - properties.insert(static_cast(PropertyType::kMaxCacheDuration), - kOverflowingMs); - properties.insert( - static_cast(PropertyType::kSubgroupDeliveryTimeout), - kOverflowingMs); - 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()); + // 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) { diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc index d5f711365..ba83ca6d5 100644 --- a/quiche/quic/moqt/moqt_parser.cc +++ b/quiche/quic/moqt/moqt_parser.cc @@ -756,8 +756,12 @@ absl::StatusOr MoqtControlMessageParser::ProcessRequestError( (raw_interval == 0) ? std::nullopt : std::make_optional( - quic::QuicTimeDelta::TryFromMilliseconds(raw_interval - 1) - .value_or(quic::QuicTimeDelta::Infinite())); + (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 1c9d37816..046997cbe 100644 --- a/quiche/quic/moqt/moqt_parser_test.cc +++ b/quiche/quic/moqt/moqt_parser_test.cc @@ -2425,4 +2425,23 @@ TEST_F(MoqtMessageSpecificTest, RequestErrorOversizedRetryInterval) { 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