send_packets: fix miscalibrated overflow guards in calc_sleep_time (#974)#999
Merged
Conversation
) The speed_mbpsrate and speed_packetrate branches of calc_sleep_time() each had a tiered guard meant to avoid overflowing COUNTER (64-bit) before dividing, gated on COUNTER_OVERFLOW_RISK (COUNTER_MAX >> 23, ~2.2e12). In both cases the guard's threshold was many orders of magnitude higher than the point where the guarded multiply actually overflows: - speed_mbpsrate: bits_sent * 1e9 overflows once bits_sent exceeds ~1.8e10 (~2.3GB sent) -- ~119x below the guard's threshold. - speed_packetrate: pkts_sent * 1e9 * 3600 overflows once pkts_sent exceeds ~5.1M packets -- ~4.3e5x below the guard's threshold. So for any real long-running -M/-p replay, the "safe" branch was essentially never taken and the multiply silently overflowed, wrapping next_tx_ns to a small/garbage value and causing tcpreplay to stop sleeping between packets near EOF, exactly matching #974's reported symptom (confirmed against the reporter's log: their divergence starts at ~2.3GB sent, matching the computed boundary). Replace both guarded multiplies with a widened-precision multiply (unsigned __int128, available on GCC/Clang for all listed 64-bit targets) divided by the rate, which cannot overflow for any COUNTER-representable input, with a portable non-__int128 fallback that unconditionally uses the previously-underused-but-safe reordering (multiply/divide split around the division) instead of a threshold guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Credit @blaa (reported #974) and @AB-Coder96 (diagnosed the bug, validated repro numbers, prototype fix in #989). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fklassen
marked this pull request as ready for review
July 16, 2026 21:04
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #974 (constant-rate
-M/-preplays stop sleeping and blast the remainder at fullspeed near EOF on long/high-volume replays).
Root cause: in
calc_sleep_time()(src/send_packets.c), both thespeed_mbpsrateandspeed_packetratebranches guard a multiply-before-divide againstCOUNTER(64-bit)overflow using
COUNTER_OVERFLOW_RISK(COUNTER_MAX >> 23, ≈ 2.199e12). That threshold ismiscalibrated by orders of magnitude relative to where the guarded multiply actually
overflows, so the "safe" branch is essentially never taken in practice and the multiply
silently overflows on any realistically long replay.
Root-cause arithmetic
COUNTER_OVERFLOW_RISK = (COUNTER_MAX) >> 23 = 2^41 ≈ 2.199e12(for the default 64-bitCOUNTER).speed_mbpsrateguardsbits_sent * 1000000000. That product overflowsCOUNTERonce:That's ~119x below the guard's 2.199e12 threshold — so the guard essentially never
protects the multiply. This lines up almost exactly with the reporter's log in #974: their
divergence starts right around 2.3GB sent (
"Actual: 11462618 packets (2302609947 bytes)..."followed a few thousand packets later by
nap=0s 0nsecfor the rest of the run), which isthe computed overflow boundary (2,305,843,009 bytes) to within 0.14%.
speed_packetrateguardspkts_sent * 1000000000 * 3600. That overflows once:~4.3e5x below the same 2.199e12 guard threshold. This branch is untouched by #989 and has
the same class of bug — just as live, but with a much lower packet-count trigger point (~5.1M
packets, vs. millions/tens-of-millions being a completely normal packetrate-replay size), so
it's a latent bug that just hasn't been reported yet.
Verified both boundary computations and the reporter's log alignment with a throwaway C
program (compiled, run, then deleted — not part of this diff) comparing the old formula, a
long doublereference, and the new formula across: the exact reporter boundary, one bit pastit, multi-terabyte/petabyte-scale
bits_sentat lowbps, and 40–100 Gbps-classbpsatshort duration.
Is #989 sufficient?
#989 (thanks @AB-Coder96 for the reproduction and validation work — the before/after Mbps
numbers on their synthetic 22M-packet pcap are a good confirmation of the
speed_mbpsratesymptom) replaces the guarded multiply with quotient/remainder decomposition:
This fixes the reported long-duration/low-rate case, but the remainder term
(bits_sent % bps) * 1000000000can itself overflow:bits_sent % bpscan be as large asbps - 1, so this term overflows oncebps > COUNTER_MAX / 1e9 ≈ 1.8447e10bits/sec(~18.4 Gbps). tcpreplay is a line-rate packet generator (per this project's own design
goals) and 40/100 Gbps-class
-Mvalues are realistic, not hypothetical. I confirmed thisconcretely with a crafted worst-case remainder at
bps = 40 Gbps:So #989 alone would fix the reported case but reintroduce a (smaller, but real) overflow
window at high
-Mrates. #982 is broader (touches both branches) but reorders the existingguarded-multiply pattern using the same miscalibrated
COUNTER_OVERFLOW_RISKthreshold, soit doesn't address the root cause either (the "safe" branch is still gated behind a threshold
~119x/~4.3e5x too high to ever trigger where it's actually needed).
What this PR does
For both
speed_mbpsrateandspeed_packetrate, replace the guarded-multiply / thresholdpattern with a single multiply-then-divide done in a wider-than-COUNTER intermediate
(
unsigned __int128), which cannot overflow for anyCOUNTER-representable inputs on eitherside — no threshold, no decomposition, no residual overflow window at any realistic (or even
unrealistic)
bps/pph:This is cheaper than both the old code and #989's version: one multiply + one divide per
call, vs. the old code's branch + potential extra multiply, or #989's extra
%+ multiply +divide. No new syscalls, no heap allocation.
__int128/unsigned __int128is a GCC/Clang extension, not standard C. It's available onGCC and Clang for all of this project's stated 64-bit targets (Linux, macOS/Darwin, *BSD,
Solaris, Haiku, Cygwin) when built with those compilers, but it is non-standard, so I've
gated it behind
#if defined(__SIZEOF_INT128__)(the compiler-defined macro that indicatesthe type's availability) with a portable fallback for any compiler where it's absent:
This fallback is exactly the old code's own "safe" branch (the one previously gated behind
the miscalibrated threshold), just applied unconditionally instead of behind a guard that
almost never triggered it. It remains overflow-free for
bits_sentup to ~1.8e16(~2.3 petabytes sent) /
pkts_sentup to ~1.8e13 packets — far beyond any real replay — soit's a safe portability fallback, not a compromise on correctness for realistic inputs.
Flagging for review: the
__int128usage is a portability tradeoff worth a humansign-off per project convention — happy to drop the
__int128path entirely and just ship theunconditional-fallback formula everywhere if that's preferred (it's already correct for every
realistic input;
__int128only buys correctness at truly extreme, unrealistic values).Out of scope / noted for a follow-up: while auditing this function, I noticed
*skip_length = ((tx_ns - next_tx_ns) * bps) / 8000000000;a few lines below thespeed_mbpsratefix has a similar-shaped multiply that can overflow at very highbpscombined with a large scheduling-stall-induced
tx_ns - next_tx_nsdelta (back-of-envelope:~184ms stall at 100Gbps is enough). I did not touch it since it's a distinct code path from
what #974 reports and wasn't touched by #989/#982 either — flagging in case a human wants a
follow-up.
Verification
mkdir build && cd build && ../autogen.sh && ../configure --disable-local-libopts --enable-debug && make— succeeds with no warnings onsend_packets.c(verified for bothtcpreplayandtcpreplay-editobject builds, the twobinaries that compile this file).
arithmetic verification is the primary gate here, per the numbers above and the boundary
tests below.
exercising the old formula, the
__int128fix, the fallback formula, and Fix Mbps pacing overflow on long replays #989's ownformula, cross-checked against a
long doublereference, across:bits_sent ≈ 1.8447e10,bps=-M 30) — reproduces theold code's bug (result jumps from a correct ~6.15e11 ns to just 9 ns one bit later);
-M 1, andbits_sentnear2^63at-M 1(multi-terabyte/petabyte-scale,low bps);
bpsat short duration (1GB/10GB sent);(shown above);
speed_packetrateat the ~5.12M-packet boundary, at [Bug] Tcpreplay ceases to sleep between packets when executed with -M 10 #974's own 22M-packet scale, and at4 billion packets (spanning the 32-bit boundary).
__int128fix matched thelong doublereference; the fallbackformula matched to within sub-millisecond truncation (the same truncation
characteristics as the old code's already-shipped "safe" branch — not a new regression).
References
closed by this PR)