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); +}