From 21d5986157688601abb35e22d421107a4329728e Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Sun, 30 Mar 2014 18:07:52 +0900
Subject: [PATCH] Fail nghttp2_submit_settings if there is pending SETTINGS
 frame in-flight

pending_local_max_concurrent_stream is now set in
nghttp2_session_add_settings, rather than after frame was sent.
---
 lib/includes/nghttp2/nghttp2.h |  4 ++
 lib/nghttp2_session.c          | 80 ++++++++++++++++++++--------------
 tests/nghttp2_session_test.c   |  2 -
 3 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h
index f4ae44d0..b4291164 100644
--- a/lib/includes/nghttp2/nghttp2.h
+++ b/lib/includes/nghttp2/nghttp2.h
@@ -2259,6 +2259,10 @@ int nghttp2_submit_rst_stream(nghttp2_session *session, uint8_t flags,
  * :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.
  */
diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c
index ddaa809b..ca2e7d6b 100644
--- a/lib/nghttp2_session.c
+++ b/lib/nghttp2_session.c
@@ -1315,21 +1315,11 @@ static int nghttp2_session_predicate_window_update_send
 /*
  * This function checks SETTINGS can be sent at this time.
  *
- * This function returns 0 if it succeeds, or one of the following
- * negative error codes:
- *
- * 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.
+ * Currently this function always returns 0.
  */
 static int nghttp2_session_predicate_settings_send(nghttp2_session *session,
                                                    nghttp2_frame *frame)
 {
-  if((frame->hd.flags & NGHTTP2_FLAG_ACK) == 0 &&
-     session->inflight_niv != -1) {
-    return NGHTTP2_ERR_TOO_MANY_INFLIGHT_SETTINGS;
-  }
   return 0;
 }
 
@@ -2024,28 +2014,9 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session)
         return rv;
       }
       break;
-    case NGHTTP2_SETTINGS: {
-      size_t i;
-      if(frame->hd.flags & NGHTTP2_FLAG_ACK) {
-        break;
-      }
-      /* Extract NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS here and use
-         it to refuse the incoming streams with RST_STREAM. */
-      for(i = frame->settings.niv; i > 0; --i) {
-        if(frame->settings.iv[i - 1].settings_id ==
-           NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS) {
-          session->pending_local_max_concurrent_stream =
-            frame->settings.iv[i - 1].value;
-          break;
-        }
-      }
-      assert(session->inflight_niv == -1);
-      session->inflight_iv = frame->settings.iv;
-      session->inflight_niv = frame->settings.niv;
-      frame->settings.iv = NULL;
-      frame->settings.niv = 0;
+    case NGHTTP2_SETTINGS:
+      /* nothing to do */
       break;
-    }
     case NGHTTP2_PUSH_PROMISE:
       /* nothing to do */
       break;
@@ -5204,7 +5175,17 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
 {
   nghttp2_frame *frame;
   nghttp2_settings_entry *iv_copy;
+  size_t i;
   int rv;
+
+  if(flags & NGHTTP2_FLAG_ACK) {
+    if(niv != 0) {
+      return NGHTTP2_ERR_INVALID_ARGUMENT;
+    }
+  } else if(session->inflight_niv != -1) {
+    return NGHTTP2_ERR_TOO_MANY_INFLIGHT_SETTINGS;
+  }
+
   if(!nghttp2_iv_check(iv, niv)) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
@@ -5217,15 +5198,50 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
     free(frame);
     return NGHTTP2_ERR_NOMEM;
   }
+
+  if((flags & NGHTTP2_FLAG_ACK) == 0) {
+    if(niv > 0) {
+      session->inflight_iv = nghttp2_frame_iv_copy(iv, niv);
+
+      if(session->inflight_iv == NULL) {
+        free(iv_copy);
+        free(frame);
+        return NGHTTP2_ERR_NOMEM;
+      }
+    } else {
+      session->inflight_iv = NULL;
+    }
+
+    session->inflight_niv = niv;
+  }
+
   nghttp2_frame_settings_init(&frame->settings, flags, iv_copy, niv);
   rv = nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, NULL);
   if(rv != 0) {
     /* The only expected error is fatal one */
     assert(nghttp2_is_fatal(rv));
+
+    if((flags & NGHTTP2_FLAG_ACK) == 0) {
+      free(session->inflight_iv);
+      session->inflight_iv = NULL;
+      session->inflight_niv = -1;
+    }
+
     nghttp2_frame_settings_free(&frame->settings);
     free(frame);
+
     return rv;
   }
+
+  /* Extract NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS here and use
+     it to refuse the incoming streams with RST_STREAM. */
+  for(i = niv; i > 0; --i) {
+    if(iv[i - 1].settings_id == NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS) {
+      session->pending_local_max_concurrent_stream = iv[i - 1].value;
+      break;
+    }
+  }
+
   return 0;
 }
 
diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c
index c421ea6e..115db74f 100644
--- a/tests/nghttp2_session_test.c
+++ b/tests/nghttp2_session_test.c
@@ -5355,8 +5355,6 @@ void test_nghttp2_session_keep_closed_stream(void)
 
   nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, &iv, 1);
 
-  nghttp2_session_send(session);
-
   for(i = 0; i < max_concurrent_streams; ++i) {
     open_stream(session, i * 2 + 1);
   }