BUG/MINOR: quic: fix ACK_ECN parsing after optional ack ranges

QUIC acknowledgements are conveyed in ACK and ACK_ECN frame types. The
latter shares a similar structure, but also contains 3 varints at the
end related to explicit congestion notification.

The following patch differentiates ACK and ACK_ECN to ensure the latter
is correctly parsed.

  7aa839296de9e6d330c6a952ae2197aa49db1c66
  BUG/MEDIUM: quic: fix ACK ECN frame parsing

However, ACK_ECN frame parsing is incorrect if the optional ack range
field is present. This patch fixes it by restoring a common parsing
function for ACK and ACK_ECN, which extracts only the first fields prior
to the optional ack ranges. ECN fields are now parsed later, at the end
of qc_parse_ack_frm(), after the optional ack ranges.

This must be backported up to 2.6.

diff --git a/src/quic_frame.c b/src/quic_frame.c
index 35c8c23eb532ea1088ea47b7f5b7427d15ba0b66..5318698a8d8e8e0c2d19b673d646cbd31b74fe4e 100644 (file)
--- a/src/quic_frame.c
+++ b/src/quic_frame.c
@@ -356,25 +356,6 @@ static int quic_build_ack_ecn_frame(unsigned char **pos, const unsigned char *en
                quic_enc_int(pos, end, ack_frm->ack_range_num);
 }
 
-/* Parse an ACK_ECN frame at <pos> buffer position with <end> as end into <frm> frame.
- * Return 1 if succeeded (enough at <pos> buffer position to parse this frame), 0 if not.
- */
-static int quic_parse_ack_ecn_frame(struct quic_frame *frm, struct quic_conn *qc,
-                                    const unsigned char **pos, const unsigned char *end)
-{
-       struct qf_ack *ack_frm = &frm->ack;
-       /* TODO implement ECN advertising */
-       uint64_t ect0, ect1, ecn_ce;
-
-       return quic_dec_int(&ack_frm->largest_ack, pos, end) &&
-              quic_dec_int(&ack_frm->ack_delay, pos, end) &&
-              quic_dec_int(&ack_frm->ack_range_num, pos, end) &&
-              quic_dec_int(&ack_frm->first_ack_range, pos, end) &&
-              quic_dec_int(&ect0, pos, end) &&
-              quic_dec_int(&ect1, pos, end) &&
-              quic_dec_int(&ecn_ce, pos, end);
-}
-
 /* Encode a RESET_STREAM frame at <pos> buffer position.
  * Returns 1 if succeeded (enough room at <pos> buffer position to encode the frame), 0 if not.
  */
@@ -1143,7 +1124,7 @@ const struct quic_frame_parser quic_frame_parsers[] = {
        [QUIC_FT_PADDING]              = { .func = quic_parse_padding_frame,              .flags = 0,                               .mask = QUIC_FT_PKT_TYPE_IH01_BITMASK, },
        [QUIC_FT_PING]                 = { .func = quic_parse_ping_frame,                 .flags = QUIC_FL_RX_PACKET_ACK_ELICITING, .mask = QUIC_FT_PKT_TYPE_IH01_BITMASK, },
        [QUIC_FT_ACK]                  = { .func = quic_parse_ack_frame_header,           .flags = 0,                               .mask = QUIC_FT_PKT_TYPE_IH_1_BITMASK, },
-       [QUIC_FT_ACK_ECN]              = { .func = quic_parse_ack_ecn_frame,              .flags = 0,                               .mask = QUIC_FT_PKT_TYPE_IH_1_BITMASK, },
+       [QUIC_FT_ACK_ECN]              = { .func = quic_parse_ack_frame_header,           .flags = 0,                               .mask = QUIC_FT_PKT_TYPE_IH_1_BITMASK, },
        [QUIC_FT_RESET_STREAM]         = { .func = quic_parse_reset_stream_frame,         .flags = QUIC_FL_RX_PACKET_ACK_ELICITING, .mask = QUIC_FT_PKT_TYPE___01_BITMASK, },
        [QUIC_FT_STOP_SENDING]         = { .func = quic_parse_stop_sending_frame,         .flags = QUIC_FL_RX_PACKET_ACK_ELICITING, .mask = QUIC_FT_PKT_TYPE___01_BITMASK, },
        [QUIC_FT_CRYPTO]               = { .func = quic_parse_crypto_frame,               .flags = QUIC_FL_RX_PACKET_ACK_ELICITING, .mask = QUIC_FT_PKT_TYPE_IH_1_BITMASK, },

diff --git a/src/quic_rx.c b/src/quic_rx.c
index e295ab9284f2403a4c82daac6a480fb7074582ff..a6472a67140b015d27f0e6b03add6e5541a81ded 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -526,6 +526,7 @@ static int qc_parse_ack_frm(struct quic_conn *qc,
        unsigned int time_sent, pkt_flags;
        struct list newly_acked_pkts = LIST_HEAD_INIT(newly_acked_pkts);
        struct list lost_pkts = LIST_HEAD_INIT(lost_pkts);
+       uint64_t ect0, ect1, ecn_ce;
        int ret = 0, new_largest_acked_pn = 0;
        struct quic_tx_packet *pkt, *tmp;
 
@@ -605,6 +606,16 @@ static int qc_parse_ack_frm(struct quic_conn *qc,
                            qc, NULL, &largest, &smallest);
        } while (1);
 
+       /* Decode ECN counts if ACK frame type contains them.
+        * TODO use these values to adjust the sending window
+        */
+       if (frm->type == QUIC_FT_ACK_ECN &&
+           (!quic_dec_int(&ect0, pos, end) || !quic_dec_int(&ect1, pos, end) ||
+            !quic_dec_int(&ecn_ce, pos, end))) {
+               TRACE_ERROR("error on parsing ACK ECN fields", QUIC_EV_CONN_PRSAFRM, qc);
+               goto err;
+       }
+
        if (!LIST_ISEMPTY(&newly_acked_pkts)) {
                unsigned int bytes_lost = 0;