walkero
/
nghttp2
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix GOAWAY race with new incoming stream on server side 

Revert part of 16c46114dc 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).
This commit is contained in:
Tatsuhiro Tsujikawa 2016-09-09 22:08:34 +09:00
parent 8103f43b65
commit 2c0afa00aa
2 changed files with 11 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
lib/nghttp2_session.c
View File

@ -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;

13
tests/nghttp2_session_test.c
View File

@ -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);