From f2568a64c0933c3a0bf959568df400cb1d16c2bf Mon Sep 17 00:00:00 2001 From: Kim De Mey Date: Tue, 3 Sep 2024 16:48:24 +0200 Subject: [PATCH] Fix potential uTP clock drift overflow (#726) --- eth/utp/clock_drift_calculator.nim | 25 ++++++++++++++++++----- tests/utp/test_clock_drift_calculator.nim | 9 ++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/eth/utp/clock_drift_calculator.nim b/eth/utp/clock_drift_calculator.nim index 6d2d0a1..ba0128f 100644 --- a/eth/utp/clock_drift_calculator.nim +++ b/eth/utp/clock_drift_calculator.nim @@ -38,24 +38,39 @@ proc init*(T: type ClockDriftCalculator, currentTime: Moment): T = averageSampleTime: currentTime + averageTime ) +## This is a port from the clock drift calculation implemented in libutp: +## https://github.com/bittorrent/libutp/blob/2b364cbb0650bdab64a5de2abb4518f9f228ec44/utp_internal.cpp#L2026 +## +## We limit actualDelay to int32.max to avoid overflow in calculations later on, +## more specifically at the drift calculation. This might/will influence the +## actual result, however, dealing which such high values bears the question +## if we should not just drop the connection in the first place. +## +## It also does not resolve faulty(?) behaviour in the algorithm, where +## currentDelaySum can go negative even when the actualDelay keeps increasing. +## +## TODO: With the above issues in mind and the current complexity of the algorithm, +## we should reconsider if this is still required and if so, whether a simpler +## algorithm would suffice. proc addSample*(c: var ClockDriftCalculator, actualDelay: uint32, currentTime: Moment) = if (actualDelay == 0): return + let delay = min(actualDelay, uint32(int32.high)) # this is our first sample, initialise our delay base if c.averageDelayBase == 0: - c.averageDelayBase = actualDelay + c.averageDelayBase = delay - let distDown = c.averageDelayBase - actualDelay + let distDown = c.averageDelayBase - delay - let distUp = actualDelay - c.averageDelayBase + let distUp = delay - c.averageDelayBase let averageDelaySample = if (distDown > distUp): - # averageDelayBase is smaller that actualDelay, sample should be positive + # averageDelayBase is smaller that delay, sample should be positive int64(distUp) else: - # averageDelayBase is bigger or equal to actualDelay, sample should be negative + # averageDelayBase is bigger or equal to delay, sample should be negative -int64(distDown) c.currentDelaySum = c.currentDelaySum + averageDelaySample diff --git a/tests/utp/test_clock_drift_calculator.nim b/tests/utp/test_clock_drift_calculator.nim index 03f19fe..945766a 100644 --- a/tests/utp/test_clock_drift_calculator.nim +++ b/tests/utp/test_clock_drift_calculator.nim @@ -68,3 +68,12 @@ suite "uTP clock drift calculator": check: calculator1.clockDrift == -calculator2.clockDrift calculator1.lastClockDrift == -calculator2.lastClockDrift + + test "Clock drift limits": + let currentTime = Moment.now() + var calculator = ClockDriftCalculator.init(currentTime) + + for i in 0..6'u32: + calculator.addSample(1'u32 + i*i*100_000_000'u32, currentTime + seconds(5 * i)) + + calculator.addSample(500_000_000'u32, currentTime + seconds(5 * 7))