QUIC RCIDM: Minor updates

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/23022)

5 files changed, 53 insertions, 25 deletions

diff --git a/doc/designs/quic-design/glossary.md b/doc/designs/quic-design/glossary.md
index 3993c6f845..1cf0f06cdd 100644
--- a/doc/designs/quic-design/glossary.md
+++ b/doc/designs/quic-design/glossary.md
@@ -128,6 +128,12 @@ in which API calls can be made on different threads.
 **MSST:** Multi-stream single-thread. Refers to a type of multi-stream QUIC
 usage in which API calls must not be made concurrently.
 
+**NCID:** New Connection ID. Refers to a QUIC `NEW_CONNECTION_ID` frame.
+
+**Numbered CID:** Refers to a Connection ID which has a sequence number assigned
+to it. All CIDs other than Initial ODCIDs and Retry ODCIDs have a sequence
+number assigned. See also Unnumbered CID.
+
 **ODCID:** Original Destination CID. This is the DCID found in the first Initial
 packet sent by a client, and is used to generate the secrets for encrypting
 Initial packets. It is only used temporarily.
@@ -195,7 +201,8 @@ an API object. As such, a `QUIC_CONNECTION` is to a `QUIC_CHANNEL` what a
 `QUIC_XSO` is to a `QUIC_STREAM`.
 
 **RCID:** Remote CID. Refers to a CID which has been provided to us by a peer
-and which we can place in the DCID field of an outgoing packet. See also LCID.
+and which we can place in the DCID field of an outgoing packet. See also LCID,
+Unnumbered CID and Numbered CID.
 
 **RCIDM:** Remote CID Manager. Tracks RCIDs which have been provided to us by a
 peer. See also LCIDM.
@@ -284,6 +291,12 @@ to a *send stream buffer*, and that data is eventually TX'd by the TXP and QTX.)
 **Uni:** Abbreviation of unidirectional, referring to a QUIC unidirectional
 stream.
 
+**Unnumbered CID:** Refers to a CID which does not have a sequence number
+associated with it and therefore cannot be referred to by a `NEW_CONNECTION_ID`
+or `RETIRE_CONNECTION_ID` frame's sequence number fields. The only unnumbered
+CIDs are Initial ODCIDs and Retry ODCIDs. These CIDs are exceptionally retired
+automatically during handshake confirmation. See also Numbered CID.
+
 **URXE:** Unprocessed RX entry. Structure containing yet-undecrypted received
 datagrams pending processing. Stored in a queue known as the URXL.
 Ownership of URXEs is shared between DEMUX and QRX.
diff --git a/fuzz/quic-rcidm.c b/fuzz/quic-rcidm.c
index 51919b8d70..825fe0c2fd 100644
--- a/fuzz/quic-rcidm.c
+++ b/fuzz/quic-rcidm.c
@@ -129,7 +129,7 @@ int FuzzerTestOneInput(const uint8_t *buf, size_t len)
                 goto err;
             }
 
-            ossl_quic_rcidm_add_from_initial(rcidm, &arg_cid);
+            ossl_quic_rcidm_add_from_server_retry(rcidm, &arg_cid);
             break;
 
         case CMD_ADD_FROM_NCID:
diff --git a/include/internal/quic_rcidm.h b/include/internal/quic_rcidm.h
index 4837fed480..8eeaaf550e 100644
--- a/include/internal/quic_rcidm.h
+++ b/include/internal/quic_rcidm.h
@@ -22,10 +22,10 @@
  * QUIC Remote Connection ID Manager
  * =================================
  *
- * This manages connection IDs for the TX side, which  is to say that it tracks
- * remote CIDs (RCIDs) which a peer has issued to us and which we can use as the
- * DCID of packets we transmit. It is entirely separate from the LCIDM, which
- * handles routing received packets by their DCIDs.
+ * This manages connection IDs for the TX side. The RCIDM tracks remote CIDs
+ * (RCIDs) which a peer has issued to us and which we can use as the DCID of
+ * packets we transmit. It is entirely separate from the LCIDM, which handles
+ * routing received packets by their DCIDs.
  *
  * RCIDs fall into four categories:
  *
@@ -42,8 +42,8 @@ typedef struct quic_rcidm_st QUIC_RCIDM;
 /*
  * Creates a new RCIDM. Returns NULL on failure.
  *
- * For a client, initial_rcid is the client's Initial ODCID.
- * For a server, initial_rcid is NULL.
+ * For a client, initial_odcid is the client's Initial ODCID.
+ * For a server, initial_odcid is NULL.
  */
 QUIC_RCIDM *ossl_quic_rcidm_new(const QUIC_CONN_ID *initial_odcid);
 
@@ -135,12 +135,13 @@ void ossl_quic_rcidm_request_roll(QUIC_RCIDM *rcidm);
  * packets using the RCID may still be in flight. The caller must determine an
  * appropriate delay using knowledge of network conditions (RTT, etc.) which is
  * outside the scope of the RCIDM. The caller is responsible for implementing
- * this delay.
+ * this delay based on the last time a packet was transmitted using the RCID
+ * being retired.
  */
 int ossl_quic_rcidm_pop_retire_seq_num(QUIC_RCIDM *rcid, uint64_t *seq_num);
 
 /*
- * Like ossl_quic_rcidm_pop_retire_seek_num, but does not pop the item from the
+ * Like ossl_quic_rcidm_pop_retire_seq_num, but does not pop the item from the
  * queue. If this call succeeds, the next call to
  * ossl_quic_rcidm_pop_retire_seq_num is guaranteed to output the same sequence
  * number.
diff --git a/ssl/quic/quic_rcidm.c b/ssl/quic/quic_rcidm.c
index c2fd659609..90a4b2c2ae 100644
--- a/ssl/quic/quic_rcidm.c
+++ b/ssl/quic/quic_rcidm.c
@@ -37,7 +37,7 @@ static void rcidm_update(QUIC_RCIDM *rcidm);
 static void rcidm_set_preferred_rcid(QUIC_RCIDM *rcidm,
                                      const QUIC_CONN_ID *rcid);
 
-#define PACKETS_PER_RCID        1000
+#define PACKETS_PER_RCID        10000
 
 #define INITIAL_SEQ_NUM         0
 #define PREF_ADDR_SEQ_NUM       1
@@ -124,7 +124,7 @@ enum {
 };
 
 typedef struct rcid_st {
-    OSSL_LIST_MEMBER(retiring, struct rcid_st); /* valid iff retire == 1 */
+    OSSL_LIST_MEMBER(retiring, struct rcid_st); /* valid iff RETIRING */
 
     QUIC_CONN_ID    cid;        /* The actual CID string for this RCID */
     uint64_t        seq_num;
@@ -162,6 +162,11 @@ struct quic_rcidm_st {
     /*
      * The current RCID we prefer to use (value undefined if
      * !have_preferred_rcid).
+     *
+     * This is preferentially set to a numbered RCID (represented by an RCID
+     * object) if we have one (in which case preferred_rcid == cur_rcid->cid);
+     * otherwise it is set to one of the unnumbered RCIDs (the Initial ODCID or
+     * Retry ODCID) if available (and cur_rcid == NULL).
      */
     QUIC_CONN_ID                preferred_rcid;
 
@@ -189,11 +194,11 @@ struct quic_rcidm_st {
     PRIORITY_QUEUE_OF(RCID)     *rcids;
 
     /*
-     * Current RCID we are using. This may differ from the first item in the
-     * priority queue if we received NCID frames out of order. For example if we
-     * get seq 5, switch to it immediately, then get seq 4, we want to keep
-     * using seq 5 until we decide to roll again rather than immediately switch
-     * to seq 4. Never points to an object on the retiring_list.
+     * Current RCID object we are using. This may differ from the first item in
+     * the priority queue if we received NCID frames out of order. For example
+     * if we get seq 5, switch to it immediately, then get seq 4, we want to
+     * keep using seq 5 until we decide to roll again rather than immediately
+     * switch to seq 4. Never points to an object on the retiring_list.
      */
     RCID                        *cur_rcid;
 
@@ -229,6 +234,14 @@ struct quic_rcidm_st {
     unsigned int    roll_requested                  : 1;
 };
 
+/*
+ * Caller must periodically pop retired RCIDs and handle them. If the caller
+ * fails to do so, fail safely rather than start exhibiting integer rollover.
+ * Limit the total number of numbered RCIDs to an implausibly large but safe
+ * value.
+ */
+#define MAX_NUMBERED_RCIDS      (SIZE_MAX / 2)
+
 static void rcidm_transition_rcid(QUIC_RCIDM *rcidm, RCID *rcid,
                                   unsigned int state);
 
@@ -254,7 +267,6 @@ static void rcidm_check_rcid(QUIC_RCIDM *rcidm, RCID *rcid)
             || rcid->state == RCID_STATE_RETIRING);
     assert(rcidm->num_changes == 0 || rcidm->handshake_complete);
     assert(rcid->state != RCID_STATE_RETIRING || rcidm->num_retiring > 0);
-    assert(rcidm->num_retiring < SIZE_MAX / 2);
 }
 
 static int rcid_cmp(const RCID *a, const RCID *b)
@@ -263,10 +275,6 @@ static int rcid_cmp(const RCID *a, const RCID *b)
         return -1;
     if (a->seq_num > b->seq_num)
         return 1;
-    if ((uintptr_t)a < (uintptr_t)b)
-        return -1;
-    if ((uintptr_t)a > (uintptr_t)b)
-        return 1;
     return 0;
 }
 
@@ -337,7 +345,9 @@ static RCID *rcidm_create_rcid(QUIC_RCIDM *rcidm, uint64_t seq_num,
     RCID *rcid;
 
     if (cid->id_len < 1 || cid->id_len > QUIC_MAX_CONN_ID_LEN
-        || seq_num > OSSL_QUIC_VLINT_MAX)
+        || seq_num > OSSL_QUIC_VLINT_MAX
+        || ossl_pqueue_RCID_num(rcidm->rcids) + rcidm->num_retiring
+            > MAX_NUMBERED_RCIDS)
         return NULL;
 
     if ((rcid = OPENSSL_zalloc(sizeof(*rcid))) == NULL)
@@ -351,7 +361,6 @@ static RCID *rcidm_create_rcid(QUIC_RCIDM *rcidm, uint64_t seq_num,
         rcid->state = RCID_STATE_PENDING;
 
         if (!ossl_pqueue_RCID_push(rcidm->rcids, rcid, &rcid->pq_idx)) {
-            assert(0);
             OPENSSL_free(rcid);
             return NULL;
         }
@@ -501,7 +510,7 @@ static void rcidm_update(QUIC_RCIDM *rcidm)
      * If there are no RCIDs from NCID frames we can use, go through the various
      * kinds of bootstrapping RCIDs we can use in order of priority.
      */
-    if (rcidm->added_retry_odcid) {
+    if (rcidm->added_retry_odcid && !rcidm->handshake_complete) {
         rcidm_set_preferred_rcid(rcidm, &rcidm->retry_odcid);
         return;
     }
diff --git a/test/quic_rcidm_test.c b/test/quic_rcidm_test.c
index 3e841bed56..ad1c1ac588 100644
--- a/test/quic_rcidm_test.c
+++ b/test/quic_rcidm_test.c
@@ -16,6 +16,11 @@ static const QUIC_CONN_ID cid8_3 = { 8, { 3 } };
 static const QUIC_CONN_ID cid8_4 = { 8, { 4 } };
 static const QUIC_CONN_ID cid8_5 = { 8, { 5 } };
 
+/*
+ * 0: Client, Initial ODCID
+ * 1: Client, Initial ODCID + Retry ODCID
+ * 2: Server, doesn't start with Initial ODCID
+ */
 static int test_rcidm(int idx)
 {
     int testresult = 0;