From c714b6bb55e34c7cd2cb3ff7dbed374e6b6eae65 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 18 Jun 2024 15:20:32 +0200 Subject: [PATCH] BUG/MAJOR: quic: fix padding with short packets QUIC sending functions were extended to be more flexible. Of all the changes, they support now iterating over a variable instance of QEL instance of only 2 previously. This change has rendered PADDING emission less previsible, which was adjusted via the following patch : a60609f1aa3e5f61d2a2286fdb40ebf6936a80ee BUG/MINOR: quic: fix padding of INITIAL packets Its main purpose was to ensure PADDING would only be generated for the last iterated QEL instance, to avoid unnecessary padding. In parallel, a BUG_ON() statement ensure that built INITIAL packets are always padded to 1.200 bytes as necessary before emitted them. This BUG_ON() statement caused crash in one particular occurence : when building datagrams that mixed Initial long packets and 1-RTT short packets. This last occurence type does not have a length field in its header, contrary to Long packets. This caused a miscalculation for the necessary padding size, with INITIAL packets not padded enough to reach the necessary 1.200 bytes size. This issue was detected on 3.0.2. It can be reproduced by using 0-RTT combined with latency. Here are the used commands : $ ngtcp2-client --tp-file=/tmp/ngtcp2-tp.txt \ --session-file=/tmp/ngtcp2-session.txt --exit-on-all-streams-close \ 127.0.0.1 20443 "https://[::]/?s=32o" $ sudo tc qdisc add dev lo root netem latency 500ms Note that this issue cannot be reproduced on current dev version. Indeed, it seems that the following patch introduce a slight change in packet building ordering : cdfceb10ae136b02e51f9bb346321cf0045d58e0 MINOR: quic: refactor qc_prep_pkts() loop This must be backported to 3.0. This should fix github issue #2609. --- src/quic_tx.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/quic_tx.c b/src/quic_tx.c index 215a4cbd5..a1705784c 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -1888,20 +1888,23 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end, padding_len = 0; len_sz = quic_int_getsize(len); /* Add this packet size to */ - dglen += head_len + len_sz + len; - /* Note that is true only when building an Handshake packet - * coalesced to an Initial packet. - */ + dglen += head_len + len; + if (pkt->type != QUIC_PACKET_TYPE_SHORT) + dglen += len_sz; + if (padding && dglen < QUIC_INITIAL_PACKET_MINLEN) { - /* This is a maximum padding size */ padding_len = QUIC_INITIAL_PACKET_MINLEN - dglen; - /* The length field value is of this packet is + - * the size of which may be greater than the initial computed size - * . So, let's deduce the difference between these to packet - * sizes from . - */ - padding_len -= quic_int_getsize(len + padding_len) - len_sz; + len += padding_len; + /* Update size of packet length field with new PADDING data. */ + if (pkt->type != QUIC_PACKET_TYPE_SHORT) { + size_t len_sz_diff = quic_int_getsize(len) - len_sz; + if (len_sz_diff) { + padding_len -= len_sz_diff; + len_sz += len_sz_diff; + dglen += len_sz_diff; + } + } } else if (len_frms && len_frms < QUIC_PACKET_PN_MAXLEN) { len += padding_len = QUIC_PACKET_PN_MAXLEN - len_frms;