Fix bug that client may send PROTOCOL_ERROR upon canceled push stream

Previously we treat stream in NGHTTP2_STREAM_RESERVED state specially,
that is we don't increment or decrement streams counts if stream is in
that state.  Because of this, we don't change the stream state to
NGHTTP2_STREAM_CLOSING if stream is in NGHTTP2_STREAM_RESERVED.  But
it turns out that it causes a problem.  If client canceled pushed
stream before push response HEADERS, stream is still in
NGHTTP2_STREAM_RESERVED state.  If push response HEADERS arrived in
this state, library happily accepts it and passed to application.

With this commit, this bug was corrected.  We now change stream state
to NGHTTP2_STREAM_CLOSING even if it was in NGHTTP2_STREAM_RESERVED
state.  We now use NGHTTP2_STREAM_FLAG_PUSH to determine whether we
have to increase/decrase stream count.

lib/nghttp2_session.c

@@ -688,17 +688,8 @@ int nghttp2_session_add_item(nghttp2_session *session,
     switch (frame->hd.type) {
     case NGHTTP2_RST_STREAM:
       if (stream) {
-        /* We rely on the stream state to decide whether number of
-           streams should be decremented or not. For purly reserved or
-           idle streams, they are not counted to those numbers and we
-           must keep this state in order not to decrement the number.
-           We don't check against NGHTTP2_STREAM_IDLE because
-           nghttp2_session_get_stream() does not return such
-           stream. */
-        if (stream->state != NGHTTP2_STREAM_RESERVED) {
         stream->state = NGHTTP2_STREAM_CLOSING;
       }
-      }
 
       break;
     case NGHTTP2_SETTINGS:
@@ -879,6 +870,10 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session,
     }
   }
 
+  if (initial_state == NGHTTP2_STREAM_RESERVED) {
+    flags |= NGHTTP2_STREAM_FLAG_PUSH;
+  }
+
   nghttp2_stream_init(
       stream, stream_id, flags, initial_state, pri_spec->weight,
       &session->roots, session->remote_settings.initial_window_size,
@@ -1016,10 +1011,9 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id,
     }
   }
 
-  switch (stream->state) {
+  /* pushed streams which is not opened yet is not counted toward max
-  case NGHTTP2_STREAM_RESERVED:
+     concurrent limits */
-    break;
+  if ((stream->flags & NGHTTP2_STREAM_FLAG_PUSH) == 0) {
-  default:
     if (nghttp2_session_is_my_stream_id(session, stream_id)) {
       --session->num_outgoing_streams;
     } else {
@@ -1844,7 +1838,7 @@ static int session_prep_frame(nghttp2_session *session,
 
       if (!nghttp2_session_open_stream(
               session, frame->push_promise.promised_stream_id,
-              NGHTTP2_STREAM_FLAG_PUSH, &pri_spec, NGHTTP2_STREAM_RESERVED,
+              NGHTTP2_STREAM_FLAG_NONE, &pri_spec, NGHTTP2_STREAM_RESERVED,
               aux_data->stream_user_data)) {
         return NGHTTP2_ERR_NOMEM;
       }
@@ -2290,6 +2284,7 @@ static int session_after_frame_sent1(nghttp2_session *session) {
         break;
       }
       case NGHTTP2_HCAT_PUSH_RESPONSE:
+        stream->flags &= ~NGHTTP2_STREAM_FLAG_PUSH;
         ++session->num_outgoing_streams;
       /* Fall through */
       case NGHTTP2_HCAT_RESPONSE:
@@ -3982,7 +3977,7 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
                              NGHTTP2_DEFAULT_WEIGHT, 0);
 
   promised_stream = nghttp2_session_open_stream(
-      session, frame->push_promise.promised_stream_id, NGHTTP2_STREAM_FLAG_PUSH,
+      session, frame->push_promise.promised_stream_id, NGHTTP2_STREAM_FLAG_NONE,
       &pri_spec, NGHTTP2_STREAM_RESERVED, NULL);
 
   if (!promised_stream) {

lib/nghttp2_session.h

@@ -400,6 +400,9 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
  * is a pointer to the arbitrary user supplied data to be associated
  * to this stream.
  *
+ * If |initial_state| is NGHTTP2_STREAM_RESERVED, this function sets
+ * NGHTTP2_STREAM_FLAG_PUSH flag set.
+ *
  * This function returns a pointer to created new stream object, or
  * NULL.
  */

lib/nghttp2_stream.c

@@ -515,6 +515,7 @@ int nghttp2_stream_update_local_initial_window_size(
 
 void nghttp2_stream_promise_fulfilled(nghttp2_stream *stream) {
   stream->state = NGHTTP2_STREAM_OPENED;
+  stream->flags &= ~NGHTTP2_STREAM_FLAG_PUSH;
 }
 
 nghttp2_stream *nghttp2_stream_get_dep_root(nghttp2_stream *stream) {

lib/nghttp2_stream.h

@@ -84,7 +84,8 @@ typedef enum {
 
 typedef enum {
   NGHTTP2_STREAM_FLAG_NONE = 0,
-  /* Indicates that this stream is pushed stream */
+  /* Indicates that this stream is pushed stream and not opened
+     yet. */
   NGHTTP2_STREAM_FLAG_PUSH = 0x01,
   /* Indicates that this stream was closed */
   NGHTTP2_STREAM_FLAG_CLOSED = 0x02,

tests/main.c

@@ -255,6 +255,8 @@ int main(int argc _U_, char *argv[] _U_) {
                    test_nghttp2_session_delete_data_item) ||
       !CU_add_test(pSuite, "session_open_idle_stream",
                    test_nghttp2_session_open_idle_stream) ||
+      !CU_add_test(pSuite, "session_cancel_reserved_remote",
+                   test_nghttp2_session_cancel_reserved_remote) ||
       !CU_add_test(pSuite, "frame_pack_headers",
                    test_nghttp2_frame_pack_headers) ||
       !CU_add_test(pSuite, "frame_pack_headers_frame_too_large",

tests/nghttp2_session_test.c

@@ -1909,6 +1909,7 @@ void test_nghttp2_session_on_push_response_headers_received(void) {
   CU_ASSERT(1 == user_data.begin_headers_cb_called);
   CU_ASSERT(NGHTTP2_STREAM_OPENED == stream->state);
   CU_ASSERT(1 == session->num_incoming_streams);
+  CU_ASSERT(0 == (stream->flags & NGHTTP2_STREAM_FLAG_PUSH));
 
   /* If un-ACKed max concurrent streams limit is exceeded,
      RST_STREAMed */
@@ -2756,7 +2757,7 @@ void test_nghttp2_session_send_headers_push_reply(void) {
   CU_ASSERT(1 == session->num_outgoing_streams);
   stream = nghttp2_session_get_stream(session, 2);
   CU_ASSERT(NGHTTP2_STREAM_OPENED == stream->state);
-
+  CU_ASSERT(0 == (stream->flags & NGHTTP2_STREAM_FLAG_PUSH));
   nghttp2_session_del(session);
 }
 
@@ -6653,3 +6654,89 @@ void test_nghttp2_session_open_idle_stream(void) {
 
   nghttp2_session_del(session);
 }
+
+void test_nghttp2_session_cancel_reserved_remote(void) {
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  nghttp2_stream *stream;
+  nghttp2_frame frame;
+  const nghttp2_nv nv[] = {MAKE_NV(":status", "200")};
+  nghttp2_nv *nva;
+  ssize_t nvlen;
+  nghttp2_hd_deflater deflater;
+  nghttp2_mem *mem;
+  nghttp2_bufs bufs;
+  int rv;
+
+  mem = nghttp2_mem_default();
+  frame_pack_bufs_init(&bufs);
+
+  memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
+  callbacks.send_callback = null_send_callback;
+
+  nghttp2_session_client_new(&session, &callbacks, NULL);
+
+  nghttp2_hd_deflate_init(&deflater, mem);
+
+  stream = nghttp2_session_open_stream(session, 2, NGHTTP2_STREAM_FLAG_NONE,
+                                       &pri_spec_default,
+                                       NGHTTP2_STREAM_RESERVED, NULL);
+
+  session->last_recv_stream_id = 2;
+
+  nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, 2, NGHTTP2_CANCEL);
+
+  CU_ASSERT(NGHTTP2_STREAM_CLOSING == stream->state);
+
+  CU_ASSERT(0 == nghttp2_session_send(session));
+
+  nvlen = ARRLEN(nv);
+  nghttp2_nv_array_copy(&nva, nv, nvlen, mem);
+
+  nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 2,
+                             NGHTTP2_HCAT_PUSH_RESPONSE, NULL, nva, nvlen);
+  rv = nghttp2_frame_pack_headers(&bufs, &frame.headers, &deflater);
+
+  CU_ASSERT(0 == rv);
+
+  rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                nghttp2_buf_len(&bufs.head->buf));
+
+  CU_ASSERT(nghttp2_buf_len(&bufs.head->buf) == rv);
+
+  /* stream is not dangling, so assign NULL */
+  stream = NULL;
+
+  /* No RST_STREAM or GOAWAY is generated since stream should be in
+     NGHTTP2_STREAM_CLOSING and push response should be ignored. */
+  CU_ASSERT(0 == nghttp2_pq_size(&session->ob_pq));
+
+  /* Check that we can receive push response HEADERS while RST_STREAM
+     is just queued. */
+  nghttp2_session_open_stream(session, 4, NGHTTP2_STREAM_FLAG_NONE,
+                              &pri_spec_default, NGHTTP2_STREAM_RESERVED, NULL);
+
+  session->last_recv_stream_id = 4;
+
+  nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, 2, NGHTTP2_CANCEL);
+
+  nghttp2_bufs_reset(&bufs);
+
+  frame.hd.stream_id = 4;
+  rv = nghttp2_frame_pack_headers(&bufs, &frame.headers, &deflater);
+
+  CU_ASSERT(0 == rv);
+
+  rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos,
+                                nghttp2_buf_len(&bufs.head->buf));
+
+  CU_ASSERT(nghttp2_buf_len(&bufs.head->buf) == rv);
+
+  CU_ASSERT(1 == nghttp2_pq_size(&session->ob_pq));
+
+  nghttp2_frame_headers_free(&frame.headers, mem);
+
+  nghttp2_bufs_free(&bufs);
+
+  nghttp2_session_del(session);
+}

tests/nghttp2_session_test.h

@@ -120,5 +120,6 @@ void test_nghttp2_session_on_header_temporal_failure(void);
 void test_nghttp2_session_recv_client_preface(void);
 void test_nghttp2_session_delete_data_item(void);
 void test_nghttp2_session_open_idle_stream(void);
+void test_nghttp2_session_cancel_reserved_remote(void);
 
 #endif /* NGHTTP2_SESSION_TEST_H */