From dae3da604feab862651c9727e0438fe0c02a2f36 Mon Sep 17 00:00:00 2001 From: russimicro Date: Thu, 30 Jul 2026 07:45:19 -0500 Subject: [PATCH] fix(adi): the branch descent compares only the bytes of the seek key AdiIndex::compare_keys_ compares min(a, b, key_total_len_) bytes, so the dense-leaf scan honours a seek key shorter than the index key -- the prefix rule the CDX side already follows. The char-key BRANCH descent did not: it memcmp'd key_total_len_ bytes against the separator while the folded search key held only what the caller supplied, reading past the end of that buffer whenever the separator's own prefix equalled the search prefix. Honesty about the impact: I could not turn this into a wrong answer. The bytes following a std::string compare low, which selects the same child the prefix rule wants, and that held both inside the small-buffer optimisation and with a key long enough to force a heap allocation. So this is an out-of-bounds read removed and the rule stated once instead of twice, not a reproduced defect. The tests are worth more than the fix here: the ADT / ADI side of the partial-seek rule had no coverage at all, while the CDX side has had it since the seek and scope fixes. Both cases sweep every key rather than probing one, because a single wrong branch decision shows up as one miss in 1500 and any single probe is likely to land mid-page and pass regardless. The tree has to be multi-level for the branch descent to run at all, hence the row counts. Test: abi_adi_prefix_seek_multilevel_test -- 6000 rows on a 16-byte key seeking a 10-byte prefix (the shape of the ERP's `SEEK cCodigoCon + cDocumeTra` over a con+doc+seq tag), and 3000 rows on a 36-byte key seeking a 16-byte prefix so the folded key lands on the heap instead of inside std::string's inline buffer. The key is a single wide character field, not a compound expression, so a plain v1 ADI tag suffices. (cherry picked from commit ae4994afb4cc38201dcb566dd1cc7595caf1583c) --- src/drivers/adi/adi_index.cpp | 12 +- tests/CMakeLists.txt | 1 + .../abi_adi_prefix_seek_multilevel_test.cpp | 161 ++++++++++++++++++ 3 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 tests/unit/abi_adi_prefix_seek_multilevel_test.cpp diff --git a/src/drivers/adi/adi_index.cpp b/src/drivers/adi/adi_index.cpp index dcb9eccc..79bce316 100644 --- a/src/drivers/adi/adi_index.cpp +++ b/src/drivers/adi/adi_index.cpp @@ -861,6 +861,15 @@ util::Result AdiIndex::seek_key(const std::string& key, bool soft) // components fold to upper on BOTH sides so a mismatched-case // search key descends the same subtree as the stored key. const std::string fnkey = fold_for_compare_(nkey); + // A seek key SHORTER than the index key bounds by prefix, so + // compare only the bytes the caller supplied — the same rule + // compare_keys_ applies at the dense leaf. Comparing the full + // key_total_len_ here read PAST the end of fnkey: with a partial + // key whose prefix equalled the separator's, the decision was + // then made on whatever followed the string in memory, and the + // descent could take the wrong child and miss a key that exists. + const std::size_t cmp_len = + std::min(fnkey.size(), key_total_len_); int chosen = static_cast(cnt) - 1; for (int i = 0; i < static_cast(cnt); ++i) { const std::uint8_t* ek = pg.data() + ADI_TREE_ENTRY_START @@ -868,8 +877,7 @@ util::Result AdiIndex::seek_key(const std::string& key, bool soft) const std::string fek = fold_for_compare_( std::string(reinterpret_cast(ek), key_total_len_)); - if (std::memcmp(fnkey.data(), fek.data(), - key_total_len_) <= 0) { + if (std::memcmp(fnkey.data(), fek.data(), cmp_len) <= 0) { chosen = i; break; } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d1fbb3ec..e62c5653 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -118,6 +118,7 @@ add_executable(openads_unit_tests unit/abi_seek_last_partial_test.cpp unit/abi_aof_index_agreement_test.cpp unit/abi_aof_empty_result_test.cpp + unit/abi_adi_prefix_seek_multilevel_test.cpp unit/abi_prefix_seek_deleted_test.cpp unit/cdx_reindex_char_test.cpp unit/cdx_prev_empty_leaf_test.cpp diff --git a/tests/unit/abi_adi_prefix_seek_multilevel_test.cpp b/tests/unit/abi_adi_prefix_seek_multilevel_test.cpp new file mode 100644 index 00000000..78e3a973 --- /dev/null +++ b/tests/unit/abi_adi_prefix_seek_multilevel_test.cpp @@ -0,0 +1,161 @@ +// abi_adi_prefix_seek_multilevel_test.cpp -- a partial (prefix) SEEK on an +// ADT table must find its group, including once the .ADI char-key tree has +// grown branch levels. +// +// AdiIndex::compare_keys_ compares min(a, b, key_total_len_) bytes, so the +// dense-leaf scan honours a seek key shorter than the index key -- the same +// prefix rule the CDX side follows. The char-key BRANCH descent did not: it +// memcmp'd key_total_len_ bytes against the separator while the folded +// search key held only the bytes the caller supplied, reading past the end +// of that buffer whenever the separator's own prefix equalled the search +// prefix. +// +// The read is out of bounds, but no input tried here turns it into a wrong +// answer: the bytes that follow a std::string compare low, which picks the +// same child the prefix rule wants. So this file is coverage, not a +// reproducer -- the ADT/ADI side of the partial-seek rule had no test at +// all, while the CDX side has had one since the seek and scope fixes. +// +// Both cases sweep EVERY key rather than probing one: a single wrong branch +// decision shows up as one miss in 1500, and any single probe is likely to +// land mid-page and pass regardless. The tree has to be multi-level for the +// branch descent to run at all -- with one dense leaf the loop never +// executes -- hence the row counts. +// +// The key is a single wide character field rather than a compound +// expression, so the ADI v1 tag every build can create is enough. + +#include "doctest.h" +#include "openads/ace.h" + +#include +#include +#include +#include + +namespace fs = std::filesystem; + +namespace { + +// Build an ADT whose only field is a KEYLEN-wide character key, one row per +// (doc, line) pair, then index it. Sweeps every doc with a PREFIXLEN-byte +// partial seek and reports how many missed. +struct Sweep { + int misses = 0; + int wrong_row = 0; + int first_bad = 0; +}; + +Sweep run_sweep(const char* dirname, const char* table, const char* bag, + int keylen, int prefixlen, int docs, int lines) { + fs::path tmp = fs::temp_directory_path() / dirname; + { + std::error_code ec; + fs::remove_all(tmp, ec); + fs::create_directories(tmp, ec); + } + + UNSIGNED8 srv[260]{}; + std::memcpy(srv, tmp.string().c_str(), tmp.string().size()); + ADSHANDLE hConn = 0; + REQUIRE(AdsConnect60(srv, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &hConn) + == AE_SUCCESS); + + char flddef[64]; + std::snprintf(flddef, sizeof(flddef), "K,Character,%d", keylen); + ADSHANDLE hTable = 0; + REQUIRE(AdsCreateTable(hConn, (UNSIGNED8*)table, nullptr, ADS_ADT, + ADS_ANSI, 0, 0, 0, (UNSIGNED8*)flddef, &hTable) + == AE_SUCCESS); + + // Key layout: "2P" + doc(8) + line(6), right-padded to keylen. The first + // 10 bytes are the "document" the partial seek supplies, mirroring the + // ERP's SEEK cCodigoCon + cDocumeTra over a con+doc+seq tag. + for (int d = 1; d <= docs; ++d) { + for (int s = 1; s <= lines; ++s) { + char key[128]; + std::snprintf(key, sizeof(key), "2P%08d%6d", d, s); + std::size_t n = std::strlen(key); + while (n < static_cast(keylen)) key[n++] = ' '; + key[keylen] = '\0'; + REQUIRE(AdsAppendRecord(hTable) == AE_SUCCESS); + REQUIRE(AdsSetString(hTable, (UNSIGNED8*)"K", (UNSIGNED8*)key, + static_cast(keylen)) + == AE_SUCCESS); + REQUIRE(AdsWriteRecord(hTable) == AE_SUCCESS); + } + } + + ADSHANDLE hIdx = 0; + REQUIRE(AdsCreateIndex61(hTable, (UNSIGNED8*)bag, (UNSIGNED8*)"TAG1", + (UNSIGNED8*)"K", nullptr, nullptr, 0, 0, &hIdx) + == AE_SUCCESS); + + Sweep sw; + for (int d = 1; d <= docs; ++d) { + char key[128]; + std::snprintf(key, sizeof(key), "2P%08d%6d", d, 1); + REQUIRE(AdsGotoTop(hTable) == AE_SUCCESS); + UNSIGNED16 found = 0; + REQUIRE(AdsSeek(hIdx, (UNSIGNED8*)key, + static_cast(prefixlen), + ADS_STRINGKEY, 0, &found) == AE_SUCCESS); + if (!found) { + if (sw.first_bad == 0) sw.first_bad = d; + ++sw.misses; + continue; + } + // A partial seek lands on the FIRST entry of the group. + UNSIGNED8 buf[128]{}; + UNSIGNED32 len = sizeof(buf); + REQUIRE(AdsGetString(hTable, (UNSIGNED8*)"K", buf, &len, 0) + == AE_SUCCESS); + char want[128]; + std::snprintf(want, sizeof(want), "2P%08d%6d", d, 1); + if (std::string((char*)buf).compare(0, std::strlen(want), want) != 0) { + if (sw.first_bad == 0) sw.first_bad = d; + ++sw.wrong_row; + } + } + + // A prefix that matches nothing must still miss. + char absent[128]; + std::snprintf(absent, sizeof(absent), "2P%08d%6d", docs + 77, 1); + REQUIRE(AdsGotoTop(hTable) == AE_SUCCESS); + UNSIGNED16 found = 1; + REQUIRE(AdsSeek(hIdx, (UNSIGNED8*)absent, + static_cast(prefixlen), + ADS_STRINGKEY, 0, &found) == AE_SUCCESS); + CHECK(found == 0); + + AdsCloseTable(hTable); + AdsDisconnect(hConn); + std::error_code ec; + fs::remove_all(tmp, ec); + return sw; +} + +} // namespace + +TEST_CASE("ADI: partial SEEK finds its group through a multi-level tree") { + // 16-byte key, 10-byte prefix: the folded search key still fits + // std::string's small-buffer optimisation. + Sweep sw = run_sweep("openads_adi_prefix_seek", "mov.adt", "mov.adi", + /*keylen=*/16, /*prefixlen=*/10, + /*docs=*/1500, /*lines=*/4); + CAPTURE(sw.first_bad); + CHECK(sw.misses == 0); + CHECK(sw.wrong_row == 0); +} + +TEST_CASE("ADI: partial SEEK on a wide key through a multi-level tree") { + // 36-byte key, 16-byte prefix: past the small-buffer optimisation, so + // whatever follows the search key is heap content rather than the tail + // of an inline buffer. + Sweep sw = run_sweep("openads_adi_prefix_seek_wide", "movw.adt", + "movw.adi", /*keylen=*/36, /*prefixlen=*/16, + /*docs=*/1500, /*lines=*/2); + CAPTURE(sw.first_bad); + CHECK(sw.misses == 0); + CHECK(sw.wrong_row == 0); +}