From 9f8fc7b2bb5614886f4eea6dacb22f95246356a9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 17 Dec 2015 21:30:30 +0900 Subject: [PATCH] Strict error handling for frames which are not allowed after closed (remote) This makes h2spec strict mode a bit happier. We still one failing test with h2spec -S (strict mode). --- lib/nghttp2_session.c | 41 ++++++++++------- tests/main.c | 2 + tests/nghttp2_session_test.c | 86 +++++++++++++++++++++++++++++++++--- tests/nghttp2_session_test.h | 1 + 4 files changed, 107 insertions(+), 23 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 43474b96..98b75bb9 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3443,6 +3443,12 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, "request HEADERS: invalid stream_id"); } + stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed"); + } + return NGHTTP2_ERR_IGN_HEADER_BLOCK; } session->last_recv_stream_id = frame->hd.stream_id; @@ -3721,6 +3727,9 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session, } } + /* We may use stream->shut_flags for strict error checking. */ + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + rv = session_call_on_frame_received(session, frame); if (rv != 0) { return rv; @@ -4165,7 +4174,11 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: stream in idle"); } + + /* Currently, client does not retain closed stream, so we don't + check NGHTTP2_SHUT_RD condition here. */ } + rv = nghttp2_session_add_rst_stream( session, frame->push_promise.promised_stream_id, NGHTTP2_CANCEL); if (rv != 0) { @@ -4173,25 +4186,13 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, } return NGHTTP2_ERR_IGN_HEADER_BLOCK; } + if (stream->shut_flags & NGHTTP2_SHUT_RD) { - if (session->callbacks.on_invalid_frame_recv_callback) { - if (session->callbacks.on_invalid_frame_recv_callback( - session, frame, NGHTTP2_PROTOCOL_ERROR, session->user_data) != - 0) { - return NGHTTP2_ERR_CALLBACK_FAILURE; - } - } - rv = nghttp2_session_add_rst_stream(session, - frame->push_promise.promised_stream_id, - NGHTTP2_PROTOCOL_ERROR); - if (rv != 0) { - return rv; - } - return NGHTTP2_ERR_IGN_HEADER_BLOCK; + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, + "PUSH_PROMISE: stream closed"); } - /* TODO It is unclear reserved stream dpeneds on associated - stream with or without exclusive flag set */ nghttp2_priority_spec_init(&pri_spec, stream->stream_id, NGHTTP2_DEFAULT_WEIGHT, 0); @@ -4632,6 +4633,14 @@ static int session_on_data_received_fail_fast(nghttp2_session *session) { error_code = NGHTTP2_PROTOCOL_ERROR; goto fail; } + + stream = nghttp2_session_get_stream_raw(session, stream_id); + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { + failure_reason = "DATA: stream closed"; + error_code = NGHTTP2_STREAM_CLOSED; + goto fail; + } + return NGHTTP2_ERR_IGN_PAYLOAD; } if (stream->shut_flags & NGHTTP2_SHUT_RD) { diff --git a/tests/main.c b/tests/main.c index 711088a0..4aabe0d0 100644 --- a/tests/main.c +++ b/tests/main.c @@ -128,6 +128,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_on_window_update_received) || !CU_add_test(pSuite, "session_on_data_received", test_nghttp2_session_on_data_received) || + !CU_add_test(pSuite, "session_on_data_received_fail_fast", + test_nghttp2_session_on_data_received_fail_fast) || !CU_add_test(pSuite, "session_send_headers_start_stream", test_nghttp2_session_send_headers_start_stream) || !CU_add_test(pSuite, "session_send_headers_reply", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 7b503056..f4abbb9d 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -2249,6 +2249,25 @@ void test_nghttp2_session_on_request_headers_received(void) { nghttp2_frame_headers_free(&frame.headers, mem); nghttp2_session_del(session); + + nghttp2_session_server_new(&session, &callbacks, &user_data); + + /* HEADERS to closed stream */ + stream = open_stream(session, 1); + session->last_recv_stream_id = 1; + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR); + + nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 1, + NGHTTP2_HCAT_REQUEST, NULL, NULL, 0); + + CU_ASSERT(NGHTTP2_ERR_IGN_HEADER_BLOCK == + nghttp2_session_on_request_headers_received(session, &frame)); + CU_ASSERT(session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND); + + nghttp2_frame_headers_free(&frame.headers, mem); + + nghttp2_session_del(session); } void test_nghttp2_session_on_response_headers_received(void) { @@ -2742,14 +2761,20 @@ void test_nghttp2_session_on_push_promise_received(void) { CU_ASSERT(1 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 4)); item = nghttp2_session_get_next_ob_item(session); - CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); - CU_ASSERT(4 == item->frame.hd.stream_id); - CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.rst_stream.error_code); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + CU_ASSERT(NGHTTP2_STREAM_CLOSED == item->frame.goaway.error_code); CU_ASSERT(0 == nghttp2_session_send(session)); CU_ASSERT(4 == session->last_recv_stream_id); + nghttp2_session_del(session); + + nghttp2_session_client_new(&session, &callbacks, &user_data); + + stream = nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_OPENING, NULL); + /* Attempt to PUSH_PROMISE against stream in closing state */ - stream->shut_flags = NGHTTP2_SHUT_NONE; stream->state = NGHTTP2_STREAM_CLOSING; frame.push_promise.promised_stream_id = 6; @@ -2759,7 +2784,7 @@ void test_nghttp2_session_on_push_promise_received(void) { nghttp2_session_on_push_promise_received(session, &frame)); CU_ASSERT(0 == user_data.begin_headers_cb_called); - CU_ASSERT(1 == session->num_incoming_reserved_streams); + CU_ASSERT(0 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 6)); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); @@ -2767,7 +2792,7 @@ void test_nghttp2_session_on_push_promise_received(void) { CU_ASSERT(NGHTTP2_CANCEL == item->frame.rst_stream.error_code); CU_ASSERT(0 == nghttp2_session_send(session)); - /* Attempt to PUSH_PROMISE against non-existent stream */ + /* Attempt to PUSH_PROMISE against idle stream */ frame.hd.stream_id = 3; frame.push_promise.promised_stream_id = 8; @@ -2777,7 +2802,7 @@ void test_nghttp2_session_on_push_promise_received(void) { nghttp2_session_on_push_promise_received(session, &frame)); CU_ASSERT(0 == user_data.begin_headers_cb_called); - CU_ASSERT(1 == session->num_incoming_reserved_streams); + CU_ASSERT(0 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 8)); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); @@ -3161,6 +3186,53 @@ void test_nghttp2_session_on_data_received(void) { nghttp2_session_del(session); } +void test_nghttp2_session_on_data_received_fail_fast(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + uint8_t buf[9]; + nghttp2_stream *stream; + nghttp2_frame_hd hd; + nghttp2_outbound_item *item; + + memset(&callbacks, 0, sizeof(callbacks)); + + nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_NONE, 1); + nghttp2_frame_pack_frame_hd(buf, &hd); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + /* DATA to closed (remote) */ + stream = open_stream(session, 1); + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + + CU_ASSERT((ssize_t)sizeof(buf) == + nghttp2_session_mem_recv(session, buf, sizeof(buf))); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NULL != item); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + + nghttp2_session_del(session); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + /* DATA to closed stream with explicit closed (remote) */ + stream = open_stream(session, 1); + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR); + + CU_ASSERT((ssize_t)sizeof(buf) == + nghttp2_session_mem_recv(session, buf, sizeof(buf))); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NULL != item); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + + nghttp2_session_del(session); +} + void test_nghttp2_session_send_headers_start_stream(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index e834b32e..ea22e76b 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -54,6 +54,7 @@ void test_nghttp2_session_on_ping_received(void); void test_nghttp2_session_on_goaway_received(void); void test_nghttp2_session_on_window_update_received(void); void test_nghttp2_session_on_data_received(void); +void test_nghttp2_session_on_data_received_fail_fast(void); void test_nghttp2_session_send_headers_start_stream(void); void test_nghttp2_session_send_headers_reply(void); void test_nghttp2_session_send_headers_frame_size_error(void);