BUG/MINOR: quic: MIssing check when building TX packets

When building an ack-eliciting frame only packet, if we did not manage to add
at least one such a frame to the packet, we did not notify the caller about
the fact the packet is empty. This could lead the caller to believe
everything was ok and make it endlessly try to build packet again and again.
This issue was amplified by the recent changes where a while(1) loop has been
added to qc_send_app_pkt() which calls qc_do_build_pkt() through qc_prep_app_pkts()
until we could not prepare packets. Before this recent change, I guess only
one empty packet was sent.
This patch checks that non empty packets could be built by qc_do_build_pkt()
and makes this function return an error if this was the case. Also note that
such an issue could happened only when the packet building was limited by
the congestion control.

Thank you to Tristan for having reported this issue in GH #1808.

Must be backported to 2.6.

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 7d7ba0bfbebd7f0aee034e5a8cf59cbb011726f4..762a3725b7d1787bad86d8af854307fdb52dc3b7 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -6592,7 +6592,7 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                            struct quic_enc_level *qel, struct quic_conn *qc,
                            const struct quic_version *ver, struct list *frms)
 {
-       unsigned char *beg;
+       unsigned char *beg, *payload;
        size_t len, len_sz, len_frms, padding_len;
        struct quic_frame frm = { .type = QUIC_FT_CRYPTO, };
        struct quic_frame ack_frm = { .type = QUIC_FT_ACK, };
@@ -6743,6 +6743,8 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
        if (!quic_packet_number_encode(&pos, end, pn, *pn_len))
                goto no_room;
 
+       /* payload building (ack-eliciting or not frames) */
+       payload = pos;
        if (ack_frm_len) {
                if (!qc_build_frm(&pos, end, &ack_frm, pkt, qc))
                        goto no_room;
@@ -6760,10 +6762,10 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                                ssize_t room = end - pos;
                                TRACE_DEVEL("Not enough room", QUIC_EV_CONN_TXPKT,
                                            qc, NULL, NULL, &room);
-                               /* TODO: this should not have happened if qc_build_frms()
-                                * had correctly computed and sized the frames to be
-                                * added to this packet. Note that <cf> was added
-                                * from <frm_list> to <frms> list by qc_build_frms().
+                               /* TODO: this should not have happened except if we
+                                * are limited by the congestion control.
+                                * Note that <cf> was added from <frm_list> to <frms> list by
+                                * qc_build_frms().
                                 */
                                LIST_DELETE(&cf->list);
                                LIST_INSERT(frms, &cf->list);
@@ -6799,6 +6801,12 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
                        goto no_room;
        }
 
+       if (pos == payload) {
+               /* No payload was built because of congestion control */
+               TRACE_DEVEL("limited by congestion control", QUIC_EV_CONN_TXPKT, qc);
+               goto no_room;
+       }
+
        /* If this packet is ack-eliciting and we are probing let's
         * decrement the PTO probe counter.
         */