BUG/MEDIUM: quic: properly handle duplicated STREAM frames

When a STREAM frame is re-emitted, it will point to the same stream
buffer as the original one. If an ACK is received for either one of
these frame, the underlying buffer may be freed. Thus, if the second
frame is declared as lost and schedule for retransmission, we must
ensure that the underlying buffer is still allocated or interrupt the
retransmission.

Stream buffer is stored as an eb_tree indexed by the stream ID. To avoid
to lookup over a tree each time a STREAM frame is re-emitted, a lost
STREAM frame is flagged as QUIC_FL_TX_FRAME_LOST.

In most cases, this code is functional. However, there is several
potential issues which may cause a segfault :
- when explicitely probing with a STREAM frame, the frame won't be
  flagged as lost
- when splitting a STREAM frame during retransmission, the flag is not
  copied

To fix both these cases, QUIC_FL_TX_FRAME_LOST flag has been converted
to a <dup> field in quic_stream structure. This field is now properly
copied when splitting a STREAM frame. Also, as this is now an inner
quic_frame field, it will be copied automatically on qc_frm_dup()
invocation thus ensuring that it will be set on probing.

This issue was encounted randomly with the following backtrace :
 #0  __memmove_avx512_unaligned_erms ()
 #1  0x000055f4d5a48c01 in memcpy (__len=18446698486215405173, __src=<optimized out>,
 #2  quic_build_stream_frame (buf=0x7f6ac3fcb400, end=<optimized out>, frm=0x7f6a00556620,
 #3  0x000055f4d5a4a147 in qc_build_frm (buf=buf@entry=0x7f6ac3fcb5d8,
 #4  0x000055f4d5a23300 in qc_do_build_pkt (pos=<optimized out>, end=<optimized out>,
 #5  0x000055f4d5a25976 in qc_build_pkt (pos=0x7f6ac3fcba10,
 #6  0x000055f4d5a30c7e in qc_prep_app_pkts (frms=0x7f6a0032bc50, buf=0x7f6a0032bf30,
 #7  qc_send_app_pkts (qc=0x7f6a0032b310, frms=0x7f6a0032bc50) at src/quic_conn.c:4184
 #8  0x000055f4d5a35f42 in quic_conn_app_io_cb (t=0x7f6a0009c660, context=0x7f6a0032b310,

This should fix github issue #2051.

This should be backported up to 2.6.

diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h
index 3cd11710ad6c5eadf59513e804eea42e74a6b1ad..58feb444b8f2f4cca8822a8440c87c1a3bad8b54 100644 (file)
--- a/include/haproxy/quic_frame-t.h
+++ b/include/haproxy/quic_frame-t.h
@@ -99,8 +99,6 @@ enum quic_frame_type {
 
 /* Flag a TX frame as acknowledged */
 #define QUIC_FL_TX_FRAME_ACKED             0x01
-/* Flag a TX frame as lost */
-#define QUIC_FL_TX_FRAME_LOST              0x02
 
 #define QUIC_STREAM_FRAME_TYPE_FIN_BIT     0x01
 #define QUIC_STREAM_FRAME_TYPE_LEN_BIT     0x02
@@ -175,6 +173,8 @@ struct quic_stream {
         * for RX pointer into the packet buffer.
         */
        const unsigned char *data;
+
+       char dup; /* set for duplicated frame : this forces to check for the underlying qc_stream_buf instance before emitting it. */
 };
 
 struct quic_max_data {

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 4639e67a0ae4d6abf6c2f769455aafdc0ec2171f..ad41c87cf9633da6b819821308f275b873d19872 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1885,12 +1885,6 @@ static inline int qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                                close = 1;
                        }
 
-                       if (QUIC_FT_STREAM_8 <= frm->type && frm->type <= QUIC_FT_STREAM_F) {
-                               /* Mark this STREAM frame as lost. A look up their stream descriptor
-                                * will be performed to check the stream is not consumed or released.
-                                */
-                               frm->flags |= QUIC_FL_TX_FRAME_LOST;
-                       }
                        LIST_APPEND(&tmp, &frm->list);
                        TRACE_DEVEL("frame requeued", QUIC_EV_CONN_PRSAFRM, qc, frm);
                }
@@ -2513,6 +2507,8 @@ static void qc_dup_pkt_frms(struct quic_conn *qc,
                                TRACE_DEVEL("updated partially acked frame",
                                            QUIC_EV_CONN_PRSAFRM, qc, frm);
                        }
+
+                       strm_frm->dup = 1;
                        break;
                }
 
@@ -6987,7 +6983,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                        break;
 
                case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F:
-                       if (cf->flags & QUIC_FL_TX_FRAME_LOST) {
+                       if (cf->stream.dup) {
                                struct eb64_node *node = NULL;
                                struct qc_stream_desc *stream_desc = NULL;
                                struct quic_stream *strm = &cf->stream;
@@ -7094,6 +7090,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                                /* FIN bit reset */
                                new_cf->type &= ~QUIC_STREAM_FRAME_TYPE_FIN_BIT;
                                new_cf->stream.data = cf->stream.data;
+                               new_cf->stream.dup = cf->stream.dup;
                                TRACE_DEVEL("split frame", QUIC_EV_CONN_PRSAFRM, qc, new_cf);
                                if (cf->origin) {
                                        TRACE_DEVEL("duplicated frame", QUIC_EV_CONN_PRSAFRM, qc);