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/src/xprt_quic.c b/src/xprt_quic.c
index 3ab9328..931361c 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -86,7 +86,6 @@
{ .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 @@
}
- 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 @@
/* 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 @@
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 @@
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 @@
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 @@
*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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
/* 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