Strict error handling for frames which are not allowed after closed (remote)

This makes h2spec strict mode a bit happier.  We still one failing
test with h2spec -S (strict mode).

lib/nghttp2_session.c

@@ -3443,6 +3443,12 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session,
           "request HEADERS: invalid stream_id");
     }
 
+    stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id);
+    if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) {
+      return session_inflate_handle_invalid_connection(
+          session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed");
+    }
+
     return NGHTTP2_ERR_IGN_HEADER_BLOCK;
   }
   session->last_recv_stream_id = frame->hd.stream_id;
@@ -3721,6 +3727,9 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session,
     }
   }
 
+  /* We may use stream->shut_flags for strict error checking. */
+  nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
+
   rv = session_call_on_frame_received(session, frame);
   if (rv != 0) {
     return rv;
@@ -4165,7 +4174,11 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
         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);
     if (rv != 0) {
@@ -4173,25 +4186,13 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
     }
     return NGHTTP2_ERR_IGN_HEADER_BLOCK;
   }
+
   if (stream->shut_flags & NGHTTP2_SHUT_RD) {
-    if (session->callbacks.on_invalid_frame_recv_callback) {
+    return session_inflate_handle_invalid_connection(
-      if (session->callbacks.on_invalid_frame_recv_callback(
+        session, frame, NGHTTP2_ERR_STREAM_CLOSED,
-              session, frame, NGHTTP2_PROTOCOL_ERROR, session->user_data) !=
+        "PUSH_PROMISE: stream closed");
-          0) {
-        return NGHTTP2_ERR_CALLBACK_FAILURE;
-      }
-    }
-    rv = nghttp2_session_add_rst_stream(session,
-                                        frame->push_promise.promised_stream_id,
-                                        NGHTTP2_PROTOCOL_ERROR);
-    if (rv != 0) {
-      return rv;
-    }
-    return NGHTTP2_ERR_IGN_HEADER_BLOCK;
   }
 
-  /* TODO It is unclear reserved stream dpeneds on associated
-     stream with or without exclusive flag set */
   nghttp2_priority_spec_init(&pri_spec, stream->stream_id,
                              NGHTTP2_DEFAULT_WEIGHT, 0);
 
@@ -4632,6 +4633,14 @@ static int session_on_data_received_fail_fast(nghttp2_session *session) {
       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";
+      error_code = NGHTTP2_STREAM_CLOSED;
+      goto fail;
+    }
+
     return NGHTTP2_ERR_IGN_PAYLOAD;
   }
   if (stream->shut_flags & NGHTTP2_SHUT_RD) {

tests/main.c

@@ -128,6 +128,8 @@ int main(int argc _U_, char *argv[] _U_) {
                    test_nghttp2_session_on_window_update_received) ||
       !CU_add_test(pSuite, "session_on_data_received",
                    test_nghttp2_session_on_data_received) ||
+      !CU_add_test(pSuite, "session_on_data_received_fail_fast",
+                   test_nghttp2_session_on_data_received_fail_fast) ||
       !CU_add_test(pSuite, "session_send_headers_start_stream",
                    test_nghttp2_session_send_headers_start_stream) ||
       !CU_add_test(pSuite, "session_send_headers_reply",

tests/nghttp2_session_test.c

@@ -2249,6 +2249,25 @@ void test_nghttp2_session_on_request_headers_received(void) {
   nghttp2_frame_headers_free(&frame.headers, mem);
 
   nghttp2_session_del(session);
+
+  nghttp2_session_server_new(&session, &callbacks, &user_data);
+
+  /* HEADERS to closed stream */
+  stream = open_stream(session, 1);
+  session->last_recv_stream_id = 1;
+  nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
+  nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR);
+
+  nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 1,
+                             NGHTTP2_HCAT_REQUEST, NULL, NULL, 0);
+
+  CU_ASSERT(NGHTTP2_ERR_IGN_HEADER_BLOCK ==
+            nghttp2_session_on_request_headers_received(session, &frame));
+  CU_ASSERT(session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND);
+
+  nghttp2_frame_headers_free(&frame.headers, mem);
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_session_on_response_headers_received(void) {
@@ -2742,14 +2761,20 @@ void test_nghttp2_session_on_push_promise_received(void) {
   CU_ASSERT(1 == session->num_incoming_reserved_streams);
   CU_ASSERT(NULL == nghttp2_session_get_stream(session, 4));
   item = nghttp2_session_get_next_ob_item(session);
-  CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type);
+  CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type);
-  CU_ASSERT(4 == item->frame.hd.stream_id);
+  CU_ASSERT(NGHTTP2_STREAM_CLOSED == item->frame.goaway.error_code);
-  CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.rst_stream.error_code);
   CU_ASSERT(0 == nghttp2_session_send(session));
   CU_ASSERT(4 == session->last_recv_stream_id);
 
+  nghttp2_session_del(session);
+
+  nghttp2_session_client_new(&session, &callbacks, &user_data);
+
+  stream = nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE,
+                                       &pri_spec_default,
+                                       NGHTTP2_STREAM_OPENING, NULL);
+
   /* Attempt to PUSH_PROMISE against stream in closing state */
-  stream->shut_flags = NGHTTP2_SHUT_NONE;
   stream->state = NGHTTP2_STREAM_CLOSING;
   frame.push_promise.promised_stream_id = 6;
 
@@ -2759,7 +2784,7 @@ void test_nghttp2_session_on_push_promise_received(void) {
             nghttp2_session_on_push_promise_received(session, &frame));
 
   CU_ASSERT(0 == user_data.begin_headers_cb_called);
-  CU_ASSERT(1 == session->num_incoming_reserved_streams);
+  CU_ASSERT(0 == session->num_incoming_reserved_streams);
   CU_ASSERT(NULL == nghttp2_session_get_stream(session, 6));
   item = nghttp2_session_get_next_ob_item(session);
   CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type);
@@ -2767,7 +2792,7 @@ void test_nghttp2_session_on_push_promise_received(void) {
   CU_ASSERT(NGHTTP2_CANCEL == item->frame.rst_stream.error_code);
   CU_ASSERT(0 == nghttp2_session_send(session));
 
-  /* Attempt to PUSH_PROMISE against non-existent stream */
+  /* Attempt to PUSH_PROMISE against idle stream */
   frame.hd.stream_id = 3;
   frame.push_promise.promised_stream_id = 8;
 
@@ -2777,7 +2802,7 @@ void test_nghttp2_session_on_push_promise_received(void) {
             nghttp2_session_on_push_promise_received(session, &frame));
 
   CU_ASSERT(0 == user_data.begin_headers_cb_called);
-  CU_ASSERT(1 == session->num_incoming_reserved_streams);
+  CU_ASSERT(0 == session->num_incoming_reserved_streams);
   CU_ASSERT(NULL == nghttp2_session_get_stream(session, 8));
   item = nghttp2_session_get_next_ob_item(session);
   CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type);
@@ -3161,6 +3186,53 @@ void test_nghttp2_session_on_data_received(void) {
   nghttp2_session_del(session);
 }
 
+void test_nghttp2_session_on_data_received_fail_fast(void) {
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  uint8_t buf[9];
+  nghttp2_stream *stream;
+  nghttp2_frame_hd hd;
+  nghttp2_outbound_item *item;
+
+  memset(&callbacks, 0, sizeof(callbacks));
+
+  nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_NONE, 1);
+  nghttp2_frame_pack_frame_hd(buf, &hd);
+
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+
+  /* DATA to closed (remote) */
+  stream = open_stream(session, 1);
+  nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
+
+  CU_ASSERT((ssize_t)sizeof(buf) ==
+            nghttp2_session_mem_recv(session, buf, sizeof(buf)));
+
+  item = nghttp2_session_get_next_ob_item(session);
+
+  CU_ASSERT(NULL != item);
+  CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type);
+
+  nghttp2_session_del(session);
+
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+
+  /* DATA to closed stream with explicit closed (remote) */
+  stream = open_stream(session, 1);
+  nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
+  nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR);
+
+  CU_ASSERT((ssize_t)sizeof(buf) ==
+            nghttp2_session_mem_recv(session, buf, sizeof(buf)));
+
+  item = nghttp2_session_get_next_ob_item(session);
+
+  CU_ASSERT(NULL != item);
+  CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type);
+
+  nghttp2_session_del(session);
+}
+
 void test_nghttp2_session_send_headers_start_stream(void) {
   nghttp2_session *session;
   nghttp2_session_callbacks callbacks;

tests/nghttp2_session_test.h

@@ -54,6 +54,7 @@ void test_nghttp2_session_on_ping_received(void);
 void test_nghttp2_session_on_goaway_received(void);
 void test_nghttp2_session_on_window_update_received(void);
 void test_nghttp2_session_on_data_received(void);
+void test_nghttp2_session_on_data_received_fail_fast(void);
 void test_nghttp2_session_send_headers_start_stream(void);
 void test_nghttp2_session_send_headers_reply(void);
 void test_nghttp2_session_send_headers_frame_size_error(void);