[neeraj1235476]

QuicTimeDelta::FromMilliseconds(int64_t ms) returns QuicTimeDelta(ms * 1000), and several MOQT control-message parsers feed it a millisecond count taken straight from a peer-supplied MoQ varint. A varint can carry values far beyond INT64_MAX / 1000, so the multiply by 1000 overflows signed int64 (undefined behavior) for any millisecond value above roughly 9.2e15.

The affected callees are the TrackProperties timeout getters (object_delivery_timeout, max_cache_duration, subgroup_delivery_timeout) in moqt_key_value_pair.cc, which read peer track properties that Validate() places no upper bound on, and ProcessRequestError in moqt_parser.cc, which converts the REQUEST_ERROR retry_interval field. ParseMessageParameters already converts the same kind of peer millisecond values with the overflow-checked TryFromMilliseconds(...).value_or(Infinite()); these sites just missed it. Switching them to the same form keeps the overflow check next to the conversion rather than asking every caller to pre-clamp.

Added regression tests feed a millisecond value whose microsecond conversion overflows and assert the result is an infinite delta; both fail before this change and pass after.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request replaces 'QuicTimeDelta::FromMilliseconds' with 'QuicTimeDelta::TryFromMilliseconds' to safely handle peer-supplied millisecond values and prevent signed overflow by collapsing overflowing values to an infinite delta. It also adds corresponding unit tests. The review feedback identifies a critical issue where passing a 'uint64_t' value greater than 'INT64_MAX' to 'TryFromMilliseconds' (which accepts an 'int64_t') results in an implicit conversion that wraps around to a negative value, bypassing the overflow checks. The reviewer suggests explicitly checking if the values exceed 'INT64_MAX' before conversion and recommends using 'UINT64_MAX' in the unit tests to verify this wrap-around behavior.

[gemini-code-assist]

Implicitly converting a uint64_t value greater than INT64_MAX to int64_t (which is the parameter type of TryFromMilliseconds) wraps around to a negative value in C++20 (or is implementation-defined in older standards). If this wrapped negative value is greater than INT64_MIN / 1000 (for example, UINT64_MAX wraps to -1), it will bypass the overflow checks inside TryFromMilliseconds and return a negative QuicTimeDelta instead of std::nullopt.\n\nTo prevent this, we should explicitly check if the value exceeds INT64_MAX and return quic::QuicTimeDelta::Infinite() directly in that case.

             ? (*value > static_cast<uint64_t>(INT64_MAX)\n                    ? quic::QuicTimeDelta::Infinite()\n                    : quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or(\n                          quic::QuicTimeDelta::Infinite()))

[neeraj1235476]

Good catch, MoQ varints can carry the full 64 bits so UINT64_MAX would have wrapped to -1 here. Values above INT64_MAX now go straight to Infinite() via a small MillisecondsToTimeDelta helper used by all three getters.

[gemini-code-assist]

Explicitly check if the uint64_t value exceeds INT64_MAX to prevent wrap-around to a negative int64_t value, which would bypass the overflow checks inside TryFromMilliseconds.

             ? (*value > static_cast<uint64_t>(INT64_MAX)\n                    ? quic::QuicTimeDelta::Infinite()\n                    : quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or(\n                          quic::QuicTimeDelta::Infinite()))

[gemini-code-assist]

Explicitly check if the uint64_t value exceeds INT64_MAX to prevent wrap-around to a negative int64_t value, which would bypass the overflow checks inside TryFromMilliseconds.

             ? (*value > static_cast<uint64_t>(INT64_MAX)\n                    ? quic::QuicTimeDelta::Infinite()\n                    : quic::QuicTimeDelta::TryFromMilliseconds(*value).value_or(\n                          quic::QuicTimeDelta::Infinite()))

[gemini-code-assist]

Explicitly check if raw_interval - 1 exceeds INT64_MAX to prevent wrap-around to a negative int64_t value, which would bypass the overflow checks inside TryFromMilliseconds.

                (raw_interval - 1 > static_cast<uint64_t>(INT64_MAX))\n                    ? quic::QuicTimeDelta::Infinite()\n                    : quic::QuicTimeDelta::TryFromMilliseconds(raw_interval - 1)\n                          .value_or(quic::QuicTimeDelta::Infinite()));

[neeraj1235476]

Same fix here, raw_interval - 1 is range-checked before the narrowing. Added RequestErrorMaxRetryIntervalDoesNotWrap with an all-0xff varint to cover it.

[gemini-code-assist]

Using UINT64_MAX instead of uint64_t{1} << 62 is a stronger test case because it specifically verifies that values which would wrap around to negative numbers when cast to int64_t (such as UINT64_MAX wrapping to -1) are correctly handled and collapse to an infinite delta.

Suggested change

	constexpr uint64_t kOverflowingMs = uint64_t{1} << 62;
	constexpr uint64_t kOverflowingMs = UINT64_MAX;

[neeraj1235476]

Done, the test now runs both 2^62 (multiply overflow) and UINT64_MAX (wrap).