From 55c697e9f4edda42c2a036b93103e064a97614a5 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 15 Jul 2014 00:25:31 +0900 Subject: [PATCH] Handle multiple SETTINGS_HEADER_TABLE_SIZE in incoming SETTINGS frame Previously we just assumed that if same settings ID is found in SETTINGS, it is enough to process last seen entry. But it turns out it is not enough for SETTINGS_HEADER_TABLE_SIZE. If we have 0 and 4096 for SETTINGS_HEADER_TABLE_SIZE in one SETTINGS, we must first shrink dynamic table to 0 and then enlarge it to 4096. This means that we have to remember the minimum value and last value. --- lib/includes/nghttp2/nghttp2.h | 2 + lib/nghttp2_session.c | 62 +++++----- lib/nghttp2_session.h | 7 +- tests/main.c | 2 + tests/nghttp2_session_test.c | 207 +++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 6 files changed, 249 insertions(+), 32 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 3a2a328c..bc3ae658 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -516,6 +516,8 @@ typedef enum { */ NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE = 0x04 } nghttp2_settings_id; +/* Note: If we add SETTINGS, update the capacity of + NGHTTP2_INBOUND_NUM_IV as well */ /** * @macro diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index a3dcf969..b2a00a49 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -282,6 +282,9 @@ static void session_inbound_frame_reset(nghttp2_session *session) iframe->payloadleft = 0; iframe->padlen = 0; iframe->headers_payload_length = 0; + iframe->iv[NGHTTP2_INBOUND_NUM_IV - 1].settings_id = + NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iframe->iv[NGHTTP2_INBOUND_NUM_IV - 1].value = UINT32_MAX; } static void init_settings(nghttp2_settings_storage *settings) @@ -3299,11 +3302,7 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, int noack) { int rv; - int i; - int header_table_size_seen = 0; - int enable_push_seen = 0; - int max_concurrent_streams_seen = 0; - int initial_window_size_seen = 0; + size_t i; if(frame->hd.stream_id != 0) { return session_handle_invalid_connection @@ -3338,20 +3337,11 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, return session_call_on_frame_received(session, frame); } - for(i = (int)frame->settings.niv - 1; i >= 0; --i) { + for(i = 0; i < frame->settings.niv; ++i) { nghttp2_settings_entry *entry = &frame->settings.iv[i]; - /* The spec says the settings values are processed in the order - they appear in the payload. In other words, if the multiple - values for the same ID were found, use the last one and ignore - the rest. */ switch(entry->settings_id) { case NGHTTP2_SETTINGS_HEADER_TABLE_SIZE: - if(header_table_size_seen) { - break; - } - - header_table_size_seen = 1; if(entry->value > NGHTTP2_MAX_HEADER_TABLE_SIZE) { return session_handle_invalid_connection @@ -3374,11 +3364,6 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, break; case NGHTTP2_SETTINGS_ENABLE_PUSH: - if(enable_push_seen) { - break; - } - - enable_push_seen = 1; if(entry->value != 0 && entry->value != 1) { return session_handle_invalid_connection @@ -3396,21 +3381,11 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, break; case NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS: - if(max_concurrent_streams_seen) { - break; - } - - max_concurrent_streams_seen = 1; session->remote_settings.max_concurrent_streams = entry->value; break; case NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE: - if(initial_window_size_seen) { - break; - } - - initial_window_size_seen = 1; /* Update the initial window size of the all active streams */ /* Check that initial_window_size < (1u << 31) */ @@ -3458,6 +3433,27 @@ static int session_process_settings_frame(nghttp2_session *session) int rv; nghttp2_inbound_frame *iframe = &session->iframe; nghttp2_frame *frame = &iframe->frame; + size_t i; + nghttp2_settings_entry min_header_size_entry; + + min_header_size_entry = iframe->iv[NGHTTP2_INBOUND_NUM_IV - 1]; + + if(min_header_size_entry.value < UINT32_MAX) { + /* If we have less value, then we must have + SETTINGS_HEADER_TABLE_SIZE in i < iframe->niv */ + for(i = 0; i < iframe->niv; ++i) { + if(iframe->iv[i].settings_id == NGHTTP2_SETTINGS_HEADER_TABLE_SIZE) { + break; + } + } + + assert(i < iframe->niv); + + if(min_header_size_entry.value != iframe->iv[i].value) { + iframe->iv[iframe->niv++] = iframe->iv[i]; + iframe->iv[i] = min_header_size_entry; + } + } rv = nghttp2_frame_unpack_settings_payload(&frame->settings, iframe->iv, iframe->niv); @@ -4115,6 +4111,12 @@ static void inbound_frame_set_settings_entry(nghttp2_inbound_frame *iframe) if(i == iframe->niv) { iframe->iv[iframe->niv++] = iv; } + + if(iv.settings_id == NGHTTP2_SETTINGS_HEADER_TABLE_SIZE && + iv.value < iframe->iv[NGHTTP2_INBOUND_NUM_IV - 1].value) { + + iframe->iv[NGHTTP2_INBOUND_NUM_IV - 1] = iv; + } } /* diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 53885615..d9d6094a 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -81,6 +81,8 @@ typedef enum { NGHTTP2_IB_IGN_DATA } nghttp2_inbound_state; +#define NGHTTP2_INBOUND_NUM_IV 5 + typedef struct { nghttp2_frame frame; /* Storage for extension frame payload. frame->ext.payload points @@ -88,8 +90,9 @@ typedef struct { nghttp2_ext_frame_payload ext_frame_payload; /* The received SETTINGS entry. The protocol says that we only cares about the defined settings ID. If unknown ID is received, it is - subject to connection error */ - nghttp2_settings_entry iv[5]; + ignored. We use last entry to hold minimum header table size if + same settings are multiple times. */ + nghttp2_settings_entry iv[NGHTTP2_INBOUND_NUM_IV]; /* buffer pointers to small buffer, raw_sbuf */ nghttp2_buf sbuf; /* buffer pointers to large buffer, raw_lbuf */ diff --git a/tests/main.c b/tests/main.c index 0fda14a1..bb26c08f 100644 --- a/tests/main.c +++ b/tests/main.c @@ -93,6 +93,8 @@ int main(int argc, char* argv[]) test_nghttp2_session_recv_unknown_frame) || !CU_add_test(pSuite, "session_recv_unexpected_continuation", test_nghttp2_session_recv_unexpected_continuation) || + !CU_add_test(pSuite, "session_recv_settings_header_table_size", + test_nghttp2_session_recv_settings_header_table_size) || !CU_add_test(pSuite, "session_continue", test_nghttp2_session_continue) || !CU_add_test(pSuite, "session_add_frame", test_nghttp2_session_add_frame) || diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 308c3e1c..88df64bf 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1231,6 +1231,182 @@ void test_nghttp2_session_recv_unexpected_continuation(void) nghttp2_session_del(session); } +void test_nghttp2_session_recv_settings_header_table_size(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_frame frame; + nghttp2_bufs bufs; + nghttp2_buf *buf; + ssize_t rv; + my_user_data ud; + nghttp2_settings_entry iv[3]; + nghttp2_nv nv = MAKE_NV(":authority", "example.org"); + + frame_pack_bufs_init(&bufs); + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.on_frame_recv_callback = on_frame_recv_callback; + callbacks.send_callback = null_send_callback; + + nghttp2_session_client_new(&session, &callbacks, &ud); + + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 3000; + + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = 16384; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, + dup_iv(iv, 2), 2); + + rv = nghttp2_frame_pack_settings(&bufs, &frame.settings); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_settings_free(&frame.settings); + + buf = &bufs.head->buf; + assert(nghttp2_bufs_len(&bufs) == nghttp2_buf_len(buf)); + + ud.frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + + CU_ASSERT(rv == nghttp2_buf_len(buf)); + CU_ASSERT(1 == ud.frame_recv_cb_called); + + CU_ASSERT(3000 == session->remote_settings.header_table_size); + CU_ASSERT(16384 == session->remote_settings.initial_window_size); + + nghttp2_bufs_reset(&bufs); + + /* 2 SETTINGS_HEADER_TABLE_SIZE */ + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 3001; + + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = 16383; + + iv[2].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[2].value = 3001; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, + dup_iv(iv, 3), 3); + + rv = nghttp2_frame_pack_settings(&bufs, &frame.settings); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_settings_free(&frame.settings); + + buf = &bufs.head->buf; + assert(nghttp2_bufs_len(&bufs) == nghttp2_buf_len(buf)); + + ud.frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + + CU_ASSERT(rv == nghttp2_buf_len(buf)); + CU_ASSERT(1 == ud.frame_recv_cb_called); + + CU_ASSERT(3001 == session->remote_settings.header_table_size); + CU_ASSERT(16383 == session->remote_settings.initial_window_size); + + nghttp2_bufs_reset(&bufs); + + /* 2 SETTINGS_HEADER_TABLE_SIZE; first entry clears dynamic header + table. */ + + nghttp2_submit_request(session, NULL, &nv, 1, NULL, NULL); + nghttp2_session_send(session); + + CU_ASSERT(0 < session->hd_deflater.ctx.hd_table.len); + + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 0; + + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = 16382; + + iv[2].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[2].value = 4096; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, + dup_iv(iv, 3), 3); + + rv = nghttp2_frame_pack_settings(&bufs, &frame.settings); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_settings_free(&frame.settings); + + buf = &bufs.head->buf; + assert(nghttp2_bufs_len(&bufs) == nghttp2_buf_len(buf)); + + ud.frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + + CU_ASSERT(rv == nghttp2_buf_len(buf)); + CU_ASSERT(1 == ud.frame_recv_cb_called); + + CU_ASSERT(4096 == session->remote_settings.header_table_size); + CU_ASSERT(16382 == session->remote_settings.initial_window_size); + CU_ASSERT(0 == session->hd_deflater.ctx.hd_table.len); + + nghttp2_bufs_reset(&bufs); + + /* 2 SETTINGS_HEADER_TABLE_SIZE; second entry clears dynamic header + table. */ + + nghttp2_submit_request(session, NULL, &nv, 1, NULL, NULL); + nghttp2_session_send(session); + + CU_ASSERT(0 < session->hd_deflater.ctx.hd_table.len); + + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 3000; + + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = 16381; + + iv[2].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[2].value = 0; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, + dup_iv(iv, 3), 3); + + rv = nghttp2_frame_pack_settings(&bufs, &frame.settings); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_settings_free(&frame.settings); + + buf = &bufs.head->buf; + assert(nghttp2_bufs_len(&bufs) == nghttp2_buf_len(buf)); + + ud.frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + + CU_ASSERT(rv == nghttp2_buf_len(buf)); + CU_ASSERT(1 == ud.frame_recv_cb_called); + + CU_ASSERT(0 == session->remote_settings.header_table_size); + CU_ASSERT(16381 == session->remote_settings.initial_window_size); + CU_ASSERT(0 == session->hd_deflater.ctx.hd_table.len); + + nghttp2_bufs_reset(&bufs); + + nghttp2_bufs_free(&bufs); + nghttp2_session_del(session); +} + void test_nghttp2_session_continue(void) { nghttp2_session *session; @@ -1868,6 +2044,7 @@ void test_nghttp2_session_on_settings_received(void) const size_t niv = 5; nghttp2_settings_entry iv[255]; nghttp2_outbound_item *item; + nghttp2_nv nv = MAKE_NV(":authority", "example.org"); iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; iv[0].value = 50; @@ -1885,6 +2062,8 @@ void test_nghttp2_session_on_settings_received(void) iv[4].value = 0; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.send_callback = null_send_callback; + nghttp2_session_client_new(&session, &callbacks, &user_data); session->remote_settings.initial_window_size = 16*1024; @@ -1952,6 +2131,34 @@ void test_nghttp2_session_on_settings_received(void) nghttp2_frame_settings_free(&frame.settings); nghttp2_session_del(session); + + /* Check that 2 SETTINGS_HEADER_TABLE_SIZE 0 and 4096 are included + and header table size is once cleared to 0. */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + nghttp2_submit_request(session, NULL, &nv, 1, NULL, NULL); + + nghttp2_session_send(session); + + CU_ASSERT(session->hd_deflater.ctx.hd_table.len > 0); + + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 0; + + iv[1].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[1].value = 2048; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, + dup_iv(iv, 2), 2); + + CU_ASSERT(0 == nghttp2_session_on_settings_received(session, &frame, 0)); + + CU_ASSERT(0 == session->hd_deflater.ctx.hd_table.len); + CU_ASSERT(2048 == session->hd_deflater.ctx.hd_table_bufsize_max); + CU_ASSERT(2048 == session->remote_settings.header_table_size); + + nghttp2_frame_settings_free(&frame.settings); + nghttp2_session_del(session); } void test_nghttp2_session_on_push_promise_received(void) diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index f9fbbd4f..57fb9695 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -36,6 +36,7 @@ void test_nghttp2_session_recv_premature_headers(void); void test_nghttp2_session_recv_altsvc(void); void test_nghttp2_session_recv_unknown_frame(void); void test_nghttp2_session_recv_unexpected_continuation(void); +void test_nghttp2_session_recv_settings_header_table_size(void); void test_nghttp2_session_continue(void); void test_nghttp2_session_add_frame(void); void test_nghttp2_session_on_request_headers_received(void);