Latest changes

- [BUGFIX] Frame insertion mis-ID as overlap instead of dup
- http_client: fix priority range generated by -E flag

CHANGELOG

@@ -1,3 +1,8 @@
+2018-05-16
+
+    - [BUGFIX] Frame insertion mis-ID as overlap instead of dup
+    - http_client: fix priority range generated by -E flag
+
 2018-05-09
 
     - [FEATURE] Add support for Q043.

src/liblsquic/lsquic_crt_compress.c

@@ -569,5 +569,3 @@ lsquic_crt_cleanup (void)
         s_ccsbuf = NULL;
     }
 }
-
-

src/liblsquic/lsquic_data_in_if.h

@@ -6,13 +6,11 @@
 #ifndef LSQUIC_DATA_IN_IF_H
 #define LSQUIC_DATA_IN_IF_H 1
 
-
 struct data_frame;
 struct data_in;
 struct lsquic_conn_public;
 struct stream_frame;
 
-
 enum ins_frame
 {
     INS_FRAME_OK,
@@ -21,7 +19,6 @@ enum ins_frame
     INS_FRAME_OVERLAP,
 };
 
-
 struct data_in_iface
 {
     void
@@ -57,8 +54,8 @@ struct data_in_iface
 
     size_t
     (*di_mem_used) (struct data_in *);
-};
 
+};
 
 struct data_in
 {
@@ -73,7 +70,6 @@ struct data_in
     }                            di_flags;
 };
 
-
 /* This implementation does not support overlapping frame and may return
  * INS_FRAME_OVERLAP.
  */

src/liblsquic/lsquic_di_hash.c

@@ -12,6 +12,7 @@
 
 
 #include <assert.h>
+#include <inttypes.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -454,6 +455,8 @@ ctz (unsigned long long x)
     if (0 == (x & ((1ULL <<  1) - 1))) { n +=  1; x >>=  1; }
     return n;
 }
+
+
 #endif
 
 

src/liblsquic/lsquic_di_nocopy.c

@@ -102,10 +102,15 @@ struct nocopy_data_in
     struct data_in              ncdi_data_in;
     struct lsquic_conn_public  *ncdi_conn_pub;
     uint64_t                    ncdi_byteage;
+    uint64_t                    ncdi_fin_off;
     uint32_t                    ncdi_stream_id;
     unsigned                    ncdi_n_frames;
     unsigned                    ncdi_n_holes;
     unsigned                    ncdi_cons_far;
+    enum {
+        NCDI_FIN_SET        = 1 << 0,
+        NCDI_FIN_REACHED    = 1 << 1,
+    }                           ncdi_flags;
 };
 
 
@@ -137,6 +142,9 @@ data_in_nocopy_new (struct lsquic_conn_public *conn_pub, uint32_t stream_id)
     ncdi->ncdi_n_frames         = 0;
     ncdi->ncdi_n_holes          = 0;
     ncdi->ncdi_cons_far         = 0;
+    ncdi->ncdi_fin_off          = 0;
+    ncdi->ncdi_flags            = 0;
+    LSQ_DEBUG("initialized");
     return &ncdi->ncdi_data_in;
 }
 
@@ -155,6 +163,7 @@ nocopy_di_destroy (struct data_in *data_in)
     free(ncdi);
 }
 
+
 #define DF_OFF(frame) (frame)->data_frame.df_offset
 #define DF_FIN(frame) (frame)->data_frame.df_fin
 #define DF_SIZE(frame) (frame)->data_frame.df_size
@@ -177,61 +186,17 @@ frame_list_is_sane (const struct nocopy_data_in *ncdi)
     }
     return ordered && !overlaps;
 }
+
+
 #define CHECK_ORDER(ncdi) assert(frame_list_is_sane(ncdi))
 #else
 #define CHECK_ORDER(ncdi)
 #endif
 
 
-/* When inserting a new frame into the frame list, there are four cases to
+#define CASE(letter) ((int) (letter) << 8)
- * consider:
- *
- *  I.   New frame is the only frame in the list;
- *  II.  New frame only has a neighbor to its left (previous frame);
- *  III. New frame only has a neighbor to its right (next frame); and
- *  IV.  New frame has both left and right neighbors.
- *
- * I. New frame is the only frame in the list.
- *
- *      A)  If the read offset is larger than the end of the new frame and
- *          the new frame has a FIN, it is an ERROR.
- *
- *      B)  If the read offset is larger than the end of the new frame and
- *          the new frame does not have a FIN, it is a DUP.
- *
- *      C)  If the read offset is equal to the end of the new frame and
- *          the new frame has a FIN, it is an OVERLAP.
- *
- *      D)  If the read offset is equal to the end of the new frame and
- *          the new frame does not have a FIN, it is a DUP.
- *
- * II. New frame only has a neighbor to its left.
- *
- *      - (A) and (B) apply.
- *
- *      E)  New frame could be the same as the previous frame: DUP.
- *
- *      F)  New frame has the same offset and size as previous frame, but
- *          previous frame has FIN and the new frame does not: DUP.
- *
- *      G)  New frame has the same offset and size as previous frame, but
- *          previous frame does not have a FIN and the new one does: OVERLAP.
- *
- *      H)  New frame could start inside previous frame: OVERLAP.
- *
- *  III. New frame only has a neighbor to its right.
- *
- *      - (A) and (B) apply.
- *
- *      I) Right neighbor could start inside new frame: OVERLAP.
- *
- *  IV.  New frame has both left and right neighbors.
- *
- *      - (A), (B), (E), (F), (G), (H), and (I) apply.
- */
 
-
+static int
-static enum ins_frame
 insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame,
                                     uint64_t read_offset, unsigned *p_n_frames)
 {
@@ -241,9 +206,17 @@ insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame,
     if (read_offset > DF_END(new_frame))
     {
         if (DF_FIN(new_frame))
-            return INS_FRAME_ERR;               /* Case (A) */
+            return INS_FRAME_ERR                                | CASE('A');
         else
-            return INS_FRAME_DUP;               /* Case (B) */
+            return INS_FRAME_DUP                                | CASE('B');
+    }
+
+    if (ncdi->ncdi_flags & NCDI_FIN_SET)
+    {
+        if (DF_FIN(new_frame) && DF_END(new_frame) != ncdi->ncdi_fin_off)
+            return INS_FRAME_ERR                                | CASE('C');
+        if (DF_END(new_frame) > ncdi->ncdi_fin_off)
+            return INS_FRAME_ERR                                | CASE('D');
     }
 
     /* Find position in the list, going backwards.  We go backwards because
@@ -279,12 +252,21 @@ insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame,
     switch (select)
     {
     default:    /* No neighbors */
-        if (read_offset == DF_END(new_frame) && DF_SIZE(new_frame))
+        if (read_offset == DF_END(new_frame))
         {
-            if (DF_FIN(new_frame))
+            if (DF_SIZE(new_frame))
-                return INS_FRAME_OVERLAP;       /* Case (C) */
+            {
-            else
+                if (DF_FIN(new_frame)
-                return INS_FRAME_DUP;           /* Case (D) */
+                    && !((ncdi->ncdi_flags & NCDI_FIN_REACHED)
+                            && read_offset == ncdi->ncdi_fin_off))
+                    return INS_FRAME_OVERLAP                    | CASE('E');
+                else
+                    return INS_FRAME_DUP                        | CASE('F');
+            }
+            else if (!DF_FIN(new_frame)
+                     || ((ncdi->ncdi_flags & NCDI_FIN_REACHED)
+                         && read_offset == ncdi->ncdi_fin_off))
+                return INS_FRAME_DUP                            | CASE('G');
         }
         goto list_was_empty;
     case 3:     /* Both left and right neighbors */
@@ -293,22 +275,18 @@ insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame,
             && DF_SIZE(prev_frame) == DF_SIZE(new_frame))
         {
             if (!DF_FIN(prev_frame) && DF_FIN(new_frame))
-                return INS_FRAME_OVERLAP;       /* Case (G) */
+                return INS_FRAME_OVERLAP                        | CASE('H');
             else
-                return INS_FRAME_DUP;           /* Cases (E) and (F) */
+                return INS_FRAME_DUP                            | CASE('I');
         }
         if (DF_END(prev_frame) > DF_OFF(new_frame))
-            return INS_FRAME_OVERLAP;           /* Case (H) */
+            return INS_FRAME_OVERLAP                            | CASE('J');
         if (select == 2)
-        {
-            if (DF_FIN(prev_frame))
-                return INS_FRAME_ERR;
             goto have_prev;
-        }
         /* Fall-through */
     case 1:     /* Only right neighbor (next_frame) */
         if (DF_END(new_frame) > DF_OFF(next_frame))
-            return INS_FRAME_OVERLAP;           /* Case (I) */
+            return INS_FRAME_OVERLAP                            | CASE('K');
         break;
     }
 
@@ -332,11 +310,18 @@ insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame,
     }
     CHECK_ORDER(ncdi);
 
+    if (DF_FIN(new_frame))
+    {
+        ncdi->ncdi_flags |= NCDI_FIN_SET;
+        ncdi->ncdi_fin_off = DF_END(new_frame);
+        LSQ_DEBUG("FIN set at %"PRIu64, DF_END(new_frame));
+    }
+
     ++ncdi->ncdi_n_frames;
     ncdi->ncdi_byteage += DF_SIZE(new_frame);
     *p_n_frames = count;
 
-    return INS_FRAME_OK;
+    return INS_FRAME_OK                                         | CASE('Z');
 }
 
 
@@ -383,9 +368,13 @@ nocopy_di_insert_frame (struct data_in *data_in,
     struct nocopy_data_in *const ncdi = NCDI_PTR(data_in);
     unsigned count;
     enum ins_frame ins;
+    int ins_case;
 
     assert(0 == (new_frame->data_frame.df_fin & ~1));
-    ins = insert_frame(ncdi, new_frame, read_offset, &count);
+    ins_case = insert_frame(ncdi, new_frame, read_offset, &count);
+    ins = ins_case & 0xFF;
+    ins_case >>= 8;
+    LSQ_DEBUG("%s: ins: %d (case '%c')", __func__, ins, (char) ins_case);
     switch (ins)
     {
     case INS_FRAME_OK:
@@ -430,6 +419,11 @@ nocopy_di_frame_done (struct data_in *data_in, struct data_frame *data_frame)
                     frame->data_frame.df_size != first->data_frame.df_offset;
     --ncdi->ncdi_n_frames;
     ncdi->ncdi_byteage -= frame->data_frame.df_size;
+    if (DF_FIN(frame))
+    {
+        ncdi->ncdi_flags |= NCDI_FIN_REACHED;
+        LSQ_DEBUG("FIN has been reached at offset %"PRIu64, DF_END(frame));
+    }
     lsquic_packet_in_put(ncdi->ncdi_conn_pub->mm, frame->packet_in);
     lsquic_malo_put(frame);
 }
@@ -495,6 +489,7 @@ nocopy_di_mem_used (struct data_in *data_in)
     return size;
 }
 
+
 static const struct data_in_iface di_if_nocopy = {
     .di_destroy      = nocopy_di_destroy,
     .di_empty        = nocopy_di_empty,

src/liblsquic/lsquic_engine.c

@@ -1325,5 +1325,3 @@ lsquic_engine_count_attq (lsquic_engine_t *engine, int from_now)
         now += from_now;
     return attq_count_before(engine->attq, now);
 }
-
-

src/liblsquic/lsquic_ev_log.c

@@ -316,5 +316,3 @@ lsquic_ev_log_generated_http_push_promise (lsquic_cid_t cid,
                 (int)    extra_headers->headers[i].value.iov_len,
                 (char *) extra_headers->headers[i].value.iov_base);
 }
-
-

src/liblsquic/lsquic_frame_writer.c

@@ -747,5 +747,3 @@ lsquic_frame_writer_mem_used (const struct lsquic_frame_writer *fw)
 
     return size;
 }
-
-

src/liblsquic/lsquic_logger.c

@@ -389,5 +389,3 @@ lsquic_set_log_level (const char *level_str)
     else
         return -1;
 }
-
-

src/liblsquic/lsquic_mm.c

@@ -322,5 +322,3 @@ lsquic_mm_mem_used (const struct lsquic_mm *mm)
 
     return size;
 }
-
-

src/liblsquic/lsquic_parse_gquic_common.c

@@ -462,5 +462,3 @@ acki2str (const struct ack_info *acki, size_t *sz)
     *sz = off;
     return buf;
 }
-
-

src/liblsquic/lsquic_send_ctl.c

@@ -1853,5 +1853,3 @@ lsquic_send_ctl_mem_used (const struct lsquic_send_ctl *ctl)
 
     return size;
 }
-
-

src/liblsquic/lsquic_str.c

@@ -130,5 +130,3 @@ lsquic_str_copy (lsquic_str_t *lstr_dst, const lsquic_str_t *lstr_src)
     lstr_dst->len = lstr_src->len;
     return lstr_dst;
 }
-
-

src/liblsquic/lsquic_stream.c

@@ -149,6 +149,7 @@ sm_history_append (lsquic_stream_t *stream, enum stream_history_event sh_event)
                                                         stream->sm_hist_buf);
 }
 
+
 #   define SM_HISTORY_APPEND(stream, event) sm_history_append(stream, event)
 #   define SM_HISTORY_DUMP_REMAINING(stream) do {                           \
         if (stream->sm_hist_idx & SM_HIST_IDX_MASK)                         \
@@ -2134,3 +2135,5 @@ lsquic_stream_cid (const struct lsquic_stream *stream)
 {
     return LSQUIC_LOG_CONN_ID;
 }
+
+

src/liblsquic/lsquic_stream.h

@@ -7,7 +7,6 @@
 
 #define LSQUIC_STREAM_DEFAULT_PRIO 16   /* RFC 7540, Section 5.3.5 */
 
-
 struct lsquic_stream_if;
 struct lsquic_stream_ctx;
 struct lsquic_conn_public;
@@ -16,7 +15,6 @@ struct uncompressed_headers;
 
 TAILQ_HEAD(lsquic_streams_tailq, lsquic_stream);
 
-
 #ifndef LSQUIC_KEEP_STREAM_HISTORY
 #   ifdef NDEBUG
 #       define LSQUIC_KEEP_STREAM_HISTORY 0
@@ -25,14 +23,12 @@ TAILQ_HEAD(lsquic_streams_tailq, lsquic_stream);
 #   endif
 #endif
 
-
 #if LSQUIC_KEEP_STREAM_HISTORY
 #define SM_HIST_BITS 6
 #define SM_HIST_IDX_MASK ((1 << SM_HIST_BITS) - 1)
 typedef unsigned char sm_hist_idx_t;
 #endif
 
-
 struct lsquic_stream
 {
     uint32_t                        id;
@@ -133,7 +129,6 @@ struct lsquic_stream
 #endif
 };
 
-
 enum stream_ctor_flags
 {
     SCF_CALL_ON_NEW   = (1 << 0), /* Call on_new_stream() immediately */
@@ -148,7 +143,6 @@ enum stream_ctor_flags
     SCF_ALLOW_OVERLAP = (1 << 4), /* Allow STREAM frames to overlap */
 };
 
-
 lsquic_stream_t *
 lsquic_stream_new_ext (uint32_t id, struct lsquic_conn_public *conn_pub,
                        const struct lsquic_stream_if *, void *stream_if_ctx,

test/prog.c

@@ -400,5 +400,3 @@ prog_is_stopped (void)
 {
     return prog_stopped != 0;
 }
-
-

test/test_common.c

@@ -1140,5 +1140,3 @@ destroy_lsquic_reader_ctx (struct reader_ctx *ctx)
     (void) close(ctx->fd);
     free(ctx);
 }
-
-

test/unittests/test_engine_ctor.c

@@ -34,5 +34,3 @@ main (void)
 
     return 0;
 }
-
-

test/unittests/test_stream.c

@@ -1622,6 +1622,73 @@ test_overlaps (void)
 }
 
 
+static void
+test_insert_edge_cases (void)
+{
+    struct test_objs tobjs;
+    lsquic_stream_t *stream;
+    stream_frame_t *frame;
+    int s;
+    ssize_t nread;
+    const char data[] = "1234567890";
+    unsigned buf[0x1000];
+
+    init_test_objs(&tobjs, 0x4000, 0x4000);
+
+    {
+        stream = new_stream(&tobjs, 123);
+        frame = new_frame_in_ext(&tobjs, 0, 6, 1, &data[0]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Inserted frame #1", 0 == s));
+        /* Invalid frame: different FIN location */
+        frame = new_frame_in_ext(&tobjs, 3, 2, 1, &data[3]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Invalid frame: different FIN location", -1 == s));
+        lsquic_stream_destroy(stream);
+    }
+
+    {
+        stream = new_stream(&tobjs, 123);
+        frame = new_frame_in_ext(&tobjs, 0, 6, 0, &data[0]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Inserted frame #1", 0 == s));
+        nread = lsquic_stream_read(stream, buf, sizeof(buf));
+        assert(6 == nread);
+        frame = new_frame_in_ext(&tobjs, 6, 0, 0, &data[6]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Duplicate frame", 0 == s));
+        nread = lsquic_stream_read(stream, buf, sizeof(buf));
+        assert(nread == -1 && errno == EAGAIN);
+        frame = new_frame_in_ext(&tobjs, 6, 0, 1, &data[6]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Frame OK", 0 == s));
+        nread = lsquic_stream_read(stream, buf, sizeof(buf));
+        assert(nread == 0); /* Hit EOF */
+        frame = new_frame_in_ext(&tobjs, 6, 0, 1, &data[6]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Duplicate FIN frame", 0 == s));
+        lsquic_stream_destroy(stream);
+    }
+
+    {
+        stream = new_stream(&tobjs, 123);
+        frame = new_frame_in_ext(&tobjs, 0, 6, 1, &data[0]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Inserted frame #1", 0 == s));
+        nread = lsquic_stream_read(stream, buf, sizeof(buf));
+        assert(6 == nread);
+        nread = lsquic_stream_read(stream, buf, sizeof(buf));
+        assert(0 == nread); /* Hit EOF */
+        frame = new_frame_in_ext(&tobjs, 0, 6, 1, &data[0]);
+        s = lsquic_stream_frame_in(stream, frame);
+        assert(("Inserted duplicate frame", 0 == s));
+        lsquic_stream_destroy(stream);
+    }
+
+    deinit_test_objs(&tobjs);
+}
+
+
 static void
 test_writing_to_stream_schedule_stream_packets_immediately (void)
 {
@@ -2448,6 +2515,7 @@ main (int argc, char **argv)
     test_blocked_flags();
     test_reading_from_stream2();
     test_overlaps();
+    test_insert_edge_cases();
 
     {
         int idx[6];