More strict stream state handling

tatsuhiro-t · tatsuhiro-t · commit 16c46114dc72 · 2016-08-07T19:31:00.000+09:00
Previously, in server side, we used closed streams to detect the error
that the misbehaving client sends a frame on the incoming stream it
explicitly closed.  With this commit, we make a further step, and
detect one more error case.  Since we retain closed streams as long as
the sum of its size and the number of opened streams are equal or less
than max concurrent streams, we can safely say that if we get a frame
which is sent on the stream that is not found in either closed or
opened stream, it is already closed or has not existed.  Then we can
send GOAWAY.

The previous code shrinks closed streams when we closed another
stream, but now it is removed.  It is enough to adjust closed streams
when new incoming stream is created.

While creating this commit, we noticed that
NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS is defined as INT32_MAX.  But
since SETTINGS can contain value up to UINT32_MAX, it is not enough.
However, since the stream ID space is limited to INT32_MAX, it is high
enough.  We could keep this value, but this time we deprecate
NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS macro.  While it is in public
header, the effect of deprecating it is negligible because of the
reason we wrote above, and usually application sets much smaller value
(say, 100) as SETTINGS_MAX_CONCURRENT_STREAMS.
diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h
@@ -668,6 +668,10 @@ typedef enum {
 /**
  * @macro
  *
+ * .. warning::
+ *
+ *   Deprecated.  The initial max concurrent streams is 0xffffffffu.
+ *
  * Default maximum number of incoming concurrent streams.  Use
  * `nghttp2_submit_settings()` with
  * :enum:`NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS` to change the
diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c
@@ -365,7 +365,7 @@ static void session_inbound_frame_reset(nghttp2_session *session) {
 static void init_settings(nghttp2_settings_storage *settings) {
   settings->header_table_size = NGHTTP2_HD_DEFAULT_MAX_BUFFER_SIZE;
   settings->enable_push = 1;
-  settings->max_concurrent_streams = NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS;
+  settings->max_concurrent_streams = NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS;
   settings->initial_window_size = NGHTTP2_INITIAL_WINDOW_SIZE;
   settings->max_frame_size = NGHTTP2_MAX_FRAME_SIZE_MIN;
   settings->max_header_list_size = UINT32_MAX;
@@ -435,7 +435,7 @@ static int session_new(nghttp2_session **session_ptr,
   (*session_ptr)->remote_last_stream_id = (1u << 31) - 1;
 
   (*session_ptr)->pending_local_max_concurrent_stream =
-      NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS;
+      NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS;
   (*session_ptr)->pending_enable_push = 1;
 
   if (server) {
@@ -1185,13 +1185,11 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id,
        combined with the current active incoming streams to make
        dependency tree work better. */
     nghttp2_session_keep_closed_stream(session, stream);
-
-    rv = nghttp2_session_adjust_closed_stream(session);
   } else {
     rv = nghttp2_session_destroy_stream(session, stream);
-  }
-  if (rv != 0) {
-    return rv;
+    if (rv != 0) {
+      return rv;
+    }
   }
 
   return 0;
@@ -1285,8 +1283,12 @@ int nghttp2_session_adjust_closed_stream(nghttp2_session *session) {
   size_t num_stream_max;
   int rv;
 
-  num_stream_max = nghttp2_min(session->local_settings.max_concurrent_streams,
-                               session->pending_local_max_concurrent_stream);
+  if (session->local_settings.max_concurrent_streams ==
+      NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS) {
+    num_stream_max = session->pending_local_max_concurrent_stream;
+  } else {
+    num_stream_max = session->local_settings.max_concurrent_streams;
+  }
 
   DEBUGF(fprintf(stderr, "stream: adjusting kept closed streams "
                          "num_closed_streams=%zu, num_incoming_streams=%zu, "
@@ -3777,22 +3779,74 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session,
   assert(session->server);
 
   if (!session_is_new_peer_stream_id(session, frame->hd.stream_id)) {
-    /* The spec says if an endpoint receives a HEADERS with invalid
-       stream ID, it MUST issue connection error with error code
-       PROTOCOL_ERROR.  But we could get trailer HEADERS after we have
-       sent RST_STREAM to this stream and peer have not received it.
-       Then connection error is too harsh.  It means that we only use
-       connection error if stream ID refers idle stream.  Therwise we
-       just ignore HEADERS for now. */
     if (frame->hd.stream_id == 0 ||
         nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) {
       return session_inflate_handle_invalid_connection(
           session, frame, NGHTTP2_ERR_PROTO,
           "request HEADERS: invalid stream_id");
     }
 
+    /* RFC 7540 says if an endpoint receives a HEADERS with invalid
+     * stream ID (e.g, numerically smaller than previous), it MUST
+     * issue connection error with error code PROTOCOL_ERROR.  It is a
+     * bit hard to detect this, since we cannot remember all streams
+     * we observed so far.
+     *
+     * You might imagine this is really easy.  But no.  HTTP/2 is
+     * asynchronous protocol, and usually client and server do not
+     * share the complete picture of open/closed stream status.  For
+     * example, after server sends RST_STREAM for a stream, client may
+     * send trailer HEADERS for that stream.  If naive server detects
+     * that, and issued connection error, then it is a bug of server
+     * 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.
+     */
     stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id);
-    if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) {
+
+    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) {
       return session_inflate_handle_invalid_connection(
           session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed");
     }
@@ -5066,7 +5120,25 @@ 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 && (stream->shut_flags & NGHTTP2_SHUT_RD)) {
+
+    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) {
       failure_reason = "DATA: stream closed";
       error_code = NGHTTP2_STREAM_CLOSED;
       goto fail;
diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h
@@ -97,6 +97,9 @@ typedef struct {
    these frames in this number, it is considered suspicious. */
 #define NGHTTP2_MAX_OBQ_FLOOD_ITEM 10000
 
+/* The default value of maximum number of concurrent streams. */
+#define NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS 0xffffffffu
+
 /* Internal state when receiving incoming frame */
 typedef enum {
   /* Receiving frame header */
diff --git a/tests/main.c b/tests/main.c
@@ -308,6 +308,8 @@ int main(int argc _U_, char *argv[] _U_) {
                    test_nghttp2_session_set_local_window_size) ||
       !CU_add_test(pSuite, "session_cancel_from_before_frame_send",
                    test_nghttp2_session_cancel_from_before_frame_send) ||
+      !CU_add_test(pSuite, "session_removed_closed_stream",
+                   test_nghttp2_session_removed_closed_stream) ||
       !CU_add_test(pSuite, "http_mandatory_headers",
                    test_nghttp2_http_mandatory_headers) ||
       !CU_add_test(pSuite, "http_content_length",
diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c
@@ -2366,7 +2366,7 @@ void test_nghttp2_session_on_request_headers_received(void) {
 
   nghttp2_frame_headers_free(&frame.headers, mem);
   session->local_settings.max_concurrent_streams =
-      NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS;
+      NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS;
 
   /* Stream ID less than or equal to the previouly received request
      HEADERS is just ignored due to race condition */
@@ -5042,7 +5042,7 @@ void test_nghttp2_submit_settings(void) {
             nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 7));
 
   /* Make sure that local settings are not changed */
-  CU_ASSERT(NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS ==
+  CU_ASSERT(NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS ==
             session->local_settings.max_concurrent_streams);
   CU_ASSERT(NGHTTP2_INITIAL_WINDOW_SIZE ==
             session->local_settings.initial_window_size);
@@ -9715,6 +9715,186 @@ void test_nghttp2_session_cancel_from_before_frame_send(void) {
   nghttp2_session_del(session);
 }
 
+static void
+prepare_session_removed_closed_stream(nghttp2_session *session,
+                                      nghttp2_hd_deflater *deflater) {
+  int rv;
+  nghttp2_settings_entry iv;
+  nghttp2_bufs bufs;
+  nghttp2_mem *mem;
+  ssize_t nread;
+  int i;
+  nghttp2_stream *stream;
+  nghttp2_frame_hd hd;
+
+  mem = nghttp2_mem_default();
+
+  frame_pack_bufs_init(&bufs);
+
+  iv.settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
+  iv.value = 2;
+
+  rv = nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, &iv, 1);
+
+  CU_ASSERT(0 == rv);
+
+  rv = nghttp2_session_send(session);
+
+  CU_ASSERT(0 == rv);
+
+  for (i = 1; i <= 3; i += 2) {
+    rv = pack_headers(&bufs, deflater, i,
+                      NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM, reqnv,
+                      ARRLEN(reqnv), mem);
+
+    CU_ASSERT(0 == rv);
+
+    nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                     nghttp2_bufs_len(&bufs));
+
+    CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+
+    nghttp2_bufs_reset(&bufs);
+  }
+
+  nghttp2_session_close_stream(session, 3, NGHTTP2_NO_ERROR);
+
+  rv = pack_headers(&bufs, deflater, 5,
+                    NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM, reqnv,
+                    ARRLEN(reqnv), mem);
+
+  CU_ASSERT(0 == rv);
+
+  /* Receiving stream 5 will erase stream 3 from closed stream list */
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+
+  stream = nghttp2_session_get_stream_raw(session, 3);
+
+  CU_ASSERT(NULL == stream);
+
+  /* Since the current max concurrent streams is
+     NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS, receiving frame on stream
+     3 is ignored. */
+  nghttp2_bufs_reset(&bufs);
+  rv = pack_headers(&bufs, deflater, 3,
+                    NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM,
+                    trailernv, ARRLEN(trailernv), mem);
+
+  CU_ASSERT(0 == rv);
+
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+  CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session));
+
+  nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_NONE, 3);
+  nghttp2_bufs_reset(&bufs);
+  nghttp2_frame_pack_frame_hd(bufs.head->buf.last, &hd);
+  bufs.head->buf.last += NGHTTP2_FRAME_HDLEN;
+
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+  CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session));
+
+  /* Now server receives SETTINGS ACK */
+  nghttp2_frame_hd_init(&hd, 0, NGHTTP2_SETTINGS, NGHTTP2_FLAG_ACK, 0);
+  nghttp2_bufs_reset(&bufs);
+  nghttp2_frame_pack_frame_hd(bufs.head->buf.last, &hd);
+  bufs.head->buf.last += NGHTTP2_FRAME_HDLEN;
+
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+
+  nghttp2_bufs_free(&bufs);
+}
+
+void test_nghttp2_session_removed_closed_stream(void) {
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  int rv;
+  nghttp2_hd_deflater deflater;
+  nghttp2_bufs bufs;
+  nghttp2_mem *mem;
+  ssize_t nread;
+  nghttp2_frame_hd hd;
+  nghttp2_outbound_item *item;
+
+  mem = nghttp2_mem_default();
+
+  frame_pack_bufs_init(&bufs);
+
+  memset(&callbacks, 0, sizeof(callbacks));
+
+  callbacks.send_callback = null_send_callback;
+
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+
+  /* Now local max concurrent streams is still unlimited, pending max
+     concurrent streams is now 2. */
+
+  nghttp2_hd_deflate_init(&deflater, mem);
+
+  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 */
+  nghttp2_bufs_reset(&bufs);
+  rv = pack_headers(&bufs, &deflater, 3,
+                    NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM,
+                    trailernv, ARRLEN(trailernv), mem);
+
+  CU_ASSERT(0 == rv);
+
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+
+  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);
+
+  nghttp2_hd_deflate_free(&deflater);
+  nghttp2_session_del(session);
+
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+  nghttp2_hd_deflate_init(&deflater, mem);
+  /* Same setup, and then receive DATA instead of HEADERS */
+
+  prepare_session_removed_closed_stream(session, &deflater);
+
+  nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_NONE, 3);
+  nghttp2_bufs_reset(&bufs);
+  nghttp2_frame_pack_frame_hd(bufs.head->buf.last, &hd);
+  bufs.head->buf.last += NGHTTP2_FRAME_HDLEN;
+
+  nread = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                   nghttp2_bufs_len(&bufs));
+
+  CU_ASSERT((ssize_t)nghttp2_bufs_len(&bufs) == nread);
+
+  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);
+
+  nghttp2_hd_deflate_free(&deflater);
+  nghttp2_session_del(session);
+
+  nghttp2_bufs_free(&bufs);
+}
+
 static void check_nghttp2_http_recv_headers_fail(
     nghttp2_session *session, nghttp2_hd_deflater *deflater, int32_t stream_id,
     int stream_state, const nghttp2_nv *nva, size_t nvlen) {
diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h
