Allow duplicate settings ID in SETTINGS

If multiple same ID are found, use the last one.

lib/nghttp2_frame.c

@@ -703,17 +703,6 @@ nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv,
   return iv_copy;
 }
 
-static int nghttp2_settings_entry_compar(const void *lhs, const void *rhs)
-{
-  return ((nghttp2_settings_entry *)lhs)->settings_id
-    -((nghttp2_settings_entry *)rhs)->settings_id;
-}
-
-void nghttp2_frame_iv_sort(nghttp2_settings_entry *iv, size_t niv)
-{
-  qsort(iv, niv, sizeof(nghttp2_settings_entry), nghttp2_settings_entry_compar);
-}
-
 ssize_t nghttp2_frame_nv_offset(const uint8_t *head)
 {
   switch(head[2]) {
@@ -831,18 +820,28 @@ ssize_t nghttp2_nv_array_from_cstr(nghttp2_nv **nva_ptr, const char **nv)
   return nvlen;
 }
 
-int nghttp2_settings_check_duplicate(const nghttp2_settings_entry *iv,
+int nghttp2_iv_check(const nghttp2_settings_entry *iv, size_t niv,
-                                     size_t niv)
+                     int32_t flow_control_opt)
 {
-  int check[NGHTTP2_SETTINGS_MAX+1];
   size_t i;
-  memset(check, 0, sizeof(check));
   for(i = 0; i < niv; ++i) {
-    if(iv[i].settings_id > NGHTTP2_SETTINGS_MAX || iv[i].settings_id == 0 ||
+    switch(iv[i].settings_id) {
-       check[iv[i].settings_id] == 1) {
+    case NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE:
-      return 0;
+      if(iv[i].value > (uint32_t)NGHTTP2_MAX_WINDOW_SIZE) {
-    } else {
+        return 0;
-      check[iv[i].settings_id] = 1;
+      }
+      break;
+    case NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS:
+      if(flow_control_opt) {
+        if((iv[i].value & 0x1) == 0) {
+          /* Attempt to re-enabling flow-control is error */
+          return 0;
+        }
+      } else {
+        flow_control_opt = iv[i].value & 0x1;
+      }
+    default:
+      break;
     }
   }
   return 1;

lib/nghttp2_frame.h

@@ -573,13 +573,6 @@ char** nghttp2_frame_nv_norm_copy(const char **nv);
 nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv,
                                               size_t niv);
 
-/*
- * Sorts the |iv| with the ascending order of the settings_id member.
- * The number of the element in the array pointed by the |iv| is given
- * by the |niv|.
- */
-void nghttp2_frame_iv_sort(nghttp2_settings_entry *iv, size_t niv);
-
 /*
  * Returns the offset of the name/header block in the frame, including
  * frame header. The |head| is frame header. If the indicated frame
@@ -639,11 +632,12 @@ int nghttp2_nv_array_check_null(nghttp2_nv *nva, size_t nvlen);
 
 /*
  * Checks that the |iv|, which includes |niv| entries, does not have
- * duplicate IDs.
+ * invalid values. The |flow_control_opt| is current flow control
+ * option value.
  *
  * This function returns nonzero if it succeeds, or 0.
  */
-int nghttp2_settings_check_duplicate(const nghttp2_settings_entry *iv,
+int nghttp2_iv_check(const nghttp2_settings_entry *iv, size_t niv,
-                                     size_t niv);
+                     int32_t flow_control_opt);
 
 #endif /* NGHTTP2_FRAME_H */

lib/nghttp2_session.c

@@ -2044,6 +2044,7 @@ int nghttp2_session_update_local_settings(nghttp2_session *session,
     session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS];
   uint8_t new_flow_control = old_flow_control;
   int32_t new_initial_window_size = -1;
+  /* Use the value last seen. */
   for(i = 0; i < niv; ++i) {
     if(iv[i].settings_id == NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE) {
       new_initial_window_size = iv[i].value;
@@ -2061,8 +2062,9 @@ int nghttp2_session_update_local_settings(nghttp2_session *session,
     }
   }
   for(i = 0; i < niv; ++i) {
-    assert(iv[i].settings_id > 0 && iv[i].settings_id <= NGHTTP2_SETTINGS_MAX);
+    if(iv[i].settings_id > 0 && iv[i].settings_id <= NGHTTP2_SETTINGS_MAX) {
-    session->local_settings[iv[i].settings_id] = iv[i].value;
+      session->local_settings[iv[i].settings_id] = iv[i].value;
+    }
   }
   if(old_flow_control == 0 &&
      session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS]) {
@@ -2075,7 +2077,7 @@ int nghttp2_session_on_settings_received(nghttp2_session *session,
                                          nghttp2_frame *frame)
 {
   int rv;
-  size_t i;
+  int i;
   int check[NGHTTP2_SETTINGS_MAX+1];
   if(frame->hd.stream_id != 0) {
     return nghttp2_session_handle_invalid_connection(session, frame,
@@ -2083,11 +2085,13 @@ int nghttp2_session_on_settings_received(nghttp2_session *session,
   }
   /* Check ID/value pairs and persist them if necessary. */
   memset(check, 0, sizeof(check));
-  for(i = 0; i < frame->settings.niv; ++i) {
+  for(i = (int)frame->settings.niv - 1; i >= 0; --i) {
     nghttp2_settings_entry *entry = &frame->settings.iv[i];
-    /* The spec says if the multiple values for the same ID were
+    /* The spec says the settings values are processed in the order
-       found, use the first one and ignore the rest. */
+       they appear in the payload. In other words, if the multiple
-    if(entry->settings_id > NGHTTP2_SETTINGS_MAX || entry->settings_id == 0 ||
+       values for the same ID were found, use the last one and ignore
+       the rest. */
+    if(entry->settings_id > NGHTTP2_SETTINGS_MAX || entry->settings_id <= 0 ||
        check[entry->settings_id] == 1) {
       continue;
     }

lib/nghttp2_submit.c

@@ -164,7 +164,9 @@ int nghttp2_submit_settings(nghttp2_session *session,
   nghttp2_frame *frame;
   nghttp2_settings_entry *iv_copy;
   int r;
-  if(!nghttp2_settings_check_duplicate(iv, niv)) {
+  if(!nghttp2_iv_check(iv, niv,
+                       session->local_settings
+                       [NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS])) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
   frame = malloc(sizeof(nghttp2_frame));
@@ -176,7 +178,6 @@ int nghttp2_submit_settings(nghttp2_session *session,
     free(frame);
     return NGHTTP2_ERR_NOMEM;
   }
-  nghttp2_frame_iv_sort(iv_copy, niv);
   nghttp2_frame_settings_init(&frame->settings, iv_copy, niv);
 
   r = nghttp2_session_update_local_settings(session, iv_copy, niv);
@@ -361,9 +362,10 @@ int nghttp2_submit_data(nghttp2_session *session, uint8_t flags,
 ssize_t nghttp2_pack_settings_payload(uint8_t *buf,
                                       nghttp2_settings_entry *iv, size_t niv)
 {
-  if(!nghttp2_settings_check_duplicate(iv, niv)) {
+  /* Assume that current flow_control_option is 0 (which means that
+     flow control is enabled) */
+  if(!nghttp2_iv_check(iv, niv, 0)) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
-  nghttp2_frame_iv_sort(iv, niv);
   return nghttp2_frame_pack_settings_payload(buf, iv, niv);
 }

tests/main.c

@@ -218,8 +218,7 @@ int main(int argc, char* argv[])
                    test_nghttp2_frame_pack_window_update) ||
       !CU_add_test(pSuite, "nv_array_from_cstr",
                    test_nghttp2_nv_array_from_cstr) ||
-      !CU_add_test(pSuite, "settings_check_duplicate",
+      !CU_add_test(pSuite, "iv_check", test_nghttp2_iv_check) ||
-                   test_nghttp2_settings_check_duplicate) ||
       !CU_add_test(pSuite, "hd_deflate", test_nghttp2_hd_deflate) ||
       !CU_add_test(pSuite, "hd_inflate_indname_inc",
                    test_nghttp2_hd_inflate_indname_inc) ||

tests/nghttp2_frame_test.c

@@ -427,26 +427,28 @@ void test_nghttp2_nv_array_from_cstr(void)
   free(bigval);
 }
 
-void test_nghttp2_settings_check_duplicate(void)
+void test_nghttp2_iv_check(void)
 {
-  nghttp2_settings_entry set[3];
+  nghttp2_settings_entry iv[5];
 
-  set[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
+  iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
-  set[0].value = 1;
+  iv[0].value = 100;
-  set[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE;
+  iv[1].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS;
-  set[1].value = 1023;
+  iv[1].value = 0;
+  iv[2].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS;
+  iv[2].value = 1;
 
-  CU_ASSERT(nghttp2_settings_check_duplicate(set, 2));
+  CU_ASSERT(nghttp2_iv_check(iv, 2, 0));
+  CU_ASSERT(nghttp2_iv_check(iv, 3, 0));
+  /* Re-enabling flow-control*/
+  CU_ASSERT(0 == nghttp2_iv_check(iv, 2, 1));
+  CU_ASSERT(0 == nghttp2_iv_check(iv, 3, 1));
 
-  set[1] = set[0];
+  iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE;
-  CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 2));
+  iv[1].value = NGHTTP2_MAX_WINDOW_SIZE;
+  CU_ASSERT(nghttp2_iv_check(iv, 2, 0));
 
-  /* Out-of-bound data is error */
+  /* Too large window size */
-  set[0].settings_id = NGHTTP2_SETTINGS_MAX + 1;
+  iv[1].value = (uint32_t)NGHTTP2_MAX_WINDOW_SIZE + 1;
-  CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 1));
+  CU_ASSERT(0 == nghttp2_iv_check(iv, 2, 0));
-
-  /* settings_id == 0 is error */
-  set[0].settings_id = 0;
-  CU_ASSERT(0 == nghttp2_settings_check_duplicate(set, 1));
 }
-

tests/nghttp2_frame_test.h

@@ -38,6 +38,6 @@ void test_nghttp2_frame_pack_ping(void);
 void test_nghttp2_frame_pack_goaway(void);
 void test_nghttp2_frame_pack_window_update(void);
 void test_nghttp2_nv_array_from_cstr(void);
-void test_nghttp2_settings_check_duplicate(void);
+void test_nghttp2_iv_check(void);
 
 #endif /* NGHTTP2_FRAME_TEST_H */

tests/nghttp2_session_test.c

@@ -881,10 +881,10 @@ void test_nghttp2_session_on_settings_received(void)
   nghttp2_settings_entry iv[255];
 
   iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
-  iv[0].value = 1000000009;
+  iv[0].value = 50;
 
   iv[1].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
-  iv[1].value = 50;
+  iv[1].value = 1000000009;
 
   iv[2].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE;
   iv[2].value = 64*1024;
@@ -1900,10 +1900,10 @@ void test_nghttp2_submit_settings(void)
   my_user_data ud;
   nghttp2_outbound_item *item;
   nghttp2_frame *frame;
-  nghttp2_settings_entry iv[4];
+  nghttp2_settings_entry iv[5];
 
   iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
-  iv[0].value = 50;
+  iv[0].value = 5;
 
   iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE;
   iv[1].value = 16*1024;
@@ -1911,9 +1911,12 @@ void test_nghttp2_submit_settings(void)
   iv[2].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS;
   iv[2].value = 1;
 
-  /* This is duplicate entry */
   iv[3].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS;
-  iv[3].value = 150;
+  iv[3].value = 50;
+
+  /* Attempt to re-enable flow-control */
+  iv[4].settings_id = NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS;
+  iv[4].value = 0;
 
   memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
   callbacks.send_callback = null_send_callback;
@@ -1921,7 +1924,7 @@ void test_nghttp2_submit_settings(void)
   nghttp2_session_server_new(&session, &callbacks, &ud);
 
   CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT ==
-            nghttp2_submit_settings(session, iv, 4));
+            nghttp2_submit_settings(session, iv, 5));
 
   /* Make sure that local settings are not changed */
   CU_ASSERT(NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS ==
@@ -1931,8 +1934,8 @@ void test_nghttp2_submit_settings(void)
   CU_ASSERT(0 ==
             session->local_settings[NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS]);
 
-  /* Now sends without 4th one */
+  /* Now sends without 5th one */
-  CU_ASSERT(0 == nghttp2_submit_settings(session, iv, 3));
+  CU_ASSERT(0 == nghttp2_submit_settings(session, iv, 4));
 
   /* Make sure that local settings are changed */
   CU_ASSERT(50 ==
@@ -1948,8 +1951,8 @@ void test_nghttp2_submit_settings(void)
   CU_ASSERT(NGHTTP2_SETTINGS == OB_CTRL_TYPE(item));
 
   frame = item->frame;
-  CU_ASSERT(3 == frame->settings.niv);
+  CU_ASSERT(4 == frame->settings.niv);
-  CU_ASSERT(50 == frame->settings.iv[0].value);
+  CU_ASSERT(5 == frame->settings.iv[0].value);
   CU_ASSERT(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS ==
             frame->settings.iv[0].settings_id);
 
@@ -3224,10 +3227,10 @@ void test_nghttp2_pack_settings_payload(void)
   CU_ASSERT(0 == nghttp2_frame_unpack_settings_payload(&resiv, &resniv,
                                                        buf, len));
   CU_ASSERT(2 == resniv);
-  CU_ASSERT(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE == resiv[0].settings_id);
+  CU_ASSERT(NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS == resiv[0].settings_id);
-  CU_ASSERT(4095 == resiv[0].value);
+  CU_ASSERT(1 == resiv[0].value);
-  CU_ASSERT(NGHTTP2_SETTINGS_FLOW_CONTROL_OPTIONS == resiv[1].settings_id);
+  CU_ASSERT(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE == resiv[1].settings_id);
-  CU_ASSERT(1 == resiv[1].value);
+  CU_ASSERT(4095 == resiv[1].value);
 
   free(resiv);
 }