From 7c699521d1203759993275d803c5e6fb2073fdb2 Mon Sep 17 00:00:00 2001 From: Jay Howard Date: Sun, 6 Sep 2026 08:15:28 -0500 Subject: [PATCH 1/2] dictBuilder: break COVER sort ties by position, not by address stableSort() is documented to leave each dmer group ordered by position in the input, and COVER_group() depends on it: it counts how many samples a dmer occurs in using a forward-only cursor, so a group whose positions are not ascending has occurrences silently dropped. COVER_strict_cmp() tried to provide that ordering by breaking ties on `lp < rp`. But lp and rp are the addresses of the elements being compared, not the positions they hold. Those coincide only until qsort() performs its first swap; afterwards the tie-break orders by where an element currently sits rather than by what it contains. The result is a comparator that is not a function of the values it compares, with three consequences: - Output is not reproducible. When qsort() compares an element against a temporary (a pivot copy, which is common), one operand is not in the array at all, so the comparison is stack-vs-heap and ASLR decides it. The same binary on the same input produced two different dictionaries in six consecutive runs. - Output depends on the C library. glibc happens to satisfy the invariant; Apple libc and MSVC do not, so they produce different dictionaries from the same input. - An inconsistent comparator breaks quicksort's partitioning assumptions. On MSVC, training on a 16 MB corpus at d=6 takes 465s; with this fix, 8s. Comparing the stored positions instead makes the key (dmer, position). Positions are unique, so no two elements compare equal, the order is total, and every conforming qsort() must produce the same arrangement. Verified: on glibc the output is unchanged (byte-identical across 215 configurations spanning 20 corpora), and on Apple libc and MSVC the output now matches what glibc produces. --- lib/dictBuilder/cover.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/dictBuilder/cover.c b/lib/dictBuilder/cover.c index 06d1cb93a5c..d16eb0add65 100644 --- a/lib/dictBuilder/cover.c +++ b/lib/dictBuilder/cover.c @@ -310,7 +310,7 @@ static int COVER_cmp8(COVER_ctx_t *ctx, const void *lp, const void *rp) { } /** - * Same as COVER_cmp() except ties are broken by pointer value + * Same as COVER_cmp() except ties are broken by the position each element holds */ #if (ZDICT_QSORT == ZDICT_QSORT_MSVC) || (ZDICT_QSORT == ZDICT_QSORT_APPLE) static int WIN_CDECL COVER_strict_cmp(void* g_coverCtx, const void* lp, const void* rp) { @@ -321,7 +321,7 @@ static int COVER_strict_cmp(const void *lp, const void *rp) { #endif int result = COVER_cmp((COVER_ctx_t*)g_coverCtx, lp, rp); if (result == 0) { - result = lp < rp ? -1 : 1; + result = *(const U32 *)lp < *(const U32 *)rp ? -1 : 1; } return result; } @@ -337,7 +337,7 @@ static int COVER_strict_cmp8(const void *lp, const void *rp) { #endif int result = COVER_cmp8((COVER_ctx_t*)g_coverCtx, lp, rp); if (result == 0) { - result = lp < rp ? -1 : 1; + result = *(const U32 *)lp < *(const U32 *)rp ? -1 : 1; } return result; } @@ -692,7 +692,7 @@ static size_t COVER_ctx_init(COVER_ctx_t *ctx, const void *samplesBuffer, { /* suffix is a partial suffix array. * It only sorts suffixes by their first parameters.d bytes. - * The sort is stable, so each dmer group is sorted by position in input. + * Ties are broken by position, so each group is sorted by input position. */ U32 i; for (i = 0; i < ctx->suffixSize; ++i) { From 8dbec8027e86aa13626b7ed7bdafb8e9d5202daa Mon Sep 17 00:00:00 2001 From: Jay Howard Date: Thu, 5 Nov 2026 18:16:29 -0600 Subject: [PATCH 2/2] dictBuilder: return 0 when COVER tie-break positions are equal COVER_strict_cmp() and COVER_strict_cmp8() break ties between equal dmers by sample position but returned 1 when the two positions were equal, so cmp(a, a) was 1. A qsort() comparator must be consistent, and that includes comparing an element with itself. Suffix positions are unique, so the case is only reachable on a self-comparison and the change does not affect output: dictionaries are byte-identical before and after across 72 corpus/k/d combinations. Add a fuzzer test for the ordering itself. It trains COVER on low-entropy samples, where ties are frequent, and checks the dictionary hash against the one position ordering produces, for both comparator paths. In-place qsort() implementations order ties by partition state under the former address comparison, so the test fails against it (verified on macOS) and passes with position ordering. --- lib/dictBuilder/cover.c | 8 ++++++-- tests/fuzzer.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/lib/dictBuilder/cover.c b/lib/dictBuilder/cover.c index d16eb0add65..30a2c65dee0 100644 --- a/lib/dictBuilder/cover.c +++ b/lib/dictBuilder/cover.c @@ -321,7 +321,9 @@ static int COVER_strict_cmp(const void *lp, const void *rp) { #endif int result = COVER_cmp((COVER_ctx_t*)g_coverCtx, lp, rp); if (result == 0) { - result = *(const U32 *)lp < *(const U32 *)rp ? -1 : 1; + U32 const lpos = *(const U32 *)lp; + U32 const rpos = *(const U32 *)rp; + result = (lpos < rpos) ? -1 : (lpos > rpos) ? 1 : 0; } return result; } @@ -337,7 +339,9 @@ static int COVER_strict_cmp8(const void *lp, const void *rp) { #endif int result = COVER_cmp8((COVER_ctx_t*)g_coverCtx, lp, rp); if (result == 0) { - result = *(const U32 *)lp < *(const U32 *)rp ? -1 : 1; + U32 const lpos = *(const U32 *)lp; + U32 const rpos = *(const U32 *)rp; + result = (lpos < rpos) ? -1 : (lpos > rpos) ? 1 : 0; } return result; } diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 32360b9a58d..fc522217c47 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -3120,6 +3120,45 @@ static int basicUnitTests(U32 const seed, double compressibility) } DISPLAYLEVEL(3, "OK, created dictionary of size %u \n", (unsigned)dictSize); + DISPLAYLEVEL(3, "test%3i : COVER dictBuilder orders equal dmers by position : ", testNb++); + /* COVER sorts its suffix array with qsort(), breaking ties between + * equal dmers by sample position; COVER_group() relies on that order + * to count how many samples contain a dmer. A tie-break that depends on + * the sort's internal state instead (the former address comparison) + * still produces a dictionary, just a different one. Low-entropy + * samples make ties frequent, so train on them and compare against the + * dictionary the position order produces, for both comparator paths. */ + { size_t const dictCapacity = 16 KB; + void* const coverDict = malloc(dictCapacity); + unsigned const dValues[2] = { 8, 16 }; /* COVER_cmp8() and COVER_cmp() */ + XXH64_hash_t const expectedHash[2] = { 0x77EB0C1D595FF7A3ULL, 0xE02EC0FD3A5BBF6DULL }; + unsigned dIdx; + if (coverDict==NULL) goto _output_error; + RDG_genBuffer(decodedBuffer, totalSampleSize, 0.5, 0.05, 1); /* low-entropy: many repeated dmers */ + { U32 u; for (u=0; u> 32), (unsigned)hash, + (unsigned)(expectedHash[dIdx] >> 32), (unsigned)expectedHash[dIdx]); + goto _output_error; + } + } + free(coverDict); + } + DISPLAYLEVEL(3, "OK \n"); + DISPLAYLEVEL(3, "test%3i : check dictID : ", testNb++); dictID = ZDICT_getDictID(dictBuffer, dictSize); if (dictID==0) goto _output_error;