Fix header compression bug, and perform always incremental indexing

lib/nghttp2_hd.c

@@ -291,9 +291,8 @@ static nghttp2_hd_entry* add_hd_table_incremental(nghttp2_hd_context *context,
   for(i = 0; i < context->hd_tablelen &&
         context->hd_table_bufsize > NGHTTP2_HD_MAX_BUFFER_SIZE; ++i) {
     nghttp2_hd_entry *ent = context->hd_table[i];
-    --ent->ref;
     context->hd_table_bufsize -= entry_room(ent->nv.namelen, ent->nv.valuelen);
-    if(ent->ref == 0) {
+    if(--ent->ref == 0) {
       nghttp2_hd_entry_free(ent);
       free(ent);
     }
@@ -584,7 +583,7 @@ static  uint8_t* decode_length(ssize_t *res, uint8_t *in, uint8_t *last,
     *res = -1;
     return in;
   }
-  if(*in == k) {
+  if((*in & k) == k) {
     *res = k;
   } else {
     *res = (*in) & k;
@@ -610,37 +609,37 @@ static  uint8_t* decode_length(ssize_t *res, uint8_t *in, uint8_t *last,
 }
 
 static int emit_indexed_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                              size_t *offset_ptr, nghttp2_hd_entry *ent)
+                              size_t *offset_ptr, size_t index)
 {
   int rv;
   uint8_t *bufp;
-  size_t blocklen = count_encoded_length(ent->index, 7);
+  size_t blocklen = count_encoded_length(index, 7);
   rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen);
   if(rv != 0) {
     return rv;
   }
   bufp = *buf_ptr + *offset_ptr;
-  encode_length(bufp, ent->index, 7);
+  encode_length(bufp, index, 7);
   (*buf_ptr)[*offset_ptr] |= 0x80u;
   *offset_ptr += blocklen;
   return 0;
 }
 
 static int emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                              size_t *offset_ptr, nghttp2_hd_entry *ent,
+                              size_t *offset_ptr, size_t index,
                               const uint8_t *value, size_t valuelen,
                               int inc_indexing)
 {
   int rv;
   uint8_t *bufp;
-  size_t blocklen = count_encoded_length(ent->index, 5) +
+  size_t blocklen = count_encoded_length(index, 5) +
     count_encoded_length(valuelen, 8) + valuelen;
   rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen);
   if(rv != 0) {
     return rv;
   }
   bufp = *buf_ptr + *offset_ptr;
-  bufp += encode_length(bufp, ent->index + 1, 5);
+  bufp += encode_length(bufp, index + 1, 5);
   bufp += encode_length(bufp, valuelen, 8);
   memcpy(bufp, value, valuelen);
   (*buf_ptr)[*offset_ptr] |= inc_indexing ? 0x40u : 0x60u;
@@ -672,22 +671,22 @@ static int emit_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
 }
 
 static int emit_subst_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                                    size_t *offset_ptr, nghttp2_hd_entry *ent,
+                                    size_t *offset_ptr, size_t index,
                                     const uint8_t *value, size_t valuelen,
-                                    size_t index)
+                                    size_t subindex)
 {
   int rv;
   uint8_t *bufp;
-  size_t blocklen = count_encoded_length(ent->index + 1, 6) +
-    count_encoded_length(index, 8) +
+  size_t blocklen = count_encoded_length(index + 1, 6) +
+    count_encoded_length(subindex, 8) +
     count_encoded_length(valuelen, 8) + valuelen;
   rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen);
   if(rv != 0) {
     return rv;
   }
   bufp = *buf_ptr + *offset_ptr;
-  bufp += encode_length(bufp, ent->index + 1, 6);
-  bufp += encode_length(bufp, index, 8);
+  bufp += encode_length(bufp, index + 1, 6);
+  bufp += encode_length(bufp, subindex, 8);
   bufp += encode_length(bufp, valuelen, 8);
   memcpy(bufp, value, valuelen);
   *offset_ptr += blocklen;
@@ -696,12 +695,12 @@ static int emit_subst_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
 
 static int emit_subst_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
                                     size_t *offset_ptr, nghttp2_nv *nv,
-                                    size_t index)
+                                    size_t subindex)
 {
   int rv;
   uint8_t *bufp;
   size_t blocklen = 1 + count_encoded_length(nv->namelen, 8) + nv->namelen +
-    count_encoded_length(index, 8) +
+    count_encoded_length(subindex, 8) +
     count_encoded_length(nv->valuelen, 8) + nv->valuelen;
   rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen);
   if(rv != 0) {
@@ -712,7 +711,7 @@ static int emit_subst_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
   bufp += encode_length(bufp, nv->namelen, 8);
   memcpy(bufp, nv->name, nv->namelen);
   bufp += nv->namelen;
-  bufp += encode_length(bufp, index, 8);
+  bufp += encode_length(bufp, subindex, 8);
   bufp += encode_length(bufp, nv->valuelen, 8);
   memcpy(bufp, nv->value, nv->valuelen);
   *offset_ptr += blocklen;
@@ -734,35 +733,42 @@ static void create_workingset(nghttp2_hd_context *context)
   context->refsetlen = 0;
 }
 
-static int require_eviction_on_subst(nghttp2_hd_context *context,
-                                     nghttp2_nv *nv,
-                                     nghttp2_hd_entry *ent)
-{
-  return context->hd_table_bufsize - entry_room(ent->nv.namelen,
-                                                ent->nv.valuelen) +
-    entry_room(nv->namelen, nv->valuelen) > NGHTTP2_HD_MAX_BUFFER_SIZE;
-}
-
 ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater,
                               uint8_t **buf_ptr, size_t *buflen_ptr,
                               size_t nv_offset,
                               nghttp2_nv *nv, size_t nvlen)
 {
-  size_t i, offset;
+  size_t i, j, offset;
   int rv = 0;
   if(deflater->bad) {
     return NGHTTP2_ERR_HEADER_COMP;
   }
   create_workingset(deflater);
   offset = nv_offset;
-  for(i = 0; i < nvlen; ++i) {
-    nghttp2_hd_ws_entry *ws_ent;
-    ws_ent = find_in_workingset(deflater, &nv[i]);
-    if(ws_ent) {
+  /* Looks like we need to toggle first, because the index might be
+     overlapped by eviction */
+  for(i = 0; i < deflater->wslen; ++i) {
+    nghttp2_hd_ws_entry *ws_ent = &deflater->ws[i];
     if(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED) {
+      for(j = 0; j < nvlen; ++j) {
+        if(nghttp2_nv_equal(&ws_ent->indexed.entry->nv, &nv[j])) {
           ws_ent->indexed.checked = 1;
+          break;
         }
-    } else {
+      }
+      if(!ws_ent->indexed.checked) {
+        rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset,
+                                ws_ent->indexed.index);
+        if(rv < 0) {
+          goto fail;
+        }
+      }
+    }
+  }
+  for(i = 0; i < nvlen; ++i) {
+    nghttp2_hd_ws_entry *ws_ent;
+    ws_ent = find_in_workingset(deflater, &nv[i]);
+    if(!ws_ent) {
       nghttp2_hd_entry *ent;
       ent = find_in_hd_table(deflater, &nv[i]);
       if(ent) {
@@ -771,7 +777,7 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater,
         if(rv < 0) {
           goto fail;
         }
-        rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset, ent);
+        rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset, ent->index);
         if(rv < 0) {
           goto fail;
         }
@@ -779,21 +785,9 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater,
         /* Check name exists in hd_table */
         ent = find_name_in_hd_table(deflater, &nv[i]);
         if(ent) {
-          /* As long as no eviction kicked in and the same header
-             field name is not indexed and added, perform
-             substitution. Since we never evict anything, searching
-             ent->index in working set is safe. */
-          if(require_eviction_on_subst(deflater, &nv[i], ent) ||
-             find_in_workingset_by_index(deflater, ent->index)) {
-            rv = emit_indname_block(buf_ptr, buflen_ptr, &offset, ent,
-                                    nv[i].value, nv[i].valuelen, 0);
-            if(rv < 0) {
-              goto fail;
-            }
-          } else {
           nghttp2_hd_entry *new_ent;
-            /* No need to increment ent->ref here */
-            new_ent = add_hd_table_subst(deflater, &nv[i], ent->index);
+          uint8_t index = ent->index;
+          new_ent = add_hd_table_incremental(deflater, &nv[i]);
           if(!new_ent) {
             rv = NGHTTP2_ERR_HEADER_COMP;
             goto fail;
@@ -802,34 +796,30 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater,
           if(rv < 0) {
             goto fail;
           }
-            rv = emit_subst_indname_block(buf_ptr, buflen_ptr, &offset,
-                                          new_ent,
-                                          nv[i].value, nv[i].valuelen,
-                                          new_ent->index);
+          rv = emit_indname_block(buf_ptr, buflen_ptr, &offset, index,
+                                  nv[i].value, nv[i].valuelen, 1);
           if(rv < 0) {
             goto fail;
           }
-          }
         } else {
-          rv = emit_newname_block(buf_ptr, buflen_ptr, &offset, &nv[i], 0);
-          if(rv < 0) {
+          nghttp2_hd_entry *new_ent;
+          new_ent = add_hd_table_incremental(deflater, &nv[i]);
+          if(!new_ent) {
+            rv = NGHTTP2_ERR_HEADER_COMP;
             goto fail;
           }
+          rv = add_workingset(deflater, new_ent);
+          if(rv < 0) {
+            goto fail;
           }
-      }
-    }
-  }
-  for(i = 0; i < deflater->wslen; ++i) {
-    nghttp2_hd_ws_entry *ws_ent = &deflater->ws[i];
-    if(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED &&
-       !ws_ent->indexed.checked) {
-      rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset,
-                              ws_ent->indexed.entry);
+          rv = emit_newname_block(buf_ptr, buflen_ptr, &offset, &nv[i], 1);
           if(rv < 0) {
             goto fail;
           }
         }
       }
+    }
+  }
   return offset - nv_offset;
  fail:
   deflater->bad = 1;
@@ -1138,12 +1128,12 @@ int nghttp2_hd_end_headers(nghttp2_hd_context *context)
 }
 
 int nghttp2_hd_emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                                  size_t *offset_ptr, nghttp2_hd_entry *ent,
+                                  size_t *offset_ptr, size_t index,
                                   const uint8_t *value, size_t valuelen,
                                   int inc_indexing)
 {
   return emit_indname_block(buf_ptr, buflen_ptr, offset_ptr,
-                            ent, value, valuelen, inc_indexing);
+                            index, value, valuelen, inc_indexing);
 }
 
 int nghttp2_hd_emit_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
@@ -1154,18 +1144,18 @@ int nghttp2_hd_emit_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
 }
 
 int nghttp2_hd_emit_subst_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                                        size_t *offset_ptr,
-                                        nghttp2_hd_entry *ent,
+                                        size_t *offset_ptr, size_t index,
                                         const uint8_t *value, size_t valuelen,
-                                        size_t index)
+                                        size_t subindex)
 {
-  return emit_subst_indname_block(buf_ptr, buflen_ptr, offset_ptr,
-                                  ent, value, valuelen, index);
+  return emit_subst_indname_block(buf_ptr, buflen_ptr, offset_ptr, index,
+                                  value, valuelen, subindex);
 }
 
 int nghttp2_hd_emit_subst_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
                                         size_t *offset_ptr, nghttp2_nv *nv,
-                                        size_t index)
+                                        size_t subindex)
 {
-  return emit_subst_newname_block(buf_ptr, buflen_ptr, offset_ptr, nv, index);
+  return emit_subst_newname_block(buf_ptr, buflen_ptr, offset_ptr, nv,
+                                  subindex);
 }

lib/nghttp2_hd.h

@@ -224,7 +224,7 @@ int nghttp2_hd_end_headers(nghttp2_hd_context *deflater_or_inflater);
 
 /* For unittesting purpose */
 int nghttp2_hd_emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                                  size_t *offset_ptr, nghttp2_hd_entry *ent,
+                                  size_t *offset_ptr, size_t index,
                                   const uint8_t *value, size_t valuelen,
                                   int inc_indexing);
 
@@ -235,14 +235,13 @@ int nghttp2_hd_emit_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
 
 /* For unittesting purpose */
 int nghttp2_hd_emit_subst_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
-                                        size_t *offset_ptr,
-                                        nghttp2_hd_entry *ent,
+                                        size_t *offset_ptr, size_t index,
                                         const uint8_t *value, size_t valuelen,
-                                        size_t index);
+                                        size_t subindex);
 
 /* For unittesting purpose */
 int nghttp2_hd_emit_subst_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr,
                                         size_t *offset_ptr, nghttp2_nv *nv,
-                                        size_t index);
+                                        size_t subindex);
 
 #endif /* NGHTTP2_HD_COMP_H */

tests/nghttp2_frame_test.c

@@ -119,6 +119,7 @@ void test_nghttp2_frame_pack_headers()
   nghttp2_frame_headers_init(&frame, NGHTTP2_FLAG_END_STREAM, 1000000007,
                              1 << 20, nva, nvlen);
   framelen = nghttp2_frame_pack_headers(&buf, &buflen, &frame, &deflater);
+  nghttp2_hd_end_headers(&deflater);
 
   CU_ASSERT(0 == unpack_frame_with_nv_block((nghttp2_frame*)&oframe,
                                             NGHTTP2_HEADERS,
@@ -133,10 +134,13 @@ void test_nghttp2_frame_pack_headers()
   CU_ASSERT(nvvalueeq("GET", &oframe.nva[0]));
 
   nghttp2_frame_headers_free(&oframe);
+  nghttp2_hd_end_headers(&inflater);
+
   memset(&oframe, 0, sizeof(oframe));
   /* Next, include PRIORITY flag */
   frame.hd.flags |= NGHTTP2_FLAG_PRIORITY;
   framelen = nghttp2_frame_pack_headers(&buf, &buflen, &frame, &deflater);
+  nghttp2_hd_end_headers(&deflater);
 
   CU_ASSERT(0 == unpack_frame_with_nv_block((nghttp2_frame*)&oframe,
                                             NGHTTP2_HEADERS,
@@ -279,6 +283,7 @@ void test_nghttp2_frame_pack_push_promise()
   nghttp2_frame_push_promise_init(&frame, NGHTTP2_FLAG_END_PUSH_PROMISE,
                                   1000000007, (1U << 31) - 1, nva, nvlen);
   framelen = nghttp2_frame_pack_push_promise(&buf, &buflen, &frame, &deflater);
+  nghttp2_hd_end_headers(&deflater);
 
   CU_ASSERT(0 == unpack_frame_with_nv_block((nghttp2_frame*)&oframe,
                                             NGHTTP2_PUSH_PROMISE,

tests/nghttp2_hd_test.c

@@ -81,8 +81,6 @@ void test_nghttp2_hd_deflate(void)
   nghttp2_nv_array_del(resnva);
   nghttp2_hd_end_headers(&inflater);
 
-  CU_ASSERT(2 == inflater.refsetlen);
-
   /* Second headers */
   blocklen = nghttp2_hd_deflate_hd(&deflater, &buf, &buflen, nv_offset, nva2,
                                    sizeof(nva2)/sizeof(nghttp2_nv));
@@ -157,10 +155,8 @@ void test_nghttp2_hd_inflate_indname_inc(void)
   nghttp2_nv *resnva;
   nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_SERVER);
 
-  CU_ASSERT(0 == nghttp2_hd_emit_indname_block(&buf, &buflen, &offset,
-                                               inflater.hd_table[12],
-                                               nv.value, nv.valuelen,
-                                               1));
+  CU_ASSERT(0 == nghttp2_hd_emit_indname_block(&buf, &buflen, &offset, 12,
+                                               nv.value, nv.valuelen, 1));
   CU_ASSERT(1 == nghttp2_hd_inflate_hd(&inflater, &resnva, buf, offset));
   assert_nv_equal(&nv, resnva, 1);
   CU_ASSERT(39 == inflater.hd_tablelen);
@@ -184,8 +180,7 @@ void test_nghttp2_hd_inflate_indname_inc_eviction(void)
   nghttp2_nv *resnva;
   nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_SERVER);
 
-  CU_ASSERT(0 == nghttp2_hd_emit_indname_block(&buf, &buflen, &offset,
-                                               inflater.hd_table[2],
+  CU_ASSERT(0 == nghttp2_hd_emit_indname_block(&buf, &buflen, &offset, 2,
                                                value, sizeof(value), 1));
   CU_ASSERT(1 == nghttp2_hd_inflate_hd(&inflater, &resnva, buf, offset));
   CU_ASSERT(5 == resnva[0].namelen);
@@ -235,7 +230,7 @@ void test_nghttp2_hd_inflate_indname_subst(void)
   nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_SERVER);
 
   CU_ASSERT(0 == nghttp2_hd_emit_subst_indname_block(&buf, &buflen, &offset,
-                                                     inflater.hd_table[12],
+                                                     12,
                                                      nv.value, nv.valuelen,
                                                      12));
   CU_ASSERT(1 == nghttp2_hd_inflate_hd(&inflater, &resnva, buf, offset));
@@ -262,7 +257,7 @@ void test_nghttp2_hd_inflate_indname_subst_eviction(void)
   nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_SERVER);
 
   CU_ASSERT(0 == nghttp2_hd_emit_subst_indname_block(&buf, &buflen, &offset,
-                                                     inflater.hd_table[2],
+                                                     2,
                                                      value, sizeof(value), 2));
   CU_ASSERT(1 == nghttp2_hd_inflate_hd(&inflater, &resnva, buf, offset));
   CU_ASSERT(5 == resnva[0].namelen);
@@ -293,7 +288,7 @@ void test_nghttp2_hd_inflate_indname_subst_eviction_neg(void)
   nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_SERVER);
   /* Try to substitute index 0, but it will be evicted */
   CU_ASSERT(0 == nghttp2_hd_emit_subst_indname_block(&buf, &buflen, &offset,
-                                                     inflater.hd_table[2],
+                                                     2,
                                                      value, sizeof(value), 0));
   CU_ASSERT(1 == nghttp2_hd_inflate_hd(&inflater, &resnva, buf, offset));
   CU_ASSERT(5 == resnva[0].namelen);

tests/nghttp2_session_test.c

@@ -316,6 +316,8 @@ void test_nghttp2_session_recv(void)
   framelen = nghttp2_frame_pack_headers(&framedata, &framedatalen,
                                         &frame.headers,
                                         &session->hd_deflater);
+  nghttp2_hd_end_headers(&session->hd_deflater);
+
   scripted_data_feed_init(&df, framedata, framelen);
   /* Send 1 byte per each read */
   for(i = 0; i < framelen; ++i) {
@@ -336,6 +338,8 @@ void test_nghttp2_session_recv(void)
   framelen = nghttp2_frame_pack_headers(&framedata, &framedatalen,
                                         &frame.headers,
                                         &session->hd_deflater);
+  nghttp2_hd_end_headers(&session->hd_deflater);
+
   nghttp2_frame_headers_free(&frame.headers);
 
   scripted_data_feed_init(&df, framedata, framelen);
@@ -395,6 +399,8 @@ void test_nghttp2_session_recv_invalid_stream_id(void)
   framelen = nghttp2_frame_pack_headers(&framedata, &framedatalen,
                                         &frame.headers,
                                         &session->hd_deflater);
+  nghttp2_hd_end_headers(&session->hd_deflater);
+
   scripted_data_feed_init(&df, framedata, framelen);
   nghttp2_frame_headers_free(&frame.headers);
 
@@ -435,6 +441,8 @@ void test_nghttp2_session_recv_invalid_frame(void)
   framelen = nghttp2_frame_pack_headers(&framedata, &framedatalen,
                                         &frame.headers,
                                         &session->hd_deflater);
+  nghttp2_hd_end_headers(&session->hd_deflater);
+
   scripted_data_feed_init(&df, framedata, framelen);
 
   CU_ASSERT(0 == nghttp2_session_recv(session));