Merge pull request #553 from h2o/kazuho/retire-cid-during-drop

retire_prior_to and lost NCID frames
h2o · Jun 27, 2023 · 46c35da · 46c35da
2 parents 55738d3 + 606668e
commit 46c35da
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 49 deletions.
diff --git a/include/quicly.h b/include/quicly.h
@@ -622,10 +622,6 @@ struct _st_quicly_conn_public_t {
             unsigned validated : 1;
             unsigned send_probe : 1;
         } address_validation;
-        /**
-         * largest value of Retire Prior To field observed so far
-         */
-        uint64_t largest_retire_prior_to;
     } remote;
     /**
      * Retains the original DCID used by the client. Servers use this to route packets incoming packets. Clients use this when

diff --git a/lib/quicly.c b/lib/quicly.c
@@ -2414,7 +2414,6 @@ static quicly_conn_t *create_connection(quicly_context_t *ctx, uint32_t protocol
     }
     conn->super.remote.transport_params = default_transport_params;
     conn->super.version = protocol_version;
-    conn->super.remote.largest_retire_prior_to = 0;
     quicly_linklist_init(&conn->super._default_scheduler.active);
     quicly_linklist_init(&conn->super._default_scheduler.blocked);
     conn->streams = kh_init(quicly_stream_t);
@@ -6154,17 +6153,6 @@ static int handle_new_connection_id_frame(quicly_conn_t *conn, struct st_quicly_
         PTLS_LOG_ELEMENT_HEXDUMP(stateless_reset_token, frame.stateless_reset_token, QUICLY_STATELESS_RESET_TOKEN_LEN);
     });
 
-    if (frame.sequence < conn->super.remote.largest_retire_prior_to) {
-        /* An endpoint that receives a NEW_CONNECTION_ID frame with a sequence number smaller than the Retire Prior To
-         * field of a previously received NEW_CONNECTION_ID frame MUST send a corresponding RETIRE_CONNECTION_ID frame
-         * that retires the newly received connection ID, unless it has already done so for that sequence number. (19.15)
-         * TODO: "unless ..." part may not be properly addressed here (we may already have sent the RCID frame for this
-         * sequence) */
-        retire_connection_id(conn, frame.sequence);
-        /* do not install this CID */
-        return 0;
-    }
-
     uint64_t unregistered_seqs[QUICLY_LOCAL_ACTIVE_CONNECTION_ID_LIMIT];
     size_t num_unregistered_seqs;
     if ((ret = quicly_remote_cid_register(&conn->super.remote.cid_set, frame.sequence, frame.cid.base, frame.cid.len,
@@ -6175,9 +6163,6 @@ static int handle_new_connection_id_frame(quicly_conn_t *conn, struct st_quicly_
     for (size_t i = 0; i < num_unregistered_seqs; i++)
         retire_connection_id(conn, unregistered_seqs[i]);
 
-    if (frame.retire_prior_to > conn->super.remote.largest_retire_prior_to)
-        conn->super.remote.largest_retire_prior_to = frame.retire_prior_to;
-
     return 0;
 }
 

diff --git a/lib/remote_cid.c b/lib/remote_cid.c
@@ -93,8 +93,6 @@ static int do_register(quicly_remote_cid_set_t *set, uint64_t sequence, const ui
 
 static void do_unregister(quicly_remote_cid_set_t *set, size_t idx_to_unreg)
 {
-    assert(set->cids[idx_to_unreg].state != QUICLY_REMOTE_CID_UNAVAILABLE);
-
     set->cids[idx_to_unreg].state = QUICLY_REMOTE_CID_UNAVAILABLE;
     set->cids[idx_to_unreg].sequence = ++set->_largest_sequence_expected;
 }
@@ -116,10 +114,9 @@ static size_t unregister_prior_to(quicly_remote_cid_set_t *set, uint64_t seq_unr
     size_t num_unregistered = 0;
 
     for (size_t i = 0; i < PTLS_ELEMENTSOF(set->cids); i++) {
-        if (set->cids[i].state != QUICLY_REMOTE_CID_UNAVAILABLE && set->cids[i].sequence < seq_unreg_prior_to) {
+        if (set->cids[i].sequence < seq_unreg_prior_to) {
             unregistered_seqs[num_unregistered++] = set->cids[i].sequence;
             do_unregister(set, i);
-            continue;
         }
     }
 

diff --git a/t/remote_cid.c b/t/remote_cid.c
@@ -27,27 +27,33 @@
 
 /* clang-format off */
 static uint8_t cids[][CID_LEN] = {
-	{0, 1, 2, 3, 4, 5, 6, 7}, /* 0 */
-	{1, 2, 3, 4, 5, 6, 7, 0},
-	{2, 3, 4, 5, 6, 7, 0, 1},
-	{3, 4, 5, 6, 7, 0, 1, 2},
-	{4, 5, 6, 7, 0, 1, 2, 3},
-	{5, 6, 7, 0, 1, 2, 3, 4},
-	{6, 7, 0, 1, 2, 3, 4, 5},
-	{7, 0, 1, 2, 3, 4, 5, 6},
-	{8, 9, 10, 11, 12, 13, 14, 15}, /* 8 */
+    {0, 1, 2, 3, 4, 5, 6, 7}, /* 0 */
+    {1, 2, 3, 4, 5, 6, 7, 0},
+    {2, 3, 4, 5, 6, 7, 0, 1},
+    {3, 4, 5, 6, 7, 0, 1, 2},
+    {4, 5, 6, 7, 0, 1, 2, 3},
+    {5, 6, 7, 0, 1, 2, 3, 4},
+    {6, 7, 0, 1, 2, 3, 4, 5},
+    {7, 0, 1, 2, 3, 4, 5, 6},
+    {8, 9, 10, 11, 12, 13, 14, 15}, /* 8 */
+    {9, 10, 11, 12, 13, 14, 15, 16},
+    {10, 11, 12, 13, 14, 15, 16, 17},
+    {11, 12, 13, 14, 15, 16, 17, 18},
 };
 
 static uint8_t srts[][QUICLY_STATELESS_RESET_TOKEN_LEN] = {
-	{0},
-	{1},
-	{2},
-	{3},
-	{4},
-	{5},
-	{6},
-	{7},
-	{8},
+    {0},
+    {1},
+    {2},
+    {3},
+    {4},
+    {5},
+    {6},
+    {7},
+    {8},
+    {9},
+    {10},
+    {11},
 };
 /* clang-format on */
 
@@ -146,17 +152,22 @@ void test_received_cid(void)
     TEST_SET({6, QUICLY_REMOTE_CID_UNAVAILABLE}, {7, QUICLY_REMOTE_CID_AVAILABLE}, {5, QUICLY_REMOTE_CID_AVAILABLE},
              {8, QUICLY_REMOTE_CID_AVAILABLE});
 
-    /* unregister prior to 8 -- seq=5,7 should be unregistered at this moment */
+    /* unregister prior to 8 -- seq=5-7 should be unregistered at this moment */
     ok(quicly_remote_cid_register(&set, 8, cids[8], CID_LEN, srts[8], 8, unregistered_seqs, &num_unregistered) == 0);
     /* active CIDs = {*8} */
-    ok(num_unregistered == 2);
+    ok(num_unregistered == 3);
     /* check unregistered_seqs */
-    ok(unregistered_seqs[0] == 7);
-    ok(unregistered_seqs[1] == 5);
-    /* active CIDs = {(6), (9), (10), 8} */
-    TEST_SET({6, QUICLY_REMOTE_CID_UNAVAILABLE}, {9, QUICLY_REMOTE_CID_UNAVAILABLE}, {10, QUICLY_REMOTE_CID_UNAVAILABLE},
+    ok(unregistered_seqs[0] == 6);
+    ok(unregistered_seqs[1] == 7);
+    ok(unregistered_seqs[2] == 5);
+    /* active CIDs = {(9), (10), (11), 8} */
+    TEST_SET({9, QUICLY_REMOTE_CID_UNAVAILABLE}, {10, QUICLY_REMOTE_CID_UNAVAILABLE}, {11, QUICLY_REMOTE_CID_UNAVAILABLE},
              {8, QUICLY_REMOTE_CID_AVAILABLE});
 
-    /* FIXME right above we receive NCID with retire_prior_to=8. Then, we should start accepting CIDs 8,9,10,11. But the code does
-     * not behave that way. */
+    /* register 11 */
+    ok(quicly_remote_cid_register(&set, 11, cids[11], CID_LEN, srts[11], 8, unregistered_seqs, &num_unregistered) == 0);
+    ok(num_unregistered == 0);
+    /* active CIDs = {(9), (10), (11), 8} */
+    TEST_SET({9, QUICLY_REMOTE_CID_UNAVAILABLE}, {10, QUICLY_REMOTE_CID_UNAVAILABLE}, {11, QUICLY_REMOTE_CID_AVAILABLE},
+             {8, QUICLY_REMOTE_CID_AVAILABLE});
 }