Strict outgoing idle stream detection

Previously, we use session->next_stream_id to detect that given stream
ID was idle or not.  But this was suboptimal, since it was updated
when stream ID was assigned, and it did not necessarily mean that it
actually has been sent to the peer.  Now we introduced
session->sent_stream_id, which only updated when HEADERS/PUSH_PROMISE
has sent.  Using sent_stream_id instead of next_stream_id tightens
idle stream detection, and misbehaved peer which sends frame with
stream ID that has not been generated.

This commit also overhauls test code which involves opening streams.
Now we have some wrapper functions for nghttp2_session_open_stream()
which also take care of updating next_stream_id and
last_recv_stream_id.  They are crucial for some tests.

lib/nghttp2_session.c

@@ -130,7 +130,7 @@ static int session_detect_idle_stream(nghttp2_session *session,
                                       int32_t stream_id) {
   /* Assume that stream object with stream_id does not exist */
   if (nghttp2_session_is_my_stream_id(session, stream_id)) {
-    if (session->next_stream_id <= (uint32_t)stream_id) {
+    if (session->sent_stream_id < stream_id) {
       return 1;
     }
     return 0;
@@ -1374,7 +1374,9 @@ static int session_predicate_request_headers_send(nghttp2_session *session,
  * NGHTTP2_ERR_INVALID_STREAM_STATE
  *     The state of the stream is not valid.
  * NGHTTP2_ERR_SESSION_CLOSING
- *   This session is closing.
+ *     This session is closing.
+ * NGHTTP2_ERR_PROTO
+ *     Client side attempted to send response.
  */
 static int session_predicate_response_headers_send(nghttp2_session *session,
                                                    nghttp2_stream *stream) {
@@ -1384,6 +1386,9 @@ static int session_predicate_response_headers_send(nghttp2_session *session,
     return rv;
   }
   assert(stream);
+  if (!session->server) {
+    return NGHTTP2_ERR_PROTO;
+  }
   if (nghttp2_session_is_my_stream_id(session, stream->stream_id)) {
     return NGHTTP2_ERR_INVALID_STREAM_ID;
   }
@@ -1417,6 +1422,8 @@ static int session_predicate_response_headers_send(nghttp2_session *session,
  * NGHTTP2_ERR_START_STREAM_NOT_ALLOWED
  *   New stream cannot be created because GOAWAY is already sent or
  *   received.
+ * NGHTTP2_ERR_PROTO
+ *   Client side attempted to send push response.
  */
 static int
 session_predicate_push_response_headers_send(nghttp2_session *session,
@@ -1428,6 +1435,9 @@ session_predicate_push_response_headers_send(nghttp2_session *session,
     return rv;
   }
   assert(stream);
+  if (!session->server) {
+    return NGHTTP2_ERR_PROTO;
+  }
   if (stream->state != NGHTTP2_STREAM_RESERVED) {
     return NGHTTP2_ERR_PROTO;
   }
@@ -1861,6 +1871,11 @@ static int session_prep_frame(nghttp2_session *session,
       DEBUGF(fprintf(stderr, "send: HEADERS finally serialized in %zd bytes\n",
                      nghttp2_bufs_len(&session->aob.framebufs)));
 
+      if (frame->headers.cat == NGHTTP2_HCAT_REQUEST) {
+        assert(session->sent_stream_id < frame->hd.stream_id);
+        session->sent_stream_id = frame->hd.stream_id;
+      }
+
       break;
     }
     case NGHTTP2_PRIORITY: {
@@ -1930,6 +1945,10 @@ static int session_prep_frame(nghttp2_session *session,
         return rv;
       }
 
+      assert(session->sent_stream_id + 2 <=
+             frame->push_promise.promised_stream_id);
+      session->sent_stream_id = frame->push_promise.promised_stream_id;
+
       break;
     }
     case NGHTTP2_PING:
@@ -3714,13 +3733,14 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session,
     return session_handle_invalid_connection(session, frame, NGHTTP2_ERR_PROTO,
                                              "RST_STREAM: stream_id == 0");
   }
+
+  if (session_detect_idle_stream(session, frame->hd.stream_id)) {
+    return session_handle_invalid_connection(session, frame, NGHTTP2_ERR_PROTO,
+                                             "RST_STREAM: stream in idle");
+  }
+
   stream = nghttp2_session_get_stream(session, frame->hd.stream_id);
-  if (!stream) {
+  if (stream) {
-    if (session_detect_idle_stream(session, frame->hd.stream_id)) {
-      return session_handle_invalid_connection(
-          session, frame, NGHTTP2_ERR_PROTO, "RST_STREAM: stream in idle");
-    }
-  } else {
     /* We may use stream->shut_flags for strict error checking. */
     nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
   }
@@ -4158,21 +4178,20 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
         session, frame, NGHTTP2_ERR_PROTO,
         "PUSH_PROMISE: invalid promised_stream_id");
   }
+
+  if (session_detect_idle_stream(session, frame->hd.stream_id)) {
+    return session_inflate_handle_invalid_connection(
+        session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: stream in idle");
+  }
+
   session->last_recv_stream_id = frame->push_promise.promised_stream_id;
   stream = nghttp2_session_get_stream(session, frame->hd.stream_id);
   if (!stream || stream->state == NGHTTP2_STREAM_CLOSING ||
       !session->pending_enable_push ||
       session->num_incoming_reserved_streams >=
           session->max_incoming_reserved_streams) {
-    if (!stream) {
+    /* Currently, client does not retain closed stream, so we don't
-      if (session_detect_idle_stream(session, frame->hd.stream_id)) {
+       check NGHTTP2_SHUT_RD condition here. */
-        return session_inflate_handle_invalid_connection(
-            session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: stream in idle");
-      }
-
-      /* Currently, client does not retain closed stream, so we don't
-         check NGHTTP2_SHUT_RD condition here. */
-    }
 
     rv = nghttp2_session_add_rst_stream(
         session, frame->push_promise.promised_stream_id, NGHTTP2_CANCEL);
@@ -4324,12 +4343,14 @@ static int session_on_stream_window_update_received(nghttp2_session *session,
                                                     nghttp2_frame *frame) {
   int rv;
   nghttp2_stream *stream;
+
+  if (session_detect_idle_stream(session, frame->hd.stream_id)) {
+    return session_handle_invalid_connection(session, frame, NGHTTP2_ERR_PROTO,
+                                             "WINDOW_UPDATE to idle stream");
+  }
+
   stream = nghttp2_session_get_stream(session, frame->hd.stream_id);
   if (!stream) {
-    if (session_detect_idle_stream(session, frame->hd.stream_id)) {
-      return session_handle_invalid_connection(
-          session, frame, NGHTTP2_ERR_PROTO, "WINDOW_UPDATE to idle stream");
-    }
     return 0;
   }
   if (state_reserved_remote(session, stream)) {
@@ -4627,14 +4648,15 @@ static int session_on_data_received_fail_fast(nghttp2_session *session) {
     failure_reason = "DATA: stream_id == 0";
     goto fail;
   }
+
+  if (session_detect_idle_stream(session, stream_id)) {
+    failure_reason = "DATA: stream in idle";
+    error_code = NGHTTP2_PROTOCOL_ERROR;
+    goto fail;
+  }
+
   stream = nghttp2_session_get_stream(session, stream_id);
   if (!stream) {
-    if (session_detect_idle_stream(session, stream_id)) {
-      failure_reason = "DATA: stream in idle";
-      error_code = NGHTTP2_PROTOCOL_ERROR;
-      goto fail;
-    }
-
     stream = nghttp2_session_get_stream_raw(session, stream_id);
     if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) {
       failure_reason = "DATA: stream closed";

lib/nghttp2_session.h

@@ -246,6 +246,10 @@ struct nghttp2_session {
   size_t obq_flood_counter_;
   /* Next Stream ID. Made unsigned int to detect >= (1 << 31). */
   uint32_t next_stream_id;
+  /* The last stream ID this session initiated.  For client session,
+     this is the last stream ID it has sent.  For server session, it
+     is the last promised stream ID sent in PUSH_PROMISE. */
+  int32_t sent_stream_id;
   /* The largest stream ID received so far */
   int32_t last_recv_stream_id;
   /* The largest stream ID which has been processed in some way. This

tests/nghttp2_test_helper.c

@@ -306,3 +306,119 @@ nghttp2_outbound_item *create_data_ob_item(nghttp2_mem *mem) {
 
   return item;
 }
+
+nghttp2_stream *open_sent_stream(nghttp2_session *session, int32_t stream_id) {
+  nghttp2_priority_spec pri_spec;
+
+  nghttp2_priority_spec_init(&pri_spec, 0, NGHTTP2_DEFAULT_WEIGHT, 0);
+  return open_sent_stream3(session, stream_id, NGHTTP2_FLAG_NONE, &pri_spec,
+                           NGHTTP2_STREAM_OPENED, NULL);
+}
+
+nghttp2_stream *open_sent_stream2(nghttp2_session *session, int32_t stream_id,
+                                  nghttp2_stream_state initial_state) {
+  nghttp2_priority_spec pri_spec;
+
+  nghttp2_priority_spec_init(&pri_spec, 0, NGHTTP2_DEFAULT_WEIGHT, 0);
+  return open_sent_stream3(session, stream_id, NGHTTP2_FLAG_NONE, &pri_spec,
+                           initial_state, NULL);
+}
+
+nghttp2_stream *open_sent_stream3(nghttp2_session *session, int32_t stream_id,
+                                  uint8_t flags,
+                                  nghttp2_priority_spec *pri_spec_in,
+                                  nghttp2_stream_state initial_state,
+                                  void *stream_user_data) {
+  nghttp2_stream *stream;
+
+  assert(nghttp2_session_is_my_stream_id(session, stream_id));
+
+  stream = nghttp2_session_open_stream(session, stream_id, flags, pri_spec_in,
+                                       initial_state, stream_user_data);
+  session->sent_stream_id = nghttp2_max(session->sent_stream_id, stream_id);
+  session->next_stream_id =
+      nghttp2_max(session->next_stream_id, (uint32_t)stream_id + 2);
+
+  return stream;
+}
+
+nghttp2_stream *open_sent_stream_with_dep(nghttp2_session *session,
+                                          int32_t stream_id,
+                                          nghttp2_stream *dep_stream) {
+  return open_sent_stream_with_dep_weight(session, stream_id,
+                                          NGHTTP2_DEFAULT_WEIGHT, dep_stream);
+}
+
+nghttp2_stream *open_sent_stream_with_dep_weight(nghttp2_session *session,
+                                                 int32_t stream_id,
+                                                 int32_t weight,
+                                                 nghttp2_stream *dep_stream) {
+  nghttp2_stream *stream;
+
+  assert(nghttp2_session_is_my_stream_id(session, stream_id));
+
+  stream = open_stream_with_all(session, stream_id, weight, 0, dep_stream);
+
+  session->sent_stream_id = nghttp2_max(session->sent_stream_id, stream_id);
+  session->next_stream_id =
+      nghttp2_max(session->next_stream_id, (uint32_t)stream_id + 2);
+
+  return stream;
+}
+
+nghttp2_stream *open_recv_stream(nghttp2_session *session, int32_t stream_id) {
+  nghttp2_priority_spec pri_spec;
+
+  nghttp2_priority_spec_init(&pri_spec, 0, NGHTTP2_DEFAULT_WEIGHT, 0);
+  return open_recv_stream3(session, stream_id, NGHTTP2_FLAG_NONE, &pri_spec,
+                           NGHTTP2_STREAM_OPENED, NULL);
+}
+
+nghttp2_stream *open_recv_stream2(nghttp2_session *session, int32_t stream_id,
+                                  nghttp2_stream_state initial_state) {
+  nghttp2_priority_spec pri_spec;
+
+  nghttp2_priority_spec_init(&pri_spec, 0, NGHTTP2_DEFAULT_WEIGHT, 0);
+  return open_recv_stream3(session, stream_id, NGHTTP2_FLAG_NONE, &pri_spec,
+                           initial_state, NULL);
+}
+
+nghttp2_stream *open_recv_stream3(nghttp2_session *session, int32_t stream_id,
+                                  uint8_t flags,
+                                  nghttp2_priority_spec *pri_spec_in,
+                                  nghttp2_stream_state initial_state,
+                                  void *stream_user_data) {
+  nghttp2_stream *stream;
+
+  assert(!nghttp2_session_is_my_stream_id(session, stream_id));
+
+  stream = nghttp2_session_open_stream(session, stream_id, flags, pri_spec_in,
+                                       initial_state, stream_user_data);
+  session->last_recv_stream_id =
+      nghttp2_max(session->last_recv_stream_id, stream_id);
+
+  return stream;
+}
+
+nghttp2_stream *open_recv_stream_with_dep(nghttp2_session *session,
+                                          int32_t stream_id,
+                                          nghttp2_stream *dep_stream) {
+  return open_recv_stream_with_dep_weight(session, stream_id,
+                                          NGHTTP2_DEFAULT_WEIGHT, dep_stream);
+}
+
+nghttp2_stream *open_recv_stream_with_dep_weight(nghttp2_session *session,
+                                                 int32_t stream_id,
+                                                 int32_t weight,
+                                                 nghttp2_stream *dep_stream) {
+  nghttp2_stream *stream;
+
+  assert(!nghttp2_session_is_my_stream_id(session, stream_id));
+
+  stream = open_stream_with_all(session, stream_id, weight, 0, dep_stream);
+
+  session->last_recv_stream_id =
+      nghttp2_max(session->last_recv_stream_id, stream_id);
+
+  return stream;
+}

tests/nghttp2_test_helper.h

@@ -110,4 +110,49 @@ nghttp2_stream *open_stream_with_dep_excl(nghttp2_session *session,
 
 nghttp2_outbound_item *create_data_ob_item(nghttp2_mem *mem);
 
+/* Opens stream.  This stream is assumed to be sent from |session|,
+   and session->sent_stream_id and session->next_stream_id will be
+   adjusted accordingly. */
+nghttp2_stream *open_sent_stream(nghttp2_session *session, int32_t stream_id);
+
+nghttp2_stream *open_sent_stream2(nghttp2_session *session, int32_t stream_id,
+                                  nghttp2_stream_state initial_state);
+
+nghttp2_stream *open_sent_stream3(nghttp2_session *session, int32_t stream_id,
+                                  uint8_t flags,
+                                  nghttp2_priority_spec *pri_spec_in,
+                                  nghttp2_stream_state initial_state,
+                                  void *stream_user_data);
+
+nghttp2_stream *open_sent_stream_with_dep(nghttp2_session *session,
+                                          int32_t stream_id,
+                                          nghttp2_stream *dep_stream);
+
+nghttp2_stream *open_sent_stream_with_dep_weight(nghttp2_session *session,
+                                                 int32_t stream_id,
+                                                 int32_t weight,
+                                                 nghttp2_stream *dep_stream);
+
+/* Opens stream.  This stream is assumed to be received by |session|,
+   and session->last_recv_stream_id will be adjusted accordingly. */
+nghttp2_stream *open_recv_stream(nghttp2_session *session, int32_t stream_id);
+
+nghttp2_stream *open_recv_stream2(nghttp2_session *session, int32_t stream_id,
+                                  nghttp2_stream_state initial_state);
+
+nghttp2_stream *open_recv_stream3(nghttp2_session *session, int32_t stream_id,
+                                  uint8_t flags,
+                                  nghttp2_priority_spec *pri_spec_in,
+                                  nghttp2_stream_state initial_state,
+                                  void *stream_user_data);
+
+nghttp2_stream *open_recv_stream_with_dep(nghttp2_session *session,
+                                          int32_t stream_id,
+                                          nghttp2_stream *dep_stream);
+
+nghttp2_stream *open_recv_stream_with_dep_weight(nghttp2_session *session,
+                                                 int32_t stream_id,
+                                                 int32_t weight,
+                                                 nghttp2_stream *dep_stream);
+
 #endif /* NGHTTP2_TEST_HELPER_H */