From 92a56d034f201cbb609606184822cf1716677207 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 16:38:30 +0900 Subject: [PATCH] Fix bug that idle/closed stream may be destroyed while it is referenced --- lib/nghttp2_session.c | 133 ++++++++++++++++++++++------------- lib/nghttp2_session.h | 39 +++++----- tests/nghttp2_session_test.c | 26 ++++--- 3 files changed, 121 insertions(+), 77 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d9baa253..f94a009a 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -753,6 +753,10 @@ int nghttp2_session_add_item(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't have to call nghttp2_session_adjust_closed_stream() + here, since stream->stream_id is local stream_id, and it does + not affect closed stream count. */ + nghttp2_outbound_queue_push(&session->ob_reg, item); item->queued = 1; @@ -884,15 +888,6 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, return NULL; } } else { - if (session->server && initial_state != NGHTTP2_STREAM_IDLE && - !nghttp2_session_is_my_stream_id(session, stream_id)) { - - rv = nghttp2_session_adjust_closed_stream(session, 1); - if (rv != 0) { - return NULL; - } - } - stream = nghttp2_mem_malloc(mem, sizeof(nghttp2_stream)); if (stream == NULL) { return NULL; @@ -970,10 +965,7 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, /* Idle stream does not count toward the concurrent streams limit. This is used as anchor node in dependency tree. */ assert(session->server); - rv = nghttp2_session_keep_idle_stream(session, stream); - if (rv != 0) { - return NULL; - } + nghttp2_session_keep_idle_stream(session, stream); break; default: if (nghttp2_session_is_my_stream_id(session, stream_id)) { @@ -1078,7 +1070,9 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, /* On server side, retain stream at most MAX_CONCURRENT_STREAMS combined with the current active incoming streams to make dependency tree work better. */ - rv = nghttp2_session_keep_closed_stream(session, stream); + nghttp2_session_keep_closed_stream(session, stream); + + rv = nghttp2_session_adjust_closed_stream(session); } else { rv = nghttp2_session_destroy_stream(session, stream); } @@ -1113,10 +1107,8 @@ int nghttp2_session_destroy_stream(nghttp2_session *session, return 0; } -int nghttp2_session_keep_closed_stream(nghttp2_session *session, - nghttp2_stream *stream) { - int rv; - +void nghttp2_session_keep_closed_stream(nghttp2_session *session, + nghttp2_stream *stream) { DEBUGF(fprintf(stderr, "stream: keep closed stream(%p)=%d, state=%d\n", stream, stream->stream_id, stream->state)); @@ -1129,19 +1121,10 @@ int nghttp2_session_keep_closed_stream(nghttp2_session *session, session->closed_stream_tail = stream; ++session->num_closed_streams; - - rv = nghttp2_session_adjust_closed_stream(session, 0); - if (rv != 0) { - return rv; - } - - return 0; } -int nghttp2_session_keep_idle_stream(nghttp2_session *session, - nghttp2_stream *stream) { - int rv; - +void nghttp2_session_keep_idle_stream(nghttp2_session *session, + nghttp2_stream *stream) { DEBUGF(fprintf(stderr, "stream: keep idle stream(%p)=%d, state=%d\n", stream, stream->stream_id, stream->state)); @@ -1154,13 +1137,6 @@ int nghttp2_session_keep_idle_stream(nghttp2_session *session, session->idle_stream_tail = stream; ++session->num_idle_streams; - - rv = nghttp2_session_adjust_idle_stream(session); - if (rv != 0) { - return rv; - } - - return 0; } void nghttp2_session_detach_idle_stream(nghttp2_session *session, @@ -1191,8 +1167,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, --session->num_idle_streams; } -int nghttp2_session_adjust_closed_stream(nghttp2_session *session, - size_t offset) { +int nghttp2_session_adjust_closed_stream(nghttp2_session *session) { size_t num_stream_max; int rv; @@ -1206,7 +1181,7 @@ int nghttp2_session_adjust_closed_stream(nghttp2_session *session, num_stream_max)); while (session->num_closed_streams > 0 && - session->num_closed_streams + session->num_incoming_streams + offset > + session->num_closed_streams + session->num_incoming_streams > num_stream_max) { nghttp2_stream *head_stream; nghttp2_stream *next; @@ -1242,13 +1217,11 @@ int nghttp2_session_adjust_idle_stream(nghttp2_session *session) { size_t max; int rv; - /* Make minimum number of idle streams 2 so that allocating 2 - streams at once is easy. This happens when PRIORITY frame to - idle stream, which depends on idle stream which does not - exist. */ - max = - nghttp2_max(2, nghttp2_min(session->local_settings.max_concurrent_streams, - session->pending_local_max_concurrent_stream)); + /* Make minimum number of idle streams 16, which is arbitrary chosen + number. */ + max = nghttp2_max(16, + nghttp2_min(session->local_settings.max_concurrent_streams, + session->pending_local_max_concurrent_stream)); DEBUGF(fprintf(stderr, "stream: adjusting kept idle streams " "num_idle_streams=%zu, max=%zu\n", @@ -1799,6 +1772,9 @@ static int session_prep_frame(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't call nghttp2_session_adjust_closed_stream() here, + since we don't keep closed stream in client side */ + estimated_payloadlen = session_estimate_headers_payload( session, frame->headers.nva, frame->headers.nvlen, NGHTTP2_PRIORITY_SPECLEN); @@ -2391,6 +2367,12 @@ static int session_after_frame_sent1(nghttp2_session *session) { return rv; } + rv = nghttp2_session_adjust_idle_stream(session); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + break; } case NGHTTP2_RST_STREAM: @@ -2659,6 +2641,14 @@ static ssize_t nghttp2_session_mem_send_internal(nghttp2_session *session, aob = &session->aob; framebufs = &aob->framebufs; + /* We may have idle streams more than we expect (e.g., + nghttp2_session_change_stream_priority() or + nghttp2_session_create_idle_stream()). Adjust them here. */ + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + *data_ptr = NULL; for (;;) { switch (aob->state) { @@ -3469,7 +3459,14 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, if (!stream) { return NGHTTP2_ERR_NOMEM; } + + rv = nghttp2_session_adjust_closed_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + session->last_proc_stream_id = session->last_recv_stream_id; + rv = session_call_on_begin_headers(session, frame); if (rv != 0) { return rv; @@ -3670,6 +3667,11 @@ int nghttp2_session_on_priority_received(nghttp2_session *session, if (stream == NULL) { return NGHTTP2_ERR_NOMEM; } + + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } } else { rv = nghttp2_session_reprioritize_stream(session, stream, &frame->priority.pri_spec); @@ -3677,6 +3679,11 @@ int nghttp2_session_on_priority_received(nghttp2_session *session, if (nghttp2_is_fatal(rv)) { return rv; } + + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } } return session_call_on_frame_received(session, frame); @@ -4185,6 +4192,9 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't call nghttp2_session_adjust_closed_stream(), since we + don't keep closed stream in client side */ + session->last_proc_stream_id = session->last_recv_stream_id; rv = session_call_on_begin_headers(session, frame); if (rv != 0) { @@ -4809,6 +4819,14 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, mem = &session->mem; + /* We may have idle streams more than we expect (e.g., + nghttp2_session_change_stream_priority() or + nghttp2_session_create_idle_stream()). Adjust them here. */ + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + for (;;) { switch (iframe->state) { case NGHTTP2_IB_READ_CLIENT_MAGIC: @@ -6512,6 +6530,10 @@ static int nghttp2_session_upgrade_internal(nghttp2_session *session, if (stream == NULL) { return NGHTTP2_ERR_NOMEM; } + + /* We don't call nghttp2_session_adjust_closed_stream(), since this + should be the first stream open. */ + if (session->server) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); session->last_recv_stream_id = 1; @@ -6726,6 +6748,7 @@ int nghttp2_session_check_server_session(nghttp2_session *session) { int nghttp2_session_change_stream_priority( nghttp2_session *session, int32_t stream_id, const nghttp2_priority_spec *pri_spec) { + int rv; nghttp2_stream *stream; nghttp2_priority_spec pri_spec_copy; @@ -6741,7 +6764,18 @@ int nghttp2_session_change_stream_priority( pri_spec_copy = *pri_spec; nghttp2_priority_spec_normalize_weight(&pri_spec_copy); - return nghttp2_session_reprioritize_stream(session, stream, &pri_spec_copy); + rv = nghttp2_session_reprioritize_stream(session, stream, &pri_spec_copy); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + + /* We don't intentionally call nghttp2_session_adjust_idle_stream() + so that idle stream created by this function, and existing ones + are kept for application. We will adjust number of idle stream + in nghttp2_session_mem_send or nghttp2_session_mem_recv is + called. */ + return 0; } int nghttp2_session_create_idle_stream(nghttp2_session *session, @@ -6770,5 +6804,10 @@ int nghttp2_session_create_idle_stream(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't intentionally call nghttp2_session_adjust_idle_stream() + so that idle stream created by this function, and existing ones + are kept for application. We will adjust number of idle stream + in nghttp2_session_mem_send or nghttp2_session_mem_recv is + called. */ return 0; } diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index dd69b5e3..5b47e94f 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -73,6 +73,10 @@ typedef struct { /* The default maximum number of incoming reserved streams */ #define NGHTTP2_MAX_INCOMING_RESERVED_STREAMS 200 +/* Even if we have less SETTINGS_MAX_CONCURRENT_STREAMS than this + number, we keep NGHTTP2_MIN_IDLE_STREAMS streams in idle state */ +#define NGHTTP2_MIN_IDLE_STREAMS 16 + /* The maximum number of items in outbound queue, which is considered as flooding caused by peer. All frames are not considered here. We only consider PING + ACK and SETTINGS + ACK. This is because @@ -443,6 +447,11 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, * * This function returns a pointer to created new stream object, or * NULL. + * + * This function adjusts neither the number of closed streams or idle + * streams. The caller should manually call + * nghttp2_session_adjust_closed_stream() or + * nghttp2_session_adjust_idle_stream() respectively. */ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, int32_t stream_id, uint8_t flags, @@ -491,29 +500,17 @@ int nghttp2_session_destroy_stream(nghttp2_session *session, * limitation of maximum number of streams in memory, |stream| is not * closed and just deleted from memory (see * nghttp2_session_destroy_stream). - * - * This function returns 0 if it succeeds, or one the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory */ -int nghttp2_session_keep_closed_stream(nghttp2_session *session, - nghttp2_stream *stream); +void nghttp2_session_keep_closed_stream(nghttp2_session *session, + nghttp2_stream *stream); /* * Appends |stream| to linked list |session->idle_stream_head|. We * apply fixed limit for list size. To fit into that limit, one or * more oldest streams are removed from list as necessary. - * - * This function returns 0 if it succeeds, or one the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory */ -int nghttp2_session_keep_idle_stream(nghttp2_session *session, - nghttp2_stream *stream); +void nghttp2_session_keep_idle_stream(nghttp2_session *session, + nghttp2_stream *stream); /* * Detaches |stream| from idle streams linked list. @@ -524,9 +521,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, /* * Deletes closed stream to ensure that number of incoming streams * including active and closed is in the maximum number of allowed - * stream. If |offset| is nonzero, it is decreased from the maximum - * number of allowed stream when comparing number of active and closed - * stream and the maximum number. + * stream. * * This function returns 0 if it succeeds, or one the following * negative error codes: @@ -534,8 +529,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory */ -int nghttp2_session_adjust_closed_stream(nghttp2_session *session, - size_t offset); +int nghttp2_session_adjust_closed_stream(nghttp2_session *session); /* * Deletes idle stream to ensure that number of idle streams is in @@ -807,6 +801,9 @@ int nghttp2_session_update_local_settings(nghttp2_session *session, * |pri_spec|. Caller must ensure that stream->hd.stream_id != * pri_spec->stream_id. * + * This function does not adjust the number of idle streams. The + * caller should call nghttp2_session_adjust_idle_stream() later. + * * This function returns 0 if it succeeds, or one of the following * negative error codes: * diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 8e53aff1..2de4d028 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -7669,6 +7669,7 @@ void test_nghttp2_session_keep_closed_stream(void) { CU_ASSERT(NULL == session->closed_stream_head->closed_prev); open_stream(session, 11); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(1 == session->num_closed_streams); CU_ASSERT(5 == session->closed_stream_tail->stream_id); @@ -7677,6 +7678,7 @@ void test_nghttp2_session_keep_closed_stream(void) { CU_ASSERT(NULL == session->closed_stream_head->closed_next); open_stream(session, 13); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(0 == session->num_closed_streams); CU_ASSERT(NULL == session->closed_stream_tail); @@ -7689,6 +7691,7 @@ void test_nghttp2_session_keep_closed_stream(void) { /* server initiated stream is not counted to max concurrent limit */ open_stream(session, 2); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(1 == session->num_closed_streams); CU_ASSERT(3 == session->closed_stream_head->stream_id); @@ -7708,6 +7711,7 @@ void test_nghttp2_session_keep_idle_stream(void) { nghttp2_settings_entry iv = {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, max_concurrent_streams}; int i; + int32_t stream_id; memset(&callbacks, 0, sizeof(callbacks)); callbacks.send_callback = null_send_callback; @@ -7716,25 +7720,29 @@ void test_nghttp2_session_keep_idle_stream(void) { nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, &iv, 1); - /* We at least allow 2 idle streams even if max concurrent streams - is very low. */ - for (i = 0; i < 2; ++i) { + /* We at least allow NGHTTP2_MIN_IDLE_STREAM idle streams even if + max concurrent streams is very low. */ + for (i = 0; i < NGHTTP2_MIN_IDLE_STREAMS; ++i) { nghttp2_session_open_stream(session, i * 2 + 1, NGHTTP2_STREAM_FLAG_NONE, &pri_spec_default, NGHTTP2_STREAM_IDLE, NULL); + nghttp2_session_adjust_idle_stream(session); } - CU_ASSERT(2 == session->num_idle_streams); + CU_ASSERT(NGHTTP2_MIN_IDLE_STREAMS == session->num_idle_streams); + stream_id = (NGHTTP2_MIN_IDLE_STREAMS - 1) * 2 + 1; CU_ASSERT(1 == session->idle_stream_head->stream_id); - CU_ASSERT(3 == session->idle_stream_tail->stream_id); + CU_ASSERT(stream_id == session->idle_stream_tail->stream_id); - nghttp2_session_open_stream(session, 5, NGHTTP2_FLAG_NONE, &pri_spec_default, - NGHTTP2_STREAM_IDLE, NULL); + stream_id += 2; - CU_ASSERT(2 == session->num_idle_streams); + nghttp2_session_open_stream(session, stream_id, NGHTTP2_FLAG_NONE, + &pri_spec_default, NGHTTP2_STREAM_IDLE, NULL); + nghttp2_session_adjust_idle_stream(session); + CU_ASSERT(NGHTTP2_MIN_IDLE_STREAMS == session->num_idle_streams); CU_ASSERT(3 == session->idle_stream_head->stream_id); - CU_ASSERT(5 == session->idle_stream_tail->stream_id); + CU_ASSERT(stream_id == session->idle_stream_tail->stream_id); nghttp2_session_del(session); }