diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index cae33f5a..1d37e1e6 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -402,21 +402,27 @@ typedef enum { */ typedef struct { /** - * The |name| byte string, which is not necessarily ``NULL`` - * terminated. + * The |name| byte string. If this struct is presented from library + * (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`` - * terminated. + * The |value| byte string. If this struct is presented from + * 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| diff --git a/lib/nghttp2_frame.c b/lib/nghttp2_frame.c index d97194b2..cf6f1a49 100644 --- a/lib/nghttp2_frame.c +++ b/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; diff --git a/lib/nghttp2_frame.h b/lib/nghttp2_frame.h index 3cff6082..bb41e553 100644 --- a/lib/nghttp2_frame.h +++ b/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(). * diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index d8a17eeb..2c21afe6 100644 --- a/lib/nghttp2_hd.c +++ b/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 */ - ent->nv.name = NULL; + flags &= ~NGHTTP2_HD_FLAG_NAME_ALLOC; + 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(fwrite(ent->nv.name, ent->nv.namelen, 1, stderr)); - DEBUGF(fprintf(stderr, ": ")); - DEBUGF(fwrite(ent->nv.value, ent->nv.valuelen, 1, stderr)); - DEBUGF(fprintf(stderr, "\n")); + DEBUGF(fprintf(stderr, "inflatehd: header emission: %s: %s\n", ent->nv.name, + ent->nv.value)); /* 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(fwrite(nv->name, nv->namelen, 1, stderr)); - DEBUGF(fprintf(stderr, ": ")); - DEBUGF(fwrite(nv->value, nv->valuelen, 1, stderr)); - DEBUGF(fprintf(stderr, "\n")); + DEBUGF(fprintf(stderr, "inflatehd: header emission: %s: %s\n", nv->name, + nv->value)); *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(fwrite(ent->nv.name, ent->nv.namelen, 1, stderr)); - DEBUGF(fprintf(stderr, ": ")); - DEBUGF(fwrite(ent->nv.value, ent->nv.valuelen, 1, stderr)); - DEBUGF(fprintf(stderr, "\n")); + DEBUGF(fprintf(stderr, "hpack: remove item from header table: %s: %s\n", + ent->nv.name, ent->nv.value)); + 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(fwrite(nv->name, nv->namelen, 1, stderr)); - DEBUGF(fprintf(stderr, ": ")); - DEBUGF(fwrite(nv->value, nv->valuelen, 1, stderr)); - DEBUGF(fprintf(stderr, "\n")); + DEBUGF(fprintf(stderr, "deflatehd: deflating %s: %s\n", nv->name, nv->value)); 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->valuelen = buflen - nv->namelen; + nv->value = buf + nv->namelen + 1; + 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->valuelen = nghttp2_buf_len(pbuf) - nv->namelen; + nv->value = pbuf->pos + nv->namelen + 1; + 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 { diff --git a/src/app_helper.cc b/src/app_helper.cc index 64d16179..af7653c1 100644 --- a/src/app_helper.cc +++ b/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")); - fwrite(nv->name, nv->namelen, 1, outfile); - fprintf(outfile, "%s: ", ansi_escend()); - fwrite(nv->value, nv->valuelen, 1, outfile); - fprintf(outfile, "\n"); + fprintf(outfile, "%s%s%s: %s\n", ansi_esc("\033[1;34m"), nv->name, + ansi_escend(), nv->value); } } // namespace namespace { diff --git a/src/http2.cc b/src/http2.cc index fb72610a..1ffd6b71 100644 --- a/src/http2.cc +++ b/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); - fwrite(": ", 2, 1, out); - fwrite(nv.value.c_str(), nv.value.size(), 1, out); - fwrite("\n", 1, 1, out); + fprintf(out, "%s: %s\n", nv.name.c_str(), nv.value.c_str()); } fwrite("\n", 1, 1, out); fflush(out); diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index 98b9d8ec..974c5fd6 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/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.write(reinterpret_cast(nv.name), nv.namelen); - ss << TTY_RST << ": "; - ss.write(reinterpret_cast(nv.value), nv.valuelen); - ss << "\n"; + ss << TTY_HTTP_HD << nv.name << TTY_RST << ": " << nv.value << "\n"; } DCLOG(INFO, this) << "HTTP request headers\n" << ss.str(); } diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 2727e8b6..fb6ff799 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -1426,11 +1426,7 @@ Http2Upstream::log_response_headers(Downstream *downstream, const std::vector &nva) const { std::stringstream ss; for (auto &nv : nva) { - ss << TTY_HTTP_HD; - ss.write(reinterpret_cast(nv.name), nv.namelen); - ss << TTY_RST << ": "; - ss.write(reinterpret_cast(nv.value), nv.valuelen); - ss << "\n"; + ss << TTY_HTTP_HD << nv.name << TTY_RST << ": " << nv.value << "\n"; } ULOG(INFO, this) << "HTTP response headers. stream_id=" << downstream->get_stream_id() << "\n" << ss.str(); diff --git a/tests/nghttp2_hd_test.c b/tests/nghttp2_hd_test.c index d47da23e..027ac639 100644 --- a/tests/nghttp2_hd_test.c +++ b/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);