From 15337fd8085288ac10de66bb048c8d655fbb0f25 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 20 Dec 2022 14:47:16 +0100 Subject: [PATCH] BUG/MEDIUM: mux-quic: fix double delete from qcc.opening_list qcs instances for bidirectional streams are inserted in . 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 . 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. --- include/haproxy/mux_quic.h | 2 +- src/mux_quic.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/haproxy/mux_quic.h b/include/haproxy/mux_quic.h index 14de1e061..908a02fba 100644 --- 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 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 0c716bb26..8e48493d0 100644 --- 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));