From f74674aa7faa755194a69df68c7bc6efd93b198c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 27 Jul 2013 18:59:30 +0900 Subject: [PATCH] Fix header compression (again) --- lib/nghttp2_hd.c | 105 ++++++++++++++---------- lib/nghttp2_hd.h | 5 +- tests/main.c | 2 + tests/nghttp2_hd_test.c | 173 ++++++++++++++++++++++++++++++++++++++++ tests/nghttp2_hd_test.h | 1 + 5 files changed, 244 insertions(+), 42 deletions(-) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index b4dd1ac3..80dcef42 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -292,6 +292,7 @@ static nghttp2_hd_entry* add_hd_table_incremental(nghttp2_hd_context *context, context->hd_table_bufsize > NGHTTP2_HD_MAX_BUFFER_SIZE; ++i) { nghttp2_hd_entry *ent = context->hd_table[i]; context->hd_table_bufsize -= entry_room(ent->nv.namelen, ent->nv.valuelen); + ent->index = NGHTTP2_HD_INVALID_INDEX; if(--ent->ref == 0) { nghttp2_hd_entry_free(ent); free(ent); @@ -340,12 +341,12 @@ static nghttp2_hd_entry* add_hd_table_subst(nghttp2_hd_context *context, for(i = 0; i < context->hd_tablelen && context->hd_table_bufsize > NGHTTP2_HD_MAX_BUFFER_SIZE; ++i, --k) { nghttp2_hd_entry *ent = context->hd_table[i]; - --ent->ref; if(i != subindex) { context->hd_table_bufsize -= entry_room(ent->nv.namelen, ent->nv.valuelen); } - if(ent->ref == 0) { + ent->index = NGHTTP2_HD_INVALID_INDEX; + if(--ent->ref == 0) { nghttp2_hd_entry_free(ent); free(ent); } @@ -369,6 +370,7 @@ static nghttp2_hd_entry* add_hd_table_subst(nghttp2_hd_context *context, } if(k >= 0) { nghttp2_hd_entry *ent = context->hd_table[k]; + ent->index = NGHTTP2_HD_INVALID_INDEX; if(--ent->ref == 0) { nghttp2_hd_entry_free(ent); free(ent); @@ -397,7 +399,6 @@ static int add_workingset(nghttp2_hd_context *context, nghttp2_hd_entry *ent) ws_ent->cat = NGHTTP2_HD_CAT_INDEXED; ws_ent->indexed.entry = ent; ws_ent->indexed.index = ent->index; - ws_ent->indexed.checked = 1; ++ent->ref; return 0; } @@ -478,6 +479,28 @@ static nghttp2_hd_ws_entry* find_in_workingset_by_index return NULL; } +static size_t remove_from_workingset_by_index +(nghttp2_hd_context *context, size_t index) +{ + size_t i; + size_t res = 0; + for(i = 0; i < context->wslen; ++i) { + nghttp2_hd_ws_entry *ws_ent = &context->ws[i]; + /* Compare against *frozen* index, not the current header table + index. */ + if(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED && + ws_ent->indexed.index == index) { + ++res; + if(--ws_ent->indexed.entry->ref == 0) { + nghttp2_hd_entry_free(ws_ent->indexed.entry); + free(ws_ent->indexed.entry); + } + ws_ent->cat = NGHTTP2_HD_CAT_NONE; + } + } + return res; +} + static nghttp2_hd_entry* find_in_hd_table(nghttp2_hd_context *context, nghttp2_nv *nv) { @@ -523,20 +546,20 @@ static size_t count_encoded_length(size_t n, int prefix) { size_t k = (1 << prefix) - 1; size_t len = 0; - if(n > k) { + if(n >= k) { n -= k; ++len; } else { return 1; } - while(n) { + do { ++len; if(n >= 128) { n >>= 7; } else { break; } - } + } while(n); return len; } @@ -544,7 +567,7 @@ static size_t encode_length(uint8_t *buf, size_t n, int prefix) { size_t k = (1 << prefix) - 1; size_t len = 0; - if(n > k) { + if(n >= k) { *buf++ = k; n -= k; ++len; @@ -552,7 +575,7 @@ static size_t encode_length(uint8_t *buf, size_t n, int prefix) *buf++ = n; return 1; } - while(n) { + do { ++len; if(n >= 128) { *buf++ = (1 << 7) | (n & 0x7f); @@ -561,7 +584,7 @@ static size_t encode_length(uint8_t *buf, size_t n, int prefix) *buf++ = n; break; } - } + } while(n); return len; } @@ -602,7 +625,7 @@ static uint8_t* decode_length(ssize_t *res, uint8_t *in, uint8_t *last, } if(*in & (1 << 7)) { *res = -1; - return 0; + return NULL; } else { return in + 1; } @@ -632,7 +655,7 @@ static int emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr, { int rv; uint8_t *bufp; - size_t blocklen = count_encoded_length(index, 5) + + size_t blocklen = count_encoded_length(index + 1, 5) + count_encoded_length(valuelen, 8) + valuelen; rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen); if(rv != 0) { @@ -643,6 +666,7 @@ static int emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr, bufp += encode_length(bufp, valuelen, 8); memcpy(bufp, value, valuelen); (*buf_ptr)[*offset_ptr] |= inc_indexing ? 0x40u : 0x60u; + assert(bufp+valuelen - (*buf_ptr + *offset_ptr) == (ssize_t)blocklen); *offset_ptr += blocklen; return 0; } @@ -726,7 +750,6 @@ static void create_workingset(nghttp2_hd_context *context) ent->cat = NGHTTP2_HD_CAT_INDEXED; ent->indexed.entry = context->refset[i]; ent->indexed.index = ent->indexed.entry->index; - ent->indexed.checked = 0; context->refset[i] = NULL; } context->wslen = context->refsetlen; @@ -749,30 +772,34 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater, 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; - } + int found = 0; + assert(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED); + for(j = 0; j < nvlen; ++j) { + if(nghttp2_nv_equal(&ws_ent->indexed.entry->nv, &nv[j])) { + found = 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; - } + } + if(!found) { + rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset, + ws_ent->indexed.index); + if(rv < 0) { + goto fail; } + if(--ws_ent->indexed.entry->ref == 0) { + nghttp2_hd_entry_free(ws_ent->indexed.entry); + free(ws_ent->indexed.entry); + } + ws_ent->cat = NGHTTP2_HD_CAT_NONE; } } for(i = 0; i < nvlen; ++i) { - nghttp2_hd_ws_entry *ws_ent; - ws_ent = find_in_workingset(deflater, &nv[i]); - if(!ws_ent) { + if(!find_in_workingset(deflater, &nv[i])) { nghttp2_hd_entry *ent; ent = find_in_hd_table(deflater, &nv[i]); - if(ent) { - /* If nv[i] is found in hd_table, use Indexed Header repr */ + if(ent && find_in_workingset_by_index(deflater, ent->index) == NULL) { + /* If nv[i] is found in hd_table and its index is not shadowed + by working set, use Indexed Header repr */ rv = add_workingset(deflater, ent); if(rv < 0) { goto fail; @@ -847,7 +874,6 @@ static ssize_t build_nv_array(nghttp2_hd_context *inflater, switch(ent->cat) { case NGHTTP2_HD_CAT_INDEXED: *nv = ent->indexed.entry->nv; - ent->indexed.checked = 1; ++nv; break; case NGHTTP2_HD_CAT_INDNAME: @@ -883,22 +909,15 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, uint8_t c = *in; if(c & 0x80u) { /* Indexed Header Repr */ - nghttp2_hd_ws_entry *ws_ent; ssize_t index; in = decode_length(&index, in, last, 7); if(index < 0) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; } - ws_ent = find_in_workingset_by_index(inflater, index); - if(ws_ent) { - assert(ws_ent->cat == NGHTTP2_HD_CAT_INDEXED); - if(--ws_ent->indexed.entry->ref == 0) { - nghttp2_hd_entry_free(ws_ent->indexed.entry); - free(ws_ent->indexed.entry); - } - ws_ent->cat = NGHTTP2_HD_CAT_NONE; - } else { + /* If nothing was removed, add entry from header table to + workingset */ + if(remove_from_workingset_by_index(inflater, index) == 0) { nghttp2_hd_entry *ent; if(inflater->hd_tablelen <= index) { rv = NGHTTP2_ERR_HEADER_COMP; @@ -1099,12 +1118,16 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, int nghttp2_hd_end_headers(nghttp2_hd_context *context) { int i; + uint8_t checks[128]; + memset(checks, 0, sizeof(checks)); assert(context->refsetlen == 0); for(i = 0; i < context->wslen; ++i) { nghttp2_hd_ws_entry *ws_ent = &context->ws[i]; switch(ws_ent->cat) { case NGHTTP2_HD_CAT_INDEXED: - if(ws_ent->indexed.checked) { + if(ws_ent->indexed.entry->index != NGHTTP2_HD_INVALID_INDEX && + checks[ws_ent->indexed.entry->index] == 0) { + checks[ws_ent->indexed.entry->index] = 1; context->refset[context->refsetlen++] = ws_ent->indexed.entry; } else { if(--ws_ent->indexed.entry->ref == 0) { diff --git a/lib/nghttp2_hd.h b/lib/nghttp2_hd.h index 06a6afb6..6216b4d4 100644 --- a/lib/nghttp2_hd.h +++ b/lib/nghttp2_hd.h @@ -38,6 +38,10 @@ #define NGHTTP2_HD_MAX_BUFFER_SIZE 4096 #define NGHTTP2_HD_ENTRY_OVERHEAD 32 +/* This value is sensible to NGHTTP2_HD_MAX_BUFFER_SIZE. Currently, + the index is at most 128, so 255 is good choice */ +#define NGHTTP2_HD_INVALID_INDEX 255 + typedef enum { NGHTTP2_HD_SIDE_CLIENT = 0, NGHTTP2_HD_SIDE_SERVER = 1 @@ -74,7 +78,6 @@ typedef struct nghttp2_hd_ws_entry { struct { nghttp2_hd_entry *entry; uint8_t index; - uint8_t checked; } indexed; /* For NGHTTP2_HD_CAT_NEWNAME */ struct { diff --git a/tests/main.c b/tests/main.c index 5619130e..ca87916f 100644 --- a/tests/main.c +++ b/tests/main.c @@ -220,6 +220,8 @@ int main(int argc, char* argv[]) test_nghttp2_hd_inflate_indname_subst_eviction_neg) || !CU_add_test(pSuite, "hd_inflate_newname_subst", test_nghttp2_hd_inflate_newname_subst) || + !CU_add_test(pSuite, "hd_deflate_inflate", + test_nghttp2_hd_deflate_inflate) || !CU_add_test(pSuite, "gzip_inflate", test_nghttp2_gzip_inflate) ) { CU_cleanup_registry(); diff --git a/tests/nghttp2_hd_test.c b/tests/nghttp2_hd_test.c index 4cc14be2..5f3910bd 100644 --- a/tests/nghttp2_hd_test.c +++ b/tests/nghttp2_hd_test.c @@ -34,6 +34,7 @@ #define MAKE_NV(NAME, VALUE) \ { (uint8_t*)NAME, (uint8_t*)VALUE, strlen(NAME), strlen(VALUE) } +#define ARRLEN(ARR) (sizeof(ARR)/sizeof(ARR[0])) static void assert_nv_equal(nghttp2_nv *a, nghttp2_nv *b, size_t len) { @@ -326,3 +327,175 @@ void test_nghttp2_hd_inflate_newname_subst(void) free(buf); nghttp2_hd_inflate_free(&inflater); } + +static void check_deflate_inflate(nghttp2_hd_context *deflater, + nghttp2_hd_context *inflater, + nghttp2_nv *nva, size_t nvlen) +{ + uint8_t *buf = NULL; + size_t buflen = 0; + ssize_t blocklen; + nghttp2_nv *resnva; + ssize_t resnvlen; + + blocklen = nghttp2_hd_deflate_hd(deflater, &buf, &buflen, 0, nva, nvlen); + assert(blocklen >= 0); + nghttp2_hd_end_headers(deflater); + resnvlen = nghttp2_hd_inflate_hd(inflater, &resnva, buf, blocklen); + CU_ASSERT(resnvlen == (ssize_t)nvlen); + assert_nv_equal(nva, resnva, nvlen); + nghttp2_hd_end_headers(inflater); +} + +void test_nghttp2_hd_deflate_inflate(void) +{ + nghttp2_hd_context inflater, deflater; + nghttp2_nv nv1[] = { + MAKE_NV(":status", "200 OK"), + MAKE_NV("access-control-allow-origin", "*"), + MAKE_NV("cache-control", "private, max-age=0, must-revalidate"), + MAKE_NV("content-length", "76073"), + MAKE_NV("content-type", "text/html"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("server", "Apache"), + MAKE_NV("vary", "foobar"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "MISS from alphabravo"), + MAKE_NV("x-cache-action", "MISS"), + MAKE_NV("x-cache-age", "0"), + MAKE_NV("x-cache-lookup", "MISS from alphabravo:3128"), + MAKE_NV("x-lb-nocache", "true"), + }; + nghttp2_nv nv2[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=56682045"), + MAKE_NV("content-type", "text/css"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Thu, 14 May 2015 07:22:57 GMT"), + MAKE_NV("last-modified", "Tue, 14 May 2013 07:22:15 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128") + }; + nghttp2_nv nv3[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=56682072"), + MAKE_NV("content-type", "text/css"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Thu, 14 May 2015 07:23:24 GMT"), + MAKE_NV("last-modified", "Tue, 14 May 2013 07:22:13 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv4[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=56682022"), + MAKE_NV("content-type", "text/css"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Thu, 14 May 2015 07:22:34 GMT"), + MAKE_NV("last-modified", "Tue, 14 May 2013 07:22:14 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv5[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=4461139"), + MAKE_NV("content-type", "application/x-javascript"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Mon, 16 Sep 2013 21:34:31 GMT"), + MAKE_NV("last-modified", "Thu, 05 May 2011 09:15:59 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv6[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=18645951"), + MAKE_NV("content-type", "application/x-javascript"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Fri, 28 Feb 2014 01:48:03 GMT"), + MAKE_NV("last-modified", "Tue, 12 Jul 2011 16:02:59 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv7[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=31536000"), + MAKE_NV("content-type", "application/javascript"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("etag", "\"6807-4dc5b54e0dcc0\""), + MAKE_NV("expires", "Wed, 21 May 2014 08:32:17 GMT"), + MAKE_NV("last-modified", "Fri, 10 May 2013 11:18:51 GMT"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv8[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=31536000"), + MAKE_NV("content-type", "application/javascript"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("etag", "\"41c6-4de7d28585b00\""), + MAKE_NV("expires", "Thu, 12 Jun 2014 10:00:58 GMT"), + MAKE_NV("last-modified", "Thu, 06 Jun 2013 14:30:36 GMT"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv9[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=31536000"), + MAKE_NV("content-type", "application/javascript"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("etag", "\"19d6e-4dc5b35a541c0\""), + MAKE_NV("expires", "Wed, 21 May 2014 08:32:18 GMT"), + MAKE_NV("last-modified", "Fri, 10 May 2013 11:10:07 GMT"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + nghttp2_nv nv10[] = { + MAKE_NV(":status", "304 Not Modified"), + MAKE_NV("age", "0"), + MAKE_NV("cache-control", "max-age=56682045"), + MAKE_NV("content-type", "text/css"), + MAKE_NV("date", "Sat, 27 Jul 2013 06:22:12 GMT"), + MAKE_NV("expires", "Thu, 14 May 2015 07:22:57 GMT"), + MAKE_NV("last-modified", "Tue, 14 May 2013 07:21:53 GMT"), + MAKE_NV("vary", "Accept-Encoding"), + MAKE_NV("via", "1.1 alphabravo (squid/3.x.x), 1.1 nghttpx"), + MAKE_NV("x-cache", "HIT from alphabravo"), + MAKE_NV("x-cache-lookup", "HIT from alphabravo:3128"), + }; + + nghttp2_hd_deflate_init(&deflater, NGHTTP2_HD_SIDE_SERVER); + nghttp2_hd_inflate_init(&inflater, NGHTTP2_HD_SIDE_CLIENT); + + check_deflate_inflate(&deflater, &inflater, nv1, ARRLEN(nv1)); + check_deflate_inflate(&deflater, &inflater, nv2, ARRLEN(nv2)); + check_deflate_inflate(&deflater, &inflater, nv3, ARRLEN(nv3)); + check_deflate_inflate(&deflater, &inflater, nv4, ARRLEN(nv4)); + check_deflate_inflate(&deflater, &inflater, nv5, ARRLEN(nv5)); + check_deflate_inflate(&deflater, &inflater, nv6, ARRLEN(nv6)); + check_deflate_inflate(&deflater, &inflater, nv7, ARRLEN(nv7)); + check_deflate_inflate(&deflater, &inflater, nv8, ARRLEN(nv8)); + check_deflate_inflate(&deflater, &inflater, nv9, ARRLEN(nv9)); + check_deflate_inflate(&deflater, &inflater, nv10, ARRLEN(nv10)); +} diff --git a/tests/nghttp2_hd_test.h b/tests/nghttp2_hd_test.h index 31b486a2..133ff1a7 100644 --- a/tests/nghttp2_hd_test.h +++ b/tests/nghttp2_hd_test.h @@ -33,5 +33,6 @@ void test_nghttp2_hd_inflate_indname_subst(void); void test_nghttp2_hd_inflate_indname_subst_eviction(void); void test_nghttp2_hd_inflate_indname_subst_eviction_neg(void); void test_nghttp2_hd_inflate_newname_subst(void); +void test_nghttp2_hd_deflate_inflate(void); #endif /* NGHTTP2_HD_TEST_H */