BUG/MEDIUM: mux-quic: fix flow control connection Tx level

The flow control enforced at connection level is incorrectly calculated.
There is a risk of exceeding the limit. In most cases, this results in a
segfault produced by a BUG_ON which is here to catch this kind of error.
If not compiled with DEBUG_STRICT, this should generate a connection
closed by the client due to the flow control overflow.

The problem is encountered when transfered payload is big enough to fill
the transport congestion window. In this case, some data are rejected by
the transport layer and kept by the MUX to be reemitted later. However,
these preserved data are not counted on the connection flow control when
resubmitted, which gradually amplify the gap between expected and real
consumed flow control.

To fix this, handle the flow-control at the connection level in the same
way as the stream level. A new field qcc.tx.offsets is incremented as
soon as data are transfered between stream TX buffers. The field
qcc.tx.sent_offsets is preserved to count bytes handled by the transport
layer and stop the MUX transfer if limit is reached.

As already stated, this bug can occur during transfers with enough
emitted data, over multiple streams. When using a single stream, the
flow control at the stream level hides it.

The BUG_ON crash is reproduced systematically with quiche client :
$ quiche-client --no-verify --http-version HTTP/3 -n 10000 https://127.0.0.1:20443/10K

This must be backported up to 2.6 when confirmed to work as expected.

This should fix github issue #1738.

diff --git a/include/haproxy/mux_quic-t.h b/include/haproxy/mux_quic-t.h
index 29d3897b1b0bf5570a83b02031854c7ba1d26edd..606f4aaf3d98aee5a23d257e04f520ac7c135612 100644 (file)
--- a/include/haproxy/mux_quic-t.h
+++ b/include/haproxy/mux_quic-t.h
@@ -75,6 +75,7 @@ struct qcc {
                uint64_t max_data; /* Maximum number of bytes which may be received */
        } rx;
        struct {
+               uint64_t offsets; /* sum of all offsets prepared */
                uint64_t sent_offsets; /* sum of all offset sent */
        } tx;
 

diff --git a/src/mux_quic.c b/src/mux_quic.c
index c60fdfca8747179ba141b6b9797576cee2b65a72..8fad19f63dbc8075dc1f9e9eebf30e7e65eb857d 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -778,13 +778,12 @@ static void qc_release(struct qcc *qcc)
        TRACE_LEAVE(QMUX_EV_QCC_END);
 }
 
-/* Transfer as much as possible data on <qcs> from <in> to <out>. <max_data> is
- * the current flow-control limit on the connection which must not be exceeded.
+/* Transfer as much as possible data on <qcs> from <in> to <out>. This is done
+ * in respect with available flow-control at stream and connection level.
  *
  * Returns the total bytes of transferred data.
  */
-static int qcs_xfer_data(struct qcs *qcs, struct buffer *out,
-                         struct buffer *in, uint64_t max_data)
+static int qcs_xfer_data(struct qcs *qcs, struct buffer *out, struct buffer *in)
 {
        struct qcc *qcc = qcs->qcc;
        int left, to_xfer;
@@ -819,10 +818,10 @@ static int qcs_xfer_data(struct qcs *qcs, struct buffer *out,
        if (qcs->tx.offset + to_xfer > qcs->tx.msd)
                to_xfer = qcs->tx.msd - qcs->tx.offset;
 
-       BUG_ON_HOT(max_data > qcc->rfctl.md);
+       BUG_ON_HOT(qcc->tx.offsets > qcc->rfctl.md);
        /* do not overcome flow control limit on connection */
-       if (max_data + to_xfer > qcc->rfctl.md)
-               to_xfer = qcc->rfctl.md - max_data;
+       if (qcc->tx.offsets + to_xfer > qcc->rfctl.md)
+               to_xfer = qcc->rfctl.md - qcc->tx.offsets;
 
        if (!left && !to_xfer)
                goto out;
@@ -1033,14 +1032,12 @@ static int qc_send_frames(struct qcc *qcc, struct list *frms)
 
 /* Used internally by qc_send function. Proceed to send for <qcs>. This will
  * transfer data from qcs buffer to its quic_stream counterpart. A STREAM frame
- * is then generated and inserted in <frms> list. <qcc_max_data> is the current
- * flow-control max-data at the connection level which must not be surpassed.
+ * is then generated and inserted in <frms> list.
  *
  * Returns the total bytes transferred between qcs and quic_stream buffers. Can
  * be null if out buffer cannot be allocated.
  */
-static int _qc_send_qcs(struct qcs *qcs, struct list *frms,
-                        uint64_t qcc_max_data)
+static int _qc_send_qcs(struct qcs *qcs, struct list *frms)
 {
        struct qcc *qcc = qcs->qcc;
        struct buffer *buf = &qcs->tx.buf;
@@ -1061,13 +1058,14 @@ static int _qc_send_qcs(struct qcs *qcs, struct list *frms,
 
        /* Transfer data from <buf> to <out>. */
        if (b_data(buf)) {
-               xfer = qcs_xfer_data(qcs, out, buf, qcc_max_data);
+               xfer = qcs_xfer_data(qcs, out, buf);
                if (xfer > 0) {
                        qcs_notify_send(qcs);
                        qcs->flags &= ~QC_SF_BLK_MROOM;
                }
 
                qcs->tx.offset += xfer;
+               qcc->tx.offsets += xfer;
        }
 
        /* out buffer cannot be emptied if qcs offsets differ. */
@@ -1144,7 +1142,7 @@ static int qc_send(struct qcc *qcc)
                        continue;
                }
 
-               ret = _qc_send_qcs(qcs, &frms, qcc->tx.sent_offsets + total);
+               ret = _qc_send_qcs(qcs, &frms);
                total += ret;
                node = eb64_next(node);
        }
@@ -1161,7 +1159,7 @@ static int qc_send(struct qcc *qcc)
                BUG_ON(!b_data(&qcs->tx.buf));
                BUG_ON(qc_stream_buf_get(qcs->stream));
 
-               ret = _qc_send_qcs(qcs, &frms, qcc->tx.sent_offsets + tmp_total);
+               ret = _qc_send_qcs(qcs, &frms);
                tmp_total += ret;
                LIST_DELETE(&qcs->el);
        }
@@ -1342,7 +1340,7 @@ static int qc_init(struct connection *conn, struct proxy *prx,
        lparams = &conn->handle.qc->rx.params;
 
        qcc->rx.max_data = lparams->initial_max_data;
-       qcc->tx.sent_offsets = 0;
+       qcc->tx.sent_offsets = qcc->tx.offsets = 0;
 
        /* Client initiated streams must respect the server flow control. */
        qcc->strms[QCS_CLT_BIDI].max_streams = lparams->initial_max_streams_bidi;