BUG/MEDIUM: quic: fix loss time selection in quic_loss_pktns()

quic_loss_pktns() must return the packet number space with the earliest
loss time. But an unset loss time (TICK_ETERNITY, i.e. 0) could win the
selection and hide a real loss time.

Indeed, since a55acf993 ("BUG/MINOR: quic: ticks comparison without
ticks API use"), the candidate loss time is compared with
tick_is_lt(). An unset candidate (0) then beats any armed loss time T
whenever (0 - T) wraps to a negative value, that is to say whenever
T lies in the low half of the 32-bit tick circle. c7c87641 ("MINOR:
quic: Wrong packet number space selection in quic_loss_pktns()") had
already fixed the same problem for the naive comparison in 2021, but
the guard was inverted by 1f6cf181 in the same year.

The bug is not a rare calendar coincidence: clock.c forces the internal
time to wrap BOOT_TIME_WRAP_SEC (20) seconds after boot, after which
now_ms starts over from zero and lives in the low half of its 32-bit
circle for about 24.9 days. Thus, on every instance restarted less
than ~25 days ago, the loss time of the Initial packet number space
could be discarded. The Initial packets in flight were then never
declared lost, their loss time was never honored, and the PTO branch
kept arming probe requests on both Initial and Handshake spaces
without end, as long as the Handshake packets never reached the
client: with the Handshake probe flag cleared without emitting
(quic_tx.c), the server entered a tight timer loop sending padded
Initial PINGs over and over without ever retransmitting the Handshake
CRYPTO data. A conforming client breaks this loop within about one
second by sending the anti-deadlock Handshake probe it must arm when
it holds Handshake keys but no Handshake packet of its own was
acknowledged; a loop lasting minutes was only observed with a client
missing this behavior.

Fix the selection by considering only the spaces with an armed loss
time. If none is armed, the function returns the first space of the
list, and both callers already check tick_isset() on the returned
loss time, so nothing else changes.

Thank you to @jbevemyr for having reported this issue in GH #3552.

This could be backported as far as 2.6 where the tick_is_lt() call
was introduced.

diff --git a/src/quic_loss.c b/src/quic_loss.c
index bdd05283762e87538486e651dcf3943b80eba01f..95aa6a91ba0a47ddccd0e14c50b8d64541dcd403 100644 (file)
--- a/src/quic_loss.c
+++ b/src/quic_loss.c
@@ -63,8 +63,14 @@ struct quic_pktns *quic_loss_pktns(struct quic_conn *qc)
 
        do {
                TRACE_PROTO("TX loss pktns", QUIC_EV_CONN_SPTO, qc, p);
-               if (!tick_isset(pktns->tx.loss_time) ||
-                   tick_is_lt(p->tx.loss_time, pktns->tx.loss_time)) {
+               /* Select a loss time only if it is set: TICK_ETERNITY (0)
+                * means "no loss time", and tick_is_lt(0, T) is true when T
+                * lies in the low half of the tick circle, so an unset loss
+                * time could hide a real one.
+                */
+               if (tick_isset(p->tx.loss_time) &&
+                   (!tick_isset(pktns->tx.loss_time) ||
+                    tick_is_lt(p->tx.loss_time, pktns->tx.loss_time))) {
                        pktns = p;
                }
                p = LIST_NEXT(&p->list, struct quic_pktns *, list);