From 450a4d467c7f53c6353024d8582e139412977294 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 12 May 2026 09:53:59 +0000 Subject: [PATCH] fix: sel_key_cmp_fn padding-byte mismatch in kvtab lookups sel_key_cmp_fn compared keys with memcmp on the whole sel_node_key_t struct, including the 4-byte padding between type and op1. The stack- allocated search key in gc_sweep_select_nodes can have different padding bytes than the heap-allocated copy stored by key_add, causing key_del to miss the entry and leave a dangling pointer in the CSE intern table. Replace memcmp with field-by-field comparison on type, op1, op2. Also fix GC sweep tests that asserted pointer inequality after re-intern: calloc may lawfully reuse the freed address, so pointer inequality does not reliably indicate a successful sweep. Track the swept node's node_id before freeing and assert the re-interned node gets a fresh id. Co-Authored-By: Claude Sonnet 4.6 --- src/lib/testsuite/test_select.c | 25 +++++++++++++++---------- src/lib/xce_select.c | 11 ++++++++++- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/lib/testsuite/test_select.c b/src/lib/testsuite/test_select.c index 8241617a..35dbd914 100644 --- a/src/lib/testsuite/test_select.c +++ b/src/lib/testsuite/test_select.c @@ -726,12 +726,12 @@ static void test_gc_sweep_unreachable_freed(void) { /* After a sweep with no live jobs, every node in the table is - * unreachable. Re-interning the same arguments must return a freshly - * allocated pointer — the old node was freed and removed from the table. + * unreachable. Re-interning the same arguments must return a fresh + * node — the old node was freed and removed from the table. * - * The pointer-inequality assertion captures the contract; it becomes - * meaningful once Phase 4f wires the mark-and-sweep to the intern table. - */ + * We track node_id rather than pointer equality: calloc may lawfully + * reuse the freed address, so pointer-inequality is not a reliable + * indicator that GC swept correctly. */ sel_node_table_init(); sel_optimize_set(1); @@ -741,16 +741,18 @@ test_gc_sweep_unreachable_freed(void) xce_select_node_t *node1 = makeSelectNode(SEL_NUM, bits, 0); TEST_ASSERT_NOT_NULL(node1); + uint32_t node1_id = node1->node_id; /* No live jobs → all nodes are unreachable */ gc_clear_marks(); gc_sweep_select_nodes(NULL, 0); - /* Re-intern: node1 was freed; a new node must be allocated */ + /* Re-intern: node1 was freed; a new node must be allocated. + * node_id is monotonically assigned — a fresh intern produces a + * different id even if the heap address happens to be reused. */ xce_select_node_t *node2 = makeSelectNode(SEL_NUM, bits, 0); TEST_ASSERT_NOT_NULL(node2); - /* Phase 4f: node2 must be a distinct allocation from the freed node1 */ - TEST_ASSERT_TRUE(node2 != node1); + TEST_ASSERT_NOT_EQUAL(node1_id, node2->node_id); sel_optimize_test_reset(); } @@ -2230,6 +2232,7 @@ test_gc_clear_marks_exported(void) xce_select_node_t* n2 = makeSelectNode(SEL_NUM, b2, 0); TEST_ASSERT_NOT_NULL(n1); TEST_ASSERT_NOT_NULL(n2); + uint32_t n1_id = n1->node_id; /* manually mark n1 */ n1->gc_mark = 1; @@ -2246,10 +2249,12 @@ test_gc_clear_marks_exported(void) gc_sweep_select_nodes(NULL, 0); TEST_ASSERT_EQUAL_INT(1, n2->gc_mark); - /* verify n1 was swept — re-interning gives a fresh allocation */ + /* n1 was swept — re-interning must produce a fresh node_id. + * Pointer inequality would be unreliable because calloc may + * lawfully reuse the freed address. */ xce_select_node_t* n1_again = makeSelectNode(SEL_NUM, b1, 0); TEST_ASSERT_NOT_NULL(n1_again); - TEST_ASSERT_NOT_EQUAL(n1, n1_again); /* different pointer = n1 was freed */ + TEST_ASSERT_NOT_EQUAL(n1_id, n1_again->node_id); sel_node_table_fini(); sel_optimize_test_reset(); diff --git a/src/lib/xce_select.c b/src/lib/xce_select.c index 39b907ac..be9ec1cd 100644 --- a/src/lib/xce_select.c +++ b/src/lib/xce_select.c @@ -98,7 +98,16 @@ static uint32_t sel_key_hash_fn(void* k) static int sel_key_cmp_fn(void* a, void* b) { - return memcmp(a, b, sizeof(sel_node_key_t)); + /* Field-by-field comparison: memcmp on the whole struct would + * compare padding bytes between type and op1, which can differ + * between the stack-local search key and the heap-allocated copy + * stored in the kvtab, causing false misses in key_del. */ + sel_node_key_t* ka = (sel_node_key_t*)a; + sel_node_key_t* kb = (sel_node_key_t*)b; + if (ka->type != kb->type) return 1; + if (ka->op1 != kb->op1) return 1; + if (ka->op2 != kb->op2) return 1; + return 0; } static int sel_key_assign_fn(void** dst, void* src)