From a804117c83b0df36dc0467475651a58b03b1dad0 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 7 Jan 2015 22:53:43 +0900 Subject: [PATCH] Fix GOAWAY handling On reception of GOAWAY, new stream creation is disallowed regardless of last-stream-id in GOAWAY is larger than next stream ID. --- lib/nghttp2_session.c | 60 +++++++++++++++--------------------- lib/nghttp2_session.h | 4 +++ tests/nghttp2_session_test.c | 10 +++--- 3 files changed, 32 insertions(+), 42 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 60c7aaf0..809d0993 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1248,11 +1248,10 @@ static int stream_predicate_for_send(nghttp2_stream *stream) { */ static int session_predicate_request_headers_send(nghttp2_session *session, nghttp2_headers *frame) { - /* If we are terminating session (session->goaway_flag is nonzero), - we don't send new request. Also if received last_stream_id is - strictly less than new stream ID, cancel its transmission. */ - if (session->goaway_flags || - session->remote_last_stream_id < frame->hd.stream_id) { + /* If we are terminating session (NGHTTP2_GOAWAY_TERM_ON_SEND) or + GOAWAY was received from peer, new request is not allowed. */ + if (session->goaway_flags & + (NGHTTP2_GOAWAY_TERM_ON_SEND | NGHTTP2_GOAWAY_RECV)) { return NGHTTP2_ERR_START_STREAM_NOT_ALLOWED; } return 0; @@ -1429,7 +1428,8 @@ static int session_predicate_push_promise_send(nghttp2_session *session, if (stream->state == NGHTTP2_STREAM_CLOSING) { return NGHTTP2_ERR_STREAM_CLOSING; } - if (session->remote_last_stream_id < promised_stream_id) { + if (session->goaway_flags & + (NGHTTP2_GOAWAY_TERM_ON_SEND | NGHTTP2_GOAWAY_RECV)) { return NGHTTP2_ERR_START_STREAM_NOT_ALLOWED; } return 0; @@ -2345,6 +2345,8 @@ static int session_after_frame_sent1(nghttp2_session *session) { session->goaway_flags |= NGHTTP2_GOAWAY_TERM_SENT; } + session->goaway_flags |= NGHTTP2_GOAWAY_SENT; + rv = session_close_stream_on_goaway(session, frame->goaway.last_stream_id, 1); @@ -3227,8 +3229,8 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, } session->last_recv_stream_id = frame->hd.stream_id; - if (session->local_last_stream_id < frame->hd.stream_id) { - /* We just ignore stream rather than local_last_stream_id */ + if (session->goaway_flags & NGHTTP2_GOAWAY_SENT) { + /* We just ignore stream after GOAWAY was queued */ return NGHTTP2_ERR_IGN_HEADER_BLOCK; } @@ -4038,6 +4040,8 @@ int nghttp2_session_on_goaway_received(nghttp2_session *session, "GOAWAY: invalid last_stream_id"); } + session->goaway_flags |= NGHTTP2_GOAWAY_RECV; + session->remote_last_stream_id = frame->goaway.last_stream_id; rv = session_call_on_frame_received(session, frame); @@ -5615,10 +5619,9 @@ static size_t session_get_num_active_streams(nghttp2_session *session) { int nghttp2_session_want_read(nghttp2_session *session) { size_t num_active_streams; - /* If these flags are set, we don't want to read. The application + /* If this flag is set, we don't want to read. The application should drop the connection. */ - if ((session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND) && - (session->goaway_flags & NGHTTP2_GOAWAY_TERM_SENT)) { + if (session->goaway_flags & NGHTTP2_GOAWAY_TERM_SENT) { return 0; } @@ -5631,26 +5634,18 @@ int nghttp2_session_want_read(nghttp2_session *session) { return 1; } - /* If there is no active streams, we check last_stream_id peer sent - to us. Here is the asymmetry between server and client. For - server, if client cannot make new request, current conneciton is - no use. Server push might be going, but it is idempotent and can - be safely retried with the new connection. For client side, the - theory is the same. */ - if (session->server) { - return session->local_last_stream_id > session->last_recv_stream_id; - } else { - return (uint32_t)session->remote_last_stream_id >= session->next_stream_id; - } + /* If there is no active streams and GOAWAY has been sent or + received, we are done with this session. */ + return (session->goaway_flags & + (NGHTTP2_GOAWAY_SENT | NGHTTP2_GOAWAY_RECV)) == 0; } int nghttp2_session_want_write(nghttp2_session *session) { size_t num_active_streams; - /* If these flags are set, we don't want to write any data. The + /* If these flag is set, we don't want to write any data. The application should drop the connection. */ - if ((session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND) && - (session->goaway_flags & NGHTTP2_GOAWAY_TERM_SENT)) { + if (session->goaway_flags & NGHTTP2_GOAWAY_TERM_SENT) { return 0; } @@ -5675,17 +5670,10 @@ int nghttp2_session_want_write(nghttp2_session *session) { return 1; } - /* If there is no active streams, we check last_stream_id peer sent - to us. Here is the asymmetry between server and client. For - server, if client cannot make new request, current conneciton is - no use. Server push might be going, but it is idempotent and can - be safely retried with the new connection. For client side, the - theory is the same. */ - if (session->server) { - return session->local_last_stream_id > session->last_recv_stream_id; - } else { - return (uint32_t)session->remote_last_stream_id >= session->next_stream_id; - } + /* If there is no active streams and GOAWAY has been sent or + received, we are done with this session. */ + return (session->goaway_flags & + (NGHTTP2_GOAWAY_SENT | NGHTTP2_GOAWAY_RECV)) == 0; } int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 08577527..ff7a42cf 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -130,6 +130,10 @@ typedef enum { NGHTTP2_GOAWAY_TERM_ON_SEND = 0x1, /* Flag means GOAWAY to terminate session has been sent */ NGHTTP2_GOAWAY_TERM_SENT = 0x2, + /* Flag means GOAWAY was sent */ + NGHTTP2_GOAWAY_SENT = 0x4, + /* Flag means GOAWAY was received */ + NGHTTP2_GOAWAY_RECV = 0x8, } nghttp2_goaway_flag; struct nghttp2_session { diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 61e3b0f0..ee554a9c 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1756,11 +1756,11 @@ void test_nghttp2_session_on_request_headers_received(void) { nghttp2_frame_headers_free(&frame.headers, mem); - /* Stream ID which is greater than local_last_stream_id is - ignored */ + /* If GOAWAY has been sent, new stream is ignored */ nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 5, NGHTTP2_HCAT_REQUEST, NULL, NULL, 0); + session->goaway_flags |= NGHTTP2_GOAWAY_SENT; user_data.invalid_frame_recv_cb_called = 0; CU_ASSERT(NGHTTP2_ERR_IGN_HEADER_BLOCK == nghttp2_session_on_request_headers_received(session, &frame)); @@ -4847,10 +4847,8 @@ void test_nghttp2_session_on_ctrl_not_send(void) { NULL, 0, NULL)); user_data.frame_not_send_cb_called = 0; - /* Terminating session */ - CU_ASSERT(0 == nghttp2_session_add_goaway(session, 0, NGHTTP2_NO_ERROR, NULL, - 0, 1)); - + /* GOAWAY received */ + session->goaway_flags |= NGHTTP2_GOAWAY_RECV; session->next_stream_id = 9; CU_ASSERT(0 < nghttp2_submit_headers(session, NGHTTP2_FLAG_END_STREAM, -1,