NULL-terminate name and value in nghttp2_nv

Guaranteeing NULL-termination is very useful when name or value are
used with C functions which requires NULL-terminated string.

lib/includes/nghttp2/nghttp2.h

@@ -402,21 +402,27 @@ typedef enum {
  */
 typedef struct {
   /**
-   * The |name| byte string, which is not necessarily ``NULL``
+   * The |name| byte string.  If this struct is presented from library
-   * terminated.
+   * (e.g., :type:`nghttp2_on_frame_recv_callback`), |name| is
+   * guaranteed to be NULL-terminated.  When application is
+   * constructing this struct, |name| is not required to be
+   * NULL-terminated.
    */
   uint8_t *name;
   /**
-   * The |value| byte string, which is not necessarily ``NULL``
+   * The |value| byte string.  If this struct is presented from
-   * terminated.
+   * library (e.g., :type:`nghttp2_on_frame_recv_callback`), |value|
+   * is guaranteed to be NULL-terminated.  When application is
+   * constructing this struct, |value| is not required to be
+   * NULL-terminated.
    */
   uint8_t *value;
   /**
-   * The length of the |name|.
+   * The length of the |name|, excluding terminating NULL.
    */
   size_t namelen;
   /**
-   * The length of the |value|.
+   * The length of the |value|, excluding terminating NULL.
    */
   size_t valuelen;
   /**
@@ -1461,7 +1467,11 @@ typedef int (*nghttp2_on_begin_headers_callback)(nghttp2_session *session,
  * :type:`nghttp2_on_frame_recv_callback` for the |frame| will not be
  * invoked.
  *
- * The |value| may be ``NULL`` if the |valuelen| is 0.
+ * Both |name| and |value| are guaranteed to be NULL-terminated.  The
+ * |namelen| and |valuelen| do not include terminal NULL.  If
+ * `nghttp2_option_set_no_http_messaging()` is used with nonzero
+ * value, NULL character may be included in |name| or |value| before
+ * terminating NULL.
  *
  * Please note that unless `nghttp2_option_set_no_http_messaging()` is
  * used, nghttp2 library does perform validation against the |name|

lib/nghttp2_frame.c

@@ -739,7 +739,8 @@ int nghttp2_nv_array_copy(nghttp2_nv **nva_ptr, const nghttp2_nv *nva,
   nghttp2_nv *p;
 
   for (i = 0; i < nvlen; ++i) {
-    buflen += nva[i].namelen + nva[i].valuelen;
+    /* + 2 for null-termination */
+    buflen += nva[i].namelen + nva[i].valuelen + 2;
   }
 
   if (nvlen == 0) {
@@ -765,12 +766,14 @@ int nghttp2_nv_array_copy(nghttp2_nv **nva_ptr, const nghttp2_nv *nva,
     memcpy(data, nva[i].name, nva[i].namelen);
     p->name = data;
     p->namelen = nva[i].namelen;
+    data[p->namelen] = '\0';
     nghttp2_downcase(p->name, p->namelen);
-    data += nva[i].namelen;
+    data += nva[i].namelen + 1;
     memcpy(data, nva[i].value, nva[i].valuelen);
     p->value = data;
     p->valuelen = nva[i].valuelen;
-    data += nva[i].valuelen;
+    data[p->valuelen] = '\0';
+    data += nva[i].valuelen + 1;
     ++p;
   }
   return 0;

lib/nghttp2_frame.h

@@ -471,7 +471,9 @@ void nghttp2_nv_array_sort(nghttp2_nv *nva, size_t nvlen);
 /*
  * Copies name/value pairs from |nva|, which contains |nvlen| pairs,
  * to |*nva_ptr|, which is dynamically allocated so that all items can
- * be stored.
+ * be stored.  The resultant name and value in nghttp2_nv are
+ * guaranteed to be NULL-terminated even if the input is not
+ * null-terminated.
  *
  * The |*nva_ptr| must be freed using nghttp2_nv_array_del().
  *

lib/nghttp2_hd.c

@@ -152,10 +152,11 @@ int nghttp2_hd_entry_init(nghttp2_hd_entry *ent, uint8_t flags, uint8_t *name,
   if ((flags & NGHTTP2_HD_FLAG_NAME_ALLOC) &&
       (flags & NGHTTP2_HD_FLAG_NAME_GIFT) == 0) {
     if (namelen == 0) {
-      /* We should not allow empty header field name */
+      flags &= ~NGHTTP2_HD_FLAG_NAME_ALLOC;
-      ent->nv.name = NULL;
+      ent->nv.name = (uint8_t *)"";
     } else {
-      ent->nv.name = nghttp2_memdup(name, namelen, mem);
+      /* copy including terminating NULL byte */
+      ent->nv.name = nghttp2_memdup(name, namelen + 1, mem);
       if (ent->nv.name == NULL) {
         rv = NGHTTP2_ERR_NOMEM;
         goto fail;
@@ -167,9 +168,11 @@ int nghttp2_hd_entry_init(nghttp2_hd_entry *ent, uint8_t flags, uint8_t *name,
   if ((flags & NGHTTP2_HD_FLAG_VALUE_ALLOC) &&
       (flags & NGHTTP2_HD_FLAG_VALUE_GIFT) == 0) {
     if (valuelen == 0) {
-      ent->nv.value = NULL;
+      flags &= ~NGHTTP2_HD_FLAG_VALUE_ALLOC;
+      ent->nv.value = (uint8_t *)"";
     } else {
-      ent->nv.value = nghttp2_memdup(value, valuelen, mem);
+      /* copy including terminating NULL byte */
+      ent->nv.value = nghttp2_memdup(value, valuelen + 1, mem);
       if (ent->nv.value == NULL) {
         rv = NGHTTP2_ERR_NOMEM;
         goto fail2;
@@ -404,11 +407,8 @@ static size_t entry_room(size_t namelen, size_t valuelen) {
 }
 
 static int emit_indexed_header(nghttp2_nv *nv_out, nghttp2_hd_entry *ent) {
-  DEBUGF(fprintf(stderr, "inflatehd: header emission: "));
+  DEBUGF(fprintf(stderr, "inflatehd: header emission: %s: %s\n", ent->nv.name,
-  DEBUGF(fwrite(ent->nv.name, ent->nv.namelen, 1, stderr));
+                 ent->nv.value));
-  DEBUGF(fprintf(stderr, ": "));
-  DEBUGF(fwrite(ent->nv.value, ent->nv.valuelen, 1, stderr));
-  DEBUGF(fprintf(stderr, "\n"));
   /* ent->ref may be 0. This happens if the encoder emits literal
      block larger than header table capacity with indexing. */
   *nv_out = ent->nv;
@@ -416,11 +416,8 @@ static int emit_indexed_header(nghttp2_nv *nv_out, nghttp2_hd_entry *ent) {
 }
 
 static int emit_literal_header(nghttp2_nv *nv_out, nghttp2_nv *nv) {
-  DEBUGF(fprintf(stderr, "inflatehd: header emission: "));
+  DEBUGF(fprintf(stderr, "inflatehd: header emission: %s: %s\n", nv->name,
-  DEBUGF(fwrite(nv->name, nv->namelen, 1, stderr));
+                 nv->value));
-  DEBUGF(fprintf(stderr, ": "));
-  DEBUGF(fwrite(nv->value, nv->valuelen, 1, stderr));
-  DEBUGF(fprintf(stderr, "\n"));
   *nv_out = *nv;
   return 0;
 }
@@ -750,11 +747,9 @@ static nghttp2_hd_entry *add_hd_table_incremental(nghttp2_hd_context *context,
 
     context->hd_table_bufsize -= entry_room(ent->nv.namelen, ent->nv.valuelen);
 
-    DEBUGF(fprintf(stderr, "hpack: remove item from header table: "));
+    DEBUGF(fprintf(stderr, "hpack: remove item from header table: %s: %s\n",
-    DEBUGF(fwrite(ent->nv.name, ent->nv.namelen, 1, stderr));
+                   ent->nv.name, ent->nv.value));
-    DEBUGF(fprintf(stderr, ": "));
+
-    DEBUGF(fwrite(ent->nv.value, ent->nv.valuelen, 1, stderr));
-    DEBUGF(fprintf(stderr, "\n"));
     hd_ringbuf_pop_back(&context->hd_table);
     if (--ent->ref == 0) {
       nghttp2_hd_entry_free(ent, mem);
@@ -975,11 +970,7 @@ static int deflate_nv(nghttp2_hd_deflater *deflater, nghttp2_bufs *bufs,
   uint32_t value_hash = hash(nv->value, nv->valuelen);
   nghttp2_mem *mem;
 
-  DEBUGF(fprintf(stderr, "deflatehd: deflating "));
+  DEBUGF(fprintf(stderr, "deflatehd: deflating %s: %s\n", nv->name, nv->value));
-  DEBUGF(fwrite(nv->name, nv->namelen, 1, stderr));
-  DEBUGF(fprintf(stderr, ": "));
-  DEBUGF(fwrite(nv->value, nv->valuelen, 1, stderr));
-  DEBUGF(fprintf(stderr, "\n"));
 
   mem = deflater->ctx.mem;
 
@@ -1340,15 +1331,19 @@ static int hd_inflate_remove_bufs(nghttp2_hd_inflater *inflater, nghttp2_nv *nv,
     buflen = rv;
 
     if (value_only) {
+      /* we don't use this value, so no need to NULL-terminate */
       nv->name = NULL;
       nv->namelen = 0;
+
+      nv->value = buf;
+      nv->valuelen = buflen - 1;
     } else {
       nv->name = buf;
       nv->namelen = inflater->newnamelen;
-    }
 
-    nv->value = buf + nv->namelen;
+      nv->value = buf + nv->namelen + 1;
-    nv->valuelen = buflen - nv->namelen;
+      nv->valuelen = buflen - nv->namelen - 2;
+    }
 
     return 0;
   }
@@ -1360,15 +1355,19 @@ static int hd_inflate_remove_bufs(nghttp2_hd_inflater *inflater, nghttp2_nv *nv,
   pbuf = &inflater->nvbufs.head->buf;
 
   if (value_only) {
+    /* we don't use this value, so no need to NULL-terminate */
     nv->name = NULL;
     nv->namelen = 0;
+
+    nv->value = pbuf->pos;
+    nv->valuelen = nghttp2_buf_len(pbuf) - 1;
   } else {
     nv->name = pbuf->pos;
     nv->namelen = inflater->newnamelen;
-  }
 
-  nv->value = pbuf->pos + nv->namelen;
+    nv->value = pbuf->pos + nv->namelen + 1;
-  nv->valuelen = nghttp2_buf_len(pbuf) - nv->namelen;
+    nv->valuelen = nghttp2_buf_len(pbuf) - nv->namelen - 2;
+  }
 
   /* Resetting does not change the content of first buffer */
   nghttp2_bufs_reset(&inflater->nvbufs);
@@ -1528,6 +1527,7 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
   uint8_t *first = in;
   uint8_t *last = in + inlen;
   int rfin = 0;
+  int busy = 0;
 
   if (inflater->ctx.bad) {
     return NGHTTP2_ERR_HEADER_COMP;
@@ -1536,7 +1536,8 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
   DEBUGF(fprintf(stderr, "inflatehd: start state=%d\n", inflater->state));
   hd_inflate_keep_free(inflater);
   *inflate_flags = NGHTTP2_HD_INFLATE_NONE;
-  for (; in != last;) {
+  for (; in != last || busy;) {
+    busy = 0;
     switch (inflater->state) {
     case NGHTTP2_HD_STATE_OPCODE:
       if ((*in & 0xe0u) == 0x20u) {
@@ -1688,6 +1689,11 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
 
       inflater->newnamelen = nghttp2_bufs_len(&inflater->nvbufs);
 
+      rv = nghttp2_bufs_addb(&inflater->nvbufs, '\0');
+      if (rv != 0) {
+        goto fail;
+      }
+
       inflater->state = NGHTTP2_HD_STATE_CHECK_VALUELEN;
 
       break;
@@ -1709,6 +1715,11 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
 
       inflater->newnamelen = nghttp2_bufs_len(&inflater->nvbufs);
 
+      rv = nghttp2_bufs_addb(&inflater->nvbufs, '\0');
+      if (rv != 0) {
+        goto fail;
+      }
+
       inflater->state = NGHTTP2_HD_STATE_CHECK_VALUELEN;
 
       break;
@@ -1734,19 +1745,6 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
       }
 
       DEBUGF(fprintf(stderr, "inflatehd: valuelen=%zu\n", inflater->left));
-      if (inflater->left == 0) {
-        if (inflater->opcode == NGHTTP2_HD_OPCODE_NEWNAME) {
-          rv = hd_inflate_commit_newname(inflater, nv_out);
-        } else {
-          rv = hd_inflate_commit_indname(inflater, nv_out);
-        }
-        if (rv != 0) {
-          goto fail;
-        }
-        inflater->state = NGHTTP2_HD_STATE_OPCODE;
-        *inflate_flags |= NGHTTP2_HD_INFLATE_EMIT;
-        return (ssize_t)(in - first);
-      }
 
       if (inflater->huffman_encoded) {
         nghttp2_hd_huff_decode_context_init(&inflater->huff_decode_ctx);
@@ -1755,6 +1753,9 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
       } else {
         inflater->state = NGHTTP2_HD_STATE_READ_VALUE;
       }
+
+      busy = 1;
+
       break;
     case NGHTTP2_HD_STATE_READ_VALUEHUFF:
       rv = hd_inflate_read_huff(inflater, &inflater->nvbufs, in, last);
@@ -1773,6 +1774,11 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
         goto almost_ok;
       }
 
+      rv = nghttp2_bufs_addb(&inflater->nvbufs, '\0');
+      if (rv != 0) {
+        goto fail;
+      }
+
       if (inflater->opcode == NGHTTP2_HD_OPCODE_NEWNAME) {
         rv = hd_inflate_commit_newname(inflater, nv_out);
       } else {
@@ -1805,6 +1811,11 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_inflater *inflater, nghttp2_nv *nv_out,
         goto almost_ok;
       }
 
+      rv = nghttp2_bufs_addb(&inflater->nvbufs, '\0');
+      if (rv != 0) {
+        goto fail;
+      }
+
       if (inflater->opcode == NGHTTP2_HD_OPCODE_NEWNAME) {
         rv = hd_inflate_commit_newname(inflater, nv_out);
       } else {

src/app_helper.cc

@@ -164,11 +164,8 @@ const char *ansi_escend() { return color_output ? "\033[0m" : ""; }
 
 namespace {
 void print_nv(nghttp2_nv *nv) {
-  fprintf(outfile, "%s", ansi_esc("\033[1;34m"));
+  fprintf(outfile, "%s%s%s: %s\n", ansi_esc("\033[1;34m"), nv->name,
-  fwrite(nv->name, nv->namelen, 1, outfile);
+          ansi_escend(), nv->value);
-  fprintf(outfile, "%s: ", ansi_escend());
-  fwrite(nv->value, nv->valuelen, 1, outfile);
-  fprintf(outfile, "\n");
 }
 } // namespace
 namespace {

src/http2.cc

@@ -314,10 +314,7 @@ void dump_nv(FILE *out, const nghttp2_nv *nva, size_t nvlen) {
 
 void dump_nv(FILE *out, const Headers &nva) {
   for (auto &nv : nva) {
-    fwrite(nv.name.c_str(), nv.name.size(), 1, out);
+    fprintf(out, "%s: %s\n", nv.name.c_str(), nv.value.c_str());
-    fwrite(": ", 2, 1, out);
-    fwrite(nv.value.c_str(), nv.value.size(), 1, out);
-    fwrite("\n", 1, 1, out);
   }
   fwrite("\n", 1, 1, out);
   fflush(out);

src/shrpx_http2_downstream_connection.cc

@@ -414,11 +414,7 @@ int Http2DownstreamConnection::push_request_headers() {
   if (LOG_ENABLED(INFO)) {
     std::stringstream ss;
     for (auto &nv : nva) {
-      ss << TTY_HTTP_HD;
+      ss << TTY_HTTP_HD << nv.name << TTY_RST << ": " << nv.value << "\n";
-      ss.write(reinterpret_cast<const char *>(nv.name), nv.namelen);
-      ss << TTY_RST << ": ";
-      ss.write(reinterpret_cast<const char *>(nv.value), nv.valuelen);
-      ss << "\n";
     }
     DCLOG(INFO, this) << "HTTP request headers\n" << ss.str();
   }

src/shrpx_http2_upstream.cc

@@ -1426,11 +1426,7 @@ Http2Upstream::log_response_headers(Downstream *downstream,
                                     const std::vector<nghttp2_nv> &nva) const {
   std::stringstream ss;
   for (auto &nv : nva) {
-    ss << TTY_HTTP_HD;
+    ss << TTY_HTTP_HD << nv.name << TTY_RST << ": " << nv.value << "\n";
-    ss.write(reinterpret_cast<const char *>(nv.name), nv.namelen);
-    ss << TTY_RST << ": ";
-    ss.write(reinterpret_cast<const char *>(nv.value), nv.valuelen);
-    ss << "\n";
   }
   ULOG(INFO, this) << "HTTP response headers. stream_id="
                    << downstream->get_stream_id() << "\n" << ss.str();

tests/nghttp2_hd_test.c

@@ -307,7 +307,7 @@ void test_nghttp2_hd_inflate_indname_inc_eviction(void) {
   nghttp2_hd_inflater inflater;
   nghttp2_bufs bufs;
   ssize_t blocklen;
-  uint8_t value[1024];
+  uint8_t value[1025];
   nva_out out;
   nghttp2_nv nv;
   nghttp2_mem *mem;
@@ -319,8 +319,9 @@ void test_nghttp2_hd_inflate_indname_inc_eviction(void) {
   nghttp2_hd_inflate_init(&inflater, mem);
 
   memset(value, '0', sizeof(value));
+  value[sizeof(value) - 1] = '\0';
   nv.value = value;
-  nv.valuelen = sizeof(value);
+  nv.valuelen = sizeof(value) - 1;
 
   nv.flags = NGHTTP2_NV_FLAG_NONE;
 
@@ -338,7 +339,7 @@ void test_nghttp2_hd_inflate_indname_inc_eviction(void) {
   CU_ASSERT(4 == out.nvlen);
   CU_ASSERT(14 == out.nva[0].namelen);
   CU_ASSERT(0 == memcmp("accept-charset", out.nva[0].name, out.nva[0].namelen));
-  CU_ASSERT(sizeof(value) == out.nva[0].valuelen);
+  CU_ASSERT(sizeof(value) - 1 == out.nva[0].valuelen);
 
   nva_out_reset(&out, mem);
   nghttp2_bufs_reset(&bufs);
@@ -429,7 +430,7 @@ void test_nghttp2_hd_inflate_clearall_inc(void) {
   nghttp2_bufs bufs;
   ssize_t blocklen;
   nghttp2_nv nv;
-  uint8_t value[4060];
+  uint8_t value[4061];
   nva_out out;
   nghttp2_mem *mem;
 
@@ -441,8 +442,9 @@ void test_nghttp2_hd_inflate_clearall_inc(void) {
   nv.name = (uint8_t *)"alpha";
   nv.namelen = strlen((char *)nv.name);
   memset(value, '0', sizeof(value));
+  value[sizeof(value) - 1] = '\0';
   nv.value = value;
-  nv.valuelen = sizeof(value);
+  nv.valuelen = sizeof(value) - 1;
 
   nv.flags = NGHTTP2_NV_FLAG_NONE;
 
@@ -473,7 +475,7 @@ void test_nghttp2_hd_inflate_clearall_inc(void) {
 
   /* This time, 4096 bytes space required, which is just fits in the
      header table */
-  nv.valuelen = sizeof(value) - 1;
+  nv.valuelen = sizeof(value) - 2;
 
   CU_ASSERT(0 == nghttp2_hd_emit_newname_block(&bufs, &nv, 1));
 
@@ -547,7 +549,7 @@ void test_nghttp2_hd_ringbuf_reserve(void) {
   nv.name = (uint8_t *)"a";
   nv.namelen = strlen((const char *)nv.name);
   nv.valuelen = 4;
-  nv.value = mem->malloc(nv.valuelen, NULL);
+  nv.value = mem->malloc(nv.valuelen + 1, NULL);
   memset(nv.value, 0, nv.valuelen);
 
   nghttp2_hd_deflate_init2(&deflater, 8000, mem);