From ddd04e8ced5449c127cc5bcf708d2c8c6947e3d1 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 27 Jul 2013 00:58:38 +0900 Subject: [PATCH] Fix header compression bug, and perform always incremental indexing --- lib/nghttp2_hd.c | 160 ++++++++++++++++------------------- lib/nghttp2_hd.h | 9 +- tests/nghttp2_frame_test.c | 5 ++ tests/nghttp2_hd_test.c | 17 ++-- tests/nghttp2_session_test.c | 8 ++ 5 files changed, 98 insertions(+), 101 deletions(-) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index d3ab15d4..b4dd1ac3 100644 --- a/lib/nghttp2_hd.c +++ b/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; + /* 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; + } + } + 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) { - if(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED) { - ws_ent->indexed.checked = 1; - } - } else { + 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,39 +785,34 @@ 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); - if(!new_ent) { - rv = NGHTTP2_ERR_HEADER_COMP; - goto fail; - } - rv = add_workingset(deflater, new_ent); - 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); - if(rv < 0) { - goto fail; - } + nghttp2_hd_entry *new_ent; + uint8_t index = ent->index; + 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; + } + 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); + 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; + } + rv = emit_newname_block(buf_ptr, buflen_ptr, &offset, &nv[i], 1); if(rv < 0) { goto fail; } @@ -819,17 +820,6 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater, } } } - 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); - 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); } diff --git a/lib/nghttp2_hd.h b/lib/nghttp2_hd.h index 09b630c4..06a6afb6 100644 --- a/lib/nghttp2_hd.h +++ b/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 */ diff --git a/tests/nghttp2_frame_test.c b/tests/nghttp2_frame_test.c index 271e81d0..b0f5db4c 100644 --- a/tests/nghttp2_frame_test.c +++ b/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, diff --git a/tests/nghttp2_hd_test.c b/tests/nghttp2_hd_test.c index ae219b77..4cc14be2 100644 --- a/tests/nghttp2_hd_test.c +++ b/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); diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 137ee2a7..bf848eef 100644 --- a/tests/nghttp2_session_test.c +++ b/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));