Skip to content

Commit c27a516

Browse files
dreamwind1985dreamwind.ll
dreamwind1985
authored and
dreamwind.ll
committed
[~] #776 harden original CID accounting: add idempotency guard, fix delete_cid leak, update sender-side supply check, clear is_original on state transition
1 parent bfac44d commit c27a516

6 files changed

Lines changed: 122 additions & 9 deletions

File tree

‎src/transport/xqc_cid.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -276,9 +276,7 @@ xqc_cid_set_insert_cid(xqc_cid_set_t *cid_set,
276276
* parameter does not include the connection ID negotiated during the
277277
* handshake." Subtract original (handshake) CIDs from the active count.
278278
*/
279-
uint64_t active_cid_cnt = inner_set->unused_cnt + inner_set->used_cnt;
280-
uint64_t countable = (active_cid_cnt > inner_set->original_cid_cnt)
281-
? (active_cid_cnt - inner_set->original_cid_cnt) : 0;
279+
uint64_t countable = xqc_cid_set_countable_cnt(inner_set);
282280
if (countable >= limit) {
283281
return -XQC_EACTIVE_CID_LIMIT;
284282
}
@@ -339,6 +337,10 @@ xqc_cid_set_delete_cid(xqc_cid_set_t *cid_set, xqc_cid_t *cid, uint64_t path_id)
339337
inner_set->retired_cnt--;
340338
}
341339

340+
if (inner_cid->is_original && inner_set->original_cid_cnt > 0) {
341+
inner_set->original_cid_cnt--;
342+
}
343+
342344
xqc_list_del(pos);
343345
xqc_free(inner_cid);
344346
return XQC_OK;
@@ -442,6 +444,7 @@ xqc_cid_switch_to_next_state(xqc_cid_set_t *cid_set, xqc_cid_inner_t *cid, xqc_c
442444
if (inner_set->original_cid_cnt > 0) {
443445
inner_set->original_cid_cnt--;
444446
}
447+
cid->is_original = 0;
445448
}
446449

447450
cid->state = next_state;

‎src/transport/xqc_cid.h‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ static inline void
118118
xqc_cid_set_mark_original(xqc_cid_set_t *cid_set, xqc_cid_t *cid, uint64_t path_id)
119119
{
120120
xqc_cid_inner_t *inner = xqc_cid_in_cid_set(cid_set, cid, path_id);
121-
if (inner) {
121+
if (inner && !inner->is_original) {
122122
inner->is_original = 1;
123123
xqc_cid_set_inner_t *s = xqc_get_path_cid_set(cid_set, path_id);
124124
if (s) {
@@ -127,6 +127,19 @@ xqc_cid_set_mark_original(xqc_cid_set_t *cid_set, xqc_cid_t *cid, uint64_t path_
127127
}
128128
}
129129

130+
/*
131+
* Return the number of CIDs that count toward active_connection_id_limit.
132+
* Per RFC 9000 §5.1.1, handshake (original) CIDs are excluded.
133+
* Uses saturating subtraction to avoid uint64_t underflow.
134+
*/
135+
static inline uint64_t
136+
xqc_cid_set_countable_cnt(xqc_cid_set_inner_t *inner_set)
137+
{
138+
uint64_t active = inner_set->unused_cnt + inner_set->used_cnt;
139+
return (active > inner_set->original_cid_cnt)
140+
? (active - inner_set->original_cid_cnt) : 0;
141+
}
130142

131143
#endif /* _XQC_CID_H_INCLUDED_ */
132144

145+

‎src/transport/xqc_conn.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5260,7 +5260,7 @@ xqc_conn_try_add_new_conn_id(xqc_connection_t *conn, uint64_t retire_prior_to)
52605260
/* principle #1 there are two CIDs for the next path ID */
52615261
inner_set = xqc_get_next_unused_path_cid_set(&conn->scid_set);
52625262
while (inner_set
5263-
&& (inner_set->unused_cnt + inner_set->used_cnt) < conn->remote_settings.active_connection_id_limit
5263+
&& xqc_cid_set_countable_cnt(inner_set) < conn->remote_settings.active_connection_id_limit
52645264
&& inner_set->unused_cnt < unused_limit)
52655265
{
52665266
ret = xqc_write_mp_new_conn_id_frame_to_packet(conn, retire_prior_to, inner_set->path_id);
@@ -5278,7 +5278,7 @@ xqc_conn_try_add_new_conn_id(xqc_connection_t *conn, uint64_t retire_prior_to)
52785278
inner_set = xqc_list_entry(pos, xqc_cid_set_inner_t, next);
52795279
if (inner_set->set_state == XQC_CID_SET_USED) {
52805280
while (inner_set
5281-
&& (inner_set->unused_cnt + inner_set->used_cnt) < conn->remote_settings.active_connection_id_limit
5281+
&& xqc_cid_set_countable_cnt(inner_set) < conn->remote_settings.active_connection_id_limit
52825282
&& inner_set->unused_cnt < unused_limit)
52835283
{
52845284
ret = xqc_write_mp_new_conn_id_frame_to_packet(conn, retire_prior_to, inner_set->path_id);
@@ -5296,7 +5296,7 @@ xqc_conn_try_add_new_conn_id(xqc_connection_t *conn, uint64_t retire_prior_to)
52965296

52975297
inner_set = xqc_get_path_cid_set(&conn->scid_set, XQC_INITIAL_PATH_ID);
52985298
/* origin logic for new connection id */
5299-
while ((inner_set->used_cnt + inner_set->unused_cnt) < conn->remote_settings.active_connection_id_limit
5299+
while (xqc_cid_set_countable_cnt(inner_set) < conn->remote_settings.active_connection_id_limit
53005300
&& inner_set->unused_cnt < unused_limit)
53015301
{
53025302
ret = xqc_write_new_conn_id_frame_to_packet(conn, retire_prior_to);

‎tests/unittest/main.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ main()
6767
if (!CU_add_test(pSuite, "xqc_cid_test", xqc_test_cid)
6868
|| !CU_add_test(pSuite, "xqc_test_cid_active_limit", xqc_test_cid_active_limit)
6969
|| !CU_add_test(pSuite, "xqc_test_cid_handshake_exclusion", xqc_test_cid_handshake_exclusion)
70+
|| !CU_add_test(pSuite, "xqc_test_cid_mark_original_idempotent", xqc_test_cid_mark_original_idempotent)
71+
|| !CU_add_test(pSuite, "xqc_test_cid_delete_original", xqc_test_cid_delete_original)
7072
|| !CU_add_test(pSuite, "xqc_test_get_random", xqc_test_get_random)
7173
|| !CU_add_test(pSuite, "xqc_test_engine_create", xqc_test_engine_create)
7274
|| !CU_add_test(pSuite, "xqc_test_conn_create", xqc_test_conn_create)

‎tests/unittest/xqc_cid_test.c‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -517,4 +517,92 @@ xqc_test_cid_handshake_exclusion()
517517
CU_ASSERT(ret == XQC_OK);
518518

519519
xqc_engine_destroy(conn->engine);
520-
}
520+
}
521+
522+
/*
523+
* Test mark_original idempotency: calling xqc_cid_set_mark_original twice
524+
* on the same CID must not inflate original_cid_cnt.
525+
*/
526+
void
527+
xqc_test_cid_mark_original_idempotent()
528+
{
529+
xqc_int_t ret;
530+
xqc_connection_t *conn;
531+
532+
conn = test_engine_connect();
533+
CU_ASSERT_FATAL(conn != NULL);
534+
535+
/* test_engine_connect already marks initial DCID as original (cnt=1) */
536+
xqc_cid_set_inner_t *inner_set = xqc_get_path_cid_set(&conn->dcid_set,
537+
XQC_INITIAL_PATH_ID);
538+
CU_ASSERT_FATAL(inner_set != NULL);
539+
uint64_t cnt_before = inner_set->original_cid_cnt;
540+
541+
/* call mark_original again on the same CID — must be a no-op */
542+
xqc_cid_set_mark_original(&conn->dcid_set,
543+
&conn->dcid_set.current_dcid,
544+
XQC_INITIAL_PATH_ID);
545+
CU_ASSERT(inner_set->original_cid_cnt == cnt_before);
546+
547+
/* call a third time for good measure */
548+
xqc_cid_set_mark_original(&conn->dcid_set,
549+
&conn->dcid_set.current_dcid,
550+
XQC_INITIAL_PATH_ID);
551+
CU_ASSERT(inner_set->original_cid_cnt == cnt_before);
552+
553+
xqc_engine_destroy(conn->engine);
554+
}
555+
556+
/*
557+
* Test that xqc_cid_set_delete_cid correctly decrements original_cid_cnt
558+
* when an original CID is deleted (rather than state-transitioned).
559+
*/
560+
void
561+
xqc_test_cid_delete_original()
562+
{
563+
xqc_int_t ret;
564+
xqc_connection_t *conn;
565+
566+
conn = test_engine_connect();
567+
CU_ASSERT_FATAL(conn != NULL);
568+
569+
/* insert a new CID and mark it as original */
570+
xqc_cid_t extra_cid;
571+
ret = xqc_generate_cid(conn->engine, NULL, &extra_cid, 0);
572+
CU_ASSERT(ret == XQC_OK);
573+
ret = xqc_cid_set_insert_cid(&conn->dcid_set, &extra_cid, XQC_CID_USED,
574+
conn->local_settings.active_connection_id_limit,
575+
XQC_INITIAL_PATH_ID);
576+
CU_ASSERT(ret == XQC_OK);
577+
xqc_cid_set_mark_original(&conn->dcid_set, &extra_cid, XQC_INITIAL_PATH_ID);
578+
579+
xqc_cid_set_inner_t *inner_set = xqc_get_path_cid_set(&conn->dcid_set,
580+
XQC_INITIAL_PATH_ID);
581+
CU_ASSERT_FATAL(inner_set != NULL);
582+
uint64_t cnt_before = inner_set->original_cid_cnt;
583+
CU_ASSERT(cnt_before >= 2);
584+
585+
/* delete the extra original CID — original_cid_cnt must decrease */
586+
ret = xqc_cid_set_delete_cid(&conn->dcid_set, &extra_cid,
587+
XQC_INITIAL_PATH_ID);
588+
CU_ASSERT(ret == XQC_OK);
589+
CU_ASSERT(inner_set->original_cid_cnt == cnt_before - 1);
590+
591+
/* delete a non-original CID — original_cid_cnt must NOT change */
592+
xqc_cid_t normal_cid;
593+
ret = xqc_generate_cid(conn->engine, NULL, &normal_cid, 200);
594+
CU_ASSERT(ret == XQC_OK);
595+
ret = xqc_cid_set_insert_cid(&conn->dcid_set, &normal_cid, XQC_CID_UNUSED,
596+
conn->local_settings.active_connection_id_limit,
597+
XQC_INITIAL_PATH_ID);
598+
CU_ASSERT(ret == XQC_OK);
599+
600+
uint64_t cnt_after_insert = inner_set->original_cid_cnt;
601+
ret = xqc_cid_set_delete_cid(&conn->dcid_set, &normal_cid,
602+
XQC_INITIAL_PATH_ID);
603+
CU_ASSERT(ret == XQC_OK);
604+
CU_ASSERT(inner_set->original_cid_cnt == cnt_after_insert);
605+
606+
xqc_engine_destroy(conn->engine);
607+
}
608+

‎tests/unittest/xqc_cid_test.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,11 @@ void xqc_test_cid_active_limit();
2121
*/
2222
void xqc_test_cid_handshake_exclusion();
2323

24-
#endif
24+
/* mark_original idempotency: repeated calls must not inflate original_cid_cnt */
25+
void xqc_test_cid_mark_original_idempotent();
26+
27+
/* delete_cid must decrement original_cid_cnt when removing an original CID */
28+
void xqc_test_cid_delete_original();
29+
30+
#endif
31+

0 commit comments

Comments
 (0)