From 7bfa276e96d6f0b1418891370a51ccc9c6de7389 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 24 Aug 2014 00:50:55 +0900 Subject: [PATCH] Fix bug that NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE causes session failure Previously returning NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from on_header_callback moves input offset badly and it causes header decompression error on the subsequent frames. This commit fix this bug. --- lib/nghttp2_session.c | 13 ++++-- tests/main.c | 2 + tests/nghttp2_session_test.c | 81 ++++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index c16d02b4..35af88ba 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -2656,6 +2656,9 @@ static int inflate_header_block(nghttp2_session *session, in += proclen; inlen -= proclen; *readlen_ptr += proclen; + + DEBUGF(fprintf(stderr, "recv: proclen=%zd\n", proclen)); + if(call_header_cb && (inflate_flags & NGHTTP2_HD_INFLATE_EMIT)) { rv = session_call_on_header(session, frame, &nv); /* This handles NGHTTP2_ERR_PAUSE and @@ -4866,13 +4869,13 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, return in - first; } - in += readlen; - iframe->payloadleft -= readlen; - if(rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { /* The application says no more headers. We decompress the rest of the header block but not invoke on_header_callback and on_frame_recv_callback. */ + in += hd_proclen; + iframe->payloadleft -= hd_proclen; + rv = nghttp2_session_add_rst_stream(session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR); @@ -4883,6 +4886,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; break; } + + in += readlen; + iframe->payloadleft -= readlen; + if(rv == NGHTTP2_ERR_HEADER_COMP) { /* GOAWAY is already issued */ if(iframe->payloadleft == 0) { diff --git a/tests/main.c b/tests/main.c index 6db64cbd..d5c5cb0c 100644 --- a/tests/main.c +++ b/tests/main.c @@ -233,6 +233,8 @@ int main(int argc, char* argv[]) test_nghttp2_session_keep_closed_stream) || !CU_add_test(pSuite, "session_graceful_shutdown", test_nghttp2_session_graceful_shutdown) || + !CU_add_test(pSuite, "session_on_header_temporal_failure", + test_nghttp2_session_on_header_temporal_failure) || !CU_add_test(pSuite, "frame_pack_headers", test_nghttp2_frame_pack_headers) || !CU_add_test(pSuite, "frame_pack_headers_frame_too_large", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 0e3b13bc..3d4092f1 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -319,6 +319,19 @@ static int pause_on_header_callback(nghttp2_session *session, return NGHTTP2_ERR_PAUSE; } +static int temporal_failure_on_header_callback +(nghttp2_session *session, + const nghttp2_frame *frame, + const uint8_t *name, size_t namelen, + const uint8_t *value, size_t valuelen, + uint8_t flags, + void *user_data) +{ + on_header_callback(session, frame, name, namelen, value, valuelen, flags, + user_data); + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; +} + static int on_begin_headers_callback(nghttp2_session *session, const nghttp2_frame *frame, void *user_data) @@ -6074,3 +6087,71 @@ void test_nghttp2_session_graceful_shutdown(void) nghttp2_session_del(session); } + +void test_nghttp2_session_on_header_temporal_failure(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + my_user_data ud; + nghttp2_bufs bufs; + nghttp2_buf *buf; + nghttp2_hd_deflater deflater; + nghttp2_nv nv[] = { + MAKE_NV("alpha", "bravo"), + MAKE_NV("charlie", "delta") + }; + nghttp2_nv *nva; + size_t hdpos; + ssize_t rv; + nghttp2_frame frame; + nghttp2_frame_hd hd; + nghttp2_outbound_item *item; + + memset(&callbacks, 0, sizeof(callbacks)); + callbacks.on_header_callback = temporal_failure_on_header_callback; + + nghttp2_session_server_new(&session, &callbacks, &ud); + + frame_pack_bufs_init(&bufs); + + nghttp2_hd_deflate_init(&deflater); + + nghttp2_nv_array_copy(&nva, nv, 1); + + nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_STREAM, 1, + NGHTTP2_HCAT_REQUEST, NULL, nva, 1); + nghttp2_frame_pack_headers(&bufs, &frame.headers, &deflater); + nghttp2_frame_headers_free(&frame.headers); + + /* We are going to create CONTINUATION. First serialize header + block, and then frame header. */ + hdpos = nghttp2_bufs_len(&bufs); + + buf = &bufs.head->buf; + buf->last += NGHTTP2_FRAME_HDLEN; + + nghttp2_hd_deflate_hd_bufs(&deflater, &bufs, &nv[1], 1); + + hd.length = nghttp2_bufs_len(&bufs) - hdpos - NGHTTP2_FRAME_HDLEN; + hd.type = NGHTTP2_CONTINUATION; + hd.flags = NGHTTP2_FLAG_END_HEADERS; + hd.stream_id = 1; + + nghttp2_frame_pack_frame_hd(&buf->pos[hdpos], &hd); + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_bufs_len(&bufs)); + + CU_ASSERT(rv == nghttp2_bufs_len(&bufs)); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NGHTTP2_RST_STREAM == OB_CTRL_TYPE(item)); + + /* Make sure no header decompression error occurred */ + CU_ASSERT(NGHTTP2_GOAWAY_NONE == session->goaway_flags); + + nghttp2_bufs_free(&bufs); + + nghttp2_hd_deflate_free(&deflater); + nghttp2_session_del(session); +} diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 0cdd7394..e11f56ff 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -109,5 +109,6 @@ void test_nghttp2_session_stream_attach_data(void); void test_nghttp2_session_stream_attach_data_subtree(void); void test_nghttp2_session_keep_closed_stream(void); void test_nghttp2_session_graceful_shutdown(void); +void test_nghttp2_session_on_header_temporal_failure(void); #endif /* NGHTTP2_SESSION_TEST_H */