Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/engine/index_expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,14 @@ Value parse_atom(Lex& lx, Table& t) {
f.type == drivers::DbfFieldType::AdtMoney) {
v.is_number = true;
v.n = r.value().as_double;
} else if (f.type == drivers::DbfFieldType::Logical) {
// A bare logical field as a truthy term (e.g. FOR ACTIVE) must
// evaluate to its boolean value. Its as_string is "T"/"F" — both
// non-empty — so the truthy fallback (!s.empty()) would match
// every row and a conditional FOR index would index ALL records.
// Carry it as a number (1/0) instead.
v.is_number = true;
v.n = r.value().as_bool ? 1.0 : 0.0;
} else {
v.is_number = false;
std::string sv = r.value().as_string;
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ add_executable(openads_unit_tests
unit/engine_order_test.cpp
unit/engine_scope_test.cpp
unit/abi_index_smoke_test.cpp
unit/abi_qa_repro_test.cpp
unit/aof_expr_test.cpp
unit/aggregate_test.cpp
unit/abi_aggregate_test.cpp
Expand Down
131 changes: 131 additions & 0 deletions tests/unit/abi_qa_repro_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
// QA repros (engine-level, no rddads) for divergences found by the Harbour
// differential harness against OpenADS CDX. Native DBFCDX is the oracle.
// A: INDEX ON AGE FOR ACTIVE -> conditional FOR clause must be honored.
// C: ordScope(28..40) on numeric index -> scoped range must return rows.
// These build a real DBF/CDX table via the ABI and exercise Ads* directly.
#include "doctest.h"
#include "openads/ace.h"
#include "openads/error.h"

#include <cstring>
#include <filesystem>
#include <string>

namespace fs = std::filesystem;

using openads::AE_SUCCESS;

namespace {

void put_row(ADSHANDLE hT, const char* name, double age, bool active) {
REQUIRE(AdsAppendRecord(hT) == AE_SUCCESS);
UNSIGNED8 fN[] = "NAME";
UNSIGNED8 v[64]{};
std::strncpy(reinterpret_cast<char*>(v), name, sizeof(v) - 1);
REQUIRE(AdsSetString(hT, fN, v,
static_cast<UNSIGNED32>(std::strlen(name))) == AE_SUCCESS);
Comment on lines +23 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

If name is longer than 63 characters, std::strncpy will truncate the copied string in v to 63 characters (leaving v[63] as \0). However, std::strlen(name) is still used as the length parameter for AdsSetString. This causes AdsSetString to read past the null-terminator of v, leading to a potential out-of-bounds read (buffer over-read) on the stack.

To prevent this, use the length of the actually copied string in v instead of the original name length.

Suggested change
UNSIGNED8 v[64]{};
std::strncpy(reinterpret_cast<char*>(v), name, sizeof(v) - 1);
REQUIRE(AdsSetString(hT, fN, v,
static_cast<UNSIGNED32>(std::strlen(name))) == AE_SUCCESS);
UNSIGNED8 v[64]{};
std::strncpy(reinterpret_cast<char*>(v), name, sizeof(v) - 1);
REQUIRE(AdsSetString(hT, fN, v,
static_cast<UNSIGNED32>(std::strlen(reinterpret_cast<char*>(v)))) == AE_SUCCESS);

UNSIGNED8 fA[] = "AGE";
REQUIRE(AdsSetDouble(hT, fA, age) == AE_SUCCESS);
UNSIGNED8 fAct[] = "ACTIVE";
REQUIRE(AdsSetLogical(hT, fAct, active ? 1 : 0) == AE_SUCCESS);
REQUIRE(AdsWriteRecord(hT) == AE_SUCCESS);
}

// Same 8-row fixture as the Harbour harness; 5 rows have ACTIVE = true.
ADSHANDLE open_fixture(const fs::path& dir, const char* tbl) {
std::error_code ec;
fs::create_directories(dir, ec);
fs::remove(dir / tbl, ec);
Comment on lines +35 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The open_fixture function only removes the .dbf file (dir / tbl), but does not clean up any associated index files (like .cdx or .fpt files) that might have been created in the same directory during previous test runs. This can lead to test flakiness or unexpected behavior if stale index files are left behind.

Using std::filesystem::remove_all on the directory before recreating it ensures a completely clean and isolated test environment.

Suggested change
ADSHANDLE open_fixture(const fs::path& dir, const char* tbl) {
std::error_code ec;
fs::create_directories(dir, ec);
fs::remove(dir / tbl, ec);
ADSHANDLE open_fixture(const fs::path& dir, const char* tbl) {
std::error_code ec;
fs::remove_all(dir, ec);
fs::create_directories(dir, ec);


UNSIGNED8 srv[260]{};
std::memcpy(srv, dir.string().c_str(), dir.string().size());
Comment on lines +40 to +41

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Using std::memcpy to copy dir.string() into the fixed-size buffer srv[260] without checking the size of the path can lead to a buffer overflow if the temporary directory path is extremely long.

Adding a REQUIRE assertion to ensure the path fits within the buffer prevents any potential buffer overflow.

Suggested change
UNSIGNED8 srv[260]{};
std::memcpy(srv, dir.string().c_str(), dir.string().size());
UNSIGNED8 srv[260]{};
REQUIRE(dir.string().size() < sizeof(srv));
std::memcpy(srv, dir.string().c_str(), dir.string().size());

ADSHANDLE hConn = 0;
REQUIRE(AdsConnect60(srv, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &hConn)
== AE_SUCCESS);

UNSIGNED8 name[64]{};
std::strncpy(reinterpret_cast<char*>(name), tbl, sizeof(name) - 1);
UNSIGNED8 flddef[] = "NAME,Character,20;AGE,Numeric,3,0;ACTIVE,Logical,1";
ADSHANDLE hTable = 0;
REQUIRE(AdsCreateTable(hConn, name, nullptr, ADS_CDX, ADS_ANSI, 0, 0, 0,
flddef, &hTable) == AE_SUCCESS);

put_row(hTable, "Charlie", 30, true);
put_row(hTable, "alice", 25, false);
put_row(hTable, "Bob", 40, true);
put_row(hTable, "dave", 22, true);
put_row(hTable, "Eve", 35, false);
put_row(hTable, "bob", 28, true);
put_row(hTable, "Mary", 45, true);
put_row(hTable, "tom", 31, false);
return hTable;
}

UNSIGNED32 walk_count(ADSHANDLE hTable) {
REQUIRE(AdsGotoTop(hTable) == AE_SUCCESS);
UNSIGNED32 n = 0;
for (;;) {
UNSIGNED16 eof = 0;
REQUIRE(AdsAtEOF(hTable, &eof) == AE_SUCCESS);
if (eof) break;
++n;
REQUIRE(AdsSkip(hTable, 1) == AE_SUCCESS);
if (n > 100) break; // safety
}
return n;
}

} // namespace

// A — conditional FOR index: only the 5 ACTIVE rows must be indexed.
TEST_CASE("QA-A: CDX conditional FOR index honors the condition") {
const auto dir = fs::temp_directory_path() / "openads_qa_a";
ADSHANDLE hTable = open_fixture(dir, "qa_a.dbf");

UNSIGNED8 idxfile[] = "qa_a.cdx";
UNSIGNED8 idxname[] = "TCOND";
UNSIGNED8 expr[] = "AGE";
UNSIGNED8 cond[] = "ACTIVE"; // FOR condition
ADSHANDLE hIdx = 0;
REQUIRE(AdsCreateIndex61(hTable, idxfile, idxname, expr,
cond, nullptr, 0, 0, &hIdx) == AE_SUCCESS);

UNSIGNED32 keys = 0;
REQUIRE(AdsGetKeyCount(hIdx, 2 /*ADS_IGNOREFILTERS*/, &keys) == AE_SUCCESS);
INFO("AdsGetKeyCount on conditional index = ", keys, " (expected 5)");
CHECK(keys == 5u);

// Walking the conditional index must visit only the 5 ACTIVE rows.
UNSIGNED32 walked = walk_count(hTable);
INFO("walk over conditional index = ", walked, " (expected 5)");
CHECK(walked == 5u);

AdsCloseTable(hTable);
}

// C — numeric ordScope: range [28..40] must return the 5 in-range rows.
TEST_CASE("QA-C: numeric index ordScope returns in-range rows") {
const auto dir = fs::temp_directory_path() / "openads_qa_c";
ADSHANDLE hTable = open_fixture(dir, "qa_c.dbf");

UNSIGNED8 idxfile[] = "qa_c.cdx";
UNSIGNED8 idxname[] = "TAGE";
UNSIGNED8 expr[] = "AGE";
ADSHANDLE hIdx = 0;
REQUIRE(AdsCreateIndex61(hTable, idxfile, idxname, expr,
nullptr, nullptr, 0, 0, &hIdx) == AE_SUCCESS);

double top = 28.0, bot = 40.0;
REQUIRE(AdsSetScope(hIdx, ADS_TOP, reinterpret_cast<UNSIGNED8*>(&top),
sizeof(double), ADS_DOUBLEKEY) == AE_SUCCESS);
REQUIRE(AdsSetScope(hIdx, ADS_BOTTOM, reinterpret_cast<UNSIGNED8*>(&bot),
sizeof(double), ADS_DOUBLEKEY) == AE_SUCCESS);

UNSIGNED32 inScope = walk_count(hTable);
INFO("rows within scope [28..40] = ", inScope, " (expected 5)");
CHECK(inScope == 5u);

AdsClearScope(hIdx, ADS_TOP);
AdsClearScope(hIdx, ADS_BOTTOM);
AdsCloseTable(hTable);
}
46 changes: 46 additions & 0 deletions tools/qa-diff/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# qa-diff — differential xBase QA for OpenADS

Runs the **same** classic xBase operations through pure Harbour (rddads, no
ORM) against two RDDs and diffs the output. The native RDD is the **oracle**:
where native gives the textbook result and OpenADS differs, you have a
candidate bug. This catches "boring" correctness bugs that unit suites miss —
`INDEX ON … FOR …`, `REINDEX`, `SEEK`, ascending/descending walks, `SET FILTER`,
`ordScope`, `LOCATE`, `PACK/ZAP`, conditional indexes, memo round-trips.

Compared same-family (apples to apples):

| Oracle (native) | System under test (OpenADS) |
|-----------------|-----------------------------|
| `DBFCDX` | `ADSCDX` |
| `DBFNTX` | `ADSNTX` |

## Files
- `qamatrix.prg` — the ~17-step operation matrix; writes a normalized,
diffable log (one labelled line per checkpoint). Errors are trapped and
logged (no blocking GUI alert) so a runtime failure is recorded, not hung.
- `repro.prg` — minimal **isolated** reproducers (each test = fresh table, no
state cascade) for the divergences worth filing as bugs.
- `qamatrix.hbp` / `repro.hbp` — link lines (`-lrddads -L${OPENADS_LIB}
-l${OPENADS_ACELIB} -lrddcdx -lrddntx -lrddfpt`).
- `run.cmd` — portable build+run+diff driver. No baked-in paths.

## Usage
```cmd
:: from an MSVC x64 dev prompt, with hbmk2 + rddads available
run.cmd <folder-with-openace64.dll-and-.lib>
```
It builds `qamatrix`, runs it for DBFCDX/DBFNTX/ADSCDX/ADSNTX, then `fc`-diffs
native vs ADS. Differing lines = candidate bugs.

> Toolchain note: a headless build works with Harbour (MSVC64) + a portable
> MSVC whose `setup_x64.bat` provides the Windows SDK, plus the CRT-compat link
> flags carried in `run.cmd`. See the cookbook `console/build.cmd` for the same
> recipe.

## Methodology caveat
Native uses `rddcdx`/`rddntx`; ADS uses `rddads → openace64`. A divergence is
not *necessarily* an engine bug — it can live in the rddads→ABI mapping.
**Confirm engine-level findings with an ABI doctest** (no rddads) before filing
them as engine bugs. Example: `tests/unit/abi_qa_repro_test.cpp` confirms the
conditional-`FOR` logical-field bug at the ABI; a numeric `ordScope` divergence
seen here, by contrast, passes at the ABI (so it is a mapping issue, not engine).
7 changes: 7 additions & 0 deletions tools/qa-diff/qamatrix.hbp
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
qamatrix.prg
-lrddads
-L${OPENADS_LIB}
-l${OPENADS_ACELIB}
-lrddcdx
-lrddntx
-lrddfpt
Loading
Loading