Check request/response submission error based side of session

Disallow request from server, and response from client respectively.
When the violation is detected, return NGHTTP2_ERR_PROTO from
nghttp2_submit_request, nghttp2_submit_response,
nghttp2_submit_headers.

We also did some refactoring, and now self-dependency detection is
placed where it is only required.

lib/includes/nghttp2/nghttp2.h

@@ -3164,6 +3164,8 @@ nghttp2_priority_spec_check_default(const nghttp2_priority_spec *pri_spec);
  * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT`
  *     Trying to depend on itself (new stream ID equals
  *     ``pri_spec->stream_id``).
+ * :enum:`NGHTTP2_ERR_PROTO`
+ *     The |session| is server session.
  *
  * .. warning::
  *
@@ -3236,6 +3238,8 @@ nghttp2_submit_request(nghttp2_session *session,
  *     processed yet.  Normally, this does not happen, but when
  *     application wrongly calls `nghttp2_submit_response()` twice,
  *     this may happen.
+ * :enum:`NGHTTP2_ERR_PROTO`
+ *     The |session| is client session.
  *
  * .. warning::
  *
@@ -3380,6 +3384,8 @@ NGHTTP2_EXTERN int nghttp2_submit_trailer(nghttp2_session *session,
  *     DATA or HEADERS has been already submitted and not fully
  *     processed yet.  This happens if stream denoted by |stream_id|
  *     is in reserved state.
+ * :enum:`NGHTTP2_ERR_PROTO`
+ *     The |stream_id| is -1, and |session| is server session.
  *
  * .. warning::
  *

lib/nghttp2_submit.c

@@ -32,6 +32,35 @@
 #include "nghttp2_helper.h"
 #include "nghttp2_priority_spec.h"
 
+/*
+ * Detects the dependency error, that is stream attempted to depend on
+ * itself.  If |stream_id| is -1, we use session->next_stream_id as
+ * stream ID.
+ *
+ * This function returns 0 if it succeeds, or one of the following
+ * error codes:
+ *
+ * NGHTTP2_ERR_INVALID_ARGUMENT
+ *   Stream attempted to depend on itself.
+ */
+static int detect_self_dependency(nghttp2_session *session, int32_t stream_id,
+                                  const nghttp2_priority_spec *pri_spec) {
+  assert(pri_spec);
+
+  if (stream_id == -1) {
+    if ((int32_t)session->next_stream_id == pri_spec->stream_id) {
+      return NGHTTP2_ERR_INVALID_ARGUMENT;
+    }
+    return 0;
+  }
+
+  if (stream_id == pri_spec->stream_id) {
+    return NGHTTP2_ERR_INVALID_ARGUMENT;
+  }
+
+  return 0;
+}
+
 /* This function takes ownership of |nva_copy|. Regardless of the
    return value, the caller must not free |nva_copy| after this
    function returns. */
@@ -50,21 +79,6 @@ static int32_t submit_headers_shared(nghttp2_session *session, uint8_t flags,
 
   mem = &session->mem;
 
-  if (stream_id == 0) {
-    rv = NGHTTP2_ERR_INVALID_ARGUMENT;
-    goto fail;
-  }
-
-  if (stream_id == -1) {
-    if ((int32_t)session->next_stream_id == pri_spec->stream_id) {
-      rv = NGHTTP2_ERR_INVALID_ARGUMENT;
-      goto fail;
-    }
-  } else if (stream_id == pri_spec->stream_id) {
-    rv = NGHTTP2_ERR_INVALID_ARGUMENT;
-    goto fail;
-  }
-
   item = nghttp2_mem_malloc(mem, sizeof(nghttp2_outbound_item));
   if (item == NULL) {
     rv = NGHTTP2_ERR_NOMEM;
@@ -156,6 +170,10 @@ static int32_t submit_headers_shared_nva(nghttp2_session *session,
 
 int nghttp2_submit_trailer(nghttp2_session *session, int32_t stream_id,
                            const nghttp2_nv *nva, size_t nvlen) {
+  if (stream_id <= 0) {
+    return NGHTTP2_ERR_INVALID_ARGUMENT;
+  }
+
   return (int)submit_headers_shared_nva(session, NGHTTP2_FLAG_END_STREAM,
                                         stream_id, NULL, nva, nvlen, NULL,
                                         NULL);
@@ -166,9 +184,24 @@ int32_t nghttp2_submit_headers(nghttp2_session *session, uint8_t flags,
                                const nghttp2_priority_spec *pri_spec,
                                const nghttp2_nv *nva, size_t nvlen,
                                void *stream_user_data) {
+  int rv;
+
+  if (stream_id == -1) {
+    if (session->server) {
+      return NGHTTP2_ERR_PROTO;
+    }
+  } else if (stream_id <= 0) {
+    return NGHTTP2_ERR_INVALID_ARGUMENT;
+  }
+
   flags &= NGHTTP2_FLAG_END_STREAM;
 
   if (pri_spec && !nghttp2_priority_spec_check_default(pri_spec)) {
+    rv = detect_self_dependency(session, stream_id, pri_spec);
+    if (rv != 0) {
+      return rv;
+    }
+
     flags |= NGHTTP2_FLAG_PRIORITY;
   } else {
     pri_spec = NULL;
@@ -281,7 +314,7 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags _U_,
 
   mem = &session->mem;
 
-  if (stream_id == 0 || nghttp2_session_is_my_stream_id(session, stream_id)) {
+  if (stream_id <= 0 || nghttp2_session_is_my_stream_id(session, stream_id)) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
 
@@ -396,8 +429,18 @@ int32_t nghttp2_submit_request(nghttp2_session *session,
                                const nghttp2_data_provider *data_prd,
                                void *stream_user_data) {
   uint8_t flags;
+  int rv;
 
-  if (pri_spec && nghttp2_priority_spec_check_default(pri_spec)) {
+  if (session->server) {
+    return NGHTTP2_ERR_PROTO;
+  }
+
+  if (pri_spec && !nghttp2_priority_spec_check_default(pri_spec)) {
+    rv = detect_self_dependency(session, -1, pri_spec);
+    if (rv != 0) {
+      return rv;
+    }
+  } else {
     pri_spec = NULL;
   }
 
@@ -418,7 +461,17 @@ static uint8_t set_response_flags(const nghttp2_data_provider *data_prd) {
 int nghttp2_submit_response(nghttp2_session *session, int32_t stream_id,
                             const nghttp2_nv *nva, size_t nvlen,
                             const nghttp2_data_provider *data_prd) {
-  uint8_t flags = set_response_flags(data_prd);
+  uint8_t flags;
+
+  if (stream_id <= 0) {
+    return NGHTTP2_ERR_INVALID_ARGUMENT;
+  }
+
+  if (!session->server) {
+    return NGHTTP2_ERR_PROTO;
+  }
+
+  flags = set_response_flags(data_prd);
   return submit_headers_shared_nva(session, flags, stream_id, NULL, nva, nvlen,
                                    data_prd, NULL);
 }

tests/nghttp2_session_test.c

@@ -3897,6 +3897,15 @@ void test_nghttp2_submit_request_with_data(void) {
   CU_ASSERT(0 == ud.data_source_length);
 
   nghttp2_session_del(session);
+
+  /* nghttp2_submit_request() with server session is error */
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+
+  CU_ASSERT(NGHTTP2_ERR_PROTO == nghttp2_submit_request(session, NULL, reqnv,
+                                                        ARRLEN(reqnv), NULL,
+                                                        NULL));
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_submit_request_without_data(void) {
@@ -3984,6 +3993,19 @@ void test_nghttp2_submit_response_with_data(void) {
   CU_ASSERT(0 == ud.data_source_length);
 
   nghttp2_session_del(session);
+
+  /* Various error cases */
+  nghttp2_session_client_new(&session, &callbacks, NULL);
+
+  /* Calling nghttp2_submit_response() with client session is error */
+  CU_ASSERT(NGHTTP2_ERR_PROTO ==
+            nghttp2_submit_response(session, 1, resnv, ARRLEN(resnv), NULL));
+
+  /* Stream ID <= 0 is error */
+  CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
+            nghttp2_submit_response(session, 0, resnv, ARRLEN(resnv), NULL));
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_submit_response_without_data(void) {
@@ -4115,6 +4137,18 @@ void test_nghttp2_submit_trailer(void) {
   nghttp2_frame_headers_free(&frame.headers, mem);
   nghttp2_hd_inflate_free(&inflater);
   nghttp2_session_del(session);
+
+  /* Specifying stream ID <= 0 is error */
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+  open_recv_stream(session, 1);
+
+  CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
+            nghttp2_submit_trailer(session, 0, trailernv, ARRLEN(trailernv)));
+
+  CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
+            nghttp2_submit_trailer(session, -1, trailernv, ARRLEN(trailernv)));
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_submit_headers_start_stream(void) {
@@ -4305,6 +4339,22 @@ void test_nghttp2_submit_headers(void) {
                                    reqnv, ARRLEN(reqnv), NULL));
 
   nghttp2_session_del(session);
+
+  /* Error cases with invalid stream ID */
+  nghttp2_session_server_new(&session, &callbacks, NULL);
+
+  /* Sending nghttp2_submit_headers() with stream_id == 1 and server
+     session is error */
+  CU_ASSERT(NGHTTP2_ERR_PROTO ==
+            nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL, reqnv,
+                                   ARRLEN(reqnv), NULL));
+
+  /* Sending stream ID <= 0 is error */
+  CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
+            nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, 0, NULL, resnv,
+                                   ARRLEN(resnv), NULL));
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_submit_headers_continuation(void) {
@@ -4642,7 +4692,12 @@ void test_nghttp2_submit_push_promise(void) {
   /* submit PUSH_PROMISE while associated stream is not opened */
   CU_ASSERT(NGHTTP2_ERR_STREAM_CLOSED ==
             nghttp2_submit_push_promise(session, NGHTTP2_FLAG_NONE, 3, reqnv,
-                                        ARRLEN(reqnv), &ud));
+                                        ARRLEN(reqnv), NULL));
+
+  /* Stream ID <= 0 is error */
+  CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
+            nghttp2_submit_push_promise(session, NGHTTP2_FLAG_NONE, 0, reqnv,
+                                        ARRLEN(reqnv), NULL));
 
   nghttp2_session_del(session);
 }
@@ -4850,19 +4905,10 @@ void test_nghttp2_submit_invalid_nv(void) {
 
   CU_ASSERT(0 == nghttp2_session_server_new(&session, &callbacks, NULL));
 
-  /* nghttp2_submit_request */
-  CU_ASSERT(0 < nghttp2_submit_request(session, NULL, empty_name_nv,
-                                       ARRLEN(empty_name_nv), NULL, NULL));
-
   /* nghttp2_submit_response */
   CU_ASSERT(0 == nghttp2_submit_response(session, 2, empty_name_nv,
                                          ARRLEN(empty_name_nv), NULL));
 
-  /* nghttp2_submit_headers */
-  CU_ASSERT(0 < nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL,
-                                       empty_name_nv, ARRLEN(empty_name_nv),
-                                       NULL));
-
   /* nghttp2_submit_push_promise */
   open_recv_stream(session, 1);
 
@@ -4871,6 +4917,19 @@ void test_nghttp2_submit_invalid_nv(void) {
                                             ARRLEN(empty_name_nv), NULL));
 
   nghttp2_session_del(session);
+
+  CU_ASSERT(0 == nghttp2_session_client_new(&session, &callbacks, NULL));
+
+  /* nghttp2_submit_request */
+  CU_ASSERT(0 < nghttp2_submit_request(session, NULL, empty_name_nv,
+                                       ARRLEN(empty_name_nv), NULL, NULL));
+
+  /* nghttp2_submit_headers */
+  CU_ASSERT(0 < nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL,
+                                       empty_name_nv, ARRLEN(empty_name_nv),
+                                       NULL));
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_session_open_stream(void) {