BUG/MAJOR: mux-quic: prevent crash on QCS already set for purging

QCC connection has a purge list member. It contains QCS instances
considered as completed, whiches will be freed during the next tasklet
run. Such QCS are reported via qcs_is_completed() helper function.

QCS member <el_send> is used for purge list storage. It is also used for
send list, however an inactive QCS should not be present in the latter.
To enforce this rule, a BUG_ON() is present before purge list insertion
in qcs_close_remote(). This would also be triggered if QCS was already
inserted in the purge list before remote closing.

This BUG_ON() may be triggered when a QCS is scheduled for purge while
it still contains stom data to read. When the final read is performed,
crash will occur in qcs_close_remote(). This happens with detached QCS
only, as they are considered as completed even if only closed locally.

To prevent this situation, add an early return in qcs_close_remote() :
if QCS is already considered completed, do not change its state to fully
closed. As stated above, this only occur for detached QCS instances. The
main objective is to prevent the BUG_ON() crash while still preventing
double insert. As an added value, it guarantees that QCS state remains
constant once scheduled for purge.

Thanks to Brendon C. <bren@bren.win> for his detailled report on the
issue.

This must be backported up to 3.2, after a period of observation.

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 0e11fc7cf81d2d60bd9135109c31c6dab31a3216..19fb5c0cc7a041a20410f28267e952c19df57511 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -504,6 +504,19 @@ static void qcs_close_remote(struct qcs *qcs)
        /* This operation cannot be used multiple times. */
        BUG_ON_HOT(qcs->st == QC_SS_HREM || qcs->st == QC_SS_CLO);
 
+       /* Ignore remote closure if QCS is already completed and thus already
+        * scheduled for purging. This is possible as a detached QCS may be
+        * considered as terminated even if only locally closed. This is
+        * mandatory to avoid a double insert in purge list. This approach also
+        * preserves QCS state on completion for better diagnostics.
+        */
+       if (qcs_is_completed(qcs)) {
+               TRACE_STATE("ignore close remote on completed stream", QMUX_EV_QCS_RECV, qcs->qcc->conn, qcs);
+               COUNT_IF(!(qcs->flags & QC_SF_DETACH), "QCS reported as completed prior remote closure but not detached.");
+               COUNT_IF(!LIST_INLIST(&qcs->el_send),  "QCS reported as completed not in purge list");
+               return;
+       }
+
        if (quic_stream_is_bidi(qcs->id)) {
                qcs->st = (qcs->st == QC_SS_HLOC) ? QC_SS_CLO : QC_SS_HREM;
        }