Allow multiple in-flight SETTINGS

lib/includes/nghttp2/nghttp2.h

@@ -3278,10 +3278,6 @@ NGHTTP2_EXTERN int nghttp2_submit_rst_stream(nghttp2_session *session,
  * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT`
  *     The |iv| contains invalid value (e.g., initial window size
  *     strictly greater than (1 << 31) - 1.
- * :enum:`NGHTTP2_ERR_TOO_MANY_INFLIGHT_SETTINGS`
- *     There is already another in-flight SETTINGS.  Note that the
- *     current implementation only allows 1 in-flight SETTINGS frame
- *     without ACK flag set.
  * :enum:`NGHTTP2_ERR_NOMEM`
  *     Out of memory.
  */

lib/nghttp2_session.c

@@ -362,8 +362,6 @@ static int session_new(nghttp2_session **session_ptr,
   (*session_ptr)->local_last_stream_id = (1u << 31) - 1;
   (*session_ptr)->remote_last_stream_id = (1u << 31) - 1;
 
-  (*session_ptr)->inflight_niv = -1;
-
   (*session_ptr)->pending_local_max_concurrent_stream =
       NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS;
   (*session_ptr)->pending_enable_push = 1;
@@ -561,8 +559,42 @@ static void ob_q_free(nghttp2_outbound_queue *q, nghttp2_mem *mem) {
   }
 }
 
+static int inflight_settings_new(nghttp2_inflight_settings **settings_ptr,
+                                 const nghttp2_settings_entry *iv, size_t niv,
+                                 nghttp2_mem *mem) {
+  *settings_ptr = nghttp2_mem_malloc(mem, sizeof(nghttp2_inflight_settings));
+  if (!*settings_ptr) {
+    return NGHTTP2_ERR_NOMEM;
+  }
+
+  if (niv > 0) {
+    (*settings_ptr)->iv = nghttp2_frame_iv_copy(iv, niv, mem);
+    if (!(*settings_ptr)->iv) {
+      return NGHTTP2_ERR_NOMEM;
+    }
+  } else {
+    (*settings_ptr)->iv = NULL;
+  }
+
+  (*settings_ptr)->niv = niv;
+  (*settings_ptr)->next = NULL;
+
+  return 0;
+}
+
+static void inflight_settings_del(nghttp2_inflight_settings *settings,
+                                  nghttp2_mem *mem) {
+  if (!settings) {
+    return;
+  }
+
+  nghttp2_mem_free(mem, settings->iv);
+  nghttp2_mem_free(mem, settings);
+}
+
 void nghttp2_session_del(nghttp2_session *session) {
   nghttp2_mem *mem;
+  nghttp2_inflight_settings *settings;
 
   if (session == NULL) {
     return;
@@ -570,7 +602,11 @@ void nghttp2_session_del(nghttp2_session *session) {
 
   mem = &session->mem;
 
-  nghttp2_mem_free(mem, session->inflight_iv);
+  for (settings = session->inflight_settings_head; settings;) {
+    nghttp2_inflight_settings *next = settings->next;
+    inflight_settings_del(settings, mem);
+    settings = next;
+  }
 
   nghttp2_stream_roots_free(&session->roots);
 
@@ -3939,10 +3975,6 @@ int nghttp2_session_update_local_settings(nghttp2_session *session,
     }
   }
 
-  session->pending_local_max_concurrent_stream =
-      NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS;
-  session->pending_enable_push = 1;
-
   return 0;
 }
 
@@ -3951,6 +3983,7 @@ int nghttp2_session_on_settings_received(nghttp2_session *session,
   int rv;
   size_t i;
   nghttp2_mem *mem;
+  nghttp2_inflight_settings *settings;
 
   mem = &session->mem;
 
@@ -3964,15 +3997,21 @@ int nghttp2_session_on_settings_received(nghttp2_session *session,
           session, frame, NGHTTP2_ERR_FRAME_SIZE_ERROR,
           "SETTINGS: ACK and payload != 0");
     }
-    if (session->inflight_niv == -1) {
+
+    settings = session->inflight_settings_head;
+
+    if (!settings) {
       return session_handle_invalid_connection(
           session, frame, NGHTTP2_ERR_PROTO, "SETTINGS: unexpected ACK");
     }
-    rv = nghttp2_session_update_local_settings(session, session->inflight_iv,
+
-                                               session->inflight_niv);
+    rv = nghttp2_session_update_local_settings(session, settings->iv,
-    nghttp2_mem_free(mem, session->inflight_iv);
+                                               settings->niv);
-    session->inflight_iv = NULL;
+
-    session->inflight_niv = -1;
+    session->inflight_settings_head = settings->next;
+
+    inflight_settings_del(settings, mem);
+
     if (rv != 0) {
       if (nghttp2_is_fatal(rv)) {
         return rv;
@@ -6091,6 +6130,22 @@ int nghttp2_session_add_window_update(nghttp2_session *session, uint8_t flags,
   return 0;
 }
 
+static void
+session_append_inflight_settings(nghttp2_session *session,
+                                 nghttp2_inflight_settings *settings) {
+  nghttp2_inflight_settings *i;
+
+  if (!session->inflight_settings_head) {
+    session->inflight_settings_head = settings;
+    return;
+  }
+
+  for (i = session->inflight_settings_head; i->next; i = i->next)
+    ;
+
+  i->next = settings;
+}
+
 int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
                                  const nghttp2_settings_entry *iv, size_t niv) {
   nghttp2_outbound_item *item;
@@ -6099,15 +6154,12 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
   size_t i;
   int rv;
   nghttp2_mem *mem;
+  nghttp2_inflight_settings *inflight_settings = NULL;
 
   mem = &session->mem;
 
-  if (flags & NGHTTP2_FLAG_ACK) {
+  if ((flags & NGHTTP2_FLAG_ACK) && niv != 0) {
-    if (niv != 0) {
+    return NGHTTP2_ERR_INVALID_ARGUMENT;
-      return NGHTTP2_ERR_INVALID_ARGUMENT;
-    }
-  } else if (session->inflight_niv != -1) {
-    return NGHTTP2_ERR_TOO_MANY_INFLIGHT_SETTINGS;
   }
 
   if (!nghttp2_iv_check(iv, niv)) {
@@ -6130,19 +6182,13 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
   }
 
   if ((flags & NGHTTP2_FLAG_ACK) == 0) {
-    if (niv > 0) {
+    rv = inflight_settings_new(&inflight_settings, iv, niv, mem);
-      session->inflight_iv = nghttp2_frame_iv_copy(iv, niv, mem);
+    if (rv != 0) {
-
+      assert(nghttp2_is_fatal(rv));
-      if (session->inflight_iv == NULL) {
+      nghttp2_mem_free(mem, iv_copy);
-        nghttp2_mem_free(mem, iv_copy);
+      nghttp2_mem_free(mem, item);
-        nghttp2_mem_free(mem, item);
+      return rv;
-        return NGHTTP2_ERR_NOMEM;
-      }
-    } else {
-      session->inflight_iv = NULL;
     }
-
-    session->inflight_niv = niv;
   }
 
   nghttp2_outbound_item_init(item);
@@ -6155,11 +6201,7 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
     /* The only expected error is fatal one */
     assert(nghttp2_is_fatal(rv));
 
-    if ((flags & NGHTTP2_FLAG_ACK) == 0) {
+    inflight_settings_del(inflight_settings, mem);
-      nghttp2_mem_free(mem, session->inflight_iv);
-      session->inflight_iv = NULL;
-      session->inflight_niv = -1;
-    }
 
     nghttp2_frame_settings_free(&frame->settings, mem);
     nghttp2_mem_free(mem, item);
@@ -6167,6 +6209,8 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
     return rv;
   }
 
+  session_append_inflight_settings(session, inflight_settings);
+
   /* Extract NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS and ENABLE_PUSH
      here.  We use it to refuse the incoming stream and PUSH_PROMISE
      with RST_STREAM. */

lib/nghttp2_session.h

@@ -140,6 +140,17 @@ typedef enum {
   NGHTTP2_GOAWAY_RECV = 0x8
 } nghttp2_goaway_flag;
 
+/* nghttp2_inflight_settings stores the SETTINGS entries which local
+   endpoint has sent to the remote endpoint, and has not received ACK
+   yet. */
+struct nghttp2_inflight_settings {
+  struct nghttp2_inflight_settings *next;
+  nghttp2_settings_entry *iv;
+  size_t niv;
+};
+
+typedef struct nghttp2_inflight_settings nghttp2_inflight_settings;
+
 struct nghttp2_session {
   nghttp2_map /* <nghttp2_stream*> */ streams;
   nghttp2_stream_roots roots;
@@ -176,12 +187,9 @@ struct nghttp2_session {
   /* Points to the oldest idle stream.  NULL if there is no idle
      stream.  Only used when session is initialized as erver. */
   nghttp2_stream *idle_stream_tail;
-  /* In-flight SETTINGS values. NULL does not necessarily mean there
+  /* Queue of In-flight SETTINGS values.  SETTINGS bearing ACK is not
-     is no in-flight SETTINGS. */
+     considered as in-flight. */
-  nghttp2_settings_entry *inflight_iv;
+  nghttp2_inflight_settings *inflight_settings_head;
-  /* The number of entries in |inflight_iv|. -1 if there is no
-     in-flight SETTINGS. */
-  ssize_t inflight_niv;
   /* The number of outgoing streams. This will be capped by
      remote_settings.max_concurrent_streams. */
   size_t num_outgoing_streams;

tests/main.c

@@ -171,6 +171,8 @@ int main(int argc _U_, char *argv[] _U_) {
                    test_nghttp2_submit_settings) ||
       !CU_add_test(pSuite, "session_submit_settings_update_local_window_size",
                    test_nghttp2_submit_settings_update_local_window_size) ||
+      !CU_add_test(pSuite, "session_submit_settings_multiple_times",
+                   test_nghttp2_submit_settings_multiple_times) ||
       !CU_add_test(pSuite, "session_submit_push_promise",
                    test_nghttp2_submit_push_promise) ||
       !CU_add_test(pSuite, "submit_window_update",

tests/nghttp2_session_test.c

@@ -2300,18 +2300,11 @@ void test_nghttp2_session_on_settings_received(void) {
   nghttp2_session_server_new(&session, &callbacks, NULL);
   nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_ACK, dup_iv(iv, 1),
                               1);
-  /* Specify inflight_iv deliberately */
-  session->inflight_iv = frame.settings.iv;
-  session->inflight_niv = frame.settings.niv;
-
   CU_ASSERT(0 == nghttp2_session_on_settings_received(session, &frame, 0));
   item = nghttp2_session_get_next_ob_item(session);
   CU_ASSERT(item != NULL);
   CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type);
 
-  session->inflight_iv = NULL;
-  session->inflight_niv = -1;
-
   nghttp2_frame_settings_free(&frame.settings, mem);
   nghttp2_session_del(session);
 
@@ -4041,8 +4034,8 @@ void test_nghttp2_submit_settings(void) {
   CU_ASSERT(16 * 1024 == session->local_settings.initial_window_size);
   CU_ASSERT(0 == session->hd_inflater.ctx.hd_table_bufsize_max);
   CU_ASSERT(50 == session->local_settings.max_concurrent_streams);
-  CU_ASSERT(NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS ==
+  /* We just keep the last seen value */
-            session->pending_local_max_concurrent_stream);
+  CU_ASSERT(50 == session->pending_local_max_concurrent_stream);
 
   nghttp2_session_del(session);
 }
@@ -4113,6 +4106,86 @@ void test_nghttp2_submit_settings_update_local_window_size(void) {
   nghttp2_frame_settings_free(&ack_frame.settings, mem);
 }
 
+void test_nghttp2_submit_settings_multiple_times(void) {
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  nghttp2_settings_entry iv[4];
+  nghttp2_frame frame;
+  nghttp2_mem *mem;
+  nghttp2_inflight_settings *inflight_settings;
+
+  mem = nghttp2_mem_default();
+
+  memset(&callbacks, 0, sizeof(callbacks));
+  callbacks.send_callback = null_send_callback;
+
+  nghttp2_session_client_new(&session, &callbacks, NULL);
+
+  /* first SETTINGS */
+  iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
+  iv[0].value = 100;
+
+  iv[1].settings_id = NGHTTP2_SETTINGS_ENABLE_PUSH;
+  iv[1].value = 0;
+
+  CU_ASSERT(0 == nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 2));
+
+  inflight_settings = session->inflight_settings_head;
+
+  CU_ASSERT(NULL != inflight_settings);
+  CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS ==
+            inflight_settings->iv[0].settings_id);
+  CU_ASSERT(100 == inflight_settings->iv[0].value);
+  CU_ASSERT(2 == inflight_settings->niv);
+  CU_ASSERT(NULL == inflight_settings->next);
+
+  CU_ASSERT(100 == session->pending_local_max_concurrent_stream);
+  CU_ASSERT(0 == session->pending_enable_push);
+
+  /* second SETTINGS */
+  iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
+  iv[0].value = 99;
+
+  CU_ASSERT(0 == nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, iv, 1));
+
+  inflight_settings = session->inflight_settings_head->next;
+
+  CU_ASSERT(NULL != inflight_settings);
+  CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS ==
+            inflight_settings->iv[0].settings_id);
+  CU_ASSERT(99 == inflight_settings->iv[0].value);
+  CU_ASSERT(1 == inflight_settings->niv);
+  CU_ASSERT(NULL == inflight_settings->next);
+
+  CU_ASSERT(99 == session->pending_local_max_concurrent_stream);
+  CU_ASSERT(0 == session->pending_enable_push);
+
+  nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_ACK, NULL, 0);
+
+  /* receive SETTINGS ACK */
+  CU_ASSERT(0 == nghttp2_session_on_settings_received(session, &frame, 0));
+
+  inflight_settings = session->inflight_settings_head;
+
+  /* first inflight SETTINGS was removed */
+  CU_ASSERT(NULL != inflight_settings);
+  CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS ==
+            inflight_settings->iv[0].settings_id);
+  CU_ASSERT(99 == inflight_settings->iv[0].value);
+  CU_ASSERT(1 == inflight_settings->niv);
+  CU_ASSERT(NULL == inflight_settings->next);
+
+  CU_ASSERT(100 == session->local_settings.max_concurrent_streams);
+
+  /* receive SETTINGS ACK again */
+  CU_ASSERT(0 == nghttp2_session_on_settings_received(session, &frame, 0));
+
+  CU_ASSERT(NULL == session->inflight_settings_head);
+  CU_ASSERT(99 == session->local_settings.max_concurrent_streams);
+
+  nghttp2_session_del(session);
+}
+
 void test_nghttp2_submit_push_promise(void) {
   nghttp2_session *session;
   nghttp2_session_callbacks callbacks;

tests/nghttp2_session_test.h

@@ -79,6 +79,7 @@ void test_nghttp2_submit_headers_continuation(void);
 void test_nghttp2_submit_priority(void);
 void test_nghttp2_submit_settings(void);
 void test_nghttp2_submit_settings_update_local_window_size(void);
+void test_nghttp2_submit_settings_multiple_times(void);
 void test_nghttp2_submit_push_promise(void);
 void test_nghttp2_submit_window_update(void);
 void test_nghttp2_submit_window_update_local_window_size(void);