Release 2.19.5

- [BUGFIX] Generate frame record when moving an ACK from one buffered
  packet to another.

CHANGELOG

@@ -1,3 +1,8 @@
+2020-08-11
+    - 2.19.5
+    - [BUGFIX] Generate frame record when moving an ACK from one buffered
+      packet to another.
+
 2020-08-06
     - 2.19.4
     - [BUGFIX] Do not return an oversize MTU probe to connection twice.

docs/conf.py

@@ -26,7 +26,7 @@ author = u'LiteSpeed Technologies'
 # The short X.Y version
 version = u'2.19'
 # The full version, including alpha/beta/rc tags
-release = u'2.19.4'
+release = u'2.19.5'
 
 
 # -- General configuration ---------------------------------------------------

include/lsquic.h

@@ -25,7 +25,7 @@ extern "C" {
 
 #define LSQUIC_MAJOR_VERSION 2
 #define LSQUIC_MINOR_VERSION 19
-#define LSQUIC_PATCH_VERSION 4
+#define LSQUIC_PATCH_VERSION 5
 
 /**
  * Engine flags:

src/liblsquic/lsquic_packet_out.c

@@ -388,6 +388,7 @@ lsquic_packet_out_chop_regen (lsquic_packet_out_t *packet_out)
     assert(adj);    /* Otherwise why are we called? */
     assert(packet_out->po_regen_sz == adj);
     packet_out->po_regen_sz = 0;
+    packet_out->po_frame_types &= ~GQUIC_FRAME_REGEN_MASK;
 }
 
 

src/liblsquic/lsquic_send_ctl.c

@@ -2507,21 +2507,39 @@ send_ctl_max_bpq_count (const lsquic_send_ctl_t *ctl,
 }
 
 
-static void
+/* If error is returned, `src' is not modified */
+static int
 send_ctl_move_ack (struct lsquic_send_ctl *ctl, struct lsquic_packet_out *dst,
                     struct lsquic_packet_out *src)
 {
+    struct packet_out_frec_iter pofi;
+    const struct frame_rec *frec;
     assert(dst->po_data_sz == 0);
 
-    if (lsquic_packet_out_avail(dst) >= src->po_regen_sz)
+    /* This checks that we only ever expect to move an ACK frame from one
+     * buffered packet to another.  We don't generate any other regen frame
+     * types in buffered packets.
+     */
+    assert(!(GQUIC_FRAME_REGEN_MASK & (1 << src->po_frame_types)
+                                                        & ~QUIC_FTBIT_ACK));
+
+    if (lsquic_packet_out_avail(dst) >= src->po_regen_sz
+                && (frec = lsquic_pofi_first(&pofi, src), frec != NULL)
+                    && frec->fe_frame_type == QUIC_FRAME_ACK)
     {
         memcpy(dst->po_data, src->po_data, src->po_regen_sz);
+        if (0 != lsquic_packet_out_add_frame(dst, &ctl->sc_enpub->enp_mm,
+                    frec->fe_frame_type, QUIC_FRAME_ACK, dst->po_data_sz,
+                    src->po_regen_sz))
+            return -1;
         dst->po_data_sz = src->po_regen_sz;
         dst->po_regen_sz = src->po_regen_sz;
         dst->po_frame_types |= (GQUIC_FRAME_REGEN_MASK & src->po_frame_types);
         src->po_frame_types &= ~GQUIC_FRAME_REGEN_MASK;
         lsquic_packet_out_chop_regen(src);
     }
+
+    return 0;
 }
 
 
@@ -2589,8 +2607,14 @@ send_ctl_get_buffered_packet (lsquic_send_ctl_t *ctl,
     switch (ack_action)
     {
     case AA_STEAL:
-        send_ctl_move_ack(ctl, packet_out,
+        if (0 != send_ctl_move_ack(ctl, packet_out,
-            TAILQ_FIRST(&ctl->sc_buffered_packets[BPT_OTHER_PRIO].bpq_packets));
+            TAILQ_FIRST(&ctl->sc_buffered_packets[BPT_OTHER_PRIO].bpq_packets)))
+        {
+            LSQ_INFO("cannot move ack");
+            lsquic_packet_out_destroy(packet_out, ctl->sc_enpub,
+                                            packet_out->po_path->np_peer_ctx);
+            return NULL;
+        }
         break;
     case AA_GENERATE:
         lconn->cn_if->ci_write_ack(lconn, packet_out);

tests/test_stream.c

@@ -60,6 +60,7 @@ struct test_ctl_settings
     int     tcs_schedule_stream_packets_immediately;
     int     tcs_have_delayed_packets;
     int     tcs_can_send;
+    int     tcs_write_ack;
     enum buf_packet_type
             tcs_bp_type;
     enum packno_bits
@@ -82,6 +83,7 @@ init_test_ctl_settings (struct test_ctl_settings *settings)
     settings->tcs_schedule_stream_packets_immediately      = 1;
     settings->tcs_have_delayed_packets = 0;
     settings->tcs_can_send             = 1;
+    settings->tcs_write_ack            = 0;
     settings->tcs_bp_type              = BPT_HIGHEST_PRIO;
     settings->tcs_guess_packno_bits    = GQUIC_PACKNO_LEN_2;
     settings->tcs_calc_packno_bits     = GQUIC_PACKNO_LEN_2;
@@ -278,8 +280,8 @@ static struct test_ctx test_ctx;
 
 
 struct test_objs {
-    struct lsquic_engine_public eng_pub;
     struct lsquic_conn        lconn;
+    struct lsquic_engine_public eng_pub;
     struct lsquic_conn_public conn_pub;
     struct lsquic_send_ctl    send_ctl;
     struct lsquic_alarmset    alset;
@@ -293,13 +295,6 @@ struct test_objs {
 };
 
 
-static int
-unit_test_doesnt_write_ack (struct lsquic_conn *lconn)
-{
-    return 0;
-}
-
-
 static struct network_path network_path;
 
 static struct network_path *
@@ -309,10 +304,35 @@ get_network_path (struct lsquic_conn *lconn, const struct sockaddr *sa)
 }
 
 
+static int
+can_write_ack (struct lsquic_conn *lconn)
+{
+    return g_ctl_settings.tcs_write_ack;
+}
+
+
+static void
+write_ack (struct lsquic_conn *lconn, struct lsquic_packet_out *packet_out)
+{
+    struct test_objs *const tobjs = (void *) lconn;
+    const size_t ack_size = 9;  /* Arbitrary */
+    int s;
+
+    packet_out->po_frame_types |= 1 << QUIC_FRAME_ACK;
+    s = lsquic_packet_out_add_frame(packet_out, &tobjs->eng_pub.enp_mm, 0,
+                            QUIC_FRAME_ACK, packet_out->po_data_sz, ack_size);
+    assert(s == 0);
+    memcpy(packet_out->po_data + packet_out->po_data_sz, "ACKACKACK", 9);
+    lsquic_send_ctl_incr_pack_sz(&tobjs->send_ctl, packet_out, ack_size);
+    packet_out->po_regen_sz += ack_size;
+}
+
+
 static const struct conn_iface our_conn_if =
 {
-    .ci_can_write_ack = unit_test_doesnt_write_ack,
+    .ci_can_write_ack = can_write_ack,
     .ci_get_path      = get_network_path,
+    .ci_write_ack     = write_ack,
 };
 
 
@@ -2227,6 +2247,97 @@ test_writing_to_stream_outside_callback (void)
 }
 
 
+static void
+verify_ack (struct lsquic_packet_out *packet_out)
+{
+    struct packet_out_frec_iter pofi;
+    const struct frame_rec *frec;
+    unsigned short regen_sz;
+    enum quic_ft_bit frame_types;
+
+    assert(packet_out->po_regen_sz > 0);
+    assert(packet_out->po_frame_types & (1 << QUIC_FRAME_ACK));
+
+    regen_sz = 0;
+    frame_types = 0;
+    for (frec = lsquic_pofi_first(&pofi, packet_out); frec;
+                                                frec = lsquic_pofi_next(&pofi))
+    {
+        frame_types |= 1 << frec->fe_frame_type;
+        if (frec->fe_frame_type == QUIC_FRAME_ACK)
+        {
+            assert(frec->fe_len == 9);
+            assert(0 == memcmp(packet_out->po_data + frec->fe_off, "ACKACKACK", 9));
+            assert(regen_sz == frec->fe_off);
+            regen_sz += frec->fe_len;
+        }
+    }
+
+    assert(frame_types & (1 << QUIC_FRAME_ACK));
+    assert(regen_sz == packet_out->po_regen_sz);
+}
+
+
+/* Write to buffered streams: first to low-priority, then high-priority.  This
+ * should trigger ACK generation and move.
+ */
+static void
+test_stealing_ack (void)
+{
+    ssize_t nw;
+    struct test_objs tobjs;
+    struct lsquic_conn *const lconn = &tobjs.lconn;
+    struct lsquic_stream *lo_stream, *hi_stream;;
+    int s;
+    const struct buf_packet_q *bpq;
+
+    init_test_ctl_settings(&g_ctl_settings);
+    g_ctl_settings.tcs_schedule_stream_packets_immediately = 0;
+    g_ctl_settings.tcs_write_ack = 1;
+    g_ctl_settings.tcs_bp_type = BPT_OTHER_PRIO;
+
+    init_test_objs(&tobjs, 0x4000, 0x4000, NULL);
+
+    lo_stream = new_stream(&tobjs, 123);
+    assert(("Stream initialized", lo_stream));
+    assert(LSQUIC_STREAM_DEFAULT_PRIO == lsquic_stream_priority(lo_stream));
+    assert(lconn == lsquic_stream_conn(lo_stream));
+    nw = lsquic_stream_write(lo_stream, "Dude, where is", 14);
+    assert(("14 bytes written correctly", nw == 14));
+    s = lsquic_stream_flush(lo_stream);
+    assert(0 == s);
+
+    bpq = &tobjs.send_ctl.sc_buffered_packets[g_ctl_settings.tcs_bp_type];
+    verify_ack(TAILQ_FIRST(&bpq->bpq_packets));
+
+    g_ctl_settings.tcs_bp_type = BPT_HIGHEST_PRIO;
+
+    hi_stream = new_stream(&tobjs, 1);
+    assert(("Stream initialized", hi_stream));
+    assert(lconn == lsquic_stream_conn(hi_stream));
+    nw = lsquic_stream_write(hi_stream, "DATA", 4);
+    assert(("4 bytes written correctly", nw == 4));
+    s = lsquic_stream_flush(hi_stream);
+    assert(0 == s);
+
+    /* ACK is moved (stolen) from low-priority stream to high-priority stream */
+    /* Check old packet */
+    assert(!(TAILQ_FIRST(&bpq->bpq_packets)->po_frame_types & (1 << QUIC_FRAME_ACK)));
+    /* Check new packet */
+    bpq = &tobjs.send_ctl.sc_buffered_packets[g_ctl_settings.tcs_bp_type];
+    verify_ack(TAILQ_FIRST(&bpq->bpq_packets));
+
+    /* And now chop regen, see if we hit any asserts there */
+    lsquic_packet_out_chop_regen(TAILQ_FIRST(&bpq->bpq_packets));
+    /* And now verify that ACK is gone */
+    assert(!(TAILQ_FIRST(&bpq->bpq_packets)->po_frame_types & (1 << QUIC_FRAME_ACK)));
+
+    lsquic_stream_destroy(lo_stream);
+    lsquic_stream_destroy(hi_stream);
+    deinit_test_objs(&tobjs);
+}
+
+
 static void
 test_changing_pack_size (void)
 {
@@ -3251,6 +3362,7 @@ main (int argc, char **argv)
 
     test_writing_to_stream_schedule_stream_packets_immediately();
     test_writing_to_stream_outside_callback();
+    test_stealing_ack();
     test_changing_pack_size();
     test_window_update1();
     test_window_update2();