BUG/MEDIUM: mux-quic: fix crash on invalid fctl frame dereference

Emission of flow-control frames have been recently modified. Now, each
frame is sent one by one, via a single entry list. If a failure occurs,
emission is interrupted and frame is reinserted into the original
<qcc.lfctl.frms> list.

This code is incorrect as it only checks if qcc_send_frames() returns an
error code to perform the reinsert operation. However, an error here
does not always mean that the frame was not properly emitted by lower
quic-conn layer. As such, an extra test LIST_ISEMPTY() must be performed
prior to reinsert the frame.

This bug would cause a heap overflow. Indeed, the reinsert frame would
be a random value. A crash would occur as soon as it would be
dereferenced via <qcc.lfctl.frms> list.

This was reproduced by issuing a POST with a big file and interrupt it
after just a few seconds. This results in a crash in about a third of
the tests. Here is an example command using ngtcp2 :

 $ ngtcp2-client -q --no-quic-dump --no-http-dump \
   -m POST -d ~/infra/html/1g 127.0.0.1 20443 "http://127.0.0.1:20443/post"

Heap overflow was detected via a BUG_ON() statement from qc_frm_free()
via qcc_release() caller :

  FATAL: bug condition "!((&((*frm)->reflist))->n == (&((*frm)->reflist)))" matched at src/quic_frame.c:1270

This does not need to be backported.

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 68b913970b40154169a76659e75a0a9509c2a7fe..98438a7a1b2f353b62158e03e338cfa301bddd34 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -2616,9 +2616,11 @@ static int qcc_emit_fctl(struct qcc *qcc)
                LIST_APPEND(&single_frm, &frm->list);
 
                if (qcc_send_frames(qcc, &single_frm, 0)) {
-                       /* replace frame that cannot be sent in qcc list. */
-                       LIST_DELETE(&frm->list);
-                       LIST_INSERT(&qcc->lfctl.frms, &frm->list);
+                       if (!LIST_ISEMPTY(&single_frm)) {
+                               /* replace frame that cannot be sent in qcc list. */
+                               LIST_DELETE(&frm->list);
+                               LIST_INSERT(&qcc->lfctl.frms, &frm->list);
+                       }
                        goto err;
                }
 
@@ -2626,7 +2628,7 @@ static int qcc_emit_fctl(struct qcc *qcc)
 
                /* If frame is MAX_STREAM_DATA, reset corresponding QCS msd_frm pointer. */
                if (id >= 0) {
-                       node = eb64_lookup(&qcc->streams_by_id, frm->max_stream_data.id);
+                       node = eb64_lookup(&qcc->streams_by_id, id);
                        if (node) {
                                qcs = eb64_entry(node, struct qcs, by_id);
                                qcs->tx.msd_frm = NULL;