Skip to content

Commit 01d08d4

Browse files
committed
BUG/MINOR: quic: fix malformed probing packet building
This bug arrived with this commit: cdfceb1 MINOR: quic: refactor qc_prep_pkts() loop which prevented haproxy from sending PING only packets/datagrams (some packets/datagrams with only PING frame as ack-eliciting frames inside). Such packets/datagrams are useful in rare cases during retransmissions when one wants to probe the peer without exceeding the anti-amplification limit. Modify the condition passed to qc_build_pkt() to add padding to the current datagram. One does not want to do that when probing the peer because qc_build_pkt() calls qc_do_build_pkt() which supports this case: if <probe> is true (probing required), qc_do_build_pkt() handle the case where some padding must be added to a PING only packet/datagram. This is the case when probing with an empty <frms> frame list of ack-eliciting frame without exceeding the anti-amplification limit from qc_dgrams_retransmit(). Add some comments to qc_build_pkt() and qc_do_build_pkt() to clarify this as this code is easy to break! Thank you for @Tristan971 for having reported this issue in GH #2709. Must be backported to 3.0.
1 parent fc59063 commit 01d08d4

File tree

1 file changed

+21
-2
lines changed

1 file changed

+21
-2
lines changed

src/quic_tx.c

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -623,14 +623,18 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
623623
break;
624624
}
625625

626-
/* padding will be set for last QEL */
626+
/* padding will be set for last QEL, except when probing:
627+
* to build a PING only non coalesced Initial datagram for
628+
* instance when blocked by the anti-amplification limit,
629+
* this datagram MUST be padded.
630+
*/
627631
padding = 1;
628632
}
629633

630634
pkt_type = quic_enc_level_pkt_type(qc, qel);
631635
cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms,
632636
qc, ver, dglen, pkt_type,
633-
must_ack, padding && !next_qel,
637+
must_ack, padding && !next_qel && !probe,
634638
probe, cc, &err);
635639
if (!cur_pkt) {
636640
switch (err) {
@@ -1812,6 +1816,21 @@ static inline uint64_t quic_compute_ack_delay_us(unsigned int time_received,
18121816
* number field in this packet. <pn_len> will also have the packet number
18131817
* length as value.
18141818
*
1819+
* NOTE: This function does not build all the possible combinations of packets
1820+
* depending on its list of parameters. In most of case, <frms> frame list is
1821+
* not empty.
1822+
* So, this function first try to build this list of frames.
1823+
* Then some padding is added to this packet if <padding> boolean is set true.
1824+
* The unique case one wants to do that is when a first Initial packet was
1825+
* previously built into the same datagram as the one currently built AND when
1826+
* this packet is supposed to pad the datagram, if required, to build an at
1827+
* least 1200 bytes long Initial datagram.
1828+
* If <padding> is not true, if the packet is too short, the packet is also
1829+
* padded. This is the case when no frames are provided by <frms> and when
1830+
* probing with a only PING frame.
1831+
* Finally, if <frms> was empty, if <probe> boolean is true this function builds
1832+
* a PING only packet handling also the cases where it must be padded.
1833+
*
18151834
* Return 1 if succeeded (enough room to buile this packet), O if not.
18161835
*/
18171836
static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,

0 commit comments

Comments
 (0)