BUG/MEDIUM: mux-quic: fix double delete from qcc.opening_list

qcs instances for bidirectional streams are inserted in
<qcc.opening_list>. It is removed from the list once a full HTTP request
has been parsed. This is required to implement http-request timeout.

In case a stream is deleted before receiving full HTTP request, it also
must be removed from <qcc.opening_list>. This was not the case on first
implementation but has been fixed by the following patch :
  641a65ff3cccd394eed49378c6ccdb8ba0a101a7
  BUG/MINOR: mux-quic: remove qcs from opening-list on free

This means that now a stream can be deleted from the list in two
different functions. Sadly, as LIST_DELETE was used in both cases,
nothing prevented a double-deletion from the list, even though
LIST_INLIST was used. Both calls are replaced with LIST_DEL_INIT which
is idempotent.

This bug causes memory corruption which results in most cases in a
segfault, most of times outside of mux-quic code itself. It has been
found first by gabrieltz who reported it on the github issue #1903. Big
thanks to him for his testing.

This bug also causes failures on several 'M' transfer testcase of QUIC
interop-runner. The s2n-quic client is particularly useful in this case
as segfaults triggers were most of the times on the LIST_DELETE
operation itself. This is probably due to its encapsulating of HEADERS
frame with fin bit delayed in a following empty STREAM frame.

This must be backported wherever the above patch is, up to 2.6.

diff --git a/include/haproxy/mux_quic.h b/include/haproxy/mux_quic.h
index 14de1e0618c812864d975d28d981e6de9802a30a..908a02fbabe6b9cba2889fa7e8afb3af79a9eeff 100644 (file)
--- a/include/haproxy/mux_quic.h
+++ b/include/haproxy/mux_quic.h
@@ -120,7 +120,7 @@ static inline struct stconn *qc_attach_sc(struct qcs *qcs, struct buffer *buf)
         * it to sedesc. See <qcs_wait_http_req> for more info.
         */
        BUG_ON_HOT(!LIST_INLIST(&qcs->el_opening));
-       LIST_DELETE(&qcs->el_opening);
+       LIST_DEL_INIT(&qcs->el_opening);
 
        return qcs->sd->sc;
 }

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 0c716bb26f5803af1ad704fd0b2aeaca6c826b68..8e48493d04cd4f934ea8b126ca7cda5b5786c80b 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -59,8 +59,8 @@ static void qcs_free(struct qcs *qcs)
 
        TRACE_ENTER(QMUX_EV_QCS_END, qcc->conn, qcs);
 
-       if (LIST_INLIST(&qcs->el_opening))
-               LIST_DELETE(&qcs->el_opening);
+       /* Safe to use even if already removed from the list. */
+       LIST_DEL_INIT(&qcs->el_opening);
 
        /* Release stream endpoint descriptor. */
        BUG_ON(qcs->sd && !se_fl_test(qcs->sd, SE_FL_ORPHAN));