BUG/MEDIUM: quic: remove unsent data from qc_stream_desc buf

QCS instances use qc_stream_desc for data buffering on emission. On
stream reset, its Tx channel is closed earlier than expected. This may
leave unsent data into qc_stream_desc.

Before this patch, these unsent data would remain after QCS freeing.
This prevents the buffer to be released as no ACK reception will remove
them. The buffer is only freed when the whole connection is closed. As
qc_stream_desc buffer is limited per connection, this reduces the buffer
pool for other streams of the same connection. In the worst case if
several streams are resetted, this may completely freeze the transfer of
the remaining connection streams.

This bug was reproduced by reducing the connection buffer pool to a
single buffer instance by using the following global statement :

  tune.quic.frontend.conn-tx-buffers.limit 1.

Then a QUIC client is used which opens a stream for a large enough
object to ensure data are buffered. The client them emits a STOP_SENDING
before reading all data, which forces the corresponding QCS instance to
be resetted. The client then opens a new request but the transfer is
freezed due to this bug.

To fix this, adjust qc_stream_desc API. Add a new argument <final_size>
on qc_stream_desc_release() function. Its value is compared to the
currently buffered offset in latest qc_stream_desc buffer. If
<final_size> is inferior, it means unsent data are present in the
buffer. As such, qc_stream_desc_release() removes them to ensure the
buffer will finally be freed when all ACKs are received. It is also
possible that no data remains immediately, indicating that ACK were
already received. As such, buffer instance is immediately removed by
qc_stream_buf_free().

This must be backported up to 2.6. As this code section is known to
regression, a period of observation could be reserved before
distributing it on LTS releases.

diff --git a/include/haproxy/quic_stream.h b/include/haproxy/quic_stream.h
index 57f8c455f09604e726c231563b36ea7cb0271c98..44897287b93c0fbfccfdf1f545afa40e8cf379e7 100644 (file)
--- a/include/haproxy/quic_stream.h
+++ b/include/haproxy/quic_stream.h
@@ -10,7 +10,7 @@ struct quic_conn;
 
 struct qc_stream_desc *qc_stream_desc_new(uint64_t id, enum qcs_type, void *ctx,
                                           struct quic_conn *qc);
-void qc_stream_desc_release(struct qc_stream_desc *stream);
+void qc_stream_desc_release(struct qc_stream_desc *stream, uint64_t final_size);
 int qc_stream_desc_ack(struct qc_stream_desc **stream, size_t offset, size_t len);
 void qc_stream_desc_free(struct qc_stream_desc *stream, int closing);
 

diff --git a/src/mux_quic.c b/src/mux_quic.c
index a6af629a8d3a66e74a5d470f8564994dae8c53cd..55c395a09de8391f24484c7bee3fd1bec3837223 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -68,7 +68,7 @@ static void qcs_free(struct qcs *qcs)
                qcc->app_ops->detach(qcs);
 
        /* Release qc_stream_desc buffer from quic-conn layer. */
-       qc_stream_desc_release(qcs->stream);
+       qc_stream_desc_release(qcs->stream, qcs->tx.sent_offset);
 
        /* Free Rx/Tx buffers. */
        qcs_free_ncbuf(qcs, &qcs->rx.ncbuf);

diff --git a/src/quic_stream.c b/src/quic_stream.c
index 727636b06a1465b81e84bd04474cc96f5db42b8c..42452c2c0c7e60ab5109b10252127667027c4e49 100644 (file)
--- a/src/quic_stream.c
+++ b/src/quic_stream.c
@@ -85,8 +85,13 @@ struct qc_stream_desc *qc_stream_desc_new(uint64_t id, enum qcs_type type, void
 /* Mark the stream descriptor <stream> as released. It will be freed as soon as
  * all its buffered data are acknowledged. Does nothing if <stream> is already
  * NULL.
+ *
+ * <final_size> corresponds to the last offset sent for this stream. If there
+ * is unsent data present, they will be remove first to guarantee that buffer
+ * is freed after receiving all acknowledges.
  */
-void qc_stream_desc_release(struct qc_stream_desc *stream)
+void qc_stream_desc_release(struct qc_stream_desc *stream,
+                            uint64_t final_size)
 {
        if (!stream)
                return;
@@ -97,14 +102,31 @@ void qc_stream_desc_release(struct qc_stream_desc *stream)
        stream->release = 1;
        stream->ctx = NULL;
 
+       if (stream->buf) {
+               struct qc_stream_buf *stream_buf = stream->buf;
+               struct buffer *buf = &stream_buf->buf;
+               const uint64_t tail_offset =
+                 MAX(stream->buf_offset, stream->ack_offset) + b_data(buf);
+
+               /* final_size cannot be greater than all currently stored data. */
+               BUG_ON(final_size > tail_offset);
+
+               /* Remove unsent data from current buffer. */
+               if (final_size < tail_offset) {
+                       b_sub(buf, tail_offset - final_size);
+                       /* Remove buffer is all ACK already received. */
+                       if (!b_data(buf))
+                               qc_stream_buf_free(stream, &stream_buf);
+               }
+
+               /* A released stream does not use <stream.buf>. */
+               stream->buf = NULL;
+       }
+
        if (LIST_ISEMPTY(&stream->buf_list)) {
                /* if no buffer left we can free the stream. */
                qc_stream_desc_free(stream, 0);
        }
-       else {
-               /* A released stream does not use <stream.buf>. */
-               stream->buf = NULL;
-       }
 }
 
 /* Acknowledge data at <offset> of length <len> for <stream>. It is handled

diff --git a/src/quic_tls.c b/src/quic_tls.c
index 7e18a39264f2244450786f5cd3db70ed478c26c2..581d615ce95b28215aeee73540635459f08fa1a4 100644 (file)
--- a/src/quic_tls.c
+++ b/src/quic_tls.c
@@ -125,7 +125,7 @@ void quic_cstream_free(struct quic_cstream *cs)
 
        quic_free_ncbuf(&cs->rx.ncbuf);
 
-       qc_stream_desc_release(cs->desc);
+       qc_stream_desc_release(cs->desc, 0);
        pool_free(pool_head_quic_cstream, cs);
 }