Claude Review 01 (OPUS)

[GIC-de]

libdict — Code Review

Date: 2026-09-08
Revision reviewed: 15e8303 (branch master, clean tree)
Scope: all of src/ (8 containers + shared code), include/, demo.c, anagram.c, benchmark.c, unit_tests.c, util.h, build files, README.md, TODO — ~8,600 lines.

Method: six independent line-by-line reviews (one per file group), each cross-checked against the shared tree_common.c / dict_private.h contracts; plus empirical verification with:

• gcc -Wall -Wextra -Wshadow -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wsign-compare -Wpointer-arith
• the CUnit suite under ASan + UBSan + LSan
• a purpose-written randomized differential stress harness checking every container against a reference model (bitmap + datum array), covering insert / remove / search / search_le|lt|ge|gt / iterator-remove / traverse (incl. early stop) / nextn / prevn / itor_compare / forward+reverse iteration / select at every rank / clear+reuse / free, under ASan+UBSan+LSan across all 14 type configurations and many seeds
• targeted single-purpose repro programs for each crash claim

Findings are marked CONFIRMED where a repro was executed, CODE-GROUNDED where the defect is evident from the source but no repro was run, and UNCONFIRMED where a repro attempt failed.

---

STATUS: all findings addressed

Every finding in this document has been fixed in the working tree, except §9, which could not be reproduced and was therefore left alone rather than "fixed" speculatively.

This document is kept as the record of what was found and why each change was made. It describes the code as reviewed, not as it now stands — the line numbers refer to the pre-fix revision 15e8303.

Two defects of the same class were found while fixing and are also resolved:

• The unbounded recursion of §4.9 applied to sp_tree_verify/tr_tree_verify as well, not just the path-length helpers. Confirmed: dict_verify on a 500,000-node degenerate splay tree segfaulted. Both are now iterative (tree_verify_common).
• Tightening dict_key_func to return const void* (§4.3) exposed two callers that were silently discarding const on a live key (demo.c, unit_tests.c).

Verification of the fixed tree: library and programs compile clean under -Wall -Wextra -Wshadow -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wsign-compare -Wpointer-arith; the test suite passes (20 tests, ~7.46M assertions, 0 failures) under ASan + UBSan + LeakSanitizer and under -DNDEBUG -O3; the randomized differential stress harness passes across all 14 container configurations and many seeds. Purpose-built regression checks confirm that each reproduced crash is gone and that the repaired verifiers now reject corruption they previously accepted.

---

Baseline health

The library itself compiles clean under all the warning flags above, with a single exception (dict_private.h:105, a non-prototype declaration). The CUnit suite passes clean under ASan/UBSan/LSan. demo.c, anagram.c and benchmark.c compile clean under -Wall -Wextra -Wformat=2.

⚠ Context that amplifies everything below

.github/workflows/release.yml and package.yml build with CMAKE_BUILD_TYPE=Release, which makes CMake define NDEBUG. src/dict_private.h:61 degrades ASSERT(expr) to (void)(expr):

#else
# define ASSERT(expr)    (void)(expr)
#endif

Every ASSERT-only guard in this library is therefore inert in the shipped packages. Only test.yml uses Debug. Several findings below (5, and the alignment preconditions in §5) are held together solely by an ASSERT.

---

1. Crashes (all reproduced)

1.1 dict_traverse() on any hb_tree segfaults — two keys is enough

src/hb_tree.c:463 · CONFIRMED · severity: critical

hb_node packs the AVL balance factor into the low 2 bits of the parent pointer:

struct hb_node {
    void*           key;
    void*           datum;
    union {
        intptr_t    bal;
        hb_node*    pptr;
    };
    hb_node*        llink;
    hb_node*        rlink;
};
#define BAL_MASK        ((intptr_t)3)
#define PARENT(node)    ((hb_node*) ((node)->bal & ~BAL_MASK))

Every parent access must go through PARENT(). But hb_tree_traverse delegates straight to the generic routine:

size_t
hb_tree_traverse(hb_tree* tree, dict_visit_func visit, void* user_data)
{
    return tree_traverse(tree, visit, user_data);   /* ← src/hb_tree.c:463 */
}

tree_traverse (src/tree_common.c:252) walks with tree_node_next, which reads node->parent raw and immediately dereferences it (src/tree_common.c:97-98). Any node with a nonzero balance yields a misaligned bogus pointer.

Repro:

dict* d = hb_dict_new(int_cmp);
dict_insert(d, &k[0]);            /* 0 */
dict_insert(d, &k[1]);            /* 1 — root now has bal = NULL|1 */
dict_traverse(d, visit, NULL);

src/tree_common.c:98: runtime error: member access within misaligned
    address 0x000000000001 for type 'struct tree_node'
AddressSanitizer: SEGV on unknown address 0x000000000021
    #0 tree_node_next     src/tree_common.c:98
    #1 tree_traverse      src/tree_common.c:263
    #2 hb_tree_traverse   src/hb_tree.c:463

verify=1, count=2 immediately beforehand — the tree itself is fine. A perfectly balanced 3-node tree survives (all balances 0), which is why this was never noticed. It is reachable through the public interface: the hb_tree_vtable traverse slot (src/hb_tree.c:71) is hb_tree_traverse.

rb_tree uses the identical tagging scheme and correctly supplies its own rb_tree_traverse (src/rb_tree.c:385). hb_tree overrides clear, select and itor_next/prev/nextn/prevn — traverse is the single one that was missed.

Fix: implement hb_tree_traverse with hb's existing node_next (src/hb_tree.c:531), exactly as rb_tree_traverse does.

Punning audit (the reason this is the only leak-through): every other tree_common entry point reachable from hb_tree.c and rb_tree.c — tree_search, tree_search_{le,lt,ge,gt}[_node], tree_node_min/max, tree_count, tree_{min,max,total}_path_length, tree_iterator_{valid,invalidate,free,first,last,search*,key,datum,compare} — touches only key / datum / llink / rlink / root / count / cmp_func, which are value-compatible. The routines that read parent (tree_traverse, tree_select, tree_clear/tree_free, tree_iterator_next/prev/nextn/prevn, tree_node_rot_left/right) are correctly re-implemented locally by both trees — except this one. sp_, pr_, tr_ and wb_ all begin with TREE_NODE_FIELDS and are genuinely compatible.

1.2 dict_select(dct, n, NULL, &datum) segfaults on sp_tree and tr_tree

src/tree_common.c:274-276, 289-290 · CONFIRMED · severity: high

tree_select writes both out-parameters unconditionally:

    *key = node->key;
    *datum = node->datum;

hb_tree_select, rb_tree_select, pr_tree_select and wb_tree_select all guard with if (key) / if (datum). tree_select is the vtable select for sp_tree (src/sp_tree.c:79) and tr_tree (src/tr_tree.c:77).

sp_tree select(0, NULL, &dat):
src/tree_common.c:289: runtime error: store to null pointer of type 'const void *'
AddressSanitizer: SEGV on unknown address 0x0 (WRITE)
    #0 tree_select  src/tree_common.c:289

So the same public API call is safe on four of six sorted dicts and fatal on two. The n >= count path (line 274) crashes the same way on an empty sp_tree/tr_tree. Neither tree_common.h:100-103 nor dict.h documents a non-NULL requirement.

Fix: add the if (key) / if (datum) guards to tree_select, matching the four per-tree implementations, and document the contract.

1.3 dict_itor_compare() on any hashtable calls a NULL function pointer

include/dict.h:195 · CONFIRMED · severity: high

dict_itor_remove and dict_itor_search_le/lt/ge/gt all short-circuit on an unpopulated slot:

#define dict_itor_search_le(i,k)    ((i)->_vtable->search_le && (i)->_vtable->search_le((i)->_itor, (k)))
#define dict_itor_remove(i)         ((i)->_vtable->remove && (i)->_vtable->remove((i)->_itor))
#define dict_itor_compare(i1,i2)    ((i1)->_vtable->compare((i1)->_itor, (i2)->_itor))   /* ← no guard */

Both hash tables leave that slot NULL:

src/hashtable.c:107:    (dict_icompare_func)    NULL,/* hashtable_itor_compare not implemented yet */
src/hashtable2.c:102:   (dict_icompare_func)    NULL,/* hashtable2_itor_compare not implemented yet */

calling dict_itor_compare on a hashtable...
AddressSanitizer: SEGV on unknown address 0x0 (pc 0x0)

unit_tests.c:480 masks this by only comparing when dict_is_sorted(dct).

Fix: guard the macro like its siblings, or implement hashtable{,2}_itor_compare.

1.4 demo.c crashes on an argument-less search

demo.c:123, 133, 147, 161, 175 · CONFIRMED · severity: medium (example program)

Each of search / searchle / searchlt / searchge / searchgt rejects a third token but never tests for a missing key:

        } else if (strcmp(buf, "search") == 0) {
            if (ptr2) {                       /* ← only checks for a surplus token */
                printf("usage: search <key>\n");
                continue;
            }
            void** search = dict_search(dct, ptr);   /* ptr may be NULL */

Only remove gets it right (demo.c:189: if (!ptr || ptr2)).

$ printf 'insert a A\nsearch\n' | ./demo h
> inserted 'a': 'A'
AddressSanitizer: SEGV on unknown address 0x0
    #0 __interceptor_strcmp
    #1 tree_search_node  src/tree_common.c:132
    #2 tree_search       src/tree_common.c:146
    #3 main              demo.c:127

Fix: if (!ptr || ptr2) in all five branches.

---

2. Silent data corruption (all reproduced)

2.1 skiplist_dict_new(cmp, 1) loses every element and leaks them all

src/skiplist.c:685 · CONFIRMED · severity: critical

static inline unsigned
rand_link_count(skiplist* list)
{
    unsigned count = (unsigned) __builtin_ctz(dict_rand()) / 2 + 1;   /* always >= 1 */
    return (count >= list->max_link) ? list->max_link - 1 : count;
}

With max_link == 1, count >= 1 is always true, so the result is always max_link - 1 == 0. node_new(key, 0) produces a zero-link node (its ASSERT(link_count >= 1) is compiled out under NDEBUG), and node_insert's linking loop never executes:

    for (unsigned k = 0; k < nlinks; k++) {   /* nlinks == 0 → body never runs */
        x->link[k] = update[k]->link[k];
        update[k]->link[k] = x;
    }
    ++list->count;                            /* ← still incremented */

skiplist_new (src/skiplist.c:114) only asserts max_link > 0, so 1 is an accepted public API value.

Repro (-DNDEBUG -fsanitize=address, 4 inserts):

count=4
LeakSanitizer: 128 byte(s) leaked in 4 allocation(s)
    #1 node_new          src/skiplist.c:669
    #2 skiplist_insert   src/skiplist.c:205

Every element is invisible: count reports 4, every search returns NULL, verify fails, and clear/free walk head->link[0] == NULL and free nothing.

max_link == 0 is worse: skiplist_new allocates a head node with zero links and the first insert reads out of bounds (heap-buffer-overflow, READ of size 8 immediately past a 32-byte region). Both stem from the same root cause — skiplist_new clamps the upper bound (> MAX_LINK) but never enforces a usable lower bound.

Fix: reject max_link < 2 at the API boundary, and make the clamp MIN(count, max_link - 1) with a floor of 1.

2.2 wb_tree rebalancing breaks above ~5M elements (32-bit weight overflow)

src/wb_tree.c:196 (also 162-163, 167, 200, 402-403) · CONFIRMED · severity: high

weight is uint32_t, and the balance tests compute the products in 32 bits:

    if (weight * 1000U > n->weight * 707U) {

n->weight * 707U wraps once n->weight >= 6,075,626; weight * 1000U wraps at >= 4,294,968. When the right-hand side wraps to a small value the "left subtree far too heavy" branch fires spuriously and fixup performs an unjustified rotation at that node on every insert. tree->count is size_t, so these sizes are well within documented capacity.

Repro (sequential wb_tree_insert(0..N-1)):

N=4000000  count=4000000  verify=1
N=4300000  count=4300000  verify=1
N=5000000  count=5000000  verify=1
N=7000000  count=7000000  verify=0
  src/wb_tree.c:402 (node_verify) verification failed:
      lweight * 1000U >= node->weight * 292U

At root weight 7,000,001: 7000001 * 707 = 4,949,000,707 wraps to 654,033,411, while lweight * 1000 ≈ 3.5e9 — so the branch fires at the root on every insert.

Fix: cast the products to uint64_t. The node_verify checks at 402-403 overflow identically, so the verifier is equally unreliable at that scale and must be fixed too.

2.3 hb_itor_search() is a lower-bound search, not an exact search

src/hb_tree.c:646 · CONFIRMED · severity: medium

bool hb_itor_search(hb_itor* itor, const void* key)    { return tree_iterator_search_ge(itor, key); }
bool hb_itor_search_ge(hb_itor* itor, const void* key) { return tree_iterator_search_ge(itor, key); }

The two lines are identical — that's the tell. Every sibling uses exact tree_iterator_search (wb_tree.c:458, rb_tree.c:628, sp_tree.c:477, pr_tree.c:525, tr_tree.c:315), and so does hb's own vtable slot (src/hb_tree.c:89), so the two disagree on the same iterator.

Repro, tree containing {1, 3}:

dict_itor_search(2) = 0
hb_itor_search(2)   = 1   landed on key=3

Caller code that treats true as "the key exists" silently operates on the wrong element. unit_tests.c only ever exercises dict_itor_search.

Fix: return tree_iterator_search(itor, key);

---

3. Verifiers that do not check what they claim

These matter disproportionately: they are the safety net for §1 and §2, and dict_verify is what the test suite leans on.

3.1 rb_tree_verify does not check the black-height invariant

src/rb_tree.c:533-536, :541 · CODE-GROUNDED (repro reported by reviewer) · severity: high

        if (!node->llink && !node->rlink) {
            /* Verify that each path to a leaf contains the same number of black nodes. */
            VERIFY(black_node_count == leaf_black_node_count);
        }
        bool l = node_verify(tree, node, node->llink, black_node_count, leaf_black_node_count);
        bool r = node_verify(tree, node, node->rlink, black_node_count, leaf_black_node_count);
        return l && r;
    }
    return true;                    /* ← the node == NULL case never compares counts */

The comparison runs only at nodes with no children. Every root-to-NIL path that terminates at the missing child of a one-child node is never validated.

Counterexample: insert 10, then 5 → root 10 black, left child 5 red, 10->rlink == NULL. Flip node 5 to black. Path 10→NULL has 1 black node, path 10→5→NULL has 2 — genuinely invalid — yet rb_tree_verify() returns true.

Fix: perform the black-count comparison in the node == NULL case, i.e. treat NIL as the leaf.

3.2 No tree verifies BST ordering globally

src/rb_tree.c:518-521, src/hb_tree.c:549-553, src/wb_tree.c:389-392 · CODE-GROUNDED · severity: medium

All three compare each node only against its immediate parent:

            if (parent->llink == node) {
                VERIFY(tree->cmp_func(parent->key, node->key) > 0);
            } else {
                VERIFY(tree->cmp_func(parent->key, node->key) < 0);
            }

Local parent-child ordering does not imply a valid search tree. Counterexample all three accept: root 5, 5->llink = 3, 3->rlink = 7. Both checks pass (cmp(5,3) > 0 ✓, cmp(3,7) < 0 ✓) — yet key 7 sits in the root's left subtree, so tree_search(7) descends right from the root and returns NULL. A rotation bug that misplaces a whole subtree would pass verification.

Fix: thread (lower, upper) key bounds through the recursion.

3.3 skiplist_verify — wrong comparison, and two missing checks

src/skiplist.c:470-474 · CODE-GROUNDED · severity: medium

        for (unsigned k = 0; k < node->link_count; k++) {
            if (node->link[k]) {
                VERIFY(node->link[k]->link_count >= k);      /* should be > k */
            }
        }

A node occupying level k must have at least k+1 links, i.e. link_count > k. As written the check is a tautology at k == 0 and accepts exactly the corruption that would trip skiplist_remove's ASSERT(update[k]->link_count > k).

It also never checks list->count against the number of nodes reachable via link[0] — which is precisely finding 2.1's symptom — and never calls list->cmp_func at all, so it cannot detect a mis-ordered list.

3.4 Other verifier gaps

• src/hb_tree.c:550 uses ASSERT(parent->rlink == node) where the wb equivalent (src/wb_tree.c:383) is a proper VERIFY(parent->llink == node || parent->rlink == node). Under NDEBUG this silently accepts a node whose claimed parent has no link back to it — only the child→parent direction (PARENT(node) == parent, line 566) is checked.
• rb_tree_verify and pr_tree_verify never check tree->count against the actual node count; they only distinguish count > 0 from count == 0. pr_tree could do it cheaply: root->weight == count + 1.
• src/hb_tree.c:586-588 — hb_tree_verify runs VERIFY(tree->count == count) unconditionally after node_verify bailed out, so a structural failure is followed by a spurious second count-mismatch message on stderr.

---

4. Undefined behaviour

4.1 __builtin_ctz(dict_rand()) is UB when random() returns 0

src/skiplist.c:684, src/dict_private.h:105 · CODE-GROUNDED

dict_rand() is (unsigned) random(), whose range includes 0, and __builtin_ctz(0) is documented-undefined in both GCC and Clang. On x86-64 it compiles to a bare bsf, whose destination register is left unmodified when the source is zero — so link_count becomes whatever stale value the register held. The max_link - 1 clamp on the next line contains the damage (no OOB), so the observable effect is a silently corrupted level distribution rather than a crash. tzcnt returns 32 instead, so codegen changes behaviour. Probability ~2⁻³¹ per insert.

Also: __builtin_ctz has no non-GCC/Clang fallback in this file, unlike the guarded macros in dict_private.h.

4.2 Indeterminate pointer value read after FREE

src/tree_common.c:313-315, src/rb_tree.c:160-162, src/hb_tree.c:154-155 and :376/385, src/sp_tree.c:364-368 · CODE-GROUNDED

    tree_node* const parent = node->parent;
    FREE(node);
    *(parent ? (parent->llink == node ? &parent->llink : &parent->rlink) : &t->root) = NULL;

node is compared after its lifetime has ended. Per C11 6.2.4p2 the pointer's value becomes indeterminate on deallocation, and this is exactly the pattern a compiler is entitled to fold. No freed memory is dereferenced, so ASan/UBSan report nothing today — it is a latent miscompilation hazard, not a live bug.

src/wb_tree.c:294-299 and src/tr_tree.c:197-203 get the ordering right (splice first, FREE last). Fix: hoist the comparison above the FREE in the other five sites.

4.3 Calls through incompatible function-pointer types

include/dict.h:116, 124, 128 · CODE-GROUNDED

typedef	declared	actual implementations
dict_key_func	void* (*)(void*)	const void* f(const T*)
dict_icompare_func	int (*)(void*, void*)	int f(const T*, const T*)
dict_valid_func	bool (*)(const void*)	mixed

void* and const void* are not compatible types, so calling through the cast pointer is UB (C11 6.5.2.2p9) and traps under clang -fsanitize=function or CFI — every dict_itor_key() and dict_itor_compare() call would abort. The API also silently launders away const, handing callers a writable pointer to a live key. This is pervasive across all six tree vtables.

Fix: make the typedefs match the real signatures — which is the intended contract anyway.

4.4 char into <ctype.h>, and an out-of-bounds array write

anagram.c:36, 45; demo.c:99 · CODE-GROUNDED · severity: high for anagram

        freq[tolower(*p)]++;

With *p a byte ≥ 0x80 (any UTF-8 or Latin-1 input file), char sign-extends to a negative int. That is UB in the ctype call and an out-of-bounds write at freq[-61] etc. — stack corruption on a non-ASCII dictionary file. isupper(buf[0]) (line 36) and isspace(*p) (demo.c:99) have the read-side version.

Fix: cast to unsigned char at every ctype call site.

4.5 Division by zero

benchmark.c:240 · CODE-GROUNDED

        unsigned rv = dict_rand() % strlen(words[i]);

A blank line in the input becomes "" after strtok(buf, "\n") (benchmark.c:107) → SIGFPE. Reachable with any word list containing a trailing or embedded blank line.

4.6 %u printing size_t

benchmark.c:166, 183, 201, 290 · CODE-GROUNDED

n (line 164) and nwords (line 95) are size_t; on LP64 this is a varargs type mismatch. quit("bad count (%u - should be %u)!", n, nwords) prints nonsense exactly when the diagnostic matters. Lines 125 and 265 use %d for unsigned.

4.7 strcpy with overlapping source and destination

demo.c:101-103 · CODE-GROUNDED

        for (p = buf; *p && isspace(*p); p++)
            /* void */;
        if (buf != p) {
            strcpy(buf, p);          /* p points into buf */
        }

strcpy explicitly forbids overlap; memmove is required. With a vectorized libc strcpy, a line like " insert a 1" can produce a corrupted command string.

4.8 dict_str_cmp compares as signed char

src/dict.c:85-87 · CODE-GROUNDED

        char p = *a++, q = *b++;
        if (!p || p != q) return (p > q) - (p < q);

strcmp is specified over unsigned char. On x86 the byte 0xFF compares as -1. Note dict_str_hash two lines below deliberately uses const uint8_t*, so the two functions disagree about byte signedness. Consequence: ordering of non-ASCII keys does not match strcmp, and it flips between x86 (signed char) and ARM/PowerPC (unsigned char) — so dict_search_ge/_le and iteration order differ across targets for identical inputs.

Fix: unsigned char p = *a++, q = *b++;

4.9 Unbounded recursion in the path-length helpers

src/tree_common.c:331-337, 346-352, 361-367 · CODE-GROUNDED

node_min_path_length, node_max_path_length and node_path_length recurse once per level with no depth limit, while everything else in the file (notably tree_clear) is carefully iterative. A tr_tree with a constant dict_prio_func degenerates to a linked list; ~500k keys then overflows an 8 MB stack. Impact confined to these three diagnostic functions (used by benchmark.c:144-146).

4.10 dict_ptr_cmp relational-compares unrelated pointers

src/dict.c:75 · CODE-GROUNDED · pedantic

(k1 > k2) - (k1 < k2) on pointers into different objects is UB per C11 6.5.8p5 (only ==/!= are defined). Casting both to uintptr_t first makes it well-defined.

4.11 util.h problems

util.h:10-23 · CODE-GROUNDED

• Lines 10-16: shuffle, is_prime, next_permutation are non-static, non-inline function definitions in a header → duplicate-symbol link error from two translation units. The header also uses bool, size_t and rand() while including only <assert.h>.
• Line 12: shuffle() underflows on size == 0 — i < size - 1 becomes i < SIZE_MAX, then rand() % (0 - i) and wild writes. benchmark.c is safe only via its if (!nwords) quit(...) at line 100.
• Line 19: if (n <= 0) on an unsigned is tautological, and is_prime(1) returns true. unit_tests.c:793-798 tests 2..7 and never 1.

4.12 Falling off the end of a non-void function

benchmark.c:354-356 · CODE-GROUNDED

The default: case relies on quit() being NORETURN, but line 27 guards NORETURN with #ifdef __GNUC__ only. On any other compiler it expands to nothing and control reaches the closing brace of a dict*-returning function. demo.c:20 guards correctly (__GNUC__ || __clang__).

---

5. Iterator invalidation — undocumented, unprotected

5.1 hashtable: remove / clear / free dangle live iterators

src/hashtable.c:228, 251-267 · CODE-GROUNDED · severity: high

struct hashtable_itor stores a raw hash_node* node. remove_node calls FREE(node) and hashtable_clear frees every node, with nothing tracking or invalidating iterators.

dict_itor* it = hashtable_dict_itor_new(t);
hashtable_itor_first(it);
hashtable_remove(t, key_of_first);        /* or hashtable_clear / hashtable_free */
hashtable_itor_key(it);                   /* reads freed memory */

5.2 hashtable: resize silently corrupts in-flight iteration

src/hashtable.c:167, 318-341 · CODE-GROUNDED · severity: high

hashtable_resize keeps the node allocations (so no crash) but re-links every node into the new table at node->hash % new_size and replaces table->table. The iterator caches {node, slot} and hashtable_itor_next resumes scanning from the stale slot in the new table — so some elements are visited twice and others never. The resize happens inside hashtable_insert, so any insert during iteration hits this. hashtable_itor_prev (line 441) likewise.

Note src/hashtable.c:167 also ignores the hashtable_resize return value, where src/hashtable2.c:191 checks it.

5.3 hashtable2_itor_valid aborts on a removed element

src/hashtable2.c:451 · CODE-GROUNDED · severity: medium

    ASSERT(itor->table->table[itor->slot].hash != 0);

The iterator stores only an int slot, and hashtable2_remove both empties slots and — via remove_cleanup — relocates other elements to different slots. After removing the element under the iterator, hashtable2_itor_valid abort()s in a debug build; under NDEBUG it returns true and itor_key hands back the stale/NULL key of an empty slot, or the key of a different element that was shifted in.

Related: remove_cleanup moves elements to earlier slots, so a forward iterator at slot k has elements moved from > k to < k behind it — skipping and double-visiting. hashtable2_itor_remove (line 567) sidesteps this only by setting slot = -1, which makes remove-while-iterating impossible at all.

5.4 All trees except tr_tree: removing key K can free the node holding successor(K)

src/hb_tree.c:369, src/wb_tree.c:288, src/rb_tree.c:276, src/pr_tree.c:303, src/sp_tree.c:353-360 · CODE-GROUNDED · severity: medium

In the two-child case, out = tree_node_min(node->rlink), the key/datum are swapped into node, and out — the node object other iterators may be positioned on — is freed. An iterator parked on the successor is left dangling even though its key was never removed, and itor_valid still returns true (its node pointer is non-NULL).

tr_tree is immune because it rotates the node down and frees the node itself.

Fix: at minimum document "remove invalidates iterators positioned at the removed key's successor" — no header says so today.

5.5 Pointer-tagging alignment is guarded only by ASSERT

src/hb_tree.c:504, src/rb_tree.c:458 · CODE-GROUNDED

    ASSERT((((intptr_t)node) & 3) == 0);

This is the sole guard that the tagged-pointer scheme is safe, and dict_malloc_func is user-replaceable (src/dict_private.h:88). An NDEBUG build with a custom allocator returning under-aligned blocks silently corrupts every parent pointer in the tree, with no diagnostic — a hard-to-diagnose variant of finding 1.1. There is also no _Static_assert that sizeof(intptr_t) == sizeof(void*) or on the field offsets; on a platform where they differ, the llink/rlink offsets shift and even plain tree_search returns garbage.

Fix: static assertions on the node field offsets, plus an unconditional (non-ASSERT) alignment check, plus a comment stating the "no generic routine may touch parent" contract.

---

6. Memory leaks in the example programs

Site	Defect
anagram.c:62	On an anagram hit rb_tree_insert returns the existing node, so the just-allocated xstrdup(name) is never stored and never freed.
anagram.c:92	rb_tree_free(tree, NULL) — NULL delete func, so even the keys the tree does own leak. ~1 MB+ against /usr/share/dict/words.
anagram.c:9, 60-64	Defines an aborting xmalloc but never installs it as dict_malloc_func, so library allocations go unchecked; on failure rb_tree_insert returns {NULL,false} and line 63 dereferences NULL. (demo.c:48 and benchmark.c:77 do install it.)
benchmark.c:263-268, 292	The remove loop drains the dict discarding result.key/result.datum, so dict_free(dct, key_str_free) runs on an empty dict and key_str_free is never invoked. The whole input file leaks every run. FREE(words) frees only the pointer array.
demo.c:113-121	dict_insert(dct, xstrdup(ptr)) — ownership transfers only when result.inserted; the else branch drops the allocation, leaking on every duplicate insert.
demo.c:117, 120	*result.datum_ptr dereferenced without a NULL check. hashtable_insert returns {NULL,false} on allocation failure (src/hashtable.c:183), so !inserted does not imply a valid datum_ptr. dict.h documents none of this.
benchmark.c:306	FREE(words) releases xmalloc'd memory through the library's dict_free_func — works only because dict_free_func still defaults to free while dict_malloc_func was replaced. Becomes a real wrong-allocator free the moment dict_free_func is set.

Note: every MALLOC inside src/ is correctly NULL-checked, and the dict/dict_itor wrappers free the outer object on inner-allocation failure. The leaks are confined to the example programs.

---

7. Test-suite gaps that let these ship

unit_tests.c never calls dict_traverse. That alone is why finding 1.1 — a segfault on two keys through the public API — survived. Zero occurrences, verified by grep.

Also entirely untested: dict_itor_nextn / dict_itor_prevn (their while (count--) implementations have distinct early-return semantics from next), dict_itor_invalidate, dict_int_cmp, dict_uint_cmp, dict_long_cmp, dict_ulong_cmp, dict_ptr_cmp, dict_str_hash. And dict_free/dict_clear are only ever called with a NULL delete_func (lines 616, 667), so the delete-callback plumbing — the main source of user-facing double frees — is never exercised.

Structural weaknesses:

• unit_tests.c:315 — if (nkeys < NUM_SORTED_KEYS) continue; disables the entire le/lt/ge/gt assertion block. test_basic runs for n = 0..38, so 38 of 39 invocations exercise none of the near-search assertions — exactly the small and one-element cases where boundary bugs live.
• unit_tests.c:279, 318, 323, 338, 343, 358, 363, 378, 383 — assertions wrapped in if (dct->_vtable->select) / if (itor->_vtable->search_le). The suite can never fail for a regression that NULLs out select, search_le, search_lt, search_ge or search_gt on a sorted container, and there is no complementary assertion that sorted containers must provide them.
• unit_tests.c:306 — CU_ASSERT_FALSE(dict_select(dct, i, &key, &datum)) sits inside if (!dict_is_sorted(dct)), where _vtable->select is NULL and dict_select expands to (vtable->select && ...) — a compile-time-constant false. It can never fail. Lines 307-308 then re-test the local initializers from lines 304-305, not library behaviour.
• unit_tests.c:296-303 — eight vtable assertions re-evaluated once per closest_lookup_info inside the loop. They assert properties that cannot change between iterations; they inflate the reported assert count (6,430,319) without adding coverage.
• unit_tests.c:81-94 — the custom allocator uses assert(p) for its malloc-failure check (vanishes under NDEBUG, then p[0] = n writes to NULL) and returns &p[1], only sizeof(size_t)-aligned — weaker than malloc guarantees, though fine for the pointer-only payloads libdict stores.
• benchmark.c:152-158 — num_counts is the maximum link count, not a count, and i <= num_counts reads one past the array. In bounds only by luck with counts[16] and a 12-link skiplist; raise the link count to ≥16 and line 155 reads OOB while ASSERT(count_sum == nwords) starts failing.

Recommendation: add a dict_traverse test across all containers (with and without early stop), a delete_func test, and drop the if (vtable->...) guards in favour of asserting the expected slot set per container class.

---

8. Correctness-adjacent, dead code, and documentation

Behavioural oddities (no data corruption)

• src/sp_tree.c:255-256 — sp_tree_insert returns early on a duplicate key without splaying, the only accessor that doesn't (search, _le, _lt, _ge, _gt and even the not-found path at line 296 all splay). Repeatedly re-inserting a deep existing key stays O(depth) forever instead of amortized O(lg n).
• src/sp_tree.c:156 + :166 — rotations is double-incremented in the left-child zig case only; the right-child branch has no such increment. A terminal single rotation costs 2 for a left zig and 1 for a right zig. src/pr_tree.c:203 does rotations += 1 for a double rotation where the mirror branch (:243) correctly does += 2. Diagnostics only — rotation_count has no accessor and nothing in the tree reads it.
• src/rb_tree.c:149 — rb_tree_clear does not reset tree->rotation_count, unlike rb_tree_new.
• dict_rand() yields only 31 bits (random() returns [0, 2³¹)). Consequences: tr_tree.c:156 stores a 31-bit value into uint32_t prio, halving the priority space and doubling the tie rate the treap's analysis assumes; ctz can never exceed 30, so rand_link_count caps at 16 — MAX_LINK = 32 and the top half of skiplist_new(cmp, 32)'s head levels are unreachable by design. Nothing in the library calls srandom(), so every process gets the identical priority/level sequence.
• src/hashtable2.c:467 — while (++itor->slot < (int) itor->table->size) converts unsigned to int; for size > INT_MAX the iterator reports an empty table. Practically unreachable; hashtable.c uses unsigned slot and is unaffected. Similarly hashtable_common.c:47 — dict_prime_geq silently returns a value smaller than n for n > 4294967291, with no way for a caller to detect it.
• src/hashtable.c / hashtable2.c — new_size * sizeof(...) multiplies an unsigned by sizeof. Cannot overflow with a 64-bit size_t; on ILP32 a SIZE_MAX / sizeof guard in *_resize would close the theoretical hole regardless.
• src/skiplist.c:657-661 — skiplist_itor_remove checks the removal only with ASSERT(result.removed) and then returns true unconditionally, so a failed removal is reported as success in release builds.
• src/tr_tree.c:230-242 — node_new leaves prio uninitialized; the single caller assigns it at line 156, so it is safe today, but a second caller would inherit heap garbage as a priority.
• src/rb_tree.c:374-375 — if (node) with an unindented, unbraced SET_BLACK(node) body, where the color macros are bare expressions without do {} while (0). Correct as written, one edit from a bug.
• benchmark.c:453 — > 1000000 should be >= 1000000 in the microsecond carry; tv_usec may reach exactly 1000000, leaving a non-normalized timeval and a total time 1 s low.

Dead code

• src/hashtable2.c:222-241 — #if 0'd index_of_node_to_shift containing its own bugs (int last_index assigned from unsigned index; the loop terminates on while (index != truncated_hash) while the scan starts at the caller-supplied index). Delete rather than leave as a misleading reference implementation.
• src/hashtable.c:427-429 — return itor->node != NULL; immediately after itor->node = NULL. Unconditionally false; the three sibling functions write return false;.
• src/hashtable2.c:551 — return NULL; from a bool-returning function. Converts to false, so behaviour is right; still a type confusion that -Wint-conversion-class checks should flag.
• tree_common.h:40-52 — tree_node_base and tree_base are declared and never referenced; src/tree_common.c:33-39 defines private duplicates. Misleads readers about which type the punning targets.
• src/tr_tree.c:45, src/wb_tree.c:34 — unused #include <limits.h>. src/rb_tree.c:34 — unused #include <string.h>.
• src/wb_tree.c:420 — unsigned root_weight; is passed to node_verify and never examined.
• benchmark.c:57 — bool shuffle_keys = true; is never assigned, so if (shuffle_keys) at lines 205 and 259 is always taken (an unimplemented option).
• benchmark.c:37-40, 416-428 — ptr_hash and my_ptrcmp are defined, non-static, and never called; they escape -Wunused-function only because they have external linkage.
• demo.c:27 — void *xrealloc(void *ptr, size_t size); declared, never defined or used.
• anagram.c:40-41 — int freq[256] = { 0 }; immediately followed by a redundant memset. Line 43's ASSERT(buf[0] != '\0') can never fail (fgets never returns a zero-length string).
• anagram.c:52-56 — ASSERT(freq[i] < 10) is a correctness precondition enforced only by an assertion. Under NDEBUG, a word with ten or more of the same letter writes '0' + 10 == ':' into name, so distinct letter-multisets collide and are reported as anagrams.
• anagram.c:90 — a stray while (rb_itor_next(itor)); after the for loop, leftover from a for→do/while conversion. Harmless only because rb_itor_next returns false on an invalidated iterator; if next ever wrapped, this becomes an infinite loop over already-freed WordList nodes.
• src/wb_tree.c:404/407 — node_verify writes *weight unconditionally where hb's guards with if (height). Harmless with today's callers.

Wrong or misleading documentation

• README.md:22 inverts the insert API. "an insert call returns a boolean indicating whether or not the key was already present in the dictionary" — dict_insert_result.inserted is true when the key was not already present. A reader following the README writes if (result.inserted) { /* collision */ }, exactly backwards.
• src/hashtable2.c:2 — the file header reads "chained hash-table, with chains sorted by hash, implementation", a verbatim copy of hashtable.c:2. This is the open-addressing table; include/hashtable2.h:2 gets it right.
• src/tr_tree.c:36-38 documents a min-heap — "the priority of any node is less than the priority of either of its child nodes" — but the code is a max-heap (insert:169 sifts while parent->prio < node->prio; remove_node:190 promotes the larger-prio child; node_verify:255 asserts prio <= parent->prio). Someone writing a dict_prio_func from this comment gets the inverted tree shape, and include/dict.h:63 gives no direction either.
• src/pr_tree.c:150-153 documents the right-rotation triggers with the comparison inverted relative to the code at :210/:217. Reading the comment as spec produces a tree that fails node_verify.
• Rotation-case labels are swapped in both weight trees: src/wb_tree.c:167 labels a single left rotation /* LL */ (should be RR, per the /* RL */ convention two lines later) and :200 labels a single right rotation /* RR */; src/pr_tree.c:170/:210 have the identical swap, while their adjacent /* RL */ and /* LR */ labels do follow the usual convention.
• src/pr_tree.c:159-162 — "For single rotations… we tail recurse. For double, we make one recursive call and then tail recurse." Every rotating branch makes two non-tail calls (:173+176, :204+205, :216, :244+245), and fixup is not tail-recursive at all since results are summed into rotations. The stale comment hides that fixup's recursion depth is bounded only by tree height.
• src/hashtable.c:106 — stale comment "hashtable_itor_remove not implemented yet" sits next to the slot that now is implemented (line 542). hashtable2.c:101 has the same slot without the bogus comment.
• tree_common.h doc errors:
  • Lines 70-71: "Return the left child of |node|, or |node| if it has no right child" — two errors; it returns the leftmost descendant, or node if it has no left child.
  • Line 73: "rightmost child" → "rightmost descendant" (it descends transitively).
  • Lines 65, 68, 71, 74: "|node| must not be NULL" is wrong for tree_node_min/tree_node_max, which explicitly return NULL for NULL (tree_common.c:109-110, 120-121) — and internal callers rely on that (tree_common.c:440, 447; hb_tree.c:367).
  • Line 78: "Return the node has the key" — missing "that".
  • Lines 80-85: tree_search_le/_lt are described as returning "the first key less than or equal to" — they return the greatest such key. "First" is correct only for the ge/gt variants.
  • Line 110 vs node_min_path_length (tree_common.c:331-337): the doc says "the depth of the leaf with minimal depth", but the code returns 1 + MIN(l, r) with 0 for a missing child — the minimal path to a NULL link, not to a leaf. node_max_path_length is leaf-based, so the two are defined inconsistently.
  • Lines 100-103: tree_select's doc omits that key and datum must be non-NULL (finding 1.2).
• include/dict.h:52-53 — the comparison-function contract mislabels its own terms: "reflexive (k1>k2 implies k1<k2, etc.)" describes antisymmetry and the parenthetical is backwards (should be "k1>k2 implies k2<k1"); "symmetric (k1=k1)" describes reflexivity. Only "transitive" is labelled correctly.
• include/dict.h:70 — "Forward declarations for transparent type dict_itor" — one declaration, not several, and the full definition appears at line 174. dict_select gets no documentation at all.
• src/wb_tree.c:40-41 — "the number of nodes in its left subtree divided by the number of nodes in either subtree" — the denominator is the whole subtree (p(n) = weight(n->llink) / weight(n), which is what lines 162-163 compute).
• src/wb_tree.c:293, src/pr_tree.c:308 — "Splice in the successor, if any" where the code splices in the node's only child.
• src/pr_tree.c:395-429 — both ASCII diagrams have swallowed a row and render misaligned; line 406 reads "Only the weights of B and B's right child to be readjusted" (missing "need").
• src/dict.c:94 — "FNV 1-a string hash" → "FNV-1a". (The implementation is a correct 32-bit FNV-1a.)
• include/dict.h:117, 134 — the vtable field is invalid while the operation everywhere else is invalidate.
• include/dict.h:41-43 — <stddef.h>, <stdint.h>, <stdbool.h> are included inside BEGIN_DECL, i.e. inside extern "C" { in C++. These headers can pull in namespace-scoped templates, which are ill-formed with C language linkage.
• include/dict.h:32-36 — BEGIN_DECL/END_DECL are unprefixed, unguarded public macros; any other header defining the same names collides.
• benchmark.c:214 — quit("lookup failed for '%s'", buf) prints buf (the last line read at line 106) instead of words[i], the key that actually failed, making a genuine bug undebuggable.
• benchmark.c:355 — "type must be one of h, p, r, t, s, w or H" omits S (skiplist) and 2 (hashtable2), both advertised in the usage block at lines 68-70. demo.c:81 has the correct list.
• demo.c:138, 152, 166, 180 — printf("dict does not support that operation!") with no \n, so the next > prompt is glued on. Every other message in the file terminates with \n.
• Typos: src/tr_tree.c:38 "lexigraphical" → "lexicographical"; src/tr_tree.c:33 "each node of tree"; src/sp_tree.c:34 "will runs in"; README.md:20 "support the selecting the nth element".
• README.md:16-17 — both hashtable links point at wikipedia.org/wiki/Hashtable#…. The article is Hash_table; Hashtable is a redirect, and a redirect discards the #Separate_chaining / #Open_addressing fragment, so both links land at the top of the page.
• include/{sp,tr,hb,wb,pr,rb}_tree.h:63-64 — all declare void *_itor_free(*_itor* tree);. The parameter is an iterator, not a tree. include/skiplist.h:68 leaves it unnamed (skiplist_itor_free(skiplist_itor* );) where every neighbour names it itor.
• include/skiplist.h:57-60 — documents the histogram range as "For 0 < x < |ncounts|" where src/skiplist.c:493 fills counts[x] for 0 <= x < ncounts.
• src/dict_private.h:105 — static inline unsigned dict_rand() is a non-prototype declaration (the only warning in the library build); should be (void). The hand-written extern long random(void); duplicates the <stdlib.h> declaration while breaking on toolchains without random() (MSVC).
• src/hashtable.c:136, src/hashtable2.c:131 — *_dict_new assert hash_func and size but not cmp_func, although hashtable_new/hashtable2_new do. A caller passing cmp_func == NULL gets no diagnostic at the API boundary and crashes later at src/hashtable.c:175. hashtable2_free also lacks the ASSERT(table != NULL) that hashtable_free has.
• TODO:4 — "[X] Reformat to 80 columns" is marked done but include/dict.h:160-163 runs ~120 columns and unit_tests.c:116/:321 exceed 100.
• TODO:3 — "[ ] Implement incomplete functionality, e.g. iterator remove & compare" is still open and accurate; nothing in the README warns that iterator compare is unavailable on hash tables (finding 1.3).
• src/rb_tree.c:645 — no trailing newline.

---

9. One claim that could not be reproduced

src/hashtable2.c:243-265 (remove_cleanup) termination bound. The argument: the backward-shift loop stops on while (node != first) — first being the home slot of the deleted key — rather than on the first empty slot. On a 100%-full table the hole can end up behind first, so the element sitting at first is never shifted into it and becomes unfindable while count still includes it. hashtable2_verify would not catch it (it only counts occupied slots).

The state is reachable: hashtable2_resize (:363) only rejects count > new_size, permitting exactly 100%; and the OOM path at :189-191 explicitly comments "No memory for a bigger table, but let the insert proceed anyway", so inserts keep filling to size.

Attempted repro: 11 elements in a size-11 table forced to exactly 100% via the public hashtable2_resize(t, 11), with both a clustered hash (v % 3 + 1) and a spread hash (v + 1), removing each of the 11 elements in turn and re-searching all others — all 22 cases passed, verify=1, nothing lost. The reasoning is not obviously wrong, but there is no repro. Treat as unconfirmed; a wider search over table sizes and hash distributions would be needed to settle it.

---

10. What held up under testing

Reported so this ground is not re-covered.

Differential stress harness (insert / remove / search / search_le|lt|ge|gt / iterator-remove / traverse with early stop / nextn / prevn / itor_compare / forward+reverse iteration / select at every rank / clear+reuse / free, all checked against a reference model, under ASan+UBSan+LSan): clean across all 14 configurations — hashtable/{1,64}, hashtable2/{1,64}, hb_tree, pr_tree, rb_tree, sp_tree, tr_tree/{rand,prio}, wb_tree, skiplist/{2,4,32} — at 100k ops × 5 seeds for the hash tables and pr/rb, and 25–60k ops × 3+ seeds for the rest. No leaks, no double frees, no out-of-bounds. (skiplist/1 and hb_tree-traverse are findings 2.1 and 1.1.)

Specifically verified correct:

• Layout punning is sound for sp_node, tr_node, pr_node, wb_node (all begin with TREE_NODE_FIELDS, extras appended after) and — by offsetof inspection — for hb_node/rb_node on this platform (0/8/16/24/32, size 40). See §1.1 and §5.5 for the contract this rests on.
• Splay rotations — all four cases (src/sp_tree.c:172-228) traced pointer by pointer: zig-zig rotates the grandparent then the parent, zig-zag the node then the grandparent, and every parent back-pointer including subtree roots and the great-grandparent relink is fixed up. The pp == NULL zig case correctly sets t->root and n->parent = NULL. Splaying preserves in-order sequence and frees nothing, so an iterator surviving a search still yields the correct successor.
• Treap heap property on both insert and remove (tr_tree.c:169-177, remove_node:188-194), including the re-latch of parent after each rotation.
• rb_tree insert_fixup matches CLRS exactly, and delete_fixup's four cases, NULL-x handling, left-direction tracking across the move-up (:320-322) and the w->llink/w->rlink non-NULL preconditions (:325, :356) are all sound — checked against an independent black-height verifier written from CLRS (NIL = 1), 40 seeds × 20,000 random ops over 600 keys, plus ascending / descending / zigzag insert-then-delete of 2,000 keys. All OK. (Necessary because rb_tree_verify itself is unreliable — finding 3.1.)
• pr_tree weight arithmetic — the manual double-rotation updates (:199-201, :239-241) are correct, and the statement ordering is load-bearing and right: each line consumes the old value of the weight the next line overwrites. The unsigned expressions do underflow, but unsigned wraparound is well-defined and the sum wraps back correctly. pr_tree_select is correct for the weight = subtree-size + 1 convention including the llink == NULL case, and pr_tree_verify's balance condition (:469-475) is exactly the negation of fixup's two triggers — so unlike the rb verifier it is complete for the invariant it claims.
• hb_tree — all four rotation balance formulas (:164-275), insert balance-factor propagation and q-tracking (:293), and the delete propagation loop (:394-445). The ASSERT(nr != NULL) / ASSERT(nrl != NULL) / ASSERT(nlr != NULL) preconditions in fixup are implied by the branch conditions. wb_tree's four rotations' weight recomputations likewise. hb_tree_select and wb_tree_select agree and are correct for N = 1..200.
• Parent-chain propagation in pr_tree_insert (:281-285) and pr_tree remove_node (:318-323): saving the grandparent before fixup is correct rather than lucky, since a rotation at node replaces it with a descendant in the same slot of the same parent.
• Skiplist mechanics — flexible-array-member sizing (node_new allocates and memsets exactly sizeof(*node) + sizeof(link[0]) * link_count, no off-by-one); no OOB on level/link arrays for valid max_link; the update[] level-skipping in insert (:195) and remove (:377); level shrinking (:401-402) back to exactly the new maximum.
• Hash tables — hashtable2's probe loops all terminate even on a full table with no unsigned wraparound; remove_cleanup's core re-insert-from-home algorithm is the correct no-tombstone deletion; nonzero_hash collapsing hash == 0 to ~0u is sound because cmp_func resolves the aliasing; count bookkeeping is consistent (remove_cleanup's internal inserts deliberately do not touch it); hash is cached in the node and reused for rehash in both implementations rather than recomputed; hashtable2_resize's failure paths correctly restore table/size/count and free only the new table; no division by zero is possible since dict_prime_geq never returns less than 11.
• dict_prime_geq's table — all 30 entries verified prime and strictly ascending by trial division; kNumPrimes and the loop bounds are correct with no off-by-one.
• Allocation failure handling throughout src/ — every MALLOC is NULL-checked; insert-failure paths mutate nothing and return {NULL, false} without leaking or linking; *_dict_new / *_dict_itor_new free the outer wrapper on inner failure; skiplist_free clears then frees head then list with no double free; dict_free / dict_itor_free are clean.

---

Suggested order of work

1. Finding 1.1 — hb_tree_traverse (crash on 2 keys, public API) and add a dict_traverse test, whose absence is what let it ship.
2. Finding 2.1 — reject skiplist max_link < 2 (silent total data loss + leak).
3. Finding 2.2 — uint64_t in the wb_tree weight comparisons, node_verify included.
4. Findings 1.2, 1.3, 1.4 — NULL guards in tree_select, dict_itor_compare, and the five demo.c branches.
5. Finding 2.3 — hb_itor_search exact-match.
6. §3 — fix the verifiers (NIL-case black height, global BST bounds, skiplist_verify), since they gate everything else.
7. §4.4, §4.5, §4.6, §4.8 — the UB in anagram.c/benchmark.c and dict_str_cmp's signed char.
8. §5 — document (or fix) iterator invalidation; add static assertions for the pointer-tagging contract.
9. §4.2, §4.3 — the post-FREE comparisons and the vtable function-pointer signatures.
10. §6, §7, §8 — example-program leaks, test coverage, then documentation.