Fix bug that idle/closed stream may be destroyed while it is referenced

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);
+    nghttp2_session_keep_idle_stream(session, stream);
-    if (rv != 0) {
-      return NULL;
-    }
     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,
+void nghttp2_session_keep_closed_stream(nghttp2_session *session,
-                                       nghttp2_stream *stream) {
+                                        nghttp2_stream *stream) {
-  int rv;
-
   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,
+void nghttp2_session_keep_idle_stream(nghttp2_session *session,
-                                     nghttp2_stream *stream) {
+                                      nghttp2_stream *stream) {
-  int rv;
-
   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,
+int nghttp2_session_adjust_closed_stream(nghttp2_session *session) {
-                                         size_t offset) {
   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
+  /* Make minimum number of idle streams 16, which is arbitrary chosen
-     streams at once is easy.  This happens when PRIORITY frame to
+     number. */
-     idle stream, which depends on idle stream which does not
+  max = nghttp2_max(16,
-     exist. */
+                    nghttp2_min(session->local_settings.max_concurrent_streams,
-  max =
+                                session->pending_local_max_concurrent_stream));
-      nghttp2_max(2, 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;
 }

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,
+void nghttp2_session_keep_closed_stream(nghttp2_session *session,
-                                       nghttp2_stream *stream);
+                                        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,
+void nghttp2_session_keep_idle_stream(nghttp2_session *session,
-                                     nghttp2_stream *stream);
+                                      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
+ * stream.
- * number of allowed stream when comparing number of active and closed
- * stream and the maximum number.
  *
  * 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,
+int nghttp2_session_adjust_closed_stream(nghttp2_session *session);
-                                         size_t offset);
 
 /*
  * 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:
  *

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
+  /* We at least allow NGHTTP2_MIN_IDLE_STREAM idle streams even if
-     is very low. */
+     max concurrent streams is very low. */
-  for (i = 0; i < 2; ++i) {
+  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,
+  stream_id += 2;
-                              NGHTTP2_STREAM_IDLE, NULL);
 
-  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);
 }