MINOR: quic: make a packet build fails when qc_build_frm() fails.

Even if the size of frames built by qc_build_frm() are computed so that
not to overflow a buffer, do not rely on this and always makes a packet
build fails if we could not build a frame.
Also add traces to have an idea where qc_build_frm() fails.
Fixes a memory leak in qc_build_phdshk_apkt().

diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h
index 37cbb46f0aa874153f46d7e964aa1ed31823c7b2..16883fbad6dc98c56c98f3e95a982b018ca503f1 100644 (file)
--- a/include/haproxy/xprt_quic-t.h
+++ b/include/haproxy/xprt_quic-t.h
@@ -200,7 +200,6 @@ enum quic_pkt_type {
 #define           QUIC_EV_CONN_ADDDATA   (1ULL << 25)
 #define           QUIC_EV_CONN_FFLIGHT   (1ULL << 26)
 #define           QUIC_EV_CONN_SSLALERT  (1ULL << 27)
-#define           QUIC_EV_CONN_CPAPKT    (1ULL << 28)
 #define           QUIC_EV_CONN_RTTUPDT   (1ULL << 29)
 #define           QUIC_EV_CONN_CC        (1ULL << 30)
 #define           QUIC_EV_CONN_SPPKTS    (1ULL << 31)

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 3ab9328ecccc4a314e91a758b80d1551f87ebd14..931361cdfe5714081a6efe21eb2a73e26eda2fbe 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -86,7 +86,6 @@ static const struct trace_event quic_trace_events[] = {
        { .mask = QUIC_EV_CONN_ADDDATA,  .name = "add_hdshk_data",   .desc = "TLS stack ->add_handshake_data() call"},
        { .mask = QUIC_EV_CONN_FFLIGHT,  .name = "flush_flight",     .desc = "TLS stack ->flush_flight() call"},
        { .mask = QUIC_EV_CONN_SSLALERT, .name = "send_alert",       .desc = "TLS stack ->send_alert() call"},
-       { .mask = QUIC_EV_CONN_CPAPKT,   .name = "phdshk_cpakt",     .desc = "clear post handhshake app. packet preparation" },
        { .mask = QUIC_EV_CONN_RTTUPDT,  .name = "rtt_updt",         .desc = "RTT sampling" },
        { .mask = QUIC_EV_CONN_SPPKTS,   .name = "sppkts",           .desc = "send prepared packets" },
        { .mask = QUIC_EV_CONN_PKTLOSS,  .name = "pktloss",          .desc = "detect packet loss" },
@@ -282,7 +281,7 @@ static void quic_trace(enum trace_level level, uint64_t mask, const struct trace
 
                }
 
-               if (mask & QUIC_EV_CONN_HPKT) {
+               if (mask & (QUIC_EV_CONN_HPKT|QUIC_EV_CONN_PAPKT)) {
                        const struct quic_tx_packet *pkt = a2;
                        const struct quic_enc_level *qel = a3;
                        const ssize_t *room = a4;
@@ -3363,8 +3362,12 @@ static ssize_t qc_do_build_hdshk_pkt(struct q_buf *wbuf,
        /* Packet number encoding. */
        quic_packet_number_encode(&pos, end, pn, *pn_len);
 
-       if (ack_frm_len)
-               qc_build_frm(&pos, end, &ack_frm, pkt, conn);
+       if (ack_frm_len && !qc_build_frm(&pos, end, &ack_frm, pkt, conn)) {
+               ssize_t room = end - pos;
+               TRACE_PROTO("Not enough room", QUIC_EV_CONN_HPKT,
+                           conn->conn, NULL, NULL, &room);
+               goto err;
+       }
 
        /* Crypto frame */
        if (!LIST_ISEMPTY(&pkt->frms)) {
@@ -3374,7 +3377,12 @@ static ssize_t qc_do_build_hdshk_pkt(struct q_buf *wbuf,
                        crypto->offset = cf->crypto.offset;
                        crypto->len = cf->crypto.len;
                        crypto->qel = qel;
-                       qc_build_frm(&pos, end, &frm, pkt, conn);
+                       if (!qc_build_frm(&pos, end, &frm, pkt, conn)) {
+                               ssize_t room = end - pos;
+                               TRACE_PROTO("Not enough room", QUIC_EV_CONN_HPKT,
+                                                       conn->conn, NULL, NULL, &room);
+                               goto err;
+                       }
                }
        }
 
@@ -3451,7 +3459,7 @@ static ssize_t qc_build_hdshk_pkt(struct q_buf *buf, struct quic_conn *qc, int p
        struct quic_tls_ctx *tls_ctx;
        struct quic_tx_packet *pkt;
 
-       TRACE_ENTER(QUIC_EV_CONN_HPKT, qc->conn,, qel);
+       TRACE_ENTER(QUIC_EV_CONN_HPKT, qc->conn, NULL, qel);
        pkt = pool_alloc(pool_head_quic_tx_packet);
        if (!pkt) {
                TRACE_DEVEL("Not enough memory for a new packet", QUIC_EV_CONN_HPKT, qc->conn);
@@ -3531,7 +3539,7 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
        size_t fake_len, ack_frm_len;
        int64_t largest_acked_pn;
 
-       TRACE_ENTER(QUIC_EV_CONN_CPAPKT, conn->conn);
+       TRACE_ENTER(QUIC_EV_CONN_PAPKT, conn->conn);
        beg = pos = q_buf_getpos(wbuf);
        end = q_buf_end(wbuf);
        /* When not probing and not acking, reduce the size of this buffer to respect
@@ -3549,8 +3557,12 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
        *pn_len = quic_packet_number_length(pn, largest_acked_pn);
        /* Check there is enough room to build this packet (without payload). */
        if (end - pos < QUIC_SHORT_PACKET_MINLEN + sizeof_quic_cid(&conn->dcid) +
-           *pn_len + QUIC_TLS_TAG_LEN)
+           *pn_len + QUIC_TLS_TAG_LEN) {
+               ssize_t room = end - pos;
+               TRACE_PROTO("Not enough room", QUIC_EV_CONN_PAPKT,
+                           conn->conn, NULL, NULL, &room);
                goto err;
+       }
 
        /* Reserve enough room at the end of the packet for the AEAD TAG. */
        end -= QUIC_TLS_TAG_LEN;
@@ -3573,14 +3585,19 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
                qel->pktns->flags &= ~QUIC_FL_PKTNS_ACK_REQUIRED;
        }
 
-       if (ack_frm_len)
-               qc_build_frm(&pos, end, &ack_frm, pkt, conn);
+       if (ack_frm_len && !qc_build_frm(&pos, end, &ack_frm, pkt, conn)) {
+               ssize_t room = end - pos;
+               TRACE_PROTO("Not enough room", QUIC_EV_CONN_PAPKT,
+                           conn->conn, NULL, NULL, &room);
+               goto err;
+       }
 
        fake_len = ack_frm_len;
        if (!LIST_ISEMPTY(&qel->pktns->tx.frms) &&
            !qc_build_cfrms(pkt, end - pos, &fake_len, qel, conn)) {
-               TRACE_DEVEL("some CRYPTO frames could not be built",
-                           QUIC_EV_CONN_CPAPKT, conn->conn);
+               ssize_t room = end - pos;
+               TRACE_PROTO("some CRYPTO frames could not be built",
+                           QUIC_EV_CONN_PAPKT, conn->conn, NULL, NULL, &room);
                goto err;
        }
 
@@ -3594,7 +3611,12 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
                        crypto->offset = cf->crypto.offset;
                        crypto->len = cf->crypto.len;
                        crypto->qel = qel;
-                       qc_build_frm(&pos, end, &frm, pkt, conn);
+                       if (!qc_build_frm(&pos, end, &frm, pkt, conn)) {
+                               ssize_t room = end - pos;
+                               TRACE_PROTO("Not enough room", QUIC_EV_CONN_PAPKT,
+                                           conn->conn, NULL, NULL, &room);
+                               goto err;
+                       }
                }
        }
 
@@ -3604,7 +3626,7 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
 
                ppos = pos;
                if (!qc_build_frm(&ppos, end, frm, pkt, conn)) {
-                       TRACE_DEVEL("Frames not built", QUIC_EV_CONN_CPAPKT, conn->conn);
+                       TRACE_DEVEL("Frames not built", QUIC_EV_CONN_PAPKT, conn->conn);
                        break;
                }
 
@@ -3614,11 +3636,11 @@ static ssize_t qc_do_build_phdshk_apkt(struct q_buf *wbuf,
        }
 
  out:
-       TRACE_LEAVE(QUIC_EV_CONN_CPAPKT, conn->conn, (int *)(pos - beg));
+       TRACE_LEAVE(QUIC_EV_CONN_PAPKT, conn->conn);
        return pos - beg;
 
  err:
-       TRACE_DEVEL("leaving in error (buffer full)", QUIC_EV_CONN_CPAPKT, conn->conn);
+       TRACE_DEVEL("leaving in error (buffer full)", QUIC_EV_CONN_PAPKT, conn->conn);
        return -1;
 }
 
@@ -3665,13 +3687,13 @@ static ssize_t qc_build_phdshk_apkt(struct q_buf *wbuf, struct quic_conn *qc)
 
        tls_ctx = &qel->tls_ctx;
        if (!quic_packet_encrypt(payload, payload_len, beg, aad_len, pn, tls_ctx, qc->conn))
-               return -2;
+               goto err;
 
        end += QUIC_TLS_TAG_LEN;
        pkt_len += QUIC_TLS_TAG_LEN;
        if (!quic_apply_header_protection(beg, buf_pn, pn_len,
                                          tls_ctx->tx.hp, tls_ctx->tx.hp_key))
-               return -2;
+               goto err;
 
        q_buf_setpos(wbuf, end);
        /* Consume a packet number. */
@@ -3690,9 +3712,14 @@ static ssize_t qc_build_phdshk_apkt(struct q_buf *wbuf, struct quic_conn *qc)
        /* Attach this packet to <buf>. */
        LIST_ADDQ(&wbuf->pkts, &pkt->list);
 
-       TRACE_LEAVE(QUIC_EV_CONN_PAPKT, qc->conn);
+       TRACE_LEAVE(QUIC_EV_CONN_PAPKT, qc->conn, pkt);
 
        return pkt_len;
+
+ err:
+       free_quic_tx_packet(pkt);
+       TRACE_DEVEL("leaving in error", QUIC_EV_CONN_PAPKT, qc->conn);
+       return -2;
 }
 
 /* Prepare a maximum of QUIC Application level packets from <ctx> QUIC