test(cdx): regression guards for migration-critical index ordering#124
test(cdx): regression guards for migration-critical index ordering#124Admnwk wants to merge 1 commit into
Conversation
Community migration reports describe expression / composite CDX tags and order-by-number selection coming out "disordered". These guards pin the behaviours those reports touch, exercised at scale through the public ACE ABI; all pass on the current tree (the scenarios are correct here), so they serve as regressions guards rather than failing repros. - abi_cdx_ordinal_creation_order_test: OrdSetFocus(N) / SET ORDER TO <n> must follow tag CREATION order, not the alphabetical order of the name-keyed "tag of tags" tree. Tags created TZ-then-TA must resolve ordinal 1 -> TZ. - abi_cdx_expr_reindex_order_repro_test: a UPPER(NOME) tag and a UPPER(NOME)+CODIGO composite tag over 3000 rows must order ascending after both the bulk CREATE INDEX path and the per-record AdsReindex path. - cdx_bulk_recno_repro_test: build_bulk must keep every key bound to its own recno across multiple leaves, including prefix-sharing text keys that exercise the dup/trailing-prefix leaf compression. Test-only; no production code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds three new unit test files to verify CDX index ordering, ordinal creation order, and bulk-load record number binding. The review feedback highlights several potential buffer overflow vulnerabilities when copying directory paths or field names into fixed-size buffers without bounds checks, as well as unsafe casting of string literals to non-const pointers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| std::string read_field(ADSHANDLE hTable, const char* name) { | ||
| UNSIGNED8 fld[16]; std::memcpy(fld, name, std::strlen(name) + 1); |
There was a problem hiding this comment.
If name is longer than 15 characters, std::strlen(name) + 1 will exceed the size of fld, causing a buffer overflow. Adding a REQUIRE check will prevent potential overflows.
| UNSIGNED8 fld[16]; std::memcpy(fld, name, std::strlen(name) + 1); | |
| REQUIRE(std::strlen(name) < 16); | |
| UNSIGNED8 fld[16]; std::memcpy(fld, name, std::strlen(name) + 1); |
| UNSIGNED8 srv[256]; | ||
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
There was a problem hiding this comment.
If the temporary directory path dir.string() exceeds 255 characters, std::memcpy will overflow the srv buffer. It is safer to add a guard REQUIRE(dir.string().size() < sizeof(srv)); before copying.
| UNSIGNED8 srv[256]; | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); | |
| UNSIGNED8 srv[256]; | |
| REQUIRE(dir.string().size() < sizeof(srv)); | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
| UNSIGNED8 srv[256]; | ||
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
There was a problem hiding this comment.
If the temporary directory path dir.string() exceeds 255 characters, std::memcpy will overflow the srv buffer. It is safer to add a guard REQUIRE(dir.string().size() < sizeof(srv)); before copying.
| UNSIGNED8 srv[256]; | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); | |
| UNSIGNED8 srv[256]; | |
| REQUIRE(dir.string().size() < sizeof(srv)); | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
| UNSIGNED8 srv[256]; | ||
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
There was a problem hiding this comment.
If the temporary directory path dir.string() exceeds 255 characters, std::memcpy will overflow the srv buffer. It is safer to add a guard REQUIRE(dir.string().size() < sizeof(srv)); before copying.
| UNSIGNED8 srv[256]; | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); | |
| UNSIGNED8 srv[256]; | |
| REQUIRE(dir.string().size() < sizeof(srv)); | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
| UNSIGNED8 srv[256]; | ||
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
There was a problem hiding this comment.
If the temporary directory path dir.string() exceeds 255 characters, std::memcpy will overflow the srv buffer. It is safer to add a guard REQUIRE(dir.string().size() < sizeof(srv)); before copying.
| UNSIGNED8 srv[256]; | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); | |
| UNSIGNED8 srv[256]; | |
| REQUIRE(dir.string().size() < sizeof(srv)); | |
| std::memcpy(srv, dir.string().c_str(), dir.string().size() + 1); |
| REQUIRE(AdsCreateIndex61(hTable, fn, (UNSIGNED8*)"TZ", (UNSIGNED8*)"UPPER(NOME)", | ||
| nullptr, nullptr, 0, 512, &hZ) == 0); | ||
| REQUIRE(AdsCreateIndex61(hTable, fn, (UNSIGNED8*)"TA", (UNSIGNED8*)"CODE", | ||
| nullptr, nullptr, 0, 512, &hA) == 0); |
There was a problem hiding this comment.
Casting string literals directly to non-const pointers (UNSIGNED8*) is risky and can lead to undefined behavior if the underlying API ever attempts to modify the buffer. It is safer to use local mutable arrays on the stack.
| REQUIRE(AdsCreateIndex61(hTable, fn, (UNSIGNED8*)"TZ", (UNSIGNED8*)"UPPER(NOME)", | |
| nullptr, nullptr, 0, 512, &hZ) == 0); | |
| REQUIRE(AdsCreateIndex61(hTable, fn, (UNSIGNED8*)"TA", (UNSIGNED8*)"CODE", | |
| nullptr, nullptr, 0, 512, &hA) == 0); | |
| UNSIGNED8 tagZ[16] = "TZ"; | |
| UNSIGNED8 exprZ[32] = "UPPER(NOME)"; | |
| UNSIGNED8 tagA[16] = "TA"; | |
| UNSIGNED8 exprA[32] = "CODE"; | |
| REQUIRE(AdsCreateIndex61(hTable, fn, tagZ, exprZ, | |
| nullptr, nullptr, 0, 512, &hZ) == 0); | |
| REQUIRE(AdsCreateIndex61(hTable, fn, tagA, exprA, | |
| nullptr, nullptr, 0, 512, &hA) == 0); |
|
Withdrawing to re-verify against real-world data before resubmitting. |
Summary
Migration reports from the community describe expression / composite CDX
tags and order-by-number selection coming out "disordered". I could not
reproduce a fault on the current tree — engine-level behaviour for these cases
is correct — so this PR lands the scenarios as regression guards rather
than failing repros, exercised at scale through the public ACE ABI.
Tests added (test-only, no production changes)
abi_cdx_ordinal_creation_order_test—OrdSetFocus(N)/SET ORDER TO <n>must follow tag creation order, not the alphabetical order of thename-keyed "tag of tags" tree. Creating
TZthenTA, ordinal 1 mustresolve to
TZ.abi_cdx_expr_reindex_order_repro_test— aUPPER(NOME)tag and aUPPER(NOME)+CODIGOcomposite tag over 3000 rows must order ascending afterboth the bulk
CREATE INDEXpath and the per-recordAdsReindexpath.cdx_bulk_recno_repro_test—build_bulkmust keep every key bound toits own recno across multiple leaves, including prefix-sharing text keys that
exercise the dup/trailing-prefix leaf compression.
Result
All pass on the current tree (and on the v1.4.0 tag).
/WXclean, MSVC x64.They lock in the behaviours the reports touch so a future regression in the
bulk builder / expression-key build / ordinal sequence is caught immediately.
🤖 Generated with Claude Code (Opus 4.8)