Latest changes

- Do not send RST_STREAM when stream is closed for reading
- Raise maximum header size from 4K to 64K
- Check header name and value lengths against maximum imposed by HPACK
- Fix NULL dereference in stream flow controller

CHANGELOG

@@ -1,3 +1,10 @@
+2017-10-12
+
+    - Do not send RST_STREAM when stream is closed for reading
+    - Raise maximum header size from 4K to 64K
+    - Check header name and value lengths against maximum imposed by HPACK
+    - Fix NULL dereference in stream flow controller
+
 2017-10-09
 
     - Hide handshake implementation behind a set of function pointers

src/liblsquic/lsquic_frame_reader.c

@@ -514,7 +514,7 @@ struct header_writer_ctx
     }                            hwc_flags;
     enum pseudo_header           pseh_mask;
     char                        *pseh_bufs[N_PSEH];
-    uint16_t                     name_len,
+    hpack_strlen_t               name_len,
                                  val_len;
 };
 

src/liblsquic/lsquic_frame_writer.c

@@ -49,6 +49,8 @@ struct frame_buf
 #define frab_left_to_write(f) ((unsigned short) sizeof((f)->frab_buf) - (f)->frab_size)
 #define frab_write_to(f) ((f)->frab_buf + (f)->frab_size)
 
+#define MAX_HEADERS_SIZE (64 * 1024)
+
 /* Make sure that frab_buf is at least five bytes long, otherwise a frame
  * won't fit into two adjacent frabs.
  */
@@ -392,6 +394,19 @@ calc_headers_size (const struct lsquic_http_headers *headers)
 }
 
 
+static int
+have_oversize_strings (const struct lsquic_http_headers *headers)
+{
+    int i, have;
+    for (i = 0, have = 0; i < headers->count; ++i)
+    {
+        have |= headers->headers[i].name.iov_len  > HPACK_MAX_STRLEN;
+        have |= headers->headers[i].value.iov_len > HPACK_MAX_STRLEN;
+    }
+    return have;
+}
+
+
 static int
 check_headers_size (const struct lsquic_frame_writer *fw,
                     const struct lsquic_http_headers *headers,
@@ -435,25 +450,29 @@ check_headers_case (const struct lsquic_frame_writer *fw,
 static int
 write_headers (struct lsquic_frame_writer *fw,
                const struct lsquic_http_headers *headers,
-               struct header_framer_ctx *hfc, unsigned char *buf4k)
+               struct header_framer_ctx *hfc, unsigned char *buf,
+               const unsigned buf_sz)
 {
     unsigned char *end;
     int i, s;
 
     for (i = 0; i < headers->count; ++i)
     {
-        end = lsquic_henc_encode(fw->fw_henc, buf4k, buf4k + 0x1000,
+        end = lsquic_henc_encode(fw->fw_henc, buf, buf + buf_sz,
             headers->headers[i].name.iov_base, headers->headers[i].name.iov_len,
             headers->headers[i].value.iov_base, headers->headers[i].value.iov_len, 0);
-        if (!(end > buf4k))
+        if (end > buf)
+        {
+            s = hfc_write(hfc, buf, end - buf);
+            if (s < 0)
+                return s;
+        }
+        else
         {
             LSQ_WARN("error encoding header");
             errno = EBADMSG;
             return -1;
         }
-        s = hfc_write(hfc, buf4k, end - buf4k);
-        if (s < 0)
-            return s;
     }
 
     return 0;
@@ -481,6 +500,9 @@ lsquic_frame_writer_write_headers (struct lsquic_frame_writer *fw,
     if (0 != check_headers_case(fw, headers))
         return -1;
 
+    if (have_oversize_strings(headers))
+        return -1;
+
     if (eos)
         flags = HFHF_END_STREAM;
     else
@@ -501,11 +523,11 @@ lsquic_frame_writer_write_headers (struct lsquic_frame_writer *fw,
             return s;
     }
 
-    buf = lsquic_mm_get_4k(fw->fw_mm);
+    buf = malloc(MAX_HEADERS_SIZE);
     if (!buf)
         return -1;
-    s = write_headers(fw, headers, &hfc, buf);
+    s = write_headers(fw, headers, &hfc, buf, MAX_HEADERS_SIZE);
-    lsquic_mm_put_4k(fw->fw_mm, buf);
+    free(buf);
     if (0 == s)
     {
         EV_LOG_GENERATED_HTTP_HEADERS(LSQUIC_LOG_CONN_ID, stream_id,
@@ -556,6 +578,12 @@ lsquic_frame_writer_write_promise (struct lsquic_frame_writer *fw,
     if (extra_headers && 0 != check_headers_case(fw, extra_headers))
         return -1;
 
+    if (have_oversize_strings(&mpas))
+        return -1;
+
+    if (extra_headers && have_oversize_strings(extra_headers))
+        return -1;
+
     hfc_init(&hfc, fw, fw->fw_max_frame_sz, HTTP_FRAME_PUSH_PROMISE,
                                                             stream_id, 0);
 
@@ -565,21 +593,21 @@ lsquic_frame_writer_write_promise (struct lsquic_frame_writer *fw,
     if (s < 0)
         return s;
 
-    buf = lsquic_mm_get_4k(fw->fw_mm);
+    buf = malloc(MAX_HEADERS_SIZE);
     if (!buf)
         return -1;
 
-    s = write_headers(fw, &mpas, &hfc, buf);
+    s = write_headers(fw, &mpas, &hfc, buf, MAX_HEADERS_SIZE);
     if (s != 0)
     {
-        lsquic_mm_put_4k(fw->fw_mm, buf);
+        free(buf);
         return -1;
     }
 
     if (extra_headers)
-        s = write_headers(fw, extra_headers, &hfc, buf);
+        s = write_headers(fw, extra_headers, &hfc, buf, MAX_HEADERS_SIZE);
 
-    lsquic_mm_put_4k(fw->fw_mm, buf);
+    free(buf);
 
     if (0 == s)
     {
@@ -641,6 +669,7 @@ write_settings (struct lsquic_frame_writer *fw,
     return 0;
 }
 
+
 int
 lsquic_frame_writer_write_settings (struct lsquic_frame_writer *fw,
     const struct lsquic_http2_setting *settings, unsigned n_settings)
@@ -711,3 +740,5 @@ lsquic_frame_writer_write_priority (struct lsquic_frame_writer *fw,
 
     return lsquic_frame_writer_flush(fw);
 }
+
+

src/liblsquic/lsquic_hpack_common.h

@@ -2,6 +2,8 @@
 #ifndef LSQUIC_HPACK_COMMON_H
 #define LSQUIC_HPACK_COMMON_H
 
+#include "lsquic_hpack_types.h"
+
 #define HPACK_STATIC_TABLE_SIZE   61
 #define INITIAL_DYNAMIC_TABLE_SIZE  4096
 
@@ -21,9 +23,9 @@
     typedef struct hpack_hdr_tbl_s
     {
         const char *name;
-        uint16_t name_len;
+        hpack_strlen_t name_len;
         const char *val;
-        uint16_t val_len;
+        hpack_strlen_t val_len;
     } hpack_hdr_tbl_t;
 
 /**

src/liblsquic/lsquic_hpack_dec.h

@@ -6,6 +6,8 @@
 #ifndef LSQUIC_HPACK_DEC_H
 #define LSQUIC_HPACK_DEC_H
 
+#include "lsquic_hpack_types.h"
+
 struct lsquic_hdec
 {
     unsigned           hpd_max_capacity;       /* Maximum set by caller */
@@ -34,7 +36,8 @@ lsquic_hdec_cleanup (struct lsquic_hdec *);
 int
 lsquic_hdec_decode (struct lsquic_hdec *dec,
     const unsigned char **src, const unsigned char *src_end,
-    char *dst, char *const dst_end, uint16_t *name_len, uint16_t *val_len);
+    char *dst, char *const dst_end, hpack_strlen_t *name_len,
+    hpack_strlen_t *val_len);
 
 void
 lsquic_hdec_set_max_capacity (struct lsquic_hdec *, unsigned);
@@ -46,7 +49,8 @@ lsquic_hdec_dec_int (const unsigned char **src, const unsigned char *src_end,
 
 int
 lsquic_hdec_push_entry (struct lsquic_hdec *dec, const char *name,
-                        uint16_t name_len, const char *val, uint16_t val_len);
+                        hpack_strlen_t name_len, const char *val,
+                        hpack_strlen_t val_len);
 #endif
 
 #endif

src/liblsquic/lsquic_hpack_enc.c

@@ -29,8 +29,8 @@ struct enc_table_entry
     unsigned                        ete_id;
     unsigned                        ete_nameval_hash;
     unsigned                        ete_name_hash;
-    uint16_t                        ete_name_len;
+    hpack_strlen_t                  ete_name_len;
-    uint16_t                        ete_val_len;
+    hpack_strlen_t                  ete_val_len;
     char                            ete_buf[0];
 };
 
@@ -93,8 +93,8 @@ lsquic_henc_cleanup (struct lsquic_henc *enc)
 static
 #endif
        unsigned
-lsquic_henc_get_stx_tab_id (const char *name, uint16_t name_len,
+lsquic_henc_get_stx_tab_id (const char *name, hpack_strlen_t name_len,
-                        const char *val, uint16_t val_len, int *val_matched)
+                    const char *val, hpack_strlen_t val_len, int *val_matched)
 {
     if (name_len < 3)
         return 0;
@@ -428,7 +428,7 @@ henc_calc_table_id (const struct lsquic_henc *enc,
 
 static unsigned
 henc_find_table_id (struct lsquic_henc *enc, const char *name,
-        uint16_t name_len, const char *value, uint16_t value_len,
+        hpack_strlen_t name_len, const char *value, hpack_strlen_t value_len,
         int *val_matched)
 {
     struct enc_table_entry *entry;
@@ -561,7 +561,7 @@ static
 #endif
        int
 lsquic_henc_enc_str (unsigned char *const dst, size_t dst_len,
-                                const unsigned char *str, uint16_t str_len)
+                            const unsigned char *str, hpack_strlen_t str_len)
 {
     unsigned char size_buf[4];
     unsigned char *p;
@@ -710,7 +710,8 @@ static
 #endif
        int
 lsquic_henc_push_entry (struct lsquic_henc *enc, const char *name,
-                    uint16_t name_len, const char *value, uint16_t value_len)
+                        hpack_strlen_t name_len, const char *value,
+                        hpack_strlen_t value_len)
 {
     unsigned name_hash, nameval_hash, buckno;
     struct enc_table_entry *entry;
@@ -757,8 +758,8 @@ lsquic_henc_push_entry (struct lsquic_henc *enc, const char *name,
 
 unsigned char *
 lsquic_henc_encode (struct lsquic_henc *enc, unsigned char *dst,
-        unsigned char *dst_end, const char *name, uint16_t name_len,
+        unsigned char *dst_end, const char *name, hpack_strlen_t name_len,
-        const char *value, uint16_t value_len, int indexed_type)
+        const char *value, hpack_strlen_t value_len, int indexed_type)
 {
     //indexed_type: 0, Add, 1,: without, 2: never
     static const char indexed_prefix_number[] = {0x40, 0x00, 0x10};

src/liblsquic/lsquic_hpack_enc.h

@@ -6,6 +6,8 @@
 #ifndef LSQUIC_HPACK_ENC_H
 #define LSQUIC_HPACK_ENC_H 1
 
+#include "lsquic_hpack_types.h"
+
 struct enc_table_entry;
 
 #ifndef NDEBUG
@@ -72,24 +74,24 @@ lsquic_henc_cleanup (struct lsquic_henc *);
  */
 unsigned char *
 lsquic_henc_encode (struct lsquic_henc *henc, unsigned char *dst,
-    unsigned char *dst_end, const char *name, uint16_t name_len,
+    unsigned char *dst_end, const char *name, hpack_strlen_t name_len,
-    const char *value, uint16_t value_len, int indexed_type);
+    const char *value, hpack_strlen_t value_len, int indexed_type);
 
 void
 lsquic_henc_set_max_capacity (struct lsquic_henc *, unsigned);
 
 #ifndef NDEBUG
 unsigned
-lsquic_henc_get_stx_tab_id (const char *name, uint16_t name_len,
+lsquic_henc_get_stx_tab_id (const char *name, hpack_strlen_t name_len,
-                        const char *val, uint16_t val_len, int *val_matched);
+                    const char *val, hpack_strlen_t val_len, int *val_matched);
 
 int
 lsquic_henc_push_entry (struct lsquic_henc *enc, const char *name,
-                    uint16_t name_len, const char *value, uint16_t value_len);
+        hpack_strlen_t name_len, const char *value, hpack_strlen_t value_len);
 
 int
 lsquic_henc_enc_str (unsigned char *const dst, size_t dst_len,
-                                const unsigned char *str, uint16_t str_len);
+                            const unsigned char *str, hpack_strlen_t str_len);
 
 void
 lsquic_henc_iter_reset (struct lsquic_henc *enc);

src/liblsquic/lsquic_hpack_types.h

@@ -0,0 +1,10 @@
+/* Copyright (c) 2017 LiteSpeed Technologies Inc.  See LICENSE. */
+#ifndef LSQUIC_HPACK_TYPES_H
+#define LSQUIC_HPACK_TYPES_H 1
+
+typedef uint16_t hpack_strlen_t;
+
+#define HPACK_MAX_STRLEN (((1 << (sizeof(hpack_strlen_t) * 8 - 1)) | \
+                            (((1 << (sizeof(hpack_strlen_t) * 8 - 1)) - 1))))
+
+#endif

src/liblsquic/lsquic_sfcw.c

@@ -49,18 +49,27 @@ sfcw_maybe_increase_max_window (struct lsquic_sfcw *fc)
     if (new_max_window > fc->sf_conn_pub->enpub->enp_settings.es_max_sfcw)
         new_max_window = fc->sf_conn_pub->enpub->enp_settings.es_max_sfcw;
 
-    /* Do not increase past the connection's maximum window size.  The
+    if (fc->sf_cfcw)
-     * connection's window will be increased separately, if possible.
+    {
-     *
+        /* Do not increase past the connection's maximum window size.  The
-     * The reference implementation has the logic backwards:  Imagine
+         * connection's window will be increased separately, if possible.
-     * several concurrent streams that are not being read from fast
+         *
-     * enough by the user code.  Each of them uses only a fraction
+         * The reference implementation has the logic backwards:  Imagine
-     * of bandwidth.  Does it mean that the connection window must
+         * several concurrent streams that are not being read from fast
-     * increase?  No.
+         * enough by the user code.  Each of them uses only a fraction
-     */
+         * of bandwidth.  Does it mean that the connection window must
-    max_conn_window = lsquic_cfcw_get_max_recv_window(fc->sf_cfcw);
+         * increase?  No.
-    if (new_max_window > max_conn_window)
+         */
-        new_max_window = max_conn_window;
+        max_conn_window = lsquic_cfcw_get_max_recv_window(fc->sf_cfcw);
+        if (new_max_window > max_conn_window)
+            new_max_window = max_conn_window;
+    }
+    else
+    {
+        /* This means that this stream is not affected by connection flow
+         * controller.  No need to adjust under connection window.
+         */
+    }
 
     if (new_max_window > fc->sf_max_recv_win)
     {

src/liblsquic/lsquic_stream.c

@@ -197,7 +197,6 @@ 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)                         \
@@ -577,8 +576,6 @@ lsquic_stream_frame_in (lsquic_stream_t *stream, stream_frame_t *frame)
         max_off = frame->data_frame.df_offset + frame->data_frame.df_size;
         if (0 != lsquic_stream_update_sfcw(stream, max_off))
             return -1;
-        if ((stream->stream_flags & STREAM_U_READ_DONE))
-            lsquic_stream_reset_ext(stream, 1, 0);
         if (frame->data_frame.df_fin)
         {
             SM_HISTORY_APPEND(stream, SHE_FIN_IN);
@@ -902,24 +899,12 @@ lsquic_stream_tosend_fin (const lsquic_stream_t *stream)
 }
 
 
-static int
-readable_data_frame_remains (lsquic_stream_t *stream)
-{
-        return !stream->data_in->di_if->di_empty(stream->data_in);
-}
-
-
 static void
 stream_shutdown_read (lsquic_stream_t *stream)
 {
     if (!(stream->stream_flags & STREAM_U_READ_DONE))
     {
         SM_HISTORY_APPEND(stream, SHE_SHUTDOWN_READ);
-        if (stream->uh || readable_data_frame_remains(stream))
-        {
-            LSQ_INFO("read shut down, but there is still data to be read");
-            lsquic_stream_reset_ext(stream, 1, 1);
-        }
         stream->stream_flags |= STREAM_U_READ_DONE;
         stream_wantread(stream, 0);
         maybe_finish_stream(stream);
@@ -1314,7 +1299,6 @@ maybe_mark_as_blocked (lsquic_stream_t *stream)
     }
 }
 
-
 void
 lsquic_stream_dispatch_rw_events (lsquic_stream_t *stream)
 {
@@ -1384,7 +1368,6 @@ sbt_write (struct stream_buf_tosend *sbt, const void *buf, size_t len)
     return ntowrite;
 }
 
-
 static size_t
 sbt_read_buf (struct stream_buf_tosend *sbt, void *buf, size_t len)
 {
@@ -2403,5 +2386,3 @@ lsquic_stream_refuse_push (lsquic_stream_t *stream)
     else
         return -1;
 }
-
-

src/liblsquic/lsquic_stream.h

@@ -9,6 +9,7 @@
 
 #define LSQUIC_STREAM_DEFAULT_PRIO 16   /* RFC 7540, Section 5.3.5 */
 
+
 struct lsquic_stream_if;
 struct lsquic_stream_ctx;
 struct lsquic_conn_public;
@@ -18,6 +19,7 @@ struct uncompressed_headers;
 TAILQ_HEAD(lsquic_streams_tailq, lsquic_stream);
 TAILQ_HEAD(sbts_tailq, stream_buf_tosend);
 
+
 #ifndef LSQUIC_KEEP_STREAM_HISTORY
 #   ifdef NDEBUG
 #       define LSQUIC_KEEP_STREAM_HISTORY 0
@@ -26,12 +28,14 @@ TAILQ_HEAD(sbts_tailq, stream_buf_tosend);
 #   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;
@@ -143,6 +147,7 @@ struct lsquic_stream
 #endif
 };
 
+
 enum stream_ctor_flags
 {
     SCF_CALL_ON_NEW   = (1 << 0), /* Call on_new_stream() immediately */
@@ -156,6 +161,7 @@ enum stream_ctor_flags
     SCF_DISP_RW_ONCE  = (1 << 3),
 };
 
+
 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/unittests/test_frame_writer.c

@@ -150,6 +150,47 @@ test_one_header (void)
 }
 
 
+static void
+test_oversize_header (void)
+{
+    struct lsquic_henc henc;
+    struct lsquic_frame_writer *fw;
+    int s;
+    struct lsquic_mm mm;
+    const size_t big_len = 100 * 1000;
+    char *value;
+
+    lsquic_henc_init(&henc);
+    lsquic_mm_init(&mm);
+    fw = lsquic_frame_writer_new(&mm, NULL, 0x200, &henc, output_write,
+                                         output_navail, output_flush, 0);
+    reset_output(0);
+
+    value = malloc(big_len);
+    memset(value, 'A', big_len);
+
+    struct lsquic_http_header header_arr[] =
+    {
+        { .name = IOV(":status"), .value = IOV("302") },
+        { .name = IOV("some-header"),
+          .value = { .iov_base = value, .iov_len = big_len, } },
+    };
+
+    struct lsquic_http_headers headers = {
+        .count = sizeof(header_arr) / sizeof(header_arr[0]),
+        .headers = header_arr,
+    };
+
+    s = lsquic_frame_writer_write_headers(fw, 12345, &headers, 0, 100);
+    assert(-1 == s);
+
+    lsquic_frame_writer_destroy(fw);
+    lsquic_henc_cleanup(&henc);
+    lsquic_mm_cleanup(&mm);
+    free(value);
+}
+
+
 static void
 test_continuations (void)
 {
@@ -533,6 +574,7 @@ int
 main (void)
 {
     test_one_header();
+    test_oversize_header();
     test_continuations();
     test_settings_normal();
     test_settings_short();

test/unittests/test_stream.c

@@ -204,7 +204,6 @@ permute_and_run (uint64_t run_id,
     }
 }
 
-
 static void
 test_write_file (const char *filename)
 {
@@ -592,7 +591,7 @@ test_rem_FIN_loc_FIN (struct test_objs *tobjs)
 
 
 /* Server: we read data and close the read side before reading FIN, which
- * results in stream being reset.
+ * DOES NOT result in stream being reset.
  */
 static void
 test_rem_data_loc_close (struct test_objs *tobjs)
@@ -610,40 +609,30 @@ test_rem_data_loc_close (struct test_objs *tobjs)
     n = lsquic_stream_read(stream, buf, 60);
     assert(60 == n);
 
-    s = lsquic_stream_shutdown(stream, 0);  /* This causes a reset */
+    s = lsquic_stream_shutdown(stream, 0);
     assert(0 == s);
-    assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+    assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
-    assert((stream->stream_flags & (STREAM_SERVICE_FLAGS))
+    assert(!((stream->stream_flags & (STREAM_SERVICE_FLAGS))
-                                            == STREAM_CALL_ONCLOSE);
+                                            == STREAM_CALL_ONCLOSE));
-
-    n = lsquic_stream_write(stream, buf, 100);
-    assert(n == -1);    /* Cannot write to reset stream */
 
     n = lsquic_stream_read(stream, buf, 60);
-    assert(n == -1);    /* Cannot read from reset stream */
+    assert(n == -1);    /* Cannot read from closed stream */
 
+    /* Close write side */
     s = lsquic_stream_shutdown(stream, 1);
-    assert(-1 == s);
+    assert(0 == s);
 
-    /* Reset is scheduled to be sent out: */
+    /* STREAM frame is scheduled to be sent out: */
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
     assert((stream->stream_flags & (STREAM_SENDING_FLAGS))
-                                            == STREAM_SEND_RST);
+                                            == STREAM_SEND_DATA);
-
-    lsquic_stream_call_on_close(stream);
-    assert(!(stream->stream_flags & (STREAM_SERVICE_FLAGS)));
-
-    /* reset frame has been sent */
-    lsquic_stream_rst_frame_sent(stream);
-    assert(TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
-    assert(!(stream->stream_flags & (STREAM_SENDING_FLAGS)));
 
     s = lsquic_stream_rst_in(stream, 100, 1);
     assert(0 == s);
 
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
     assert((stream->stream_flags & (STREAM_SERVICE_FLAGS))
-                                            == STREAM_FREE_STREAM);
+                                            == STREAM_CALL_ONCLOSE);
 
     lsquic_stream_destroy(stream);
     assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
@@ -653,6 +642,7 @@ test_rem_data_loc_close (struct test_objs *tobjs)
 }
 
 
+
 /* Client: we send some data and FIN, but remote end sends some data and
  * then resets the stream.  The client gets an error when it reads from
  * stream, after which it closes and destroys the stream.
@@ -948,6 +938,7 @@ test_unlimited_stream_flush_data (struct test_objs *tobjs)
 }
 
 
+
 /* Write a little data to the stream, packetize the data, then reset the
  * stream: connection cap should NOT go back up.
  *
@@ -1947,8 +1938,5 @@ main (int argc, char **argv)
 
     test_flushing();
 
-
     return 0;
 }
-
-