fix(hash): honor a custom comparator on legacy unsorted hash pages - #144
Merged
Conversation
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (2) -- update dist/cocci/baseline.txt to lock these in. |
|
ABI diff produced no report (build skipped or no base tag). Advisory: libabigail/nm is the authoritative binary-ABI check; Coccinelle is complementary source-level early warning. See dist/cocci/README.md. |
__ham_getindex_unsorted() does the linear search over a pre-4.6
P_HASH_UNSORTED page, which libdb still reads without requiring
DB->upgrade. Its inline-key (H_KEYDATA) branch had two defects that
masked each other whenever the application configured a comparator with
DB->set_h_compare:
1. It called the comparator only to reject a non-zero result and never
stored a zero one, so `res` kept its initial 1 (not-equal), the
`if (res == 0) break;` after the switch never fired, and *match was
set to 1 (not found). DB->get returned DB_NOTFOUND for a key that
was present, and DB->put(DB_NOOVERWRITE) returned success and stored
a second record with identical key bytes even though duplicates are
disabled -- silently breaking key uniqueness.
2. It built the stored-key DBT with key->size, the length of the SEARCH
key, instead of the length of the item actually on the page. The
comparator therefore saw a truncated or over-long view of the stored
key: a search key that merely prefixed a stored key compared equal
(masked by defect 1 -- fixing only the dropped result turns the
false negative into a false positive that returns another record's
data), and an over-long search key made the comparator read past the
stored item.
Compare against the stored key at its own length and record the result,
which is what __ham_getindex_sorted's equivalent case already does
(itemlen = LEN_HKEYDATA(...), then res = t->h_compare(...)), and what the
H_OFFPAGE case next door does by passing &res through to __db_moff.
The memcmp path was always correct: it length-checks first and assigns
its result. Only the comparator path was affected, so reproducing this
needs a legacy page and an explicit DB->set_h_compare together -- a new
handle's h_compare is NULL, and current sorted pages take
__ham_getindex_sorted.
Fixes #139
test/db/hash_unsorted_cmp.c + run_hash_unsorted_cmp.sh cover the __ham_getindex_unsorted() comparator branch (issue #139), which no existing test reached: the branch needs a legacy P_HASH_UNSORTED page AND an explicitly configured DB->set_h_compare at the same time. test093 sets a comparator but only over current-format sorted pages (which take __ham_getindex_sorted), and run_upgrade.sh reads legacy pages but never sets a comparator. The legacy fixture is manufactured with the technique run_upgrade.sh already uses for old-format fixtures -- no old library and no committed binary blob. A P_HASH_UNSORTED page has the same byte layout as a P_HASH page; P_HASH merely additionally keeps its pairs in comparison order. So the driver creates a current-format Hash db (512-byte pages, inline and off-page keys), then rewrites each bucket page's PAGE.type byte from P_HASH to P_HASH_UNSORTED and the metadata version back to the 4.5.20 hash version 8. That is exactly the file __ham_getindex dispatches to the unsorted path. Checks, each run with the comparator (trigger) and without it (control) so a failure is attributable to the comparator path rather than to the fixture: * DB->get of a stored inline key succeeds and returns its value; * DB->get of a stored off-page key succeeds (the H_OFFPAGE branch next door, which passes &res to __db_moff, must stay correct); * DB->put(DB_NOOVERWRITE) over a live key returns DB_KEYEXIST and adds no record (verified by a full cursor scan); * a key that merely prefixes a stored key, and one that extends it, are both reported absent -- these catch the wrong-length stored-key DBT, including the false positive that a result-only fix would introduce. Against the unfixed library the four trigger checks fail (DB->get => DB_NOTFOUND for a stored key; DB_NOOVERWRITE => success with 22 records, 2 carrying the target key) while every control passes. Registered in the coverage subset (run_coverage.sh, full_run3_combined.sh) and documented in test/coverage/README.md.
This was referenced Sep 6, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #139
The defect
__ham_getindex_unsorted()(src/hash/hash_page.c) is the linear search over a pre-4.6P_HASH_UNSORTEDpage, a format 5.3 still reads without requiringDB->upgrade. Its inline-key (H_KEYDATA) branch had two defects, and they masked each other:The comparator's result was discarded.
resis initialized to 1 (not-equal). On equality theifis false and control reaches the trailingbreakwithout ever assigningres = 0, so theif (res == 0) break;after the switch never fires and*matchis set to 1 (not found).DB->getreturnedDB_NOTFOUNDfor a key that was present, andDB->put(DB_NOOVERWRITE)returned success and stored a second record with identical key bytes although duplicates are disabled — silently breaking key uniqueness. (This is what the report describes.)The stored-key DBT carried the wrong length. It was built with
key->size, the length of the search key, not the length of the item on the page. The comparator therefore saw a truncated or over-long view of the stored key. Defect 1 was hiding this: applying only the reported one-line fix converts the false negative into a false positive —DB->get("acct")returnsacct0000's data, because "acct" compares equal against a 4-byte-truncated view of the stored key. An over-long search key also made the comparator read past the stored item. Both are demonstrated in the test evidence below.The
memcmppath was always correct: it length-checks first and assigns its result.The fix
This is the established idiom in the same file:
__ham_getindex_sorted's equivalent case doesitemlen = LEN_HKEYDATA(dbp, p, dbp->pgsize, indx); ... res = t->h_compare(dbp, key, &tmp_dbt);, and theH_OFFPAGEcase immediately above passest->h_compareand&resthrough to__db_moff.Audit: every comparator call site in
src/hash/hash_page.c:679__ham_getindex_unsorted,H_OFFPAGE&resto__db_moff, which assigns*cmppon every path including thecmpfunc != NULLone. Only reached whentlen == key->size; that is a length pre-filter, not a substitute for the comparator, and it is conservative for equality under the documentedset_h_comparecontract (a comparator for an existing db must reproduce the ordering it was built with, so different-length keys the built-in call unequal cannot be equal). Covered by a positive check in the new test.hash_page.c:693__ham_getindex_unsorted,H_KEYDATAhash_page.c:787__ham_getindex_sorted, case 1 (offpage/offpage)__db_coff(..., t->h_compare, &res); the short-circuit above it (koff_pgno == off_pgno → res = 0) is same-page identity, correct.hash_page.c:795__ham_getindex_sorted, case 2 (offpage key, on-page probe)__db_moff(..., &res).hash_page.c:810__ham_getindex_sorted, case 3 (on-page key, offpage probe)__db_moff(..., &res), thenres = -resbecause the arguments were swapped.hash_page.c:821__ham_getindex_sorted, case 4 (on-page/on-page)itemlen = LEN_HKEYDATA(...), thenres = t->h_compare(dbp, key, &tmp_dbt)— the true stored length, result captured.hash_page.c:882–922__ham_verify_sorted_paget->h_compare != NULL— sort order under a user comparator is deliberately not verified — so the three__db_coff/__db_moffcalls below only ever run withcmpfunc == NULL. All three pass&resanyway.hash.c:1548__ham_lookupDB_GET_BOTH, off-page data__db_moff(..., dbp->dup_compare, &cmp), thencmp = -cmpfor the swapped arguments;cmpis tested. This isdup_compare(data), noth_compare.hash.c:1559-1561same, on-page datacmp = ... dup_compare(...)— result assigned and tested.hash.c:1767,hash.c:1812__ham_overwritesort-order guards__db_duperr/EINVAL). Nothing depends on recording equality; equality is the pass case.hash_dup.c:174cmpset by__ham_dsearch(below).hash_dup.c:780,800__ham_dsearch*cmpp = func(dbp, dbt, &cur)on every iteration, andfuncfalls back to__bam_defcmpwhendup_compareis NULL, so*cmppis always written.hash_dup.c:281,842,hash_open.c:220-305dup_compare == NULLused as a "sorted dups?" predicate to pick a page type / setDB_AM_DUPSORT. Correct as written.hash_verify.c:1137__ham_dups_unsortedfunc(...) > 0is the whole decision (is this dup set out of order?); the boolean is the result.Verdict:
__ham_getindex_unsorted'sH_KEYDATAbranch was the only site that dropped a comparison result. It is also the only site that built a comparison DBT from the search key's length instead of the stored item's.Regression test
test/db/hash_unsorted_cmp.c+test/db/run_hash_unsorted_cmp.sh.The branch needs a legacy
P_HASH_UNSORTEDpage and an explicitDB->set_h_compareat the same time, which is why nothing caught it:test093sets a comparator but only over current-format sorted pages (→__ham_getindex_sorted), andrun_upgrade.shreads legacy pages but never sets a comparator.Fixture, built synthetically — no old library, no committed binary blob. Approach (a) from the report, using the technique
test/db/run_upgrade.shalready uses for old-format fixtures. AP_HASH_UNSORTEDpage and aP_HASHpage have identical byte layouts;P_HASHmerely additionally keeps its pairs in comparison order, which is a subset of whatP_HASH_UNSORTEDpermits. So the driver creates a current-format Hash db (512-byte pages, 20 inline keys + one 200-byte off-page key → 2 bucket pages), then rewrites each bucket page'sPAGE.typebyte (offset 25) fromP_HASH(13) toP_HASH_UNSORTED(2) and the metadata version (offset 16) back to the 4.5.20 hash version 8. That file is exactly what__ham_getindexdispatches to__ham_getindex_unsorted. Deterministic and self-contained.Every check runs twice, with the comparator (trigger) and without (control), so a failure is attributable to the comparator path and not to the fixture. The comparator is
byte_compare, the comparison BDB itself used before 4.6 addedset_h_compare— which is what the API docs require for an existing database.Before the fix (
--enable-debug):Note
equal=1on the false miss (the comparator did report equality and it was dropped) andequal=10on the prefix probe (defect 2, invisible only because defect 1 discarded it).With only the reported one-line change (
res = t->h_compare(...), stillkey->size) — this is why the fix is two changes, not one:DB->get("acct")now returnsacct0000's data: a wrong-record read instead of a missed read.After the fix (both
--enable-debugand release):cmp_callsalso drops 10 → 1 on the successful lookup: the search now stops at the match instead of scanning the whole page.Validation
--enable-debug --enable-test --with-tcl=...and a plain release build (../dist/configure). New test passes under both.test001test003test011test093(the existingset_h_comparetest) — 0 failures. Plustest006 test017 test024 test025 test029 test031 test032 test038 test039 test044 test046 test048 test051 hsearchon hash — 0 failures.__db_moff/__db_coff):test001test093— 0 failures.test/fuzz/check-crashes.sh→ 9/9 PASS (includes the hash OOB seed), ASan-instrumented libdb.test/db/run_upgrade.sh: reaches a pre-existing failure ath_v5.db(db_verify: BDB1101 Page 0: spares array entry 1 is invalid). Confirmed pre-existing and unrelated: byte-identical output with this commit'shash_page.creverted to master. Not touched here — the whole diff sits insideif (t->h_compare != NULL), anddb_upgradenever sets a comparator, so it is inert on that path.Registered in the coverage harness (
test/coverage/run_coverage.sh,full_run3_combined.sh) and documented intest/coverage/README.md.