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 <h3c.err> 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 <h3c.err>) 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.

diff --git a/src/h3.c b/src/h3.c
index 9c6b17ea1c63ff87446aa0b9f30952ac8ec1687a..9874aea843a8bbf45c72890e2ec5aa846b6cd8db 100644 (file)
--- 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 <htx> buffer into <qcs> 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 <htx> buffer of size <count> 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. */