From 6ce4835eeafe5bcea91c9672fcd3f9ef68ebd472 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 29 Oct 2019 00:58:43 +0900 Subject: [PATCH] Fix the bug that stream is closed with wrong error code This commit fixes the bug that stream is closed with wrong error code (0). This happens when STREAM or DATA frame with END_STREAM flag set is received and it violates HTTP messaging rule (i.e., content-length does not match) and the other side of stream has been closed. In this case, nghttp2_on_stream_close_callback should be called with nonzero error code, but previously it is called with 0 (NO_ERROR). --- lib/nghttp2_session.c | 34 +++++++------ tests/nghttp2_session_test.c | 92 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 15 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index e3b00dcc..9df3d6f3 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3735,7 +3735,6 @@ static int session_end_stream_headers_received(nghttp2_session *session, static int session_after_header_block_received(nghttp2_session *session) { int rv = 0; - int call_cb = 1; nghttp2_frame *frame = &session->iframe.frame; nghttp2_stream *stream; @@ -3789,21 +3788,25 @@ static int session_after_header_block_received(nghttp2_session *session) { stream_id = frame->hd.stream_id; } - call_cb = 0; - rv = session_handle_invalid_stream2(session, stream_id, frame, NGHTTP2_ERR_HTTP_MESSAGING); if (nghttp2_is_fatal(rv)) { return rv; } + + if (frame->hd.type == NGHTTP2_HEADERS && + (frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) { + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + /* Don't call nghttp2_session_close_stream_if_shut_rdwr + because RST_STREAM has been submitted. */ + } + return 0; } } - if (call_cb) { - rv = session_call_on_frame_received(session, frame); - if (nghttp2_is_fatal(rv)) { - return rv; - } + rv = session_call_on_frame_received(session, frame); + if (nghttp2_is_fatal(rv)) { + return rv; } if (frame->hd.type != NGHTTP2_HEADERS) { @@ -4942,7 +4945,6 @@ static int session_process_extension_frame(nghttp2_session *session) { int nghttp2_session_on_data_received(nghttp2_session *session, nghttp2_frame *frame) { int rv = 0; - int call_cb = 1; nghttp2_stream *stream; /* We don't call on_frame_recv_callback if stream has been closed @@ -4958,20 +4960,22 @@ int nghttp2_session_on_data_received(nghttp2_session *session, if (session_enforce_http_messaging(session) && (frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) { if (nghttp2_http_on_remote_end_stream(stream) != 0) { - call_cb = 0; rv = nghttp2_session_add_rst_stream(session, stream->stream_id, NGHTTP2_PROTOCOL_ERROR); if (nghttp2_is_fatal(rv)) { return rv; } + + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + /* Don't call nghttp2_session_close_stream_if_shut_rdwr because + RST_STREAM has been submitted. */ + return 0; } } - if (call_cb) { - rv = session_call_on_frame_received(session, frame); - if (nghttp2_is_fatal(rv)) { - return rv; - } + rv = session_call_on_frame_received(session, frame); + if (nghttp2_is_fatal(rv)) { + return rv; } if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 0622b22a..b366a6aa 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -11288,6 +11288,8 @@ void test_nghttp2_http_content_length_mismatch(void) { MAKE_NV(":path", "/"), MAKE_NV(":method", "PUT"), MAKE_NV(":authority", "localhost"), MAKE_NV(":scheme", "https"), MAKE_NV("content-length", "20")}; + const nghttp2_nv cl_resnv[] = {MAKE_NV(":status", "200"), + MAKE_NV("content-length", "20")}; nghttp2_outbound_item *item; nghttp2_frame_hd hd; @@ -11365,7 +11367,97 @@ void test_nghttp2_http_content_length_mismatch(void) { nghttp2_session_del(session); + /* Check for client */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + nghttp2_hd_deflate_init(&deflater, mem); + + /* header says content-length: 20, but HEADERS has END_STREAM flag set */ + nghttp2_submit_request(session, NULL, reqnv, ARRLEN(reqnv), NULL, NULL); + + CU_ASSERT(0 == nghttp2_session_send(session)); + + rv = pack_headers(&bufs, &deflater, 1, + NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM, + cl_resnv, ARRLEN(cl_resnv), mem); + CU_ASSERT(0 == rv); + + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_buf_len(&bufs.head->buf)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(&bufs.head->buf) == rv); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + + CU_ASSERT(NULL != nghttp2_session_get_stream(session, 1)); + CU_ASSERT(0 == nghttp2_session_send(session)); + /* After sending RST_STREAM, stream must be closed */ + CU_ASSERT(NULL == nghttp2_session_get_stream(session, 1)); + + nghttp2_bufs_reset(&bufs); + + /* header says content-length: 20, but DATA has 0 byte */ + nghttp2_submit_request(session, NULL, reqnv, ARRLEN(reqnv), NULL, NULL); + + CU_ASSERT(0 == nghttp2_session_send(session)); + + rv = pack_headers(&bufs, &deflater, 3, NGHTTP2_FLAG_END_HEADERS, cl_resnv, + ARRLEN(cl_resnv), mem); + CU_ASSERT(0 == rv); + + nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_END_STREAM, 3); + nghttp2_frame_pack_frame_hd(bufs.head->buf.last, &hd); + bufs.head->buf.last += NGHTTP2_FRAME_HDLEN; + + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_buf_len(&bufs.head->buf)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(&bufs.head->buf) == rv); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + + CU_ASSERT(NULL != nghttp2_session_get_stream(session, 3)); + CU_ASSERT(0 == nghttp2_session_send(session)); + /* After sending RST_STREAM, stream must be closed */ + CU_ASSERT(NULL == nghttp2_session_get_stream(session, 3)); + + nghttp2_bufs_reset(&bufs); + + /* header says content-length: 20, but DATA has 21 bytes */ + nghttp2_submit_request(session, NULL, reqnv, ARRLEN(reqnv), NULL, NULL); + + CU_ASSERT(0 == nghttp2_session_send(session)); + + rv = pack_headers(&bufs, &deflater, 5, NGHTTP2_FLAG_END_HEADERS, cl_resnv, + ARRLEN(cl_resnv), mem); + CU_ASSERT(0 == rv); + + nghttp2_frame_hd_init(&hd, 21, NGHTTP2_DATA, NGHTTP2_FLAG_END_STREAM, 5); + nghttp2_frame_pack_frame_hd(bufs.head->buf.last, &hd); + bufs.head->buf.last += NGHTTP2_FRAME_HDLEN + 21; + + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_buf_len(&bufs.head->buf)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(&bufs.head->buf) == rv); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + + CU_ASSERT(NULL != nghttp2_session_get_stream(session, 5)); + CU_ASSERT(0 == nghttp2_session_send(session)); + /* After sending RST_STREAM, stream must be closed */ + CU_ASSERT(NULL == nghttp2_session_get_stream(session, 5)); + + nghttp2_bufs_reset(&bufs); + nghttp2_bufs_free(&bufs); + + nghttp2_hd_deflate_free(&deflater); + + nghttp2_session_del(session); } void test_nghttp2_http_non_final_response(void) {