From 25bf567cd746e3f56e61dfc771f4487a4c222926 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 24 Oct 2015 17:27:24 +0900 Subject: [PATCH] Don't always expect dynamic table size update The encoder is not required to send dynamic table size update if the table size is not changed from the previous value after accepting new maximum value. --- lib/nghttp2_hd.c | 30 +++++++++++++--- lib/nghttp2_hd.h | 2 ++ tests/nghttp2_hd_test.c | 68 +++++++++++++++++++++++++++++++++++- tests/nghttp2_session_test.c | 18 ++++++---- 4 files changed, 105 insertions(+), 13 deletions(-) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index 6ffe59ac..1c0cb919 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -800,6 +800,7 @@ int nghttp2_hd_inflate_init(nghttp2_hd_inflater *inflater, nghttp2_mem *mem) { } inflater->settings_hd_table_bufsize_max = NGHTTP2_HD_DEFAULT_MAX_BUFFER_SIZE; + inflater->min_hd_table_bufsize_max = UINT32_MAX; inflater->ent_keep = NULL; inflater->nv_keep = NULL; @@ -1383,9 +1384,24 @@ int nghttp2_hd_inflate_change_table_size(nghttp2_hd_inflater *inflater, return NGHTTP2_ERR_INVALID_STATE; } - inflater->state = NGHTTP2_HD_STATE_EXPECT_TABLE_SIZE; + /* It seems that encoder is not required to send dynamic table size + update if the table size is not changed after applying + SETTINGS_HEADER_TABLE_SIZE. RFC 7541 is ambiguous here, but this + is the intention of the editor. If new maximum table size is + strictly smaller than the current negotiated maximum size, + encoder must send dynamic table size update. In other cases, we + cannot expect it to do so. */ + if (inflater->ctx.hd_table_bufsize_max > settings_hd_table_bufsize_max) { + inflater->state = NGHTTP2_HD_STATE_EXPECT_TABLE_SIZE; + /* Remember minimum value, and validate that encoder sends the + value less than or equal to this. */ + inflater->min_hd_table_bufsize_max = settings_hd_table_bufsize_max; + } + inflater->settings_hd_table_bufsize_max = settings_hd_table_bufsize_max; + inflater->ctx.hd_table_bufsize_max = settings_hd_table_bufsize_max; + hd_context_shrink_table_size(&inflater->ctx, NULL); return 0; } @@ -2136,8 +2152,10 @@ ssize_t nghttp2_hd_inflate_hd2(nghttp2_hd_inflater *inflater, break; case NGHTTP2_HD_STATE_READ_TABLE_SIZE: rfin = 0; - rv = hd_inflate_read_len(inflater, &rfin, in, last, 5, - inflater->settings_hd_table_bufsize_max); + rv = hd_inflate_read_len( + inflater, &rfin, in, last, 5, + nghttp2_min(inflater->min_hd_table_bufsize_max, + inflater->settings_hd_table_bufsize_max)); if (rv < 0) { goto fail; } @@ -2146,6 +2164,7 @@ ssize_t nghttp2_hd_inflate_hd2(nghttp2_hd_inflater *inflater, goto almost_ok; } DEBUGF(fprintf(stderr, "inflatehd: table_size=%zu\n", inflater->left)); + inflater->min_hd_table_bufsize_max = UINT32_MAX; inflater->ctx.hd_table_bufsize_max = inflater->left; hd_context_shrink_table_size(&inflater->ctx, NULL); inflater->state = NGHTTP2_HD_STATE_INFLATE_START; @@ -2403,7 +2422,8 @@ ssize_t nghttp2_hd_inflate_hd2(nghttp2_hd_inflater *inflater, if (in_final) { DEBUGF(fprintf(stderr, "inflatehd: in_final set\n")); - if (inflater->state != NGHTTP2_HD_STATE_OPCODE) { + if (inflater->state != NGHTTP2_HD_STATE_OPCODE && + inflater->state != NGHTTP2_HD_STATE_INFLATE_START) { DEBUGF(fprintf(stderr, "inflatehd: unacceptable state=%d\n", inflater->state)); rv = NGHTTP2_ERR_HEADER_COMP; @@ -2415,7 +2435,7 @@ ssize_t nghttp2_hd_inflate_hd2(nghttp2_hd_inflater *inflater, return (ssize_t)(in - first); almost_ok: - if (in_final && inflater->state != NGHTTP2_HD_STATE_OPCODE) { + if (in_final) { DEBUGF(fprintf(stderr, "inflatehd: input ended prematurely\n")); rv = NGHTTP2_ERR_HEADER_COMP; diff --git a/lib/nghttp2_hd.h b/lib/nghttp2_hd.h index 3d7fc593..c667888e 100644 --- a/lib/nghttp2_hd.h +++ b/lib/nghttp2_hd.h @@ -240,6 +240,8 @@ struct nghttp2_hd_inflater { /* The maximum header table size the inflater supports. This is the same value transmitted in SETTINGS_HEADER_TABLE_SIZE */ size_t settings_hd_table_bufsize_max; + /* Minimum header table size set by nghttp2_hd_inflate_change_table_size */ + size_t min_hd_table_bufsize_max; /* The number of next shift to decode integer */ size_t shift; nghttp2_hd_opcode opcode; diff --git a/tests/nghttp2_hd_test.c b/tests/nghttp2_hd_test.c index 50ed04a1..c71117ce 100644 --- a/tests/nghttp2_hd_test.c +++ b/tests/nghttp2_hd_test.c @@ -563,13 +563,79 @@ void test_nghttp2_hd_inflate_expect_table_size_update(void) { nghttp2_hd_inflate_init(&inflater, mem); /* This will make inflater require table size update in the next inflation. */ + nghttp2_hd_inflate_change_table_size(&inflater, 4095); nghttp2_hd_inflate_change_table_size(&inflater, 4096); CU_ASSERT(NGHTTP2_ERR_HEADER_COMP == inflate_hd(&inflater, &out, &bufs, 0, mem)); nva_out_reset(&out, mem); - nghttp2_bufs_free(&bufs); nghttp2_hd_inflate_free(&inflater); + + /* This does not require for encoder to emit table size update since + * size is not changed. */ + nghttp2_hd_inflate_init(&inflater, mem); + nghttp2_hd_inflate_change_table_size(&inflater, 4096); + CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == + inflate_hd(&inflater, &out, &bufs, 0, mem)); + + nva_out_reset(&out, mem); + nghttp2_hd_inflate_free(&inflater); + + /* This does not require for encodre to emit table size update since + new size is larger than current size. */ + nghttp2_hd_inflate_init(&inflater, mem); + nghttp2_hd_inflate_change_table_size(&inflater, 4097); + CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == + inflate_hd(&inflater, &out, &bufs, 0, mem)); + + nva_out_reset(&out, mem); + nghttp2_hd_inflate_free(&inflater); + + /* Received table size is strictly larger than minimum table size */ + nghttp2_hd_inflate_init(&inflater, mem); + nghttp2_hd_inflate_change_table_size(&inflater, 111); + nghttp2_hd_inflate_change_table_size(&inflater, 4096); + + nghttp2_bufs_reset(&bufs); + nghttp2_hd_emit_table_size(&bufs, 112); + + CU_ASSERT(NGHTTP2_ERR_HEADER_COMP == + inflate_hd(&inflater, &out, &bufs, 0, mem)); + + nva_out_reset(&out, mem); + nghttp2_hd_inflate_free(&inflater); + + /* Receiving 2 table size updates, min and last value */ + nghttp2_hd_inflate_init(&inflater, mem); + nghttp2_hd_inflate_change_table_size(&inflater, 111); + nghttp2_hd_inflate_change_table_size(&inflater, 4096); + + nghttp2_bufs_reset(&bufs); + nghttp2_hd_emit_table_size(&bufs, 111); + nghttp2_hd_emit_table_size(&bufs, 4096); + + CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == + inflate_hd(&inflater, &out, &bufs, 0, mem)); + + nva_out_reset(&out, mem); + nghttp2_hd_inflate_free(&inflater); + + /* 2nd update is larger than last value */ + nghttp2_hd_inflate_init(&inflater, mem); + nghttp2_hd_inflate_change_table_size(&inflater, 111); + nghttp2_hd_inflate_change_table_size(&inflater, 4095); + + nghttp2_bufs_reset(&bufs); + nghttp2_hd_emit_table_size(&bufs, 111); + nghttp2_hd_emit_table_size(&bufs, 4096); + + CU_ASSERT(NGHTTP2_ERR_HEADER_COMP == + inflate_hd(&inflater, &out, &bufs, 0, mem)); + + nva_out_reset(&out, mem); + nghttp2_hd_inflate_free(&inflater); + + nghttp2_bufs_free(&bufs); } void test_nghttp2_hd_inflate_unexpected_table_size_update(void) { diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index c2376b30..b3441f5c 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -4214,13 +4214,16 @@ void test_nghttp2_submit_settings(void) { iv[2].value = 50; iv[3].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; - iv[3].value = 0; + iv[3].value = 111; iv[4].settings_id = UNKNOWN_ID; iv[4].value = 999; - iv[5].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; - iv[5].value = (uint32_t)NGHTTP2_MAX_WINDOW_SIZE + 1; + iv[5].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[5].value = 1023; + + iv[6].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[6].value = (uint32_t)NGHTTP2_MAX_WINDOW_SIZE + 1; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = null_send_callback; @@ -4228,7 +4231,7 @@ void test_nghttp2_submit_settings(void) { nghttp2_session_server_new(&session, &callbacks, &ud); CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 6)); + nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 7)); /* Make sure that local settings are not changed */ CU_ASSERT(NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS == @@ -4237,14 +4240,14 @@ void test_nghttp2_submit_settings(void) { session->local_settings.initial_window_size); /* Now sends without 6th one */ - CU_ASSERT(0 == nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 5)); + CU_ASSERT(0 == nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 6)); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_SETTINGS == item->frame.hd.type); frame = &item->frame; - CU_ASSERT(5 == frame->settings.niv); + CU_ASSERT(6 == frame->settings.niv); CU_ASSERT(5 == frame->settings.iv[0].value); CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS == frame->settings.iv[0].settings_id); @@ -4267,7 +4270,8 @@ void test_nghttp2_submit_settings(void) { nghttp2_frame_settings_free(&ack_frame.settings, mem); CU_ASSERT(16 * 1024 == session->local_settings.initial_window_size); - CU_ASSERT(0 == session->hd_inflater.ctx.hd_table_bufsize_max); + CU_ASSERT(1023 == session->hd_inflater.ctx.hd_table_bufsize_max); + CU_ASSERT(111 == session->hd_inflater.min_hd_table_bufsize_max); CU_ASSERT(50 == session->local_settings.max_concurrent_streams); /* We just keep the last seen value */ CU_ASSERT(50 == session->pending_local_max_concurrent_stream);