From 2c0afa00aaa265609fdb9ba45cdf8b61595a8c8c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 9 Sep 2016 22:08:34 +0900 Subject: [PATCH] Fix GOAWAY race with new incoming stream on server side Revert part of 16c46114dc724278beaa6d59462f8396f35fa4a9 to fix race condition that incoming stream after sending GOAWAY causes connection error. The strict stream handling introduced in the above commit does not handle several cases well (e.g., GOAWAY race, and refusing streams because of concurrency limit). --- lib/nghttp2_session.c | 69 ++++-------------------------------- tests/nghttp2_session_test.c | 13 +++---- 2 files changed, 11 insertions(+), 71 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 0d8734c4..20109325 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3834,52 +3834,13 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, * implementation since client is not wrong if it did not get * RST_STREAM when it issued trailer HEADERS. * - * For server session, we remember closed streams as long as the - * sum of closed streams and opened streams are under current max - * concurrent streams. We can use these closed streams to detect - * the error in some cases. - * - * If the stream cannot be found in either closed or opened - * streams, it is considered to be closed, or it has not exist - * (e.g., peer skipped sending the stream). Actually, it is - * impossible to detect which is which, since that information was - * lost forever. For these cases, we send back GOAWAY with - * PROTOCOL_ERROR. - * - * If the stream is found, and we know that it is in half closed - * (remote), or closed by peer's explicit action (e.g., received - * RST_STREAM from peer, or peer sends HEADERS/DATA frame with - * END_STREAM), getting new frame on that stream is clearly error. - * In this case, we send GOAWAY with error code STREAM_CLOSED. - * - * There is one corner case here. Server can change the max - * concurrent streams. The initial value of max concurrent - * streams is unlimited (NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS, - * which is UINT32_MAX). When sending out SETTINGS with - * MAX_CONCURRENT_STREAMS, we save its value as pending max - * concurrent streams, and use it as a cap to remember closed - * stream to save memory. This means that we might not sure that - * stream surely closed or has not exist when it is not found in - * closed or opened stream. To workaround this issue, we ignore - * incoming frame if the current max concurrent streams is - * NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS, and pending max - * concurrent streams is less than that. + * At the moment, we are very conservative here. We only use + * connection error if stream ID refers idle stream, or we are + * sure that stream is half-closed(remote) or closed. Otherwise + * we just ignore HEADERS for now. */ stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); - - if (!stream) { - if (session->local_settings.max_concurrent_streams == - NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS && - session->pending_local_max_concurrent_stream < - NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS) { - return NGHTTP2_ERR_IGN_HEADER_BLOCK; - } - - return session_inflate_handle_invalid_connection( - session, frame, NGHTTP2_ERR_PROTO, "HEADERS: stream does not exist"); - } - - if (stream->shut_flags & NGHTTP2_SHUT_RD) { + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed"); } @@ -5153,25 +5114,7 @@ static int session_on_data_received_fail_fast(nghttp2_session *session) { stream = nghttp2_session_get_stream(session, stream_id); if (!stream) { stream = nghttp2_session_get_stream_raw(session, stream_id); - - if (!stream) { - if (session->server) { - if (session->local_settings.max_concurrent_streams == - NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS && - session->pending_local_max_concurrent_stream < - NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS) { - return NGHTTP2_ERR_IGN_PAYLOAD; - } - - failure_reason = "DATA: stream does not exist"; - error_code = NGHTTP2_PROTOCOL_ERROR; - goto fail; - } - - return NGHTTP2_ERR_IGN_PAYLOAD; - } - - if (stream->shut_flags & NGHTTP2_SHUT_RD) { + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { failure_reason = "DATA: stream closed"; error_code = NGHTTP2_STREAM_CLOSED; goto fail; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 95117092..3f97a6db 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -9883,8 +9883,9 @@ void test_nghttp2_session_removed_closed_stream(void) { prepare_session_removed_closed_stream(session, &deflater); - /* Now current max concurrent streams is 2, so receiving frame on - stream 3 is treated as connection error */ + /* Now current max concurrent streams is 2. Receiving frame on + stream 3 is ignored because we have no stream object for stream + 3. */ nghttp2_bufs_reset(&bufs); rv = pack_headers(&bufs, &deflater, 3, NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM, @@ -9899,9 +9900,7 @@ void test_nghttp2_session_removed_closed_stream(void) { item = nghttp2_session_get_next_ob_item(session); - CU_ASSERT(NULL != item); - CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); - CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.goaway.error_code); + CU_ASSERT(NULL == item); nghttp2_hd_deflate_free(&deflater); nghttp2_session_del(session); @@ -9924,9 +9923,7 @@ void test_nghttp2_session_removed_closed_stream(void) { item = nghttp2_session_get_next_ob_item(session); - CU_ASSERT(NULL != item); - CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); - CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.goaway.error_code); + CU_ASSERT(NULL == item); nghttp2_hd_deflate_free(&deflater); nghttp2_session_del(session);