quinn-proto: harden congestion arithmetic and resolve token cache panic surface#2706
Open
MerlijnW70 wants to merge 3 commits into
Open
quinn-proto: harden congestion arithmetic and resolve token cache panic surface#2706MerlijnW70 wants to merge 3 commits into
MerlijnW70 wants to merge 3 commits into
Conversation
Member
|
Please use three separate commits for this. |
An extreme (forged or aggregated) ACK byte count could overflow the congestion window, byte counters, and ack-aggregation state, panicking in debug builds. Use saturating_add/saturating_mul on these paths; for all realistic values the behaviour is unchanged. Adds regression tests driving each controller with u64::MAX ACK byte counts.
A caller panicking while holding the cache mutex would poison it, after which every later insert/take panicked on lock().unwrap(). Recover the guard with PoisonError::into_inner instead: a poisoned lock only means a previous holder panicked, and the cache state remains a valid token store.
check_and_insert computed issued + lifetime while holding the state mutex. An extreme lifetime overflowed the SystemTime addition and panicked with the guard held, poisoning the mutex so every later validation panicked on lock(). Compute the expiry up front with SystemTime::checked_add and reject an overflowing lifetime as token reuse, and recover a poisoned lock instead of unwrapping it. Adds a regression test using an overflowing token lifetime.
MerlijnW70
force-pushed
the
fix/proto-stability
branch
from
June 26, 2026 09:59
5247ea0 to
c41a271
Compare
Author
|
Split into three commits as requested — one per subsystem (congestion arithmetic, token-cache lock recovery, BloomTokenLog lifetime), each carrying its own regression test. |
Member
|
Are you interested in backporting these to the |
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
2 participants
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.
While reviewing the congestion control and token management logic, I
identified several scenarios — particularly under extreme ACK-aggregation
and token-replay conditions — where
unwrap()calls and arithmeticoperations could trigger panics.
This PR implements hardening measures so the protocol degrades gracefully
rather than crashing under these edge cases:
Congestion control — replaced potentially overflowing arithmetic in
NewReno and BBR with saturating operations (
saturating_add,saturating_mul) on the ACK-driven window, byte counters, andack-aggregation state.
Token cache — improved safety in
token_memory_cache.rsandbloom_token_log.rsby recovering from a poisoned lock(
PoisonError::into_inner) instead oflock().unwrap(), so one panickingcaller can no longer poison the cache for the whole system.
BloomTokenLogadditionally computes token expiry withSystemTime::checked_add, rejecting an overflowing lifetime as token reuseinstead of panicking while the lock is held.
Regression tests — added 3 unit tests driving these paths with extreme
inputs (
u64::MAXbyte counts, an overflowing token lifetime) to verifythey are now contained and panic-free. Each test panicked prior to the fix.
This addresses the stability concerns outlined in #2702. The changes are
regression-tested and introduce no public API signature changes.