diff --git a/lib/nghttp2_frame.c b/lib/nghttp2_frame.c index a2462d98..d365bb4f 100644 --- a/lib/nghttp2_frame.c +++ b/lib/nghttp2_frame.c @@ -703,17 +703,6 @@ nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv, return iv_copy; } -static int nghttp2_settings_entry_compar(const void *lhs, const void *rhs) -{ - return ((nghttp2_settings_entry *)lhs)->settings_id - -((nghttp2_settings_entry *)rhs)->settings_id; -} - -void nghttp2_frame_iv_sort(nghttp2_settings_entry *iv, size_t niv) -{ - qsort(iv, niv, sizeof(nghttp2_settings_entry), nghttp2_settings_entry_compar); -} - ssize_t nghttp2_frame_nv_offset(const uint8_t *head) { switch(head[2]) { @@ -831,18 +820,28 @@ ssize_t nghttp2_nv_array_from_cstr(nghttp2_nv **nva_ptr, const char **nv) return nvlen; } -int nghttp2_settings_check_duplicate(const nghttp2_settings_entry *iv, - size_t niv) +int nghttp2_iv_check(const nghttp2_settings_entry *iv, size_t niv, + int32_t flow_control_opt) { - int check[NGHTTP2_SETTINGS_MAX+1]; size_t i; - memset(check, 0, sizeof(check)); for(i = 0; i < niv; ++i) { - if(iv[i].settings_id > NGHTTP2_SETTINGS_MAX || iv[i].settings_id == 0 || - check[iv[i].settings_id] == 1) { - return 0; - } else { - check[iv[i].settings_id] = 1; + switch(iv[i].settings_id) { + case NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE: + if(iv[i].value > (uint32_t)NGHTTP2_MAX_WINDOW_SIZE) { + return 0; + } + break; + case NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS: + if(flow_control_opt) { + if((iv[i].value & 0x1) == 0) { + /* Attempt to re-enabling flow-control is error */ + return 0; + } + } else { + flow_control_opt = iv[i].value & 0x1; + } + default: + break; } } return 1; diff --git a/lib/nghttp2_frame.h b/lib/nghttp2_frame.h index 23301dad..3648b98f 100644 --- a/lib/nghttp2_frame.h +++ b/lib/nghttp2_frame.h @@ -573,13 +573,6 @@ char** nghttp2_frame_nv_norm_copy(const char **nv); nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv, size_t niv); -/* - * Sorts the |iv| with the ascending order of the settings_id member. - * The number of the element in the array pointed by the |iv| is given - * by the |niv|. - */ -void nghttp2_frame_iv_sort(nghttp2_settings_entry *iv, size_t niv); - /* * Returns the offset of the name/header block in the frame, including * frame header. The |head| is frame header. If the indicated frame @@ -639,11 +632,12 @@ int nghttp2_nv_array_check_null(nghttp2_nv *nva, size_t nvlen); /* * Checks that the |iv|, which includes |niv| entries, does not have - * duplicate IDs. + * invalid values. The |flow_control_opt| is current flow control + * option value. * * This function returns nonzero if it succeeds, or 0. */ -int nghttp2_settings_check_duplicate(const nghttp2_settings_entry *iv, - size_t niv); +int nghttp2_iv_check(const nghttp2_settings_entry *iv, size_t niv, + int32_t flow_control_opt); #endif /* NGHTTP2_FRAME_H */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index f1cbac35..cf18a393 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -2044,6 +2044,7 @@ int nghttp2_session_update_local_settings(nghttp2_session *session, session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS]; uint8_t new_flow_control = old_flow_control; int32_t new_initial_window_size = -1; + /* Use the value last seen. */ for(i = 0; i < niv; ++i) { if(iv[i].settings_id == NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE) { new_initial_window_size = iv[i].value; @@ -2061,8 +2062,9 @@ int nghttp2_session_update_local_settings(nghttp2_session *session, } } for(i = 0; i < niv; ++i) { - assert(iv[i].settings_id > 0 && iv[i].settings_id <= NGHTTP2_SETTINGS_MAX); - session->local_settings[iv[i].settings_id] = iv[i].value; + if(iv[i].settings_id > 0 && iv[i].settings_id <= NGHTTP2_SETTINGS_MAX) { + session->local_settings[iv[i].settings_id] = iv[i].value; + } } if(old_flow_control == 0 && session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS]) { @@ -2075,7 +2077,7 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, nghttp2_frame *frame) { int rv; - size_t i; + int i; int check[NGHTTP2_SETTINGS_MAX+1]; if(frame->hd.stream_id != 0) { return nghttp2_session_handle_invalid_connection(session, frame, @@ -2083,11 +2085,13 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, } /* Check ID/value pairs and persist them if necessary. */ memset(check, 0, sizeof(check)); - for(i = 0; i < frame->settings.niv; ++i) { + for(i = (int)frame->settings.niv - 1; i >= 0; --i) { nghttp2_settings_entry *entry = &frame->settings.iv[i]; - /* The spec says if the multiple values for the same ID were - found, use the first one and ignore the rest. */ - if(entry->settings_id > NGHTTP2_SETTINGS_MAX || entry->settings_id == 0 || + /* 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. */ + if(entry->settings_id > NGHTTP2_SETTINGS_MAX || entry->settings_id <= 0 || check[entry->settings_id] == 1) { continue; } diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 9216b138..32ffdd79 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -164,7 +164,9 @@ int nghttp2_submit_settings(nghttp2_session *session, nghttp2_frame *frame; nghttp2_settings_entry *iv_copy; int r; - if(!nghttp2_settings_check_duplicate(iv, niv)) { + if(!nghttp2_iv_check(iv, niv, + session->local_settings + [NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS])) { return NGHTTP2_ERR_INVALID_ARGUMENT; } frame = malloc(sizeof(nghttp2_frame)); @@ -176,7 +178,6 @@ int nghttp2_submit_settings(nghttp2_session *session, free(frame); return NGHTTP2_ERR_NOMEM; } - nghttp2_frame_iv_sort(iv_copy, niv); nghttp2_frame_settings_init(&frame->settings, iv_copy, niv); r = nghttp2_session_update_local_settings(session, iv_copy, niv); @@ -361,9 +362,10 @@ int nghttp2_submit_data(nghttp2_session *session, uint8_t flags, ssize_t nghttp2_pack_settings_payload(uint8_t *buf, nghttp2_settings_entry *iv, size_t niv) { - if(!nghttp2_settings_check_duplicate(iv, niv)) { + /* Assume that current flow_control_option is 0 (which means that + flow control is enabled) */ + if(!nghttp2_iv_check(iv, niv, 0)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } - nghttp2_frame_iv_sort(iv, niv); return nghttp2_frame_pack_settings_payload(buf, iv, niv); } diff --git a/tests/main.c b/tests/main.c index 900d74a1..52f23ac0 100644 --- a/tests/main.c +++ b/tests/main.c @@ -218,8 +218,7 @@ int main(int argc, char* argv[]) test_nghttp2_frame_pack_window_update) || !CU_add_test(pSuite, "nv_array_from_cstr", test_nghttp2_nv_array_from_cstr) || - !CU_add_test(pSuite, "settings_check_duplicate", - test_nghttp2_settings_check_duplicate) || + !CU_add_test(pSuite, "iv_check", test_nghttp2_iv_check) || !CU_add_test(pSuite, "hd_deflate", test_nghttp2_hd_deflate) || !CU_add_test(pSuite, "hd_inflate_indname_inc", test_nghttp2_hd_inflate_indname_inc) || diff --git a/tests/nghttp2_frame_test.c b/tests/nghttp2_frame_test.c index 6085a9c5..8d4cc7fa 100644 --- a/tests/nghttp2_frame_test.c +++ b/tests/nghttp2_frame_test.c @@ -427,26 +427,28 @@ void test_nghttp2_nv_array_from_cstr(void) free(bigval); } -void test_nghttp2_settings_check_duplicate(void) +void test_nghttp2_iv_check(void) { - nghttp2_settings_entry set[3]; + nghttp2_settings_entry iv[5]; - set[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - set[0].value = 1; - set[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; - set[1].value = 1023; + iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; + iv[0].value = 100; + iv[1].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS; + iv[1].value = 0; + iv[2].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS; + iv[2].value = 1; - CU_ASSERT(nghttp2_settings_check_duplicate(set, 2)); + CU_ASSERT(nghttp2_iv_check(iv, 2, 0)); + CU_ASSERT(nghttp2_iv_check(iv, 3, 0)); + /* Re-enabling flow-control*/ + CU_ASSERT(0 == nghttp2_iv_check(iv, 2, 1)); + CU_ASSERT(0 == nghttp2_iv_check(iv, 3, 1)); - set[1] = set[0]; - CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 2)); + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = NGHTTP2_MAX_WINDOW_SIZE; + CU_ASSERT(nghttp2_iv_check(iv, 2, 0)); - /* Out-of-bound data is error */ - set[0].settings_id = NGHTTP2_SETTINGS_MAX + 1; - CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 1)); - - /* settings_id == 0 is error */ - set[0].settings_id = 0; - CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 1)); + /* Too large window size */ + iv[1].value = (uint32_t)NGHTTP2_MAX_WINDOW_SIZE + 1; + CU_ASSERT(0 == nghttp2_iv_check(iv, 2, 0)); } - diff --git a/tests/nghttp2_frame_test.h b/tests/nghttp2_frame_test.h index 8649a2d3..7eb52e62 100644 --- a/tests/nghttp2_frame_test.h +++ b/tests/nghttp2_frame_test.h @@ -38,6 +38,6 @@ void test_nghttp2_frame_pack_ping(void); void test_nghttp2_frame_pack_goaway(void); void test_nghttp2_frame_pack_window_update(void); void test_nghttp2_nv_array_from_cstr(void); -void test_nghttp2_settings_check_duplicate(void); +void test_nghttp2_iv_check(void); #endif /* NGHTTP2_FRAME_TEST_H */ diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 72a854de..42bd983b 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -881,10 +881,10 @@ void test_nghttp2_session_on_settings_received(void) nghttp2_settings_entry iv[255]; iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - iv[0].value = 1000000009; + iv[0].value = 50; iv[1].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - iv[1].value = 50; + iv[1].value = 1000000009; iv[2].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; iv[2].value = 64*1024; @@ -1900,10 +1900,10 @@ void test_nghttp2_submit_settings(void) my_user_data ud; nghttp2_outbound_item *item; nghttp2_frame *frame; - nghttp2_settings_entry iv[4]; + nghttp2_settings_entry iv[5]; iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - iv[0].value = 50; + iv[0].value = 5; iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; iv[1].value = 16*1024; @@ -1911,9 +1911,12 @@ void test_nghttp2_submit_settings(void) iv[2].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS; iv[2].value = 1; - /* This is duplicate entry */ iv[3].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - iv[3].value = 150; + iv[3].value = 50; + + /* Attempt to re-enable flow-control */ + iv[4].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS; + iv[4].value = 0; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = null_send_callback; @@ -1921,7 +1924,7 @@ void test_nghttp2_submit_settings(void) nghttp2_session_server_new(&session, &callbacks, &ud); CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_submit_settings(session, iv, 4)); + nghttp2_submit_settings(session, iv, 5)); /* Make sure that local settings are not changed */ CU_ASSERT(NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS == @@ -1931,8 +1934,8 @@ void test_nghttp2_submit_settings(void) CU_ASSERT(0 == session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS]); - /* Now sends without 4th one */ - CU_ASSERT(0 == nghttp2_submit_settings(session, iv, 3)); + /* Now sends without 5th one */ + CU_ASSERT(0 == nghttp2_submit_settings(session, iv, 4)); /* Make sure that local settings are changed */ CU_ASSERT(50 == @@ -1948,8 +1951,8 @@ void test_nghttp2_submit_settings(void) CU_ASSERT(NGHTTP2_SETTINGS == OB_CTRL_TYPE(item)); frame = item->frame; - CU_ASSERT(3 == frame->settings.niv); - CU_ASSERT(50 == frame->settings.iv[0].value); + CU_ASSERT(4 == frame->settings.niv); + CU_ASSERT(5 == frame->settings.iv[0].value); CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS == frame->settings.iv[0].settings_id); @@ -3224,10 +3227,10 @@ void test_nghttp2_pack_settings_payload(void) CU_ASSERT(0 == nghttp2_frame_unpack_settings_payload(&resiv, &resniv, buf, len)); CU_ASSERT(2 == resniv); - CU_ASSERT(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE == resiv[0].settings_id); - CU_ASSERT(4095 == resiv[0].value); - CU_ASSERT(NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS == resiv[1].settings_id); - CU_ASSERT(1 == resiv[1].value); + CU_ASSERT(NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS == resiv[0].settings_id); + CU_ASSERT(1 == resiv[0].value); + CU_ASSERT(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE == resiv[1].settings_id); + CU_ASSERT(4095 == resiv[1].value); free(resiv); }