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);
-    lsquic_mm_put_4k(fw->fw_mm, buf);
+    s = write_headers(fw, headers, &hfc, buf, MAX_HEADERS_SIZE);
+    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;
-    uint16_t                        ete_val_len;
+    hpack_strlen_t                  ete_name_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,
-                        const char *val, uint16_t val_len, int *val_matched)
+lsquic_henc_get_stx_tab_id (const char *name, hpack_strlen_t name_len,
+                    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,
-        const char *value, uint16_t value_len, int indexed_type)
+        unsigned char *dst_end, const char *name, hpack_strlen_t name_len,
+        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,
-    const char *value, uint16_t value_len, int indexed_type);
+    unsigned char *dst_end, const char *name, hpack_strlen_t name_len,
+    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,
-                        const char *val, uint16_t val_len, int *val_matched);
+lsquic_henc_get_stx_tab_id (const char *name, hpack_strlen_t name_len,
+                    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
-     * connection's window will be increased separately, if possible.
-     *
-     * The reference implementation has the logic backwards:  Imagine
-     * several concurrent streams that are not being read from fast
-     * enough by the user code.  Each of them uses only a fraction
-     * of bandwidth.  Does it mean that the connection window must
-     * increase?  No.
-     */
-    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;
+    if (fc->sf_cfcw)
+    {
+        /* Do not increase past the connection's maximum window size.  The
+         * connection's window will be increased separately, if possible.
+         *
+         * The reference implementation has the logic backwards:  Imagine
+         * several concurrent streams that are not being read from fast
+         * enough by the user code.  Each of them uses only a fraction
+         * of bandwidth.  Does it mean that the connection window must
+         * increase?  No.
+         */
+        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)
     {