From 2144d2418651c1f76b91cc1f6e745feecdefcb00 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Fri, 22 Dec 2023 09:00:13 +0100 Subject: [PATCH] BUG/MINOR: h3: close connection on sending alloc errors When encoding new HTTP/3 frames, QCS Tx buffer must be allocated if currently NULL. Previously, allocation failure was not properly checked, leaving the connection in an unspecified state, or worse risking a crash. Fix this by setting to H3_INTERNAL_ERROR each time the allocation fails. This will stop sending and close the connection. In the future, it may be better to put the connection on pause waiting for allocation to succeed but this is too complicated to implement for now in a reliable way. Along with the current change, return of all HTX parsing functions (h3_resp_*_send) were set to a negative value in case of error. A new BUG_ON() in h3_snd_buf() ensures that if such a value is returned, either a connection error is register (via ) or buffer is temporarily full (flag QC_SF_BLK_MROOM). This should fix github issue #2389. This should be backported up to 2.6. Note that qcc_get_stream_txbuf() does not exist in 2.9 and below. mux_get_buf() is its equivalent. An explicit check b_is_null(&qcs.tx.buf) should be used there. --- src/h3.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/h3.c b/src/h3.c index 9c6b17ea1c..9874aea843 100644 --- a/src/h3.c +++ b/src/h3.c @@ -1542,8 +1542,11 @@ static int h3_resp_headers_send(struct qcs *qcs, struct htx *htx) list[hdr].n = ist(""); - if (!(res = qcc_get_stream_txbuf(qcs))) + if (!(res = qcc_get_stream_txbuf(qcs))) { + TRACE_ERROR("cannot allocate Tx buffer", H3_EV_TX_FRAME|H3_EV_TX_HDR, qcs->qcc->conn, qcs); + h3c->err = H3_INTERNAL_ERROR; goto err; + } /* At least 5 bytes to store frame type + length as a varint max size */ if (b_room(res) < 5) @@ -1617,7 +1620,7 @@ static int h3_resp_headers_send(struct qcs *qcs, struct htx *htx) err: TRACE_DEVEL("leaving on error", H3_EV_TX_FRAME|H3_EV_TX_HDR, qcs->qcc->conn, qcs); - return 0; + return -1; } /* Convert a series of HTX trailer blocks from buffer into buffer @@ -1681,8 +1684,11 @@ static int h3_resp_trailers_send(struct qcs *qcs, struct htx *htx) } list[hdr].n = ist(""); - if (!(res = qcc_get_stream_txbuf(qcs))) + if (!(res = qcc_get_stream_txbuf(qcs))) { + TRACE_ERROR("cannot allocate Tx buffer", H3_EV_TX_FRAME|H3_EV_TX_HDR, qcs->qcc->conn, qcs); + h3c->err = H3_INTERNAL_ERROR; goto err; + } /* At least 9 bytes to store frame type + length as a varint max size */ if (b_room(res) < 9) { @@ -1758,7 +1764,7 @@ static int h3_resp_trailers_send(struct qcs *qcs, struct htx *htx) err: TRACE_DEVEL("leaving on error", H3_EV_TX_FRAME|H3_EV_TX_HDR, qcs->qcc->conn, qcs); - return 0; + return -1; } /* Convert a series of HTX data blocks from buffer of size into @@ -1772,6 +1778,8 @@ static int h3_resp_trailers_send(struct qcs *qcs, struct htx *htx) static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, struct buffer *buf, size_t count) { + struct h3s *h3s = qcs->ctx; + struct h3c *h3c = h3s->h3c; struct buffer outbuf; struct buffer *res; size_t total = 0; @@ -1796,7 +1804,9 @@ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, goto end; if (!(res = qcc_get_stream_txbuf(qcs))) { - /* TODO */ + TRACE_ERROR("cannot allocate Tx buffer", H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); + h3c->err = H3_INTERNAL_ERROR; + goto err; } /* If HTX contains only one DATA block, try to exchange it with MUX @@ -1844,8 +1854,9 @@ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, * on SEND. */ if (b_size(&outbuf) <= hsize) { + TRACE_STATE("not enough room for data frame", H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); qcs->flags |= QC_SF_BLK_MROOM; - goto end; + goto err; } if (b_size(&outbuf) < hsize + fsize) @@ -1873,6 +1884,10 @@ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, end: TRACE_LEAVE(H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); return total; + + err: + TRACE_DEVEL("leaving on error", H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); + return -1; } static size_t h3_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count) @@ -1885,7 +1900,7 @@ static size_t h3_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count) struct htx_blk *blk; uint32_t bsize; int32_t idx; - int ret; + int ret = 0; h3_debug_printf(stderr, "%s\n", __func__); @@ -1944,6 +1959,11 @@ static size_t h3_snd_buf(struct qcs *qcs, struct buffer *buf, size_t count) count -= bsize; break; } + + /* If an error occured, either buffer space or connection error + * must be set to break current loop. + */ + BUG_ON(ret < 0 && !(qcs->flags & QC_SF_BLK_MROOM) && !h3c->err); } /* Interrupt sending on connection error. */ -- 2.47.3