Don't pack multiple empty header values in one header field

SPDY spec does not allow multiple empty header values in one header
field. This change makes out-going framer ignore such empty header
value if there is non-empty header value with the same name.

lib/spdylay_frame.c

@@ -250,17 +250,28 @@ size_t spdylay_frame_count_nv_space(char **nv, size_t len_size)
   int i;
   const char *prev = "";
   size_t prevlen = 0;
+  size_t prevvallen = 0;
   for(i = 0; nv[i]; i += 2) {
     const char *key = nv[i];
     const char *val = nv[i+1];
     size_t keylen = strlen(key);
     size_t vallen = strlen(val);
     if(prevlen == keylen && memcmp(prev, key, keylen) == 0) {
-      /* Join previous value, with NULL character */
+      if(vallen) {
-      sum += vallen+1;
+        if(prevvallen) {
+          /* Join previous value, with NULL character */
+          sum += vallen+1;
+          prevvallen = vallen;
+        } else {
+          /* Previous value is empty. In this case, drop the
+             previous. */
+          sum += vallen;
+        }
+      }
     } else {
       prev = key;
       prevlen = keylen;
+      prevvallen = vallen;
       /* SPDY NV header does not include terminating NULL byte */
       sum += keylen+vallen+len_size*2;
     }
@@ -273,28 +284,43 @@ ssize_t spdylay_frame_pack_nv(uint8_t *buf, char **nv, size_t len_size)
   int i;
   uint8_t *bufp = buf+len_size;
   uint32_t num_nv = 0;
-  /* TODO Join values with same keys, using '\0' as a delimiter */
   const char *prev = "";
-  uint8_t *prev_vallen_buf = NULL;
+  uint8_t *cur_vallen_buf = NULL;
-  uint32_t prev_vallen = 0;
+  uint32_t cur_vallen = 0;
+  size_t prevkeylen = 0;
+  size_t prevvallen = 0;
   for(i = 0; nv[i]; i += 2) {
     const char *key = nv[i];
     const char *val = nv[i+1];
     size_t keylen = strlen(key);
     size_t vallen = strlen(val);
-    if(strcmp(prev, key) == 0) {
+    if(prevkeylen == keylen && memcmp(prev, key, keylen) == 0) {
-      prev_vallen += vallen+1;
+      if(vallen) {
-      spdylay_frame_put_nv_len(prev_vallen_buf, prev_vallen, len_size);
+        if(prevvallen) {
-      *bufp = '\0';
+          /* Join previous value, with NULL character */
-      ++bufp;
+          cur_vallen += vallen+1;
-      memcpy(bufp, val, vallen);
+          spdylay_frame_put_nv_len(cur_vallen_buf, cur_vallen, len_size);
-      bufp += vallen;
+          *bufp = '\0';
+          ++bufp;
+          memcpy(bufp, val, vallen);
+          bufp += vallen;
+        } else {
+          /* Previous value is empty. In this case, drop the
+             previous. */
+          cur_vallen += vallen;
+          spdylay_frame_put_nv_len(cur_vallen_buf, cur_vallen, len_size);
+          memcpy(bufp, val, vallen);
+          bufp += vallen;
+        }
+      }
     } else {
       ++num_nv;
       bufp = spdylay_pack_str(bufp, key, keylen, len_size);
       prev = key;
-      prev_vallen_buf = bufp;
+      cur_vallen_buf = bufp;
-      prev_vallen = vallen;
+      cur_vallen = vallen;
+      prevkeylen = keylen;
+      prevvallen = vallen;
       bufp = spdylay_pack_str(bufp, val, vallen, len_size);
     }
   }

tests/main.c

@@ -210,6 +210,10 @@ int main(int argc, char* argv[])
                    test_spdylay_frame_nv_downcase) ||
       !CU_add_test(pSuite, "frame_pack_nv_duplicate_keys",
                    test_spdylay_frame_pack_nv_duplicate_keys) ||
+      !CU_add_test(pSuite, "frame_pack_nv_empty_value_spdy2",
+                   test_spdylay_frame_pack_nv_empty_value_spdy2) ||
+      !CU_add_test(pSuite, "frame_pack_nv_empty_value_spdy3",
+                   test_spdylay_frame_pack_nv_empty_value_spdy3) ||
       !CU_add_test(pSuite, "frame_nv_2to3", test_spdylay_frame_nv_2to3) ||
       !CU_add_test(pSuite, "frame_nv_3to2", test_spdylay_frame_nv_3to2) ||
       !CU_add_test(pSuite, "frame_unpack_nv_check_name_spdy2",

tests/spdylay_frame_test.c

@@ -24,12 +24,29 @@
  */
 #include "spdylay_frame_test.h"
 
+#include <assert.h>
+
 #include <CUnit/CUnit.h>
 
 #include "spdylay_frame.h"
 #include "spdylay_helper.h"
 #include "spdylay_test_helper.h"
 
+/* Reads |len_size| byte from |data| as |len_size| byte network byte
+   order integer, and returns it in host byte order. Currently, we
+   only support len_size == 2 or 4 */
+static int get_packed_hd_len(uint8_t *data, size_t len_size)
+{
+  if(len_size == 2) {
+    return spdylay_get_uint16(data);
+  } else if(len_size == 4) {
+    return spdylay_get_uint32(data);
+  } else {
+    /* Not supported */
+    assert(0);
+  }
+}
+
 static const char *headers[] = {
   "method", "GET",
   "scheme", "https",
@@ -181,12 +198,99 @@ void test_spdylay_frame_pack_nv_duplicate_keys(void)
   spdylay_frame_nv_del(nv);
 }
 
+static const char *multi_empty_headers1[] = {
+  "a", "",
+  "a", "",
+  NULL
+};
+
+static const char *multi_empty_headers2[] = {
+  "a", "/",
+  "a", "",
+  NULL
+};
+
+static const char *multi_empty_headers3[] = {
+  "a", "",
+  "a", "/",
+  NULL
+};
+
 void test_spdylay_frame_count_nv_space(void)
 {
   size_t len_size = 2;
   CU_ASSERT(85 == spdylay_frame_count_nv_space((char**)headers, len_size));
   len_size = 4;
   CU_ASSERT(111 == spdylay_frame_count_nv_space((char**)headers, len_size));
+  /* only ("a", "") is counted */
+  CU_ASSERT(13 == spdylay_frame_count_nv_space((char**)multi_empty_headers1,
+                                               len_size));
+  /* only ("a", "/") is counted */
+  CU_ASSERT(14 == spdylay_frame_count_nv_space((char**)multi_empty_headers2,
+                                               len_size));
+  /* only ("a", "/") is counted */
+  CU_ASSERT(14 == spdylay_frame_count_nv_space((char**)multi_empty_headers3,
+                                               len_size));
+}
+
+static void frame_pack_nv_empty_value_check(uint8_t *outptr,
+                                            int vallen,
+                                            const char *val,
+                                            size_t len_size)
+{
+  int len;
+  len = get_packed_hd_len(outptr, len_size);
+  CU_ASSERT(1 == len);
+  outptr += len_size;
+  len = get_packed_hd_len(outptr, len_size);
+  CU_ASSERT(1 == len);
+  outptr += len_size;
+  CU_ASSERT(0 == memcmp("a", outptr, len));
+  outptr += len;
+  len = get_packed_hd_len(outptr, len_size);
+  CU_ASSERT(vallen == len);
+  len += len_size;
+  if(vallen == len) {
+    CU_ASSERT(0 == memcmp(val, outptr, vallen));
+  }
+}
+
+static void test_spdylay_frame_pack_nv_empty_value_with(size_t len_size)
+{
+  uint8_t out[256];
+  char **nv;
+  ssize_t rv;
+  int off = (len_size == 2 ? -6 : 0);
+
+  nv = spdylay_frame_nv_copy(multi_empty_headers1);
+  rv = spdylay_frame_pack_nv(out, nv, len_size);
+  CU_ASSERT(13+off == rv);
+  frame_pack_nv_empty_value_check(out, 0, NULL, len_size);
+  spdylay_frame_nv_del(nv);
+
+  nv = spdylay_frame_nv_copy(multi_empty_headers2);
+  rv = spdylay_frame_pack_nv(out, nv, len_size);
+  CU_ASSERT(14+off == rv);
+  frame_pack_nv_empty_value_check(out, 1, "/", len_size);
+  spdylay_frame_nv_del(nv);
+
+  nv = spdylay_frame_nv_copy(multi_empty_headers3);
+  rv = spdylay_frame_pack_nv(out, nv, len_size);
+  CU_ASSERT(14+off == rv);
+  frame_pack_nv_empty_value_check(out, 1, "/", len_size);
+  spdylay_frame_nv_del(nv);
+}
+
+void test_spdylay_frame_pack_nv_empty_value_spdy2(void)
+{
+  test_spdylay_frame_pack_nv_empty_value_with
+    (spdylay_frame_get_len_size(SPDYLAY_PROTO_SPDY2));
+}
+
+void test_spdylay_frame_pack_nv_empty_value_spdy3(void)
+{
+  test_spdylay_frame_pack_nv_empty_value_with
+    (spdylay_frame_get_len_size(SPDYLAY_PROTO_SPDY3));
 }
 
 void test_spdylay_frame_count_unpack_nv_space(void)

tests/spdylay_frame_test.h

@@ -28,6 +28,8 @@
 void test_spdylay_frame_unpack_nv_spdy2(void);
 void test_spdylay_frame_unpack_nv_spdy3(void);
 void test_spdylay_frame_pack_nv_duplicate_keys(void);
+void test_spdylay_frame_pack_nv_empty_value_spdy2(void);
+void test_spdylay_frame_pack_nv_empty_value_spdy3(void);
 void test_spdylay_frame_count_nv_space(void);
 void test_spdylay_frame_count_unpack_nv_space(void);
 void test_spdylay_frame_pack_ping(void);