diff --git a/.bazelrc b/.bazelrc index 89982ece5..7bf162412 100644 --- a/.bazelrc +++ b/.bazelrc @@ -111,6 +111,21 @@ test:ubsan --test_timeout=300,300,900,3600 # Hardcoded rpath: .../lib/clang/21/... must match MODULE.bazel llvm major and Xcode. build:ubsan_macos --linkopt=-Wl,-rpath,/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/21/lib/darwin +# libFuzzer configuration (//library/tests/fuzz:*_fuzz) +# Usage: bazel run --config=fuzz //library/tests/fuzz:pbn_fuzz -- -runs=100000 +# Uses the registered hermetic LLVM toolchain, which ships libclang_rt.fuzzer; +# Apple's toolchain does not, so do not combine with --config=asan on macOS +# (build:asan_macos switches to the Xcode toolchain). On Linux, adding +# --config=asan gives fuzzing with memory-error detection. The corpus-replay +# tests (//library/tests/fuzz:*_fuzz_corpus_test) need none of this and run +# under the ordinary configs. +build:fuzz --copt=-fsanitize=fuzzer-no-link +build:fuzz --copt=-fno-omit-frame-pointer +build:fuzz --linkopt=-fsanitize=fuzzer +build:fuzz --strip=never +build:fuzz --features=dbg +build:fuzz --compilation_mode=dbg + # MemorySanitizer configuration (Linux x86_64 only) # Usage: bazel test --config=msan //path:target # Selects @llvm_toolchain_msan (instrumented libc++ overlay) and enables the diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..9617886ad --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,154 @@ +# Security Policy + +## What DDS is, for threat-modelling purposes + +DDS is an **in-process library**, not a service. It opens no sockets and +crosses no privilege boundary. In the normal deployment the input is a bridge +deal supplied by the calling application — usually that application's own data, +or a hand a user typed in themselves. + +It does, however, carry **process-local solver resources** that outlive an +individual call: a transposition table and per-thread working memory managed +through the legacy `SetResources()` and `FreeMemory()`, and a worker pool held +in a function-local static that persists for the lifetime of the process. +Calls are therefore not isolated from one another, so corruption caused by one +call can be observed by a later one. + +Thread counts work differently from memory, and the distinction matters: + +- The legacy **memory** settings are process-wide. One component's + `SetResources()` choice applies to every other user of the library in the + same process. +- The **worker pool** is shared process-wide, but how many of its workers a + given call uses is decided **per call**, by the `maxThreads` argument of the + `*N` and `*X` batch entry points. A value of 0 selects hardware concurrency. +- `SetMaxThreads()` sets nothing. It is a deprecated alias of + `InitializeStaticMemory()` and its argument is ignored; internal batch + threading was removed. Do not treat it as a resource limit. In the modern + C++ API the embedding application controls concurrency, typically with one + `SolverContext` per worker thread. + +This matters when judging the severity of a memory-safety bug here. A defect +reachable only from data the caller already controls, in a library running in +the caller's own address space, is a robustness problem rather than a +privilege-escalation vector: there is no boundary being crossed. + +Two deployments raise the stakes, and one lowers them: + +- **PBN input from files** is the one classic untrusted-input path. + `convert_from_pbn()` and the `*PBN` entry points parse externally authored + text. +- **Server-side use** — any service that accepts user-submitted deals or PBN + over a network — makes everything below remotely reachable. If you are + building one, read "If you expose DDS to untrusted input" below. +- **The WebAssembly build** contains the blast radius. A memory error stays + inside the module's linear memory and cannot corrupt the host page, so the + browser deployment is substantially better protected than the native ones. + +## Input trust model + +**DDS assumes trusted, well-formed input.** It validates enough to catch honest +caller mistakes; it is not hardened against adversarial input, and the coverage +is uneven across entry points: + +- `SolveBoard()` validates thoroughly — parameter ranges, card counts, + duplicate cards, already-played cards — and returns a specific `RETURN_*` + code. See `board_range_checks()` and `board_value_checks()` in + `library/src/solver_if.cpp`. +- The par entry points validate the double dummy table + (`par_table_checks()`) and their `dealer` and `vulnerable` parameters. Both + checks were added after fuzzing found an out-of-range table overflowing a + fixed character buffer and a negative `dealer` indexing a string table. +- `CalcDDtable()`, `CalcDDtablePBN()`, `CalcAllTables*()` and the C++ + `calc_dd_table()` overloads validate the deal (`table_deal_checks()`) with + the same three rules `SolveBoard()` enforces: rank bits in range, no + duplicate cards, equal card counts per hand. +- `CalcAllTablesN()` and `CalcAllTablesPBNN()` additionally range-check + `no_of_tables` against the fixed capacity of their arrays. **`CalcAllTablesX()` + and `CalcAllTablesPBNX()` do not**: they exist precisely to accept an + arbitrary deal count, allocate for it, and guard only against integer + overflow. Their count is bounded by the caller, not by the library, and the + array must actually hold that many deals — so a caller exposing these two to + untrusted input must bound the count itself, both against a hostile value + and against memory exhaustion. +- `convert_from_pbn()` silently ignores characters it does not recognise + rather than rejecting the string, so a PBN deal with an invalid rank parses + one card short. The resulting deal is now rejected downstream, but the error + code says `RETURN_CARD_COUNT` rather than `RETURN_PBN_FAULT`. + +Callers that cannot guarantee well-formed input should validate at their own +boundary rather than rely on the library to do it. + +## Known unfixed issues + +There are currently **no known unfixed memory-safety defects**. + +Findings are tracked, with reproducers and analysis, in +[`library/tests/fuzz/findings/README.md`](library/tests/fuzz/findings/README.md), +which also records those already fixed and the reproducers kept as regression +seeds. Open findings are documented there openly, because DDS is a library +whose consumers need the information to judge their own exposure. + +## If you expose DDS to untrusted input + +The library was not designed for this. If you must: + +1. **Validate at your boundary.** Reject deals that are not 13 cards per hand + and tables whose entries fall outside 0-13, before calling DDS. If you use + `CalcAllTablesX()` or `CalcAllTablesPBNX()`, bound the deal count yourself: + those two accept an arbitrary count by design and the library will not cap + it for you. +2. **Check return codes.** Every entry point that validates returns a specific + `RETURN_*` value rather than throwing; a caller that ignores it will treat + an unset result structure as a real answer. +3. **Sandbox it.** Run the solver in a separate process with memory and CPU + limits. DDS allocates a large transposition table and its search is + recursive, so resource exhaustion is a denial-of-service consideration + independent of any memory-safety bug. Worker counts are chosen per call by + the `maxThreads` argument of the `*N` and `*X` entry points, where 0 means + hardware concurrency; pass an explicit cap rather than relying on + `SetMaxThreads()`, which is deprecated and ignores its argument. +4. **Consider the WebAssembly build**, whose sandbox contains memory errors by + construction. +5. Note that `DumpInput()` writes a `dump.txt` file into the process working + directory whenever input is rejected. Define `DDS_NO_DUMP_ON_ERROR` to + compile it out. + +## Testing and tooling + +The project runs AddressSanitizer, ThreadSanitizer and UndefinedBehaviorSanitizer +in CI on Linux and macOS, and carries libFuzzer harnesses for five +input-handling surfaces — four single-deal entry points plus the batch table +API: + +``` +bazel test --config=asan //library/... +bazel test //library/tests/fuzz/... +bazel run --config=fuzz //library/tests/fuzz:pbn_fuzz -- \ + library/tests/fuzz/corpus/pbn -runs=1000000 +``` + +See [`library/tests/fuzz/README.md`](library/tests/fuzz/README.md) for how to +run a campaign and triage a finding. New reproducers are welcome as pull +requests against `library/tests/fuzz/corpus/` once the underlying defect is +fixed. + +## Reporting a vulnerability + +Please report suspected security issues through +**[GitHub's private vulnerability reporting](https://github.com/dds-bridge/dds/security/advisories/new)** +on this repository, which keeps the report private until a fix is available. + +If you would rather not use GitHub, open a regular issue asking for a private +contact, without including details of the problem. + +Please include the DDS version or commit, the platform and compiler, a +reproducer (a corpus file for one of the fuzz harnesses is ideal), and any +sanitizer output. + +**What to expect.** DDS is maintained by a small group of volunteers, so please +allow time for a response. Issues in the categories described under "Input trust +model" above are likely to be treated as ordinary bugs and fixed in the open, +since the trust model is documented rather than implied. Issues that break the +documented model — anything reachable with well-formed, legal input, or any +out-of-bounds *write* — will be handled privately until fixed. diff --git a/doc/dll-description.html b/doc/dll-description.html index 7fb9ac01c..2348dcf45 100644 --- a/doc/dll-description.html +++ b/doc/dll-description.html @@ -22,7 +22,7 @@

The Multi-Thread Double

The double dummy trick values for all 5 * 4 = 20 possible combinations of a hand's trump strain and declarer hand alternatives are solved by a single call to one of the functions CalcDDtable and CalcDDtablePBN. Threads are allocated per strain. in order to save computations.

To obtain better utilization of available threads, the double dummy (DD) tables can be grouped using one of the functions CalcAllTables and CalcAllTablesPBN.

Solving hands can be done much more quickly using one of the multi-thread alternatives for calling SolveBoard. Then a number of hands are grouped for a single call to one of the functions SolveAllBoards, SolveAllChunksBin and SolveAllChunksPBN. The hands are then solved in parallel using the available threads.

-

The number of threads is automatically configured by DDS on Windows, taking into account the number of processor cores and available memory. The number of threads can be influenced using by calling SetMaxThreads. This function should probably always be called on Linux/Mac, with a zero argument for auto-configuration.

+

The number of threads is automatically configured by DDS, taking into account the number of processor cores and available memory. SetMaxThreads no longer influences this: its argument is ignored and it is a deprecated alias of InitializeStaticMemory(). Worker counts are chosen per call by the maxThreads argument of the *N and *X entry points, where 0 selects auto-configuration.

Calling FreeMemory causes DDS to give up its dynamically allocated memory.

The PAR Calculation Functions

The PAR calculation functions find the optimal contract(s) assuming open cards and optimal bidding from both sides. In very rare cases it matters which side or hand that starts the bidding, i.e. which side or hand that is first to bid its optimal contract.

@@ -315,7 +315,7 @@

Double Dummy Value Analyser Funct   -SetMaxThreadsint userThreadsPBNUsed at initial start and can also be called with a request for allocating memory for a specified number of threads. Is apparently¸mandatory on Linux and Mac (optional on Windows) +SetMaxThreadsint userThreadsPBNDeprecated alias of InitializeStaticMemory(); userThreads is ignored and it does not limit the thread count.   @@ -1120,14 +1120,14 @@

Functions

-

SetMaxThreads returns the actual number of threads.

+

SetMaxThreads returns nothing and ignores its argument; it is a deprecated alias of InitializeStaticMemory().

DDS has a preferred memory size per thread, currently about 95 MB, and a maximum memory size per thread, currently about 160 MB. It will also not use more than 70% of the available memory. It will not create more threads than there are processor cores, as this will only require more memory and will not improve performance. Within these constraints, DDS auto-configures the number of threads.

DDS first detects the number of cores and the available memory. If this doesn't work for some reason, it defaults to 1 thread which is allowed to use the maximum memory size per thread.

DDS then checks whether a number of threads equal to the number of cores will fit within the available memory when each thread may use the maximum memory per thread. If there is not enough memory for this, DDS scales back its ambition. If there is enough memory for the preferred memory size, then DDS still creates a number of threads equal to the number of cores. If there is not even enough memory for this, DDS scales back the number of threads to fit within the memory.

-

The user can suggest to DDS a number of threads by calling SetMaxThreads. DDS will never create more threads than requested, but it may create fewer if there is not enough memory, calculated as above. Calling SetMaxThreads is optional, not mandatory. DDS will always select a suitable number of threads on its own.

-

It may be possible, especially on non-Windows systems, to call SetMaxThreads() actively, even though the user does not want to influence the default values. In this case, use a 0 argument.

-

SetMaxThreads can be called multiple times even within the same session. So it is theoretically possible to change the number of threads dynamically.

+

SetMaxThreads no longer influences the thread count: its argument is ignored and internal, global batch threading was removed. To cap workers, pass an explicit maxThreads to the *N or *X entry points, or manage concurrency in the calling application (typically one SolverContext per thread). DDS will otherwise select a suitable number of threads on its own.

+

Calling SetMaxThreads() is harmless but has no effect beyond initialisation; InitializeStaticMemory() is the non-deprecated spelling.

+

SetMaxThreads can be called multiple times, but it cannot change the number of threads: use the per-call maxThreads argument instead.

It is possible to ask DDS to give up its dynamically allocated memory by calling FreeMemory. This could be useful for instance if there is a long pause where DDS is not used within a session. DDS will free its memory when the DLL detaches from the user program, so there is no need for the user to call this function before detaching.

Return codes

@@ -1217,6 +1217,9 @@

Return codes

-301RETURN_CHUNK_SIZESolveAllChunks\*(), returned when the chunk size is < 1. + +-401RETURN_PAR_TABLE_FAULTPar(), SidesPar(), SidesParBin(), DealerPar(), DealerParBin(), returned when the double dummy table pointer is null, or a table entry is outside the range 0 to 13. + diff --git a/doc/dll-description.md b/doc/dll-description.md index 8b5726f0b..50f8e70dd 100644 --- a/doc/dll-description.md +++ b/doc/dll-description.md @@ -24,7 +24,7 @@ To obtain better utilization of available threads, the double dummy (DD) tables Solving hands can be done much more quickly using one of the multi-thread alternatives for calling SolveBoard. Then a number of hands are grouped for a single call to one of the functions `SolveAllBoards`, `SolveAllBoardsBin`, `SolveAllChunksBin` and `SolveAllChunksPBN`. The hands are then solved in parallel using the available threads. -The number of threads is automatically configured by DDS on Windows, taking into account the number of processor cores and available memory. The number of threads can be influenced using by calling `SetMaxThreads`. This function should probably always be called on Linux/Mac, with a zero argument for auto-configuration. +The number of threads is automatically configured by DDS, taking into account the number of processor cores and available memory. **`SetMaxThreads` no longer influences this: its argument is ignored and it is a deprecated alias of `InitializeStaticMemory()`.** Worker counts are chosen per call by the `maxThreads` argument of the `*N` and `*X` entry points, where 0 selects auto-configuration. Calling `FreeMemory` causes DDS to give up its dynamically allocated memory. @@ -335,7 +335,7 @@ of the dealer.   -SetMaxThreadsint userThreadsPBNUsed at initial start and can also be called with a request for allocating memory for a specified number of threads. Is apparently¸mandatory on Linux and Mac (optional on Windows) +SetMaxThreadsint userThreadsPBNDeprecated alias of InitializeStaticMemory(); userThreads is ignored and it does not limit the thread count.   @@ -1201,7 +1201,7 @@ Concerning chunkSize, exactly the 21 same remarks apply as with [SolveAllChunksB -SetMaxThreads returns the actual number of threads. +SetMaxThreads returns nothing and ignores its argument; it is a deprecated alias of InitializeStaticMemory(). DDS has a preferred memory size per thread, currently about 95 MB, and a maximum memory size per thread, currently about 160 MB. It will also not use more than 70% of the available memory. It will not create more threads than there are processor cores, as this will only require more memory and will not improve performance. Within these constraints, DDS auto-configures the number of threads. @@ -1210,11 +1210,11 @@ DDS first detects the number of cores and the available memory. If this doesn't DDS then checks whether a number of threads equal to the number of cores will fit within the available memory when each thread may use the maximum memory per thread. If there is not enough memory for this, DDS scales back its ambition. If there is enough memory for the preferred memory size, then DDS still creates a number of threads equal to the number of cores. If there is not even enough memory for this, DDS scales back the number of threads to fit within the memory. -The user can suggest to DDS a number of threads by calling SetMaxThreads. DDS will never create more threads than requested, but it may create fewer if there is not enough memory, calculated as above. Calling SetMaxThreads is optional, not mandatory. DDS will always select a suitable number of threads on its own. +SetMaxThreads no longer influences the thread count: its argument is ignored and internal batch threading was removed. To cap workers, pass an explicit maxThreads to the *N or *X entry points, or manage concurrency in the calling application (typically one SolverContext per thread). DDS will otherwise select a suitable number of threads on its own. -It may be possible, especially on non-Windows systems, to call SetMaxThreads() actively, even though the user does not want to influence the default values. In this case, use a 0 argument. +Calling SetMaxThreads() is harmless but has no effect beyond initialisation; InitializeStaticMemory() is the non-deprecated spelling. -SetMaxThreads can be called multiple times even within the same session. So it is theoretically possible to change the number of threads dynamically. +SetMaxThreads can be called multiple times, but it cannot change the number of threads: use the per-call maxThreads argument instead. It is possible to ask DDS to give up its dynamically allocated memory by calling FreeMemory. This could be useful for instance if there is a long pause where DDS is not used within a session. DDS will free its memory when the DLL detaches from the user program, so there is no need for the user to call this function before detaching. @@ -1306,6 +1306,9 @@ Invalid suit or rank supplied. (c) A played card is not held by the right player -301RETURN_CHUNK_SIZESolveAllChunks\*(), returned when the chunk size is < 1. + +-401RETURN_PAR_TABLE_FAULTPar(), SidesPar(), SidesParBin(), DealerPar(), DealerParBin(), returned when the double dummy table pointer is null, or a table entry is outside the range 0 to 13. + diff --git a/docs/dotnet_interface.md b/docs/dotnet_interface.md index e1f41fe65..c2b768466 100644 --- a/docs/dotnet_interface.md +++ b/docs/dotnet_interface.md @@ -211,7 +211,11 @@ var ctx = new SolverContext(new SolverConfig { ... }); ### Configuration & Resources - **SetMaxThreads(int userThreads)** - Sets the maximum number of threads used by the legacy solver backend. + Deprecated, and a no-op beyond initialisation: `userThreads` is ignored and + the internal batch threading it once configured has been removed. It is an + alias of `InitializeStaticMemory()`. Worker counts are chosen per call by the + `maxThreads` argument of the `*N` and `*X` entry points, or by the embedding + application in the modern API (typically one `SolverContext` per thread). - **SetThreading(int code)** Selects the threading backend. Returns `1` on success. @@ -282,8 +286,9 @@ and should be used with caution, as they may not manage resources as efficiently ### Configuration & Resources - **SetMaxThreads(int userThreads)** - Deprecated. This sets the maximum number of threads used by the legacy solver backend. - The modern API manages threading implicitly via `SolverContext` and does not require manual configuration. + Deprecated, and a no-op beyond initialisation: `userThreads` is ignored, not + honoured as a thread limit. The modern API manages threading via + `SolverContext`, and the `*N`/`*X` entry points take a per-call `maxThreads`. - **SetThreading(int code)** Deprecated. configures the threading backend for the legacy API. diff --git a/jni/java/org/dds/ffm/DdsStatus.java b/jni/java/org/dds/ffm/DdsStatus.java index 84deda16e..24880a618 100644 --- a/jni/java/org/dds/ffm/DdsStatus.java +++ b/jni/java/org/dds/ffm/DdsStatus.java @@ -76,6 +76,8 @@ private DdsStatus() { public static final int RETURN_TOO_MANY_TABLES = -202; /** Chunk size is less than 1. */ public static final int RETURN_CHUNK_SIZE = -301; + /** Double dummy table entry outside the range 0 to 13. */ + public static final int RETURN_PAR_TABLE_FAULT = -401; /** * Symbolic name of a status code (e.g. {@code "RETURN_TRUMP_WRONG"}), or @@ -111,6 +113,7 @@ public static String name(int code) { case RETURN_NO_SUIT: return "RETURN_NO_SUIT"; case RETURN_TOO_MANY_TABLES: return "RETURN_TOO_MANY_TABLES"; case RETURN_CHUNK_SIZE: return "RETURN_CHUNK_SIZE"; + case RETURN_PAR_TABLE_FAULT: return "RETURN_PAR_TABLE_FAULT"; default: return "RETURN(" + code + ")"; } } diff --git a/library/src/BUILD.bazel b/library/src/BUILD.bazel index 40b4c1271..1099f0ce8 100644 --- a/library/src/BUILD.bazel +++ b/library/src/BUILD.bazel @@ -97,6 +97,7 @@ cc_library( "//:__pkg__", # allow root package to wrap/export "//library/tests:__pkg__", "//library/tests/ab_search:__pkg__", + "//library/tests/fuzz:__pkg__", "//library/tests/heuristic_sorting:__pkg__", "//library/tests/moves:__pkg__", "//library/tests/quick_tricks:__pkg__", diff --git a/library/src/api/dll.h b/library/src/api/dll.h index dc83dc836..9cd6ce1ba 100644 --- a/library/src/api/dll.h +++ b/library/src/api/dll.h @@ -152,6 +152,10 @@ #define RETURN_CHUNK_SIZE -301 #define TEXT_CHUNK_SIZE "Chunk size is less than 1" +// Par(), SidesPar(), SidesParBin(), DealerPar(), DealerParBin() +#define RETURN_PAR_TABLE_FAULT -401 +#define TEXT_PAR_TABLE_FAULT "Missing double dummy table, or an entry outside the range 0 to 13" + /** diff --git a/library/src/calc_dd_table.cpp b/library/src/calc_dd_table.cpp index 5318958f2..42c2066cd 100644 --- a/library/src/calc_dd_table.cpp +++ b/library/src/calc_dd_table.cpp @@ -8,6 +8,7 @@ */ #include +#include #include #include #include @@ -26,6 +27,12 @@ auto calc_dd_table( const DdTableDeal& table_deal, DdTableResults* table_results) -> int { + // This overload builds Boards directly rather than going through + // CalcDDtableN(), so it needs the same deal validation. Both the + // context-free overload and calc_dd_table_pbn() delegate here. + if (int const check = table_deal_checks(table_deal); check != RETURN_NO_FAULT) + return check; + Deal dl; Boards bo; SolvedBoards solved; diff --git a/library/src/calc_tables.cpp b/library/src/calc_tables.cpp index c9ed8a583..7e69f831f 100644 --- a/library/src/calc_tables.cpp +++ b/library/src/calc_tables.cpp @@ -9,12 +9,14 @@ #include "calc_tables.hpp" #include +#include #include #include #include #include #include +#include #include #include #include @@ -245,6 +247,9 @@ int STDCALL CalcDDtableN( DdTableResults * tablep, int maxThreads) { + if (int const check = table_deal_checks(tableDeal); check != RETURN_NO_FAULT) + return check; + Deal dl; Boards bo; SolvedBoards solved; @@ -317,6 +322,17 @@ int STDCALL CalcAllTablesN( mode = 3: par calculation, vulnerability EW mode = -1: no par calculation */ + // dealsp->deals is a fixed MAXNOOFTABLES * DDS_STRAINS array, and the + // capacity check below multiplies by count. Bound no_of_tables first, so + // that multiply cannot overflow signed int and wrap past the check, and so + // the per-deal loops cannot read past the array. + if (dealsp == nullptr) + return RETURN_UNKNOWN_FAULT; + + if (dealsp->no_of_tables < 0 || + dealsp->no_of_tables > MAXNOOFTABLES * DDS_STRAINS) + return RETURN_TOO_MANY_TABLES; + Boards bo; SolvedBoards solved; int count = 0; @@ -337,10 +353,22 @@ int STDCALL CalcAllTablesN( if (count * dealsp->no_of_tables > MAXNOOFTABLES * DDS_STRAINS) return RETURN_TOO_MANY_TABLES; + for (int m = 0; m < dealsp->no_of_tables; m++) + { + int const check = table_deal_checks(dealsp->deals[m]); + if (check != RETURN_NO_FAULT) + return check; + } + int ind = 0; - int lastIndex = 0; resp->no_of_boards = 0; + // With no deals the loop below writes no boards, and bo is an uninitialised + // local -- solving a board from it reads indeterminate values. Return early, + // matching CalcAllTablesX(). + if (dealsp->no_of_tables == 0) + return RETURN_NO_FAULT; + for (int m = 0; m < dealsp->no_of_tables; m++) { for (int tr = DDS_STRAINS-1; tr >= 0; tr--) @@ -364,12 +392,14 @@ int STDCALL CalcAllTablesN( bo.target[ind] = -1; bo.solutions[ind] = 1; bo.mode[ind] = 1; - lastIndex = ind; ind++; } } - bo.no_of_boards = lastIndex + 1; + // ind counts the boards actually written; deriving the count from a + // last-index variable initialised to 0 claimed one board even when none + // had been filled in. + bo.no_of_boards = ind; int res = calc_all_boards_n(&bo, &solved, maxThreads); if (res != 1) @@ -430,6 +460,16 @@ int STDCALL CalcAllTablesPBNN( AllParResults * presp, int maxThreads) { + // dls.deals and dealsp->deals both hold MAXNOOFTABLES * DDS_STRAINS + // entries. Bound the count before the conversion loop: unchecked, this + // wrote past the fixed-size local. + if (dealsp == nullptr) + return RETURN_UNKNOWN_FAULT; + + if (dealsp->no_of_tables < 0 || + dealsp->no_of_tables > MAXNOOFTABLES * DDS_STRAINS) + return RETURN_TOO_MANY_TABLES; + DdTableDeals dls; for (int k = 0; k < dealsp->no_of_tables; k++) if (convert_from_pbn(dealsp->deals[k].cards, dls.deals[k].cards) != 1) @@ -498,6 +538,37 @@ auto calc_single_deal_scores( } // namespace +namespace +{ + +/* The *X entry points accept an arbitrary deal count by design, so the only + cheap guard available is the board-count product. Run it before any + O(numDeals) work -- allocating, converting or validating a count that is + already guaranteed to be rejected is wasted effort and, for the PBN + variant, a memory-exhaustion path. Also reports how many strains survive + the filter, which the caller needs anyway. */ +auto batch_count_preflight( + const int numDeals, + int const trumpFilter[DDS_STRAINS], + int& included) -> int +{ + included = 0; + for (int k = 0; k < DDS_STRAINS; k++) + if (!trumpFilter[k]) + included++; + + if (included == 0) + return RETURN_NO_SUIT; + + if (numDeals > std::numeric_limits::max() / included) + return RETURN_TOO_MANY_TABLES; + + return RETURN_NO_FAULT; +} + +} // namespace + + int STDCALL CalcAllTablesX( int numDeals, DdTableDeal const * deals, @@ -520,21 +591,27 @@ int STDCALL CalcAllTablesX( return RETURN_UNKNOWN_FAULT; int included = 0; - for (int k = 0; k < DDS_STRAINS; k++) - { - if (!trumpFilter[k]) - included++; - } - if (included == 0) - return RETURN_NO_SUIT; + if (int const check = batch_count_preflight(numDeals, trumpFilter, included); + check != RETURN_NO_FAULT) + return check; const bool want_par = (mode > -1) && (mode < 4) && (included == DDS_STRAINS); if (want_par && par == nullptr) return RETURN_UNKNOWN_FAULT; + // This path builds its board list directly rather than going through + // CalcDDtableN, so it needs the same deal validation. + for (int m = 0; m < numDeals; m++) + { + int const check = table_deal_checks(deals[m]); + if (check != RETURN_NO_FAULT) + return check; + } + // Expand every deal×included-strain into one board list and solve in a // single parallel_all_boards_n job (heap-backed). This is the ddss-style // large-batch shape; legacy CalcAllTablesN remains capped at MAXNOOFTABLES. + // Overflow already ruled out by batch_count_preflight() above. const int nboards = numDeals * included; std::vector boards(static_cast(nboards)); std::vector> scores(static_cast(nboards)); @@ -650,6 +727,13 @@ int STDCALL CalcAllTablesPBNX( if (deals == nullptr || results == nullptr || trumpFilter == nullptr) return RETURN_UNKNOWN_FAULT; + // Share CalcAllTablesX's preflight so a count that cannot succeed is + // rejected before this allocates and converts numDeals records. + int included = 0; + if (int const check = batch_count_preflight(numDeals, trumpFilter, included); + check != RETURN_NO_FAULT) + return check; + std::vector binary(static_cast(numDeals)); for (int i = 0; i < numDeals; ++i) { diff --git a/library/src/dealer_par.cpp b/library/src/dealer_par.cpp index 5bf204765..d110ea8a6 100644 --- a/library/src/dealer_par.cpp +++ b/library/src/dealer_par.cpp @@ -12,6 +12,7 @@ #include #include +#include using namespace std; @@ -186,6 +187,18 @@ int STDCALL DealerPar( /* dealer 0: North 1: East 2: South 3: West */ /* vulnerable 0: None 1: Both 2: NS 3: EW */ + if (int const check = par_table_checks(tablep); check != RETURN_NO_FAULT) + return check; + + /* Both parameters reach array subscripts: vulnerable indexes VUL_LOOKUP + below, and dealer propagates into the par tables via pno_list[]. */ + if (int const check = par_vulnerable_checks(vulnerable); + check != RETURN_NO_FAULT) + return check; + + if (dealer < 0 || dealer > 3) + return RETURN_UNKNOWN_FAULT; + int const * vul_by_side = VUL_LOOKUP[vulnerable]; data_type data; list_type list[2][DDS_STRAINS]; @@ -615,6 +628,27 @@ void reduce_contract( } +/* These tables are indexed by values derived from caller-supplied parameters. + Guard the subscript instead of casting to unsigned: the cast turns a + negative index into a multi-gigabyte offset, converting a detectable bug + into a wild read. With DealerPar()'s range checks in place these should be + unreachable, so a "?" in the output means a new defect upstream. */ + +string contract_text(const int no) +{ + if (no < 0 || static_cast(no) >= NUMBER_TO_CONTRACT.size()) + return "?"; + return NUMBER_TO_CONTRACT[static_cast(no)]; +} + +string player_text(const int pno) +{ + if (pno < 0 || static_cast(pno) >= NUMBER_TO_PLAYER.size()) + return "?"; + return NUMBER_TO_PLAYER[static_cast(pno)]; +} + + string contract_as_text( const DdTableResults& table, const int side, @@ -627,10 +661,10 @@ string contract_as_text( const int tb = t[side + 2]; const int t_max = (ta > tb ? ta : tb); - return NUMBER_TO_CONTRACT[static_cast(no)] + + return contract_text(no) + (delta < 0 ? "*-" : "-") + - (ta == t_max ? NUMBER_TO_PLAYER[static_cast(side)] : "") + - (tb == t_max ? NUMBER_TO_PLAYER[static_cast(side + 2)] : "") + + (ta == t_max ? player_text(side) : "") + + (tb == t_max ? player_text(side + 2) : "") + (delta > 0 ? "+" : "") + (delta == 0 ? "" : to_string(delta)); } @@ -641,7 +675,7 @@ string sacrifice_as_text( const int pno, const int down) { - return NUMBER_TO_CONTRACT[static_cast(no)] + "-" + - NUMBER_TO_PLAYER[static_cast(pno)] + "-" + + return contract_text(no) + "-" + + player_text(pno) + "-" + to_string(down); } diff --git a/library/src/dump.cpp b/library/src/dump.cpp index 2ef42df6c..b9cff22c8 100644 --- a/library/src/dump.cpp +++ b/library/src/dump.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include "dump.hpp" #include @@ -270,6 +271,62 @@ std::string TopMove( } +namespace { + +/* DumpInput() renders values that board_range_checks() is in the process of + rejecting, so they must never be used as unchecked table subscripts. Each + helper falls back to the raw integer when the value is out of range, which + is also more useful in a diagnostic than a wrong character would be. */ + +/* card_suit[] covers the five strains, but the legal range depends on which + field is being printed: trump may be 0..4, where 4 is no-trump, while + board_range_checks() accepts only 0..3 for a trick suit. Sharing one bound + would render an invalid trick suit of 4 as "N" and hide the rejected value, + so the caller supplies the bound. */ + +auto suit_text(const int suit, const int strain_count) -> std::string +{ + if (suit < 0 || suit >= strain_count) + return "?(" + std::to_string(suit) + ")"; + return std::string(1, static_cast(card_suit[suit])); +} + +/// Trump: 0..3 plus DDS_NOTRUMP. +auto trump_text(const int trump) -> std::string +{ + return suit_text(trump, DDS_STRAINS); +} + +/// Suit led in the current trick: no-trump is not a legal value. +auto trick_suit_text(const int suit) -> std::string +{ + return suit_text(suit, DDS_SUITS); +} + +auto hand_text(const int hand) -> std::string +{ + if (hand < 0 || hand >= DDS_HANDS) + return "?(" + std::to_string(hand) + ")"; + return std::string(1, static_cast(card_hand[hand])); +} + +/* card_rank[] has 16 entries, but indices 0, 1 and 15 hold the sentinels 'x' + and '-'. board_range_checks() accepts only 2..14 for a trick rank, so + bounding by the array size would render a rejected rank of 1 or 15 as a + sentinel character and hide the invalid value. Bound by the legal range. */ + +auto rank_text(const int rank) -> std::string +{ + constexpr int min_rank = 2; // deuce + constexpr int max_rank = 14; // ace + if (rank < min_rank || rank > max_rank) + return "?(" + std::to_string(rank) + ")"; + return std::string(1, static_cast(card_rank[rank])); +} + +} // namespace + + int DumpInput( const int errCode, const Deal& dl, @@ -288,8 +345,8 @@ int DumpInput( if (dl.trump == DDS_NOTRUMP) fout << "N\n"; else - fout << card_suit[dl.trump] << "\n"; - fout << "first=" << card_hand[dl.first] << "\n"; + fout << trump_text(dl.trump) << "\n"; + fout << "first=" << hand_text(dl.first) << "\n"; unsigned short ranks[4][4]; @@ -297,8 +354,8 @@ int DumpInput( if (dl.currentTrickRank[k] != 0) { fout << "index=" << k << - " currentTrickSuit=" << card_suit[dl.currentTrickSuit[k]] << - " currentTrickRank= " << card_rank[dl.currentTrickRank[k]] << "\n"; + " currentTrickSuit=" << trick_suit_text(dl.currentTrickSuit[k]) << + " currentTrickRank= " << rank_text(dl.currentTrickRank[k]) << "\n"; } for (int h = 0; h < DDS_HANDS; h++) diff --git a/library/src/init.cpp b/library/src/init.cpp index 57492bd52..17feb0f29 100644 --- a/library/src/init.cpp +++ b/library/src/init.cpp @@ -492,6 +492,9 @@ void STDCALL ErrorMessage(int code, char line[80]) case RETURN_CHUNK_SIZE: strcpy(line, TEXT_CHUNK_SIZE); break; + case RETURN_PAR_TABLE_FAULT: + strcpy(line, TEXT_PAR_TABLE_FAULT); + break; default: strcpy(line, "Not a DDS error code"); break; diff --git a/library/src/par.cpp b/library/src/par.cpp index 421ecbd9a..a40f4d3ec 100644 --- a/library/src/par.cpp +++ b/library/src/par.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -227,6 +228,12 @@ int STDCALL SidesParBin( /* The Par function computes the par result and contracts. */ + if (int const check = par_table_checks(tablep); check != RETURN_NO_FAULT) + return check; + + if (int const check = par_vulnerable_checks(vulnerable); + check != RETURN_NO_FAULT) + return check; int denom_conv[5] = { 4, 0, 1, 2, 3 }; /* Preallocate for efficiency. These hold result from last direction @@ -698,6 +705,13 @@ int STDCALL SidesParBin( int vulnerable) { + if (int const check = par_table_checks(tablep); check != RETURN_NO_FAULT) + return check; + + if (int const check = par_vulnerable_checks(vulnerable); + check != RETURN_NO_FAULT) + return check; + int res, h, hbest[2], i, k, m, index; parResultsMaster parRes2[4]; int cross_index[4][5] = { diff --git a/library/src/par_validate.hpp b/library/src/par_validate.hpp new file mode 100644 index 000000000..d324b57e3 --- /dev/null +++ b/library/src/par_validate.hpp @@ -0,0 +1,63 @@ +/* + DDS, a bridge double dummy solver. + + Copyright (C) 2006-2014 by Bo Haglund / + 2014-2018 by Bo Haglund & Soren Hein. + + See LICENSE and README. +*/ + +#pragma once + +#include + + +/** + * @brief Validate a caller-supplied double dummy table before par calculation. + * + * Every entry of DdTableResults::res_table is a trick count and must lie in + * [0, 13]. The par calculation derives contract levels directly from these + * values and formats them into fixed-size character buffers, so an entry far + * outside the legal range overflows those buffers. CalcDDtable() always + * produces legal tables, but the par entry points are exported and a caller + * may hand-build or deserialise a table, so the range is checked here rather + * than assumed. + * + * @param tablep Table to validate. May be nullptr. + * @return RETURN_NO_FAULT when every entry is in range, + * RETURN_PAR_TABLE_FAULT otherwise (including a nullptr table). + */ +inline auto par_table_checks(DdTableResults const * tablep) -> int +{ + if (tablep == nullptr) + return RETURN_PAR_TABLE_FAULT; + + for (int d = 0; d < DDS_STRAINS; d++) + for (int h = 0; h < DDS_HANDS; h++) + if (tablep->res_table[d][h] < 0 || tablep->res_table[d][h] > 13) + return RETURN_PAR_TABLE_FAULT; + + return RETURN_NO_FAULT; +} + + +/** + * @brief Validate the vulnerability argument shared by the par entry points. + * + * DealerPar() indexes VUL_LOOKUP with this value, so it must be range-checked + * there. SidesParBin() only compares against it, which is memory-safe but + * silently treats any out-of-range value as "none vulnerable" -- so Par() and + * SidesPar() would return RETURN_NO_FAULT with a result computed under the + * wrong vulnerability while DealerPar() rejected the same input. Both use this + * helper so the entry points agree. + * + * @param vulnerable 0 = None, 1 = Both, 2 = NS, 3 = EW. + * @return RETURN_NO_FAULT when in range, RETURN_UNKNOWN_FAULT otherwise. + */ +inline auto par_vulnerable_checks(int const vulnerable) -> int +{ + if (vulnerable < 0 || vulnerable > 3) + return RETURN_UNKNOWN_FAULT; + + return RETURN_NO_FAULT; +} diff --git a/library/src/solver_if.cpp b/library/src/solver_if.cpp index 125706e29..544b0375d 100644 --- a/library/src/solver_if.cpp +++ b/library/src/solver_if.cpp @@ -1154,6 +1154,17 @@ auto board_value_checks( for (int k = 0; k < hand_rel_first; k++) { + /* board_range_checks() only validates currentTrickSuit[k] when the + matching rank is non-zero, but hand_rel_first is derived from the card + count rather than from the trick entries, so this loop can reach an + entry whose suit was never checked and index remainCards out of + bounds. Validate it here, where it is actually used as a subscript. */ + if (dl.currentTrickSuit[k] < 0 || dl.currentTrickSuit[k] >= DDS_SUITS) + { + DumpInput(RETURN_SUIT_OR_RANK, dl, target, solutions, mode); + return RETURN_SUIT_OR_RANK; + } + unsigned short int aggrRemain = 0; for (int h = 0; h < DDS_HANDS; h++) aggrRemain |= (dl.remainCards[h][dl.currentTrickSuit[k]] >> 2); diff --git a/library/src/table_deal_validate.hpp b/library/src/table_deal_validate.hpp new file mode 100644 index 000000000..3c8e28c49 --- /dev/null +++ b/library/src/table_deal_validate.hpp @@ -0,0 +1,82 @@ +/* + DDS, a bridge double dummy solver. + + Copyright (C) 2006-2014 by Bo Haglund / + 2014-2018 by Bo Haglund & Soren Hein. + + See LICENSE and README. +*/ + +#pragma once + +#include + + +/** + * @brief Validate a caller-supplied deal before a double dummy table is built. + * + * SolveBoard() rejects malformed deals in board_range_checks() and + * board_value_checks() before the search runs. The CalcDDtable* entry points + * did not, so a deal whose hands held unequal numbers of cards reached the + * search and indexed the relative-rank tables out of bounds. This applies the + * same three rules SolveBoard() already enforces, so nothing is rejected here + * that the solver would have accepted: + * + * - every holding is either empty or confined to the rank bits (2..A), + * - no card appears in more than one hand, + * - all four hands hold the same number of cards. + * + * Applied at every entry point that turns a DdTableDeal into boards: the C + * API's CalcDDtableN(), CalcAllTablesN() and CalcAllTablesX(), and the C++ + * calc_dd_table(ctx, ...) overload that the context-free and PBN overloads + * (and the dds_c_* shims) delegate to. Reachable in particular through any + * PBN file that is short of a card. + * + * This checks one deal. It does not bound how *many* deals a batch entry + * point was given: CalcAllTablesN() and CalcAllTablesPBNN() must range-check + * no_of_tables against the fixed capacity of their arrays before they index + * or convert, which they do separately. + * + * @param table_deal Deal to validate. + * @return RETURN_NO_FAULT when the deal is well formed, otherwise + * RETURN_SUIT_OR_RANK, RETURN_DUPLICATE_CARDS or RETURN_CARD_COUNT. + */ +inline auto table_deal_checks(DdTableDeal const & table_deal) -> int +{ + // Ranks 2..A occupy bits 2..14; see Deal::remainCards in dll.h. + constexpr unsigned rank_mask = 0x7FFCu; + + int cards_in_hand[DDS_HANDS] = {0, 0, 0, 0}; + + for (int h = 0; h < DDS_HANDS; h++) + { + for (int s = 0; s < DDS_SUITS; s++) + { + unsigned const holding = table_deal.cards[h][s]; + + if ((holding & ~rank_mask) != 0) + return RETURN_SUIT_OR_RANK; + + for (unsigned bit = holding; bit != 0; bit &= bit - 1) + cards_in_hand[h]++; + } + } + + for (int s = 0; s < DDS_SUITS; s++) + { + unsigned seen = 0; + for (int h = 0; h < DDS_HANDS; h++) + { + unsigned const holding = table_deal.cards[h][s]; + if ((seen & holding) != 0) + return RETURN_DUPLICATE_CARDS; + seen |= holding; + } + } + + for (int h = 1; h < DDS_HANDS; h++) + if (cards_in_hand[h] != cards_in_hand[0]) + return RETURN_CARD_COUNT; + + return RETURN_NO_FAULT; +} diff --git a/library/tests/BUILD.bazel b/library/tests/BUILD.bazel index ff30e5092..0abcf766b 100644 --- a/library/tests/BUILD.bazel +++ b/library/tests/BUILD.bazel @@ -18,6 +18,8 @@ filegroup( "loop_par_test.cpp", # Uses GoogleTest, compiled separately "dds_c_api_test.cpp", # Uses GoogleTest, compiled separately "pbn_test.cpp", # Uses GoogleTest, compiled separately + "par_validation_test.cpp", # Uses GoogleTest, compiled separately + "deal_input_validation_test.cpp", # Uses GoogleTest, compiled separately ], ), ) @@ -79,6 +81,37 @@ cc_test( ], ) +# Regression tests for deal validation on the CalcDDtable* paths and for the +# safety of DumpInput(); see deal_input_validation_test.cpp. +cc_test( + name = "deal_input_validation_test", + srcs = ["deal_input_validation_test.cpp"], + size = "small", + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, + deps = [ + "//library/src:dds", + "//library/src/api:dds_c_api", + "@googletest//:gtest_main", + ], +) + +# Regression tests for double dummy table validation in the par API; see +# par_validation_test.cpp for the overflow this guards against. +cc_test( + name = "par_validation_test", + srcs = ["par_validation_test.cpp"], + size = "small", + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, + deps = [ + "//library/src:dds", + "@googletest//:gtest_main", + ], +) + # Exercises the pure-C shim through its own ABI, including the null guards and # catch-all wrappers that exist only at that boundary and would be bypassed by # calling the reference-taking dds_* functions directly. diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp new file mode 100644 index 000000000..0cf0703ad --- /dev/null +++ b/library/tests/deal_input_validation_test.cpp @@ -0,0 +1,455 @@ +/// @file deal_input_validation_test.cpp +/// @brief Regression tests for deal validation on the CalcDDtable* paths and +/// for the safety of the error-reporting path itself. +/// +/// Both defects were found by the fuzz harnesses in library/tests/fuzz: +/// +/// - CalcDDtable()/CalcDDtablePBN() did not check that the four hands held +/// equal numbers of cards, so a 51-card deal reached the search and read +/// 14248 bytes past rel_rank_storage. SolveBoard() rejected the same deal. +/// +/// - DumpInput() indexed card_suit[], card_hand[] and card_rank[] with the +/// very values board_range_checks() was rejecting as out of range, so the +/// error path read out of bounds. It is compiled in unless +/// DDS_NO_DUMP_ON_ERROR is defined, which the build does not define. +/// +/// Most meaningful under --config=asan, where a regression aborts rather than +/// merely returning an unexpected code. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +constexpr char kLegalDeal[] = + "N:QJ6.K652.J85.T98 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3"; + +/// The same deal one card short: north's spades are T8, not T98. Contains +/// only legal PBN characters -- the shape a truncated PBN file takes. +constexpr char kShortOneCard[] = + "N:QJ6.K652.J85.T8 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3"; + +/// The same deal with a card replaced by an invalid rank. convert_from_pbn() +/// silently skips unrecognised characters, so this also arrives one card short. +constexpr char kBadRank[] = + "N:QJ6.K652.J85.TZ8 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3"; + +auto pbn_deal(const char * cards) -> DdTableDealPBN +{ + DdTableDealPBN deal; + std::memset(&deal, 0, sizeof(deal)); + std::strncpy(deal.cards, cards, sizeof(deal.cards) - 1); + return deal; +} + +/// A legal binary deal: each hand holds one complete suit. +auto one_suit_each() -> DdTableDeal +{ + constexpr unsigned kAllRanks = 0x7FFC; // ranks 2..A + DdTableDeal deal; + std::memset(&deal, 0, sizeof(deal)); + for (int h = 0; h < DDS_HANDS; h++) + deal.cards[h][h] = kAllRanks; + return deal; +} + +// --------------------------------------------------------------------------- +// Finding 01: unbalanced deals must not reach the search. +// --------------------------------------------------------------------------- + +TEST(CalcTableValidation, ShortPbnDealIsRejected) +{ + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(CalcDDtablePBN(pbn_deal(kShortOneCard), &table), + RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, InvalidRankCharacterIsRejected) +{ + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + // Rejected for the card count, since the invalid rank is skipped by the + // parser rather than refused outright. + EXPECT_EQ(CalcDDtablePBN(pbn_deal(kBadRank), &table), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, UnbalancedBinaryDealIsRejected) +{ + DdTableDeal deal = one_suit_each(); + deal.cards[0][0] &= ~0x4000u; // remove north's ace + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(CalcDDtable(deal, &table), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, DuplicateCardIsRejected) +{ + DdTableDeal deal = one_suit_each(); + // Give north a card east already holds, keeping the hand counts equal. + deal.cards[0][1] |= 0x4000u; + deal.cards[0][0] &= ~0x4000u; + deal.cards[1][1] |= 0x4000u; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(CalcDDtable(deal, &table), RETURN_DUPLICATE_CARDS); +} + +TEST(CalcTableValidation, BitsOutsideRankRangeAreRejected) +{ + DdTableDeal deal = one_suit_each(); + deal.cards[2][2] |= 0x8000u; // above the ace bit + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(CalcDDtable(deal, &table), RETURN_SUIT_OR_RANK); + + DdTableDeal low = one_suit_each(); + low.cards[1][1] |= 0x0001u; // below the deuce bit + EXPECT_EQ(CalcDDtable(low, &table), RETURN_SUIT_OR_RANK); +} + +TEST(CalcTableValidation, LegalDealStillProducesATable) +{ + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + ASSERT_EQ(CalcDDtablePBN(pbn_deal(kLegalDeal), &table), RETURN_NO_FAULT); + + for (int d = 0; d < DDS_STRAINS; d++) + for (int h = 0; h < DDS_HANDS; h++) + EXPECT_GE(table.res_table[d][h], 0) << "strain " << d << " hand " << h; + for (int d = 0; d < DDS_STRAINS; d++) + for (int h = 0; h < DDS_HANDS; h++) + EXPECT_LE(table.res_table[d][h], 13) << "strain " << d << " hand " << h; +} + +TEST(CalcTableValidation, LegalBinaryDealStillProducesATable) +{ + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(CalcDDtable(one_suit_each(), &table), RETURN_NO_FAULT); +} + +TEST(CalcTableValidation, CalcAllTablesRejectsUnbalancedDeal) +{ + DdTableDeals deals; + std::memset(&deals, 0, sizeof(deals)); + deals.no_of_tables = 1; + deals.deals[0] = one_suit_each(); + deals.deals[0].cards[3][3] &= ~0x4000u; // west one card short + + DdTablesRes res; + std::memset(&res, 0, sizeof(res)); + AllParResults par; + std::memset(&par, 0, sizeof(par)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + EXPECT_EQ(CalcAllTables(&deals, -1, filter, &res, &par), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, CppOverloadRejectsUnbalancedDeal) +{ + // The C++ calc_dd_table() overloads build Boards directly rather than going + // through CalcDDtableN(), so they need the same guard. + DdTableDeal deal = one_suit_each(); + deal.cards[0][0] &= ~0x4000u; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(calc_dd_table(deal, &table), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, CppContextOverloadRejectsUnbalancedDeal) +{ + DdTableDeal deal = one_suit_each(); + deal.cards[0][0] &= ~0x4000u; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + SolverContext ctx; + EXPECT_EQ(calc_dd_table(ctx, deal, &table), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, CppPbnOverloadRejectsShortDeal) +{ + DdTableDealPBN const deal = pbn_deal(kShortOneCard); + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + EXPECT_EQ(calc_dd_table_pbn(deal, &table), RETURN_CARD_COUNT); +} + +TEST(CalcTableValidation, CShimRejectsUnbalancedDeal) +{ + // dds_c_calc_dd_table delegates to the C++ overload guarded above. + DdTableDeal deal = one_suit_each(); + deal.cards[0][0] &= ~0x4000u; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + + DDS_C_SOLVER_CTX ctx = dds_c_create_solvercontext_default(); + ASSERT_NE(ctx, nullptr); + EXPECT_EQ(dds_c_calc_dd_table(ctx, &deal, &table), RETURN_CARD_COUNT); + dds_c_destroy_solvercontext(ctx); +} + +TEST(CalcTableValidation, CShimPbnRejectsShortDeal) +{ + DdTableDealPBN const deal = pbn_deal(kShortOneCard); + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + + DDS_C_SOLVER_CTX ctx = dds_c_create_solvercontext_default(); + ASSERT_NE(ctx, nullptr); + EXPECT_EQ(dds_c_calc_dd_table_pbn(ctx, &deal, &table), RETURN_CARD_COUNT); + dds_c_destroy_solvercontext(ctx); +} + +// --------------------------------------------------------------------------- +// Table-count boundary. DdTableDeals/DdTableDealsPBN carry a fixed +// MAXNOOFTABLES * DDS_STRAINS array, and no_of_tables was never checked +// against it -- CalcAllTablesPBNN() copied that many records into a +// fixed-size local before any validation ran. +// --------------------------------------------------------------------------- + +TEST(CalcTableValidation, CalcAllTablesRejectsOversizedTableCount) +{ + DdTableDeals deals; + std::memset(&deals, 0, sizeof(deals)); + deals.no_of_tables = MAXNOOFTABLES * DDS_STRAINS + 1; + for (int i = 0; i < MAXNOOFTABLES * DDS_STRAINS; i++) + deals.deals[i] = one_suit_each(); + + DdTablesRes res; + std::memset(&res, 0, sizeof(res)); + AllParResults par; + std::memset(&par, 0, sizeof(par)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + EXPECT_EQ(CalcAllTables(&deals, -1, filter, &res, &par), + RETURN_TOO_MANY_TABLES); +} + +TEST(CalcTableValidation, CalcAllTablesRejectsNegativeTableCount) +{ + DdTableDeals deals; + std::memset(&deals, 0, sizeof(deals)); + deals.no_of_tables = -1; + + DdTablesRes res; + std::memset(&res, 0, sizeof(res)); + AllParResults par; + std::memset(&par, 0, sizeof(par)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + EXPECT_EQ(CalcAllTables(&deals, -1, filter, &res, &par), + RETURN_TOO_MANY_TABLES); +} + +TEST(CalcTableValidation, CalcAllTablesWithZeroDealsSolvesNothing) +{ + // With no deals the board-building loop writes nothing, but the board count + // was derived from a last-index variable initialised to 0 and so claimed + // one board -- solving an uninitialised entry of a stack-local Boards. + // MemorySanitizer reports it; found by the calc_all_tables fuzz harness. + DdTableDeals deals; + std::memset(&deals, 0, sizeof(deals)); + deals.no_of_tables = 0; + + DdTablesRes res; + std::memset(&res, 0, sizeof(res)); + AllParResults par; + std::memset(&par, 0, sizeof(par)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + EXPECT_EQ(CalcAllTables(&deals, -1, filter, &res, &par), RETURN_NO_FAULT); + EXPECT_EQ(res.no_of_boards, 0); +} + +TEST(CalcTableValidation, CalcAllTablesPbnRejectsOversizedTableCount) +{ + // Before the guard this copied no_of_tables records into a fixed-size + // local DdTableDeals, overflowing it on the stack. + auto deals = std::make_unique(); + std::memset(deals.get(), 0, sizeof(DdTableDealsPBN)); + deals->no_of_tables = MAXNOOFTABLES * DDS_STRAINS + 64; + for (int i = 0; i < MAXNOOFTABLES * DDS_STRAINS; i++) + std::strncpy(deals->deals[i].cards, kLegalDeal, + sizeof(deals->deals[i].cards) - 1); + + auto res = std::make_unique(); + std::memset(res.get(), 0, sizeof(DdTablesRes)); + auto par = std::make_unique(); + std::memset(par.get(), 0, sizeof(AllParResults)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + EXPECT_EQ(CalcAllTablesPBN(deals.get(), -1, filter, res.get(), par.get()), + RETURN_TOO_MANY_TABLES); +} + +// --------------------------------------------------------------------------- +// Finding 03: the error path must not index tables with the values it rejects. +// --------------------------------------------------------------------------- + +TEST(DumpInputSafety, OutOfRangeTrickSuitAndRankAreReportedNotIndexed) +{ + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 0; + deal.first = 0; + for (int k = 0; k < 3; k++) + { + deal.currentTrickSuit[k] = 7; // card_suit has 5 entries + deal.currentTrickRank[k] = 99; // card_rank has 16 entries + } + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_SUIT_OR_RANK); +} + +TEST(DumpInputSafety, TrickSuitOfFourIsRejectedNotRenderedAsNoTrump) +{ + // 4 is a legal trump (no-trump) but not a legal trick suit. Sharing one + // bound between the two would render it as "N" in the dump and hide the + // value that was rejected. + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 4; // no-trump: legal + deal.first = 0; + deal.currentTrickSuit[0] = 4; // not a legal suit + deal.currentTrickRank[0] = 5; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_SUIT_OR_RANK); +} + +TEST(DumpInputSafety, OutOfRangeTrumpIsReportedNotIndexed) +{ + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 99; + deal.first = 0; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_TRUMP_WRONG); +} + +TEST(DumpInputSafety, OutOfRangeFirstIsReportedNotIndexed) +{ + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 0; + deal.first = 99; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_FIRST_WRONG); +} + +TEST(DumpInputSafety, NegativeTrickValuesAreReportedNotIndexed) +{ + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 0; + deal.first = 0; + deal.currentTrickSuit[0] = -3; + deal.currentTrickRank[0] = -7; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_SUIT_OR_RANK); +} + +TEST(DumpInputSafety, UncheckedTrickSuitIsNotUsedAsSubscript) +{ + // board_range_checks() only validates currentTrickSuit[k] when the matching + // rank is non-zero, but hand_rel_first is derived from the card count, so + // board_value_checks() could reach an unchecked suit and index remainCards + // out of bounds. Found by the solve_board fuzz harness. + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 4; + deal.first = 0; + deal.currentTrickSuit[1] = 24832; // never validated: rank below is zero + deal.remainCards[0][2] = 0x100; + deal.remainCards[1][1] = 0x40; + deal.remainCards[2][2] = 0x40; + deal.remainCards[3][2] = 0x2000; + deal.remainCards[3][3] = 0x4000; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + EXPECT_EQ(SolveBoard(deal, 0, 3, 0, &fut, 0), RETURN_SUIT_OR_RANK); +} + +TEST(CalcTableValidation, CalcAllTablesPbnXRejectsOverflowingCountWithoutAllocating) +{ + // numDeals * included would overflow, so the result is already decided. + // The preflight must run before the vector allocation and the O(numDeals) + // conversion loop; otherwise this either exhausts memory or reports + // RETURN_UNKNOWN_FAULT from a caught bad_alloc. + DdTableResults results; + std::memset(&results, 0, sizeof(results)); + ParResults par; + std::memset(&par, 0, sizeof(par)); + int const filter[DDS_STRAINS] = {0, 0, 0, 0, 0}; + + DdTableDealPBN one; + std::memset(&one, 0, sizeof(one)); + std::strncpy(one.cards, kLegalDeal, sizeof(one.cards) - 1); + + EXPECT_EQ(CalcAllTablesPBNX(2147483647, &one, -1, filter, &results, &par, 1), + RETURN_TOO_MANY_TABLES); + EXPECT_EQ(CalcAllTablesX(2147483647, nullptr, -1, filter, &results, &par, 1), + RETURN_UNKNOWN_FAULT); // null array caught before the count +} + +TEST(DumpInputSafety, RejectedRanksAreShownRawNotAsSentinels) +{ + // card_rank[] holds sentinels 'x' at 0..1 and '-' at 15, but a trick rank is + // only legal in 2..14. Bounding by the array size rendered a rejected rank + // of 1 or 15 as a sentinel character, hiding the value in dump.txt. + for (int bad_rank : {1, 15}) + { + std::remove("dump.txt"); + + Deal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.trump = 0; + deal.first = 0; + deal.currentTrickSuit[0] = 0; + deal.currentTrickRank[0] = bad_rank; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + ASSERT_EQ(SolveBoard(deal, -1, 1, 1, &fut, 0), RETURN_SUIT_OR_RANK) + << "rank " << bad_rank; + + std::ifstream dump("dump.txt"); + if (!dump) + continue; // DDS_NO_DUMP_ON_ERROR build: nothing to check. + + std::string const text((std::istreambuf_iterator(dump)), + std::istreambuf_iterator()); + EXPECT_NE(text.find("?(" + std::to_string(bad_rank) + ")"), std::string::npos) + << "rank " << bad_rank << " not reported raw in dump.txt"; + } + std::remove("dump.txt"); +} + +} // namespace diff --git a/library/tests/fuzz/BUILD.bazel b/library/tests/fuzz/BUILD.bazel new file mode 100644 index 000000000..3ff33e914 --- /dev/null +++ b/library/tests/fuzz/BUILD.bazel @@ -0,0 +1,60 @@ +# Fuzz harnesses for the DDS input-handling surfaces. +# +# Each harness defines LLVMFuzzerTestOneInput() and is exposed two ways: +# +# *_fuzz_corpus_test a cc_test that replays the checked-in seed corpus +# through the harness. Builds with any toolchain and +# runs under --config=asan/ubsan, so the corpus works +# as a regression suite in ordinary CI. +# +# *_fuzz a libFuzzer binary for actual fuzzing campaigns. +# Requires --config=fuzz (hermetic LLVM; Apple's +# toolchain ships no libFuzzer) and is tagged manual so +# //... stays buildable without it. +# +# See README.md for how to run a campaign and how to triage a finding. + +load("@rules_cc//cc:defs.bzl", "cc_library") +load("//:CPPVARIABLES.bzl", "DDS_CPPOPTS", "DDS_LINKOPTS", "DDS_LOCAL_DEFINES") +load(":fuzz.bzl", "dds_fuzz_harness") + +package(default_visibility = ["//visibility:private"]) + +# Replay driver: supplies main() so a harness can run without libFuzzer. +cc_library( + name = "fuzz_corpus_main", + srcs = ["fuzz_corpus_main.cpp"], + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, +) + +dds_fuzz_harness( + name = "pbn", + src = "pbn_fuzz.cpp", + corpus_dir = "corpus/pbn", +) + +dds_fuzz_harness( + name = "par", + src = "par_fuzz.cpp", + corpus_dir = "corpus/par", +) + +dds_fuzz_harness( + name = "solve_board", + src = "solve_board_fuzz.cpp", + corpus_dir = "corpus/solve_board", +) + +dds_fuzz_harness( + name = "calc_dd_table_pbn", + src = "calc_dd_table_pbn_fuzz.cpp", + corpus_dir = "corpus/calc_dd_table_pbn", +) + +dds_fuzz_harness( + name = "calc_all_tables", + src = "calc_all_tables_fuzz.cpp", + corpus_dir = "corpus/calc_all_tables", +) diff --git a/library/tests/fuzz/README.md b/library/tests/fuzz/README.md new file mode 100644 index 000000000..080894837 --- /dev/null +++ b/library/tests/fuzz/README.md @@ -0,0 +1,127 @@ +# Fuzz harnesses + +Coverage-guided fuzzing for the five DDS surfaces that consume caller- or +file-supplied data — four single-deal entry points plus the batch table API: + +| Harness | Entry point | Why | +|---|---|---| +| `pbn` | `convert_from_pbn()` | The main text parser; the one path that routinely sees externally authored data (PBN files). | +| `calc_dd_table_pbn` | `CalcDDtablePBN()` | The full PBN-to-solver path: parse, then calculate a DD table. | +| `solve_board` | `SolveBoard()` | The main solver, including its input validation layer. | +| `par` | `Par()`, `SidesPar()`, `SidesParBin()`, `DealerPar()`, `DealerParBin()` | Derives contract text from a caller-supplied table into fixed-size buffers. | +| `calc_all_tables` | `CalcAllTablesN()`, `CalcAllTablesPBNN()`, `CalcAllTablesX()` | The batch entry points, and in particular their handling of a caller-supplied deal *count*. | + +## Two ways to run each harness + +**Corpus replay (runs in ordinary CI).** Every harness is also an ordinary +`cc_test` that replays the checked-in seed corpus. No libFuzzer needed, so it +works on every platform and under the sanitizer configs: + +``` +bazel test //library/tests/fuzz/... +bazel test --config=asan //library/tests/fuzz/... +``` + +This is what keeps a fixed bug fixed: a reproducer added to `corpus/` becomes a +permanent regression seed. + +**Fuzzing campaign.** The `*_fuzz` targets are libFuzzer binaries, tagged +`manual` and built only under `--config=fuzz`: + +``` +bazel run --config=fuzz //library/tests/fuzz:pbn_fuzz -- \ + library/tests/fuzz/corpus/pbn -runs=1000000 +``` + +`--config=fuzz` uses the registered hermetic LLVM toolchain, which ships +`libclang_rt.fuzzer`. Apple's toolchain does not, which is why the config does +not chain `--config=asan` — on macOS that switches to the Xcode toolchain and +the link fails. On Linux, add `--config=asan` for fuzzing with memory-error +detection: + +``` +bazel run --config=fuzz --config=asan //library/tests/fuzz:solve_board_fuzz -- \ + library/tests/fuzz/corpus/solve_board -runs=10000000 +``` + +> **Use `bazel run`, not `bazel-bin/...` directly.** `--config=fuzz`, `asan`, +> `ubsan` and `tsan` all build with `--compilation_mode=dbg`, so they share the +> `darwin_arm64-dbg` / `k8-dbg` output directory name. A path taken from +> `bazel info --config=fuzz bazel-bin` can therefore point at a binary left +> behind by a *different* sanitizer config, which silently reproduces (or fails +> to reproduce) the wrong thing. + +### Why the batch harness exists + +The four single-deal harnesses drive one deal at a time, so none of them +exercised how the batch entry points handle `no_of_tables`. That is exactly +where `CalcAllTablesPBNN()` copied a caller-supplied count of records into a +fixed-size local before validating it — a stack-buffer-overflow *write* that +was found in code review rather than by fuzzing. `calc_all_tables_fuzz` covers +that surface, and removing either count guard reproduces a crash under it. On +its first CI run it also found an unrelated defect of its own — see finding 05 +— which only MemorySanitizer detects, so run this harness under `--config=msan` +on Linux as well as `--config=asan`. + +Two details in that harness are load-bearing: + +- Slots the input does not perturb are pre-filled with a **valid** deal. + `CalcAllTablesPBNN()` stops at the first slot `convert_from_pbn()` rejects, + so with zeroed slots the loop returns `RETURN_PBN_FAULT` immediately and + never reaches the boundary. The first version of this harness made that + mistake and did not catch the bug it was written for. +- The fill deal holds **one card per hand**, not a full 52. This harness + targets count and batch handling, not search depth; a full deal in every + slot drops throughput from ~75000 executions in four minutes to ~1700. + +### Throughput, and why `calc_dd_table_pbn` is slow + +Harnesses pass an explicit `maxThreads` of 1 to the `*N` entry points. That is +deliberate: the non-`N` variants delegate with `maxThreads = 0`, which selects +hardware concurrency, so a single input would fan out across every core — +wrong for a fuzzer, which gets its parallelism from running many processes. +`SetMaxThreads()` cannot be used for this: it is a deprecated alias of +`InitializeStaticMemory()` and ignores its argument. + +The consequence is that `calc_dd_table_pbn` is compute-bound. A full 52-card +deal is 20 double dummy solves, and under coverage instrumentation on one +worker the hardest seed in the corpus takes about 36 seconds. `-max_total_time` +is only checked between runs, so it will not interrupt one of those. Run this +target with an explicit per-input cap and expect low throughput: + +``` +bazel run --config=fuzz //library/tests/fuzz:calc_dd_table_pbn_fuzz -- \ + library/tests/fuzz/corpus/calc_dd_table_pbn -timeout=60 -max_total_time=1800 +``` + +The other four are fast: `par` and `pbn` reach hundreds of thousands of +executions per minute, and `calc_all_tables` around 75000 in four minutes. + +## Harness contract + +Each harness defines `LLVMFuzzerTestOneInput()` and `LLVMFuzzerInitialize()`. +libFuzzer treats the initializer as optional via a weak symbol, but weak +references are not portable between ELF and Mach-O, so the replay driver +requires it — harnesses with nothing to configure define a trivial one. + +Harnesses must supply well-formed *containers* even for malformed content: the +PBN entry points take NUL-terminated strings, so the harness terminates the +buffer itself. Handing the library a non-terminated array would report a +harness bug as a library bug. + +## Triaging a finding + +1. libFuzzer writes the input to `./crash-`. +2. Reproduce it under ASan with the replay driver, which gives a far better + report than libFuzzer alone: + ``` + mkdir -p /tmp/f && cp crash- /tmp/f/ + bazel build --config=asan //library/tests/fuzz:par_fuzz_corpus_test + ./bazel-bin/library/tests/fuzz/par_fuzz_corpus_test /tmp/f + ``` +3. If it is not yet fixed, put the reproducer in `findings/` and describe it in + `findings/README.md` so the corpus tests stay green. +4. Once fixed, move it into the matching `corpus/` directory. + +`findings/` records what the harnesses have found, fixed and open — see +`findings/README.md`. There are currently no open findings. diff --git a/library/tests/fuzz/calc_all_tables_fuzz.cpp b/library/tests/fuzz/calc_all_tables_fuzz.cpp new file mode 100644 index 000000000..b42c82a7a --- /dev/null +++ b/library/tests/fuzz/calc_all_tables_fuzz.cpp @@ -0,0 +1,211 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file calc_all_tables_fuzz.cpp +/// @brief Fuzz harness for the batch table entry points. +/// +/// The four single-deal harnesses drive one deal at a time and so never +/// exercised the batch entry points' *count* handling. That is where +/// CalcAllTablesPBNN() converted no_of_tables records into a fixed-size local +/// before validating anything -- a stack-buffer-overflow write found in +/// review rather than by fuzzing. This harness covers that surface. +/// +/// Two different kinds of count are involved, and only one of them is the +/// library's business: +/// +/// - DdTableDeals::no_of_tables and DdTableDealsPBN::no_of_tables are +/// fields inside a fixed-capacity struct, so any value at all is +/// legitimate fuzzer input and the library must bound it itself. The +/// harness passes them through verbatim. +/// +/// - CalcAllTablesX() takes a count alongside a caller-allocated array, so +/// passing a count larger than the array would be a harness bug, not a +/// library one. The harness allocates exactly the number it declares, and +/// caps it so one input cannot allocate unboundedly. + +#include +#include +#include +#include +#include + +#include + +namespace { + +/// Upper bound on the array CalcAllTablesX() is asked to read, to keep any +/// single input's allocation and solve time bounded. +constexpr int kMaxDealsForX = 32; + +/// Deal slots perturbed from the input. Every *other* slot is pre-filled with +/// a valid deal rather than left zeroed. That matters: CalcAllTablesPBNN() +/// stops at the first slot convert_from_pbn() rejects, so with zeroed slots +/// the conversion loop returns RETURN_PBN_FAULT immediately and never reaches +/// the count boundary that the whole harness exists to exercise. +constexpr int kMaxPopulated = 4; + +/// A valid PBN deal used to fill the slots the input does not perturb. One +/// card per hand rather than a full 52: this harness targets count and batch +/// handling, not search depth -- the single-deal harnesses cover that -- and +/// a full deal in every slot drops throughput to a few executions a second. +/// Seeds can still place full deals in the perturbed slots. +constexpr char kFillPbn[] = "N:A... Q... K... J..."; + +class Reader +{ +public: + Reader(const uint8_t * data, size_t size) : data_(data), left_(size) {} + + auto take(void * out, size_t n) -> bool + { + if (left_ < n) + return false; + if (n == 0) + return true; // memcpy's source is declared nonnull; data_ may be null. + std::memcpy(out, data_, n); + data_ += n; + left_ -= n; + return true; + } + + auto byte(uint8_t fallback) -> uint8_t + { + uint8_t v = fallback; + take(&v, 1); + return v; + } + + auto remaining() const -> size_t { return left_; } + +private: + const uint8_t * data_; + size_t left_; +}; + +/// The binary equivalent of kFillPbn: one spade each, so every slot is a +/// valid deal that solves immediately. +auto legal_deal() -> DdTableDeal +{ + DdTableDeal deal; + std::memset(&deal, 0, sizeof(deal)); + deal.cards[0][0] = 0x4000; // A + deal.cards[1][0] = 0x1000; // Q + deal.cards[2][0] = 0x2000; // K + deal.cards[3][0] = 0x0800; // J + return deal; +} + +} // namespace + +extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int +{ + // SetMaxThreads() is a deprecated alias of InitializeStaticMemory() whose + // thread argument is ignored, so it never capped anything here. Worker + // counts come from each call's explicit maxThreads instead. + InitializeStaticMemory(); + return 0; +} + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int +{ + Reader reader(data, size); + + // Passed to the library verbatim: bounding it is the library's job. + int32_t raw_count = 0; + if (!reader.take(&raw_count, sizeof(raw_count))) + return 0; + + int trump_filter[DDS_STRAINS]; + for (int k = 0; k < DDS_STRAINS; k++) + trump_filter[k] = (reader.byte(0) & 1); + + // -1 disables the par calculation; 0..3 select a vulnerability. Values + // outside that are worth passing too. + int const mode = static_cast(reader.byte(0) % 8) - 2; + + uint8_t const selector = reader.byte(0); + int const populate = static_cast(reader.byte(0) % (kMaxPopulated + 1)); + + auto results = std::make_unique(); + std::memset(results.get(), 0, sizeof(DdTablesRes)); + auto par = std::make_unique(); + std::memset(par.get(), 0, sizeof(AllParResults)); + + switch (selector % 3) + { + case 0: + { + auto deals = std::make_unique(); + std::memset(deals.get(), 0, sizeof(DdTableDeals)); + deals->no_of_tables = raw_count; + + DdTableDeal const fill = legal_deal(); + for (auto & slot : deals->deals) + slot = fill; + + // Perturb the first few from the input so malformed deals are + // reachable too, while the rest stay valid. + for (int i = 0; i < populate; i++) + reader.take(&deals->deals[i], sizeof(DdTableDeal)); + + CalcAllTablesN(deals.get(), mode, trump_filter, + results.get(), par.get(), 1); + break; + } + + case 1: + { + auto deals = std::make_unique(); + std::memset(deals.get(), 0, sizeof(DdTableDealsPBN)); + deals->no_of_tables = raw_count; + + for (auto & slot : deals->deals) + std::memcpy(slot.cards, kFillPbn, sizeof(kFillPbn)); + + for (int i = 0; i < populate; i++) + { + // cards is a fixed char[80] the library reads as a C string, so the + // harness terminates it; feeding a non-terminated array would report + // a harness bug as a library one. + auto & cards = deals->deals[i].cards; + reader.take(cards, sizeof(cards) - 1); + cards[sizeof(cards) - 1] = '\0'; + } + + CalcAllTablesPBNN(deals.get(), mode, trump_filter, + results.get(), par.get(), 1); + break; + } + + default: + { + // Count and array must agree here: see the file comment. + int num_deals = raw_count < 0 ? 0 : raw_count % (kMaxDealsForX + 1); + + std::vector deals(static_cast(num_deals)); + for (int i = 0; i < num_deals; i++) + { + deals[static_cast(i)] = legal_deal(); + if (i < populate) + reader.take(&deals[static_cast(i)], sizeof(DdTableDeal)); + } + + // CalcAllTablesX writes results[m] and par[m] for m < num_deals (see + // the writes near the end of CalcAllTablesX), so one entry each per + // deal. The +1 keeps .data() non-null when num_deals is 0. + std::vector table_results( + static_cast(num_deals) + 1); + std::vector par_results( + static_cast(num_deals) + 1); + + CalcAllTablesX(num_deals, deals.data(), mode, trump_filter, + table_results.data(), par_results.data(), 1); + break; + } + } + + return 0; +} diff --git a/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp new file mode 100644 index 000000000..54e0dec7c --- /dev/null +++ b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp @@ -0,0 +1,54 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file calc_dd_table_pbn_fuzz.cpp +/// @brief Fuzz harness for CalcDDtablePBN(). +/// +/// This is the path a PBN file takes into the solver: text parsing followed by +/// a full double dummy table calculation. DdTableDealPBN::cards is a fixed +/// char[80], so the harness copies at most 79 bytes and terminates the buffer +/// itself; handing the library a non-terminated array would be a harness bug +/// rather than a library one. + +#include +#include +#include + +#include + +extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int +{ + // SetMaxThreads() is a deprecated alias of InitializeStaticMemory() whose + // thread argument is ignored, so it never capped anything here. Worker + // counts come from each call's explicit maxThreads instead. + InitializeStaticMemory(); + return 0; +} + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int +{ + DdTableDealPBN table_deal; + std::memset(&table_deal, 0, sizeof(table_deal)); + + size_t const n = size < sizeof(table_deal.cards) - 1 + ? size + : sizeof(table_deal.cards) - 1; + // libFuzzer may pass (nullptr, 0), and memcpy's source is declared nonnull, + // so an empty copy from a null pointer is undefined even though it moves + // nothing. UBSan on glibc reports it; guard rather than rely on the libc. + if (n > 0) + std::memcpy(table_deal.cards, data, n); + table_deal.cards[n] = '\0'; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + + // The non-N entry point delegates with maxThreads = 0, which selects + // hardware concurrency; call the N variant so one input uses one worker. + CalcDDtablePBNN(table_deal, &table, 1); + + return 0; +} diff --git a/library/tests/fuzz/corpus/calc_all_tables/all_strains_filtered.bin b/library/tests/fuzz/corpus/calc_all_tables/all_strains_filtered.bin new file mode 100644 index 000000000..a373a3fd2 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/all_strains_filtered.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_at_capacity.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_at_capacity.bin new file mode 100644 index 000000000..eac653455 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_at_capacity.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_count_huge.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_count_huge.bin new file mode 100644 index 000000000..3bffd268c Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_count_huge.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_count_int_min.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_count_int_min.bin new file mode 100644 index 000000000..3e51ac2bb Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_count_int_min.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_count_just_over.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_count_just_over.bin new file mode 100644 index 000000000..8747d0166 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_count_just_over.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_count_mul_overflow.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_count_mul_overflow.bin new file mode 100644 index 000000000..58a9520cd Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_count_mul_overflow.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_count_negative.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_count_negative.bin new file mode 100644 index 000000000..0af02cc3b Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_count_negative.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_one_legal_deal.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_one_legal_deal.bin new file mode 100644 index 000000000..2e6c8a5ae Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_one_legal_deal.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_two_legal_deals.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_two_legal_deals.bin new file mode 100644 index 000000000..72f559d1d Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_two_legal_deals.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/bin_with_par.bin b/library/tests/fuzz/corpus/calc_all_tables/bin_with_par.bin new file mode 100644 index 000000000..a581cc857 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/bin_with_par.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/pbn_count_just_over.bin b/library/tests/fuzz/corpus/calc_all_tables/pbn_count_just_over.bin new file mode 100644 index 000000000..653cf3941 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/pbn_count_just_over.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/pbn_count_overflow.bin b/library/tests/fuzz/corpus/calc_all_tables/pbn_count_overflow.bin new file mode 100644 index 000000000..d2c689e20 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/pbn_count_overflow.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/pbn_two_real_deals.bin b/library/tests/fuzz/corpus/calc_all_tables/pbn_two_real_deals.bin new file mode 100644 index 000000000..7c302a3a0 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/pbn_two_real_deals.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/x_count_huge.bin b/library/tests/fuzz/corpus/calc_all_tables/x_count_huge.bin new file mode 100644 index 000000000..418021d40 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/x_count_huge.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/x_count_negative.bin b/library/tests/fuzz/corpus/calc_all_tables/x_count_negative.bin new file mode 100644 index 000000000..46adce0c0 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/x_count_negative.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/x_two_deals.bin b/library/tests/fuzz/corpus/calc_all_tables/x_two_deals.bin new file mode 100644 index 000000000..adee929e1 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/x_two_deals.bin differ diff --git a/library/tests/fuzz/corpus/calc_all_tables/zero_tables.bin b/library/tests/fuzz/corpus/calc_all_tables/zero_tables.bin new file mode 100644 index 000000000..e7f1dd580 Binary files /dev/null and b/library/tests/fuzz/corpus/calc_all_tables/zero_tables.bin differ diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_compass.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_compass.txt new file mode 100644 index 000000000..5b9a6d969 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_compass.txt @@ -0,0 +1 @@ +X:QJ6.K652.J85.T98 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_rank.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_rank.txt new file mode 100644 index 000000000..f8464c1f1 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_rank.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85.TZ8 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/east_first.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/east_first.txt new file mode 100644 index 000000000..7397de9ef --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/east_first.txt @@ -0,0 +1 @@ +E:QJT5432.T.6.QJ82 .J97543.K7532.94 87.A62.QJT4.AT75 AK96.KQ8.A98.K63 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/empty.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/empty.txt new file mode 100644 index 000000000..e69de29bb diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/extra_dots.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/extra_dots.txt new file mode 100644 index 000000000..d88c593fc --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/extra_dots.txt @@ -0,0 +1 @@ +N:....... ....... ....... ....... \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/lowercase.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/lowercase.txt new file mode 100644 index 000000000..85d525844 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/lowercase.txt @@ -0,0 +1 @@ +n:qj6.k652.j85.t98 873.j97.at764.q4 k5.t83.kq9.a7652 at942.aq4.32.kj3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/no_colon.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/no_colon.txt new file mode 100644 index 000000000..92c0edd58 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/no_colon.txt @@ -0,0 +1 @@ +NQJ6.K652.J85.T98 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/north_first.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/north_first.txt new file mode 100644 index 000000000..5b6ed3653 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/north_first.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85.T98 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/south_first.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/south_first.txt new file mode 100644 index 000000000..f75fa9d2a --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/south_first.txt @@ -0,0 +1 @@ +S:A7.KQ92.AK54.J83 KQ983.876.T7.QT9 J642.AJT.QJ63.A5 T5.543.982.K7642 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/truncated.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/truncated.txt new file mode 100644 index 000000000..abe898764 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/truncated.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/unbalanced_51_cards.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/unbalanced_51_cards.txt new file mode 100644 index 000000000..b93522d95 --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/unbalanced_51_cards.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85.T8 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/void_suits.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/void_suits.txt new file mode 100644 index 000000000..95d2a128a --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/void_suits.txt @@ -0,0 +1 @@ +N:AKQJT98765432... .AKQJT98765432.. ..AKQJT98765432. ...AKQJT98765432 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/calc_dd_table_pbn/west_first.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/west_first.txt new file mode 100644 index 000000000..14be797cd --- /dev/null +++ b/library/tests/fuzz/corpus/calc_dd_table_pbn/west_first.txt @@ -0,0 +1 @@ +W:KQ.A32.KQJT9.QJT A98.KQJ.8765.987 JT765432.T98..65 .7654.A432.AK432 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/par/all_thirteen.bin b/library/tests/fuzz/corpus/par/all_thirteen.bin new file mode 100644 index 000000000..d8ed76e32 Binary files /dev/null and b/library/tests/fuzz/corpus/par/all_thirteen.bin differ diff --git a/library/tests/fuzz/corpus/par/all_zero.bin b/library/tests/fuzz/corpus/par/all_zero.bin new file mode 100644 index 000000000..149420a23 Binary files /dev/null and b/library/tests/fuzz/corpus/par/all_zero.bin differ diff --git a/library/tests/fuzz/corpus/par/int_min.bin b/library/tests/fuzz/corpus/par/int_min.bin new file mode 100644 index 000000000..4e1d00621 Binary files /dev/null and b/library/tests/fuzz/corpus/par/int_min.bin differ diff --git a/library/tests/fuzz/corpus/par/legal_balanced.bin b/library/tests/fuzz/corpus/par/legal_balanced.bin new file mode 100644 index 000000000..2a240df40 Binary files /dev/null and b/library/tests/fuzz/corpus/par/legal_balanced.bin differ diff --git a/library/tests/fuzz/corpus/par/legal_slam.bin b/library/tests/fuzz/corpus/par/legal_slam.bin new file mode 100644 index 000000000..f1e63861b Binary files /dev/null and b/library/tests/fuzz/corpus/par/legal_slam.bin differ diff --git a/library/tests/fuzz/corpus/par/legal_vul_both.bin b/library/tests/fuzz/corpus/par/legal_vul_both.bin new file mode 100644 index 000000000..8a6dd3d92 Binary files /dev/null and b/library/tests/fuzz/corpus/par/legal_vul_both.bin differ diff --git a/library/tests/fuzz/corpus/par/legal_vul_ew.bin b/library/tests/fuzz/corpus/par/legal_vul_ew.bin new file mode 100644 index 000000000..ebac3c8b6 Binary files /dev/null and b/library/tests/fuzz/corpus/par/legal_vul_ew.bin differ diff --git a/library/tests/fuzz/corpus/par/legal_vul_ns.bin b/library/tests/fuzz/corpus/par/legal_vul_ns.bin new file mode 100644 index 000000000..40a7d94c1 Binary files /dev/null and b/library/tests/fuzz/corpus/par/legal_vul_ns.bin differ diff --git a/library/tests/fuzz/corpus/par/mixed_edge.bin b/library/tests/fuzz/corpus/par/mixed_edge.bin new file mode 100644 index 000000000..4a72de81b Binary files /dev/null and b/library/tests/fuzz/corpus/par/mixed_edge.bin differ diff --git a/library/tests/fuzz/corpus/par/negative.bin b/library/tests/fuzz/corpus/par/negative.bin new file mode 100644 index 000000000..42edf8d5b --- /dev/null +++ b/library/tests/fuzz/corpus/par/negative.bin @@ -0,0 +1 @@ +ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ \ No newline at end of file diff --git a/library/tests/fuzz/corpus/par/overflow_regression.bin b/library/tests/fuzz/corpus/par/overflow_regression.bin new file mode 100644 index 000000000..171433265 Binary files /dev/null and b/library/tests/fuzz/corpus/par/overflow_regression.bin differ diff --git a/library/tests/fuzz/corpus/par/regression_negative_dealer.bin b/library/tests/fuzz/corpus/par/regression_negative_dealer.bin new file mode 100644 index 000000000..ea263fee7 Binary files /dev/null and b/library/tests/fuzz/corpus/par/regression_negative_dealer.bin differ diff --git a/library/tests/fuzz/corpus/pbn/bad_compass.txt b/library/tests/fuzz/corpus/pbn/bad_compass.txt new file mode 100644 index 000000000..5b9a6d969 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/bad_compass.txt @@ -0,0 +1 @@ +X:QJ6.K652.J85.T98 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/bad_rank_parser_only.txt b/library/tests/fuzz/corpus/pbn/bad_rank_parser_only.txt new file mode 100644 index 000000000..f8464c1f1 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/bad_rank_parser_only.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85.TZ8 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/east_first.txt b/library/tests/fuzz/corpus/pbn/east_first.txt new file mode 100644 index 000000000..7397de9ef --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/east_first.txt @@ -0,0 +1 @@ +E:QJT5432.T.6.QJ82 .J97543.K7532.94 87.A62.QJT4.AT75 AK96.KQ8.A98.K63 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/empty.txt b/library/tests/fuzz/corpus/pbn/empty.txt new file mode 100644 index 000000000..e69de29bb diff --git a/library/tests/fuzz/corpus/pbn/extra_dots.txt b/library/tests/fuzz/corpus/pbn/extra_dots.txt new file mode 100644 index 000000000..d88c593fc --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/extra_dots.txt @@ -0,0 +1 @@ +N:....... ....... ....... ....... \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/lowercase.txt b/library/tests/fuzz/corpus/pbn/lowercase.txt new file mode 100644 index 000000000..85d525844 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/lowercase.txt @@ -0,0 +1 @@ +n:qj6.k652.j85.t98 873.j97.at764.q4 k5.t83.kq9.a7652 at942.aq4.32.kj3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/no_colon.txt b/library/tests/fuzz/corpus/pbn/no_colon.txt new file mode 100644 index 000000000..92c0edd58 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/no_colon.txt @@ -0,0 +1 @@ +NQJ6.K652.J85.T98 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/north_first.txt b/library/tests/fuzz/corpus/pbn/north_first.txt new file mode 100644 index 000000000..5b6ed3653 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/north_first.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85.T98 873.J97.AT764.Q4 K5.T83.KQ9.A7652 AT942.AQ4.32.KJ3 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/south_first.txt b/library/tests/fuzz/corpus/pbn/south_first.txt new file mode 100644 index 000000000..f75fa9d2a --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/south_first.txt @@ -0,0 +1 @@ +S:A7.KQ92.AK54.J83 KQ983.876.T7.QT9 J642.AJT.QJ63.A5 T5.543.982.K7642 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/truncated.txt b/library/tests/fuzz/corpus/pbn/truncated.txt new file mode 100644 index 000000000..abe898764 --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/truncated.txt @@ -0,0 +1 @@ +N:QJ6.K652.J85 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/void_suits.txt b/library/tests/fuzz/corpus/pbn/void_suits.txt new file mode 100644 index 000000000..95d2a128a --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/void_suits.txt @@ -0,0 +1 @@ +N:AKQJT98765432... .AKQJT98765432.. ..AKQJT98765432. ...AKQJT98765432 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/pbn/west_first.txt b/library/tests/fuzz/corpus/pbn/west_first.txt new file mode 100644 index 000000000..14be797cd --- /dev/null +++ b/library/tests/fuzz/corpus/pbn/west_first.txt @@ -0,0 +1 @@ +W:KQ.A32.KQJT9.QJT A98.KQJ.8765.987 JT765432.T98..65 .7654.A432.AK432 \ No newline at end of file diff --git a/library/tests/fuzz/corpus/solve_board/all_zero.bin b/library/tests/fuzz/corpus/solve_board/all_zero.bin new file mode 100644 index 000000000..7e5294ecc Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/all_zero.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/legal_notrump.bin b/library/tests/fuzz/corpus/solve_board/legal_notrump.bin new file mode 100644 index 000000000..b0f536999 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/legal_notrump.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/legal_partial_trick.bin b/library/tests/fuzz/corpus/solve_board/legal_partial_trick.bin new file mode 100644 index 000000000..a2a2b6d39 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/legal_partial_trick.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/legal_spades.bin b/library/tests/fuzz/corpus/solve_board/legal_spades.bin new file mode 100644 index 000000000..21f4bee19 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/legal_spades.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/one_card_each.bin b/library/tests/fuzz/corpus/solve_board/one_card_each.bin new file mode 100644 index 000000000..9995ce760 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/one_card_each.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/regression_out_of_range_deal.bin b/library/tests/fuzz/corpus/solve_board/regression_out_of_range_deal.bin new file mode 100644 index 000000000..9e2ce35f6 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/regression_out_of_range_deal.bin differ diff --git a/library/tests/fuzz/corpus/solve_board/regression_unchecked_trick_suit.bin b/library/tests/fuzz/corpus/solve_board/regression_unchecked_trick_suit.bin new file mode 100644 index 000000000..97ccb27d5 Binary files /dev/null and b/library/tests/fuzz/corpus/solve_board/regression_unchecked_trick_suit.bin differ diff --git a/library/tests/fuzz/findings/README.md b/library/tests/fuzz/findings/README.md new file mode 100644 index 000000000..f18790e64 --- /dev/null +++ b/library/tests/fuzz/findings/README.md @@ -0,0 +1,133 @@ +# Findings + +Reproducers for defects the harnesses have found that are **not yet fixed** +live here rather than in `corpus/`, so the corpus-replay tests stay green. A +fuzzing campaign will rediscover them immediately, which is expected. + +When a defect is fixed, move its reproducer into the matching `corpus/` +directory so it becomes a permanent regression seed, and record it below. + +## Open findings + +**None.** + +## Open hardening items + +These are not memory-safety defects, but they are worth knowing about. + +- `convert_from_pbn()` silently ignores any character that is not a card, `.`, + ` ` or a compass letter (`pbn.cpp:113-116`), so a PBN string with an invalid + rank parses "successfully" one card short rather than being refused. Since + the `CalcDDtable*` paths now reject the resulting unbalanced deal, this is a + diagnostics problem (`RETURN_CARD_COUNT` where `RETURN_PBN_FAULT` would be + clearer) rather than a safety one. Tightening it needs care: PBN text from + files often carries trailing newlines or `\r`, which the current loop + tolerates, so a strict version needs an explicit whitespace allowance. + +## Fixed + +### 01 — unbalanced deal reached the search (CalcDDtable path) + +`CalcDDtable()` and `CalcDDtablePBN()` did not check that the four hands held +equal numbers of cards, so a 51-card deal reached the search and read 14248 +bytes past `rel_rank_storage` (`lookup_tables.cpp`, 122880 bytes) in +`weight_alloc_trump0()` / `QuickTricksPartnerHandNT()`. `SolveBoard()` rejected +the same deal via `board_value_checks()`; only the CalcDDtable path was +exposed. Reachable from an ordinary PBN file that is short of a card — the +reproducer contains **only legal PBN characters**. + +Fixed by `table_deal_checks()` (`library/src/table_deal_validate.hpp`), applied +in `CalcDDtableN()`, `CalcAllTablesN()` and `CalcAllTablesX()`. It enforces the +same three rules `SolveBoard()` already did, so nothing is rejected that the +solver would have accepted. + +Seeds: `corpus/calc_dd_table_pbn/unbalanced_51_cards.txt`, +`corpus/calc_dd_table_pbn/bad_rank.txt`. +Tests: `library/tests/deal_input_validation_test.cpp` (`CalcTableValidation`). + +### 02 — `DealerPar()` did not validate `dealer` + +A negative `dealer` propagated into `pno_list[]` and reached +`sacrifice_as_text()`, where `NUMBER_TO_PLAYER[static_cast(pno)]` +turned `-1` into 4294967295 and indexed a `std::string` array far out of +bounds. The crashing input used a **legal** `res_table`; only `dealer` was out +of range. + +Same class as the `vulnerable` bug fixed by hand in `2abb260e`, which guarded +one parameter of the pair and missed the other. The fuzzer found it within +50000 runs. + +Fixed by range-checking `dealer` in `DealerPar()`, and by replacing the +`static_cast` subscripts with the guarded `contract_text()` and +`player_text()` helpers — the cast is what turned a detectable bug into a wild +read. + +Seed: `corpus/par/regression_negative_dealer.bin`. +Tests: `library/tests/par_validation_test.cpp` (`ParValidation`). + +### 03 — `DumpInput()` read out of bounds while reporting invalid input + +`board_range_checks()` correctly detected an out-of-range deal, then called +`DumpInput()` to log it — and `DumpInput()` indexed `card_suit[5]`, +`card_hand[4]` and `card_rank[16]` with the very values it was reporting as +invalid (`dump.cpp:288-298`). Present in release builds, since `DumpInput()` is +compiled in unless `DDS_NO_DUMP_ON_ERROR` is defined and the build does not +define it. + +Fixed by the `suit_text()` / `hand_text()` / `rank_text()` helpers in +`dump.cpp`, which fall back to printing the raw integer when the value is out +of range — more useful in a diagnostic than a wrong character, and safe by +construction. + +Still true, and unchanged here: `DumpInput()` writes `dump.txt` into the +process working directory whenever input is rejected, which is surprising for a +library. Define `DDS_NO_DUMP_ON_ERROR` to compile it out. + +Seed: `corpus/solve_board/regression_out_of_range_deal.bin`. +Tests: `library/tests/deal_input_validation_test.cpp` (`DumpInputSafety`). + +### 04 — `board_value_checks()` indexed with an unvalidated trick suit + +Found while fuzzing the fixes for 01-03. `board_range_checks()` validates +`currentTrickSuit[k]` only when the matching `currentTrickRank[k]` is +non-zero, but `hand_rel_first` is derived from the total card count +(`hand_rel_first = (48 - ini_depth) % 4`, `solver_if.cpp:151`) rather than +from the trick entries. A deal with five cards and all trick ranks zero +therefore gives `hand_rel_first == 3`, and the loop in +`board_value_checks()` evaluates +`dl.remainCards[h][dl.currentTrickSuit[k]]` with a suit that was never +checked — a stack read far out of bounds. As with 03, the crash is inside the +validation logic itself. + +Fixed by range-checking `currentTrickSuit[k]` inside that loop, where it is +actually used as a subscript, rather than in `board_range_checks()` — this +rejects only inputs that would genuinely have been read out of bounds, and +leaves callers that pass an uninitialised suit alongside a zero rank working +as before whenever the value is never used. + +Seed: `corpus/solve_board/regression_unchecked_trick_suit.bin`. +Tests: `library/tests/deal_input_validation_test.cpp` (`DumpInputSafety`). + +### 05 — `CalcAllTablesN()` solved an uninitialised board when given no deals + +Found by the `calc_all_tables` harness on its first CI run, under +MemorySanitizer (`zero_tables.bin`). + +`Boards bo;` is an uninitialised stack local. The board count was derived from +a `lastIndex` variable initialised to 0 and only assigned inside the +board-building loop, so `bo.no_of_boards = lastIndex + 1` claimed **one** board +even when `no_of_tables == 0` and the loop had written none. +`calc_all_boards_n()` then solved `bo.deals[0]`, `bo.target[0]`, +`bo.solutions[0]` and `bo.mode[0]`, none of which had ever been written. + +ASan and UBSan do not see this; only MSan does, which is why it survived the +local sweep and surfaced in CI. + +Fixed by returning `RETURN_NO_FAULT` early for zero deals, matching what +`CalcAllTablesX()` already did, and by taking the board count from `ind` -- +the number of boards actually written -- rather than from a last-index +variable that starts at a valid-looking 0. + +Seed: `corpus/calc_all_tables/zero_tables.bin`. +Tests: `library/tests/deal_input_validation_test.cpp` +(`CalcAllTablesWithZeroDealsSolvesNothing`). diff --git a/library/tests/fuzz/fuzz.bzl b/library/tests/fuzz/fuzz.bzl new file mode 100644 index 000000000..d3a039fae --- /dev/null +++ b/library/tests/fuzz/fuzz.bzl @@ -0,0 +1,55 @@ +"""Macro pairing each fuzz harness with a corpus-replay test and a libFuzzer binary.""" + +load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_test") +load("//:CPPVARIABLES.bzl", "DDS_CPPOPTS", "DDS_LINKOPTS", "DDS_LOCAL_DEFINES") + +_HARNESS_DEPS = [ + "//library/src:testable_dds", + "//library/src/api:api_definitions", +] + +def dds_fuzz_harness(name, src, corpus_dir): + """Define a fuzz harness, its corpus-replay test, and its libFuzzer binary. + + Args: + name: base name; targets are _harness, _fuzz_corpus_test + and _fuzz. + src: the .cpp defining LLVMFuzzerTestOneInput(). + corpus_dir: workspace-relative seed corpus directory, passed to the + replay driver as a plain path (it walks directories recursively). + """ + cc_library( + name = name + "_harness", + srcs = [src], + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, + deps = _HARNESS_DEPS, + # The harness exports no symbol the driver references directly. + alwayslink = True, + ) + + cc_test( + name = name + "_fuzz_corpus_test", + size = "small", + # Runfiles-relative: a cc_test runs with its cwd at the runfiles root. + args = [native.package_name() + "/" + corpus_dir], + data = native.glob([corpus_dir + "/**"]), + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, + deps = [ + ":" + name + "_harness", + ":fuzz_corpus_main", + ], + ) + + cc_binary( + name = name + "_fuzz", + copts = DDS_CPPOPTS, + linkopts = DDS_LINKOPTS, + local_defines = DDS_LOCAL_DEFINES, + # libFuzzer supplies main(); requires --config=fuzz. + tags = ["manual"], + deps = [":" + name + "_harness"], + ) diff --git a/library/tests/fuzz/fuzz_corpus_main.cpp b/library/tests/fuzz/fuzz_corpus_main.cpp new file mode 100644 index 000000000..1abbda63a --- /dev/null +++ b/library/tests/fuzz/fuzz_corpus_main.cpp @@ -0,0 +1,216 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file fuzz_corpus_main.cpp +/// @brief Standalone driver that replays a seed corpus through a fuzz harness. +/// +/// libFuzzer is only available under --config=fuzz. This driver lets the same +/// LLVMFuzzerTestOneInput() harnesses run as ordinary cc_tests on every +/// platform and under --config=asan/ubsan, so the corpus acts as a regression +/// suite even where no fuzzer is linked. +/// +/// Each argument is a file or a directory; directories are walked recursively. + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int; + +// Every harness in this package defines this, even when it has nothing to set +// up. libFuzzer treats it as optional via a weak symbol, but weak references +// are not portable across ELF and Mach-O linkers, so it is required here. +extern "C" auto LLVMFuzzerInitialize(int * argc, char *** argv) -> int; + +namespace { + +namespace fs = std::filesystem; + +/* Corpus directories arrive as workspace-relative paths and must be resolved + through the runfiles. Mirrors library/tests/test_dtest_nothing_makes.py: + prefer a runfiles tree (RUNFILES_DIR / TEST_SRCDIR), and fall back to + RUNFILES_MANIFEST_FILE, which is what Windows uses when symlinked runfiles + are disabled and no tree exists on disk. */ + +auto env_path(char const * name) -> std::string +{ + char const * value = std::getenv(name); + return value == nullptr ? std::string() : std::string(value); +} + +/// Files under a runfiles tree, if one contains `relpath` as a directory. +auto files_from_tree(std::string const & relpath) -> std::vector +{ + std::vector found; + + for (char const * key : {"RUNFILES_DIR", "TEST_SRCDIR"}) + { + std::string const root = env_path(key); + if (root.empty()) + continue; + + for (fs::path const & candidate : + {fs::path(root) / relpath, fs::path(root) / "_main" / relpath}) + { + std::error_code ec; + if (!fs::is_directory(candidate, ec)) + continue; + + for (auto const & entry : fs::recursive_directory_iterator(candidate, ec)) + if (entry.is_regular_file()) + found.push_back(entry.path()); + + if (!found.empty()) + return found; + } + } + + return found; +} + +/// Files under `relpath` named by the runfiles manifest (Windows). +auto files_from_manifest(std::string const & relpath) -> std::vector +{ + std::vector found; + + std::string const manifest = env_path("RUNFILES_MANIFEST_FILE"); + if (manifest.empty()) + return found; + + std::ifstream in(manifest); + if (!in) + return found; + + // Manifest keys use forward slashes and may or may not carry the repo name. + std::string const with_repo = "_main/" + relpath + "/"; + std::string const bare = relpath + "/"; + + std::string line; + while (std::getline(in, line)) + { + if (line.empty() || line.front() == '[' || line.front() == ' ') + continue; + + auto const space = line.find(' '); + if (space == std::string::npos) + continue; + + std::string const key = line.substr(0, space); + std::string const value = line.substr(space + 1); + if (value.empty()) + continue; + + if (key.rfind(with_repo, 0) != 0 && key.rfind(bare, 0) != 0) + continue; + + std::error_code ec; + if (fs::is_regular_file(value, ec)) + found.emplace_back(value); + } + + return found; +} + +/// Every file under `arg`, whether it names a runfiles directory, a plain +/// directory, or a single file. +auto corpus_files(std::string const & arg) -> std::vector +{ + std::vector found = files_from_tree(arg); + if (!found.empty()) + return found; + + found = files_from_manifest(arg); + if (!found.empty()) + return found; + + // Direct invocation from a shell, where the path is simply on disk. + std::error_code ec; + if (fs::is_directory(arg, ec)) + { + for (auto const & entry : fs::recursive_directory_iterator(arg, ec)) + if (entry.is_regular_file()) + found.push_back(entry.path()); + } + else if (fs::is_regular_file(arg, ec)) + { + found.emplace_back(arg); + } + + return found; +} + +auto run_one(fs::path const & path) -> bool +{ + std::ifstream in(path, std::ios::binary); + if (!in) + { + std::fprintf(stderr, "cannot open %s\n", path.string().c_str()); + return false; + } + + std::vector const bytes( + (std::istreambuf_iterator(in)), std::istreambuf_iterator()); + + LLVMFuzzerTestOneInput(bytes.data(), bytes.size()); + return true; +} + +} // namespace + +auto main(int argc, char ** argv) -> int +{ + // libFuzzer calls this before the first input; the replay driver must too, + // or harnesses relying on it (e.g. InitializeStaticMemory) run unconfigured. + LLVMFuzzerInitialize(&argc, &argv); + + // Degenerate inputs every harness must survive, independent of the corpus. + uint8_t const zero[32] = {0}; + uint8_t const ones[32] = { + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}; + LLVMFuzzerTestOneInput(nullptr, 0); + LLVMFuzzerTestOneInput(zero, sizeof(zero)); + LLVMFuzzerTestOneInput(ones, sizeof(ones)); + + int files = 0; + bool ok = true; + + for (int i = 1; i < argc; i++) + { + std::vector const found = corpus_files(argv[i]); + + if (found.empty()) + { + std::fprintf(stderr, "no corpus files under: %s\n", argv[i]); + ok = false; + continue; + } + + for (fs::path const & path : found) + { + ok = run_one(path) && ok; + files++; + } + } + + // A corpus that silently resolves to nothing would make this test vacuous. + if (argc > 1 && files == 0) + { + std::fprintf(stderr, "corpus resolved to 0 files\n"); + return 1; + } + + std::printf("replayed %d corpus file(s)\n", files); + return ok ? 0 : 1; +} diff --git a/library/tests/fuzz/par_fuzz.cpp b/library/tests/fuzz/par_fuzz.cpp new file mode 100644 index 000000000..1c02df27d --- /dev/null +++ b/library/tests/fuzz/par_fuzz.cpp @@ -0,0 +1,93 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file par_fuzz.cpp +/// @brief Fuzz harness for the par calculation entry points. +/// +/// The par API derives contract levels from a caller-supplied DdTableResults +/// and formats them into fixed-size character buffers. An unvalidated table +/// overflowed those buffers (see library/tests/par_validation_test.cpp); this +/// harness drives all five exported entry points with arbitrary tables and +/// vulnerability values so any further gap in that validation surfaces here. + +#include +#include +#include + +#include + +namespace { + +/// Consume `n` bytes from the input, or return false if too few remain. +class Reader +{ +public: + Reader(const uint8_t * data, size_t size) : data_(data), left_(size) {} + + auto take(void * out, size_t n) -> bool + { + if (left_ < n) + return false; + if (n == 0) + return true; // memcpy's source is declared nonnull; data_ may be null. + std::memcpy(out, data_, n); + data_ += n; + left_ -= n; + return true; + } + +private: + const uint8_t * data_; + size_t left_; +}; + +} // namespace + +extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int +{ + // Nothing to configure; defined so the corpus-replay driver links. + return 0; +} + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int +{ + Reader reader(data, size); + + DdTableResults table; + if (!reader.take(&table, sizeof(table))) + return 0; + + uint8_t selector = 0; + if (!reader.take(&selector, sizeof(selector))) + return 0; + + // Exercise the full legal vulnerability range plus out-of-range values, + // since DealerPar() indexes a lookup table with this parameter. + int const vulnerable = static_cast(selector % 8) - 2; + int const dealer = static_cast((selector / 8) % 6) - 1; + + ParResults par_results; + std::memset(&par_results, 0, sizeof(par_results)); + Par(&table, &par_results, vulnerable); + + ParResultsDealer sides[2]; + std::memset(sides, 0, sizeof(sides)); + SidesPar(&table, sides, vulnerable); + + ParResultsMaster sides_bin[2]; + std::memset(sides_bin, 0, sizeof(sides_bin)); + SidesParBin(&table, sides_bin, vulnerable); + + ParResultsDealer dealer_res; + std::memset(&dealer_res, 0, sizeof(dealer_res)); + DealerPar(&table, &dealer_res, dealer, vulnerable); + + ParResultsMaster dealer_bin; + std::memset(&dealer_bin, 0, sizeof(dealer_bin)); + DealerParBin(&table, &dealer_bin, dealer, vulnerable); + + return 0; +} diff --git a/library/tests/fuzz/pbn_fuzz.cpp b/library/tests/fuzz/pbn_fuzz.cpp new file mode 100644 index 000000000..367e15d70 --- /dev/null +++ b/library/tests/fuzz/pbn_fuzz.cpp @@ -0,0 +1,46 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file pbn_fuzz.cpp +/// @brief Fuzz harness for the PBN deal-string parser. +/// +/// convert_from_pbn() is the library's main text-parsing surface and the one +/// path that routinely sees externally authored data (PBN files). It takes a +/// NUL-terminated string, so the harness terminates the input itself rather +/// than handing the parser a non-terminated buffer, which would report a +/// harness bug as a library bug. + +#include +#include +#include + +#include +#include + +extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int +{ + // Nothing to configure; defined so the corpus-replay driver links. + return 0; +} + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int +{ + // PBN deal strings are bounded in practice; keep inputs in that range so + // the fuzzer spends its budget on parser states rather than on length. + if (size > 4096) + return 0; + + // Same nonnull caveat as the memcpy in calc_dd_table_pbn_fuzz.cpp: building + // a string from (nullptr, 0) is undefined, so handle the empty case first. + std::string const deal = + size == 0 ? std::string() + : std::string(reinterpret_cast(data), size); + + unsigned int remain_cards[DDS_HANDS][DDS_SUITS]; + convert_from_pbn(deal.c_str(), remain_cards); + + return 0; +} diff --git a/library/tests/fuzz/solve_board_fuzz.cpp b/library/tests/fuzz/solve_board_fuzz.cpp new file mode 100644 index 000000000..41cd52770 --- /dev/null +++ b/library/tests/fuzz/solve_board_fuzz.cpp @@ -0,0 +1,54 @@ +/* + DDS, a bridge double dummy solver. + + See LICENSE and README. +*/ + +/// @file solve_board_fuzz.cpp +/// @brief Fuzz harness for SolveBoard(), the main solver entry point. +/// +/// SolveBoard() validates its input thoroughly (see solver_if.cpp) before +/// handing control to the search. This harness drives both halves: arbitrary +/// bytes mostly exercise the validation layer, while the seed corpus starts +/// the fuzzer from legal deals so coverage feedback can reach the search +/// itself. + +#include +#include +#include + +#include + +extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int +{ + // SetMaxThreads() is a deprecated alias of InitializeStaticMemory() whose + // thread argument is ignored, so it never capped anything here. Worker + // counts come from each call's explicit maxThreads instead. + InitializeStaticMemory(); + return 0; +} + +extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int +{ + // trump, first, currentTrickSuit[3], currentTrickRank[3], remainCards[4][4], + // plus one selector byte for target/solutions/mode. + Deal deal; + if (size < sizeof(deal) + 1) + return 0; + + std::memcpy(&deal, data, sizeof(deal)); + uint8_t const selector = data[sizeof(deal)]; + + // Cover the documented ranges and a little either side of them, so the + // parameter validation is exercised as well as the search. + int const target = static_cast(selector % 16) - 1; + int const solutions = static_cast((selector / 16) % 5) - 1; + int const mode = static_cast((selector / 80) % 4) - 1; + + FutureTricks fut; + std::memset(&fut, 0, sizeof(fut)); + + SolveBoard(deal, target, solutions, mode, &fut, 0); + + return 0; +} diff --git a/library/tests/par_validation_test.cpp b/library/tests/par_validation_test.cpp new file mode 100644 index 000000000..15f5fb158 --- /dev/null +++ b/library/tests/par_validation_test.cpp @@ -0,0 +1,386 @@ +/// @file par_validation_test.cpp +/// @brief Regression tests for double dummy table validation in the par API. +/// +/// The par entry points derive contract levels directly from +/// DdTableResults::res_table and format them into fixed-size character +/// buffers. Before par_table_checks() was added, an out-of-range table +/// overflowed those buffers: a table full of 2000000000 produced a +/// stack-buffer-overflow in Par() (strcat into `char temp[8]`, par.cpp:121) +/// and a silent 26-character write into the `char[10]` field +/// ParResultsDealer::contracts[0] via SidesPar(), both while still returning +/// RETURN_NO_FAULT. +/// +/// These tests are most meaningful under --config=asan, where an unguarded +/// regression aborts rather than merely returning the wrong code. + +#include +#include +#include +#include + +namespace { + +/// A legal table: trick counts per strain summing to 13 across the two sides. +auto legal_table() -> DdTableResults +{ + DdTableResults tab; + std::memset(&tab, 0, sizeof(tab)); + for (int d = 0; d < DDS_STRAINS; d++) + { + tab.res_table[d][0] = 7; // North + tab.res_table[d][1] = 6; // East + tab.res_table[d][2] = 7; // South + tab.res_table[d][3] = 6; // West + } + return tab; +} + +/// A table with every entry set to `v`. +auto uniform_table(int v) -> DdTableResults +{ + DdTableResults tab; + std::memset(&tab, 0, sizeof(tab)); + for (int d = 0; d < DDS_STRAINS; d++) + for (int h = 0; h < DDS_HANDS; h++) + tab.res_table[d][h] = v; + return tab; +} + +// --------------------------------------------------------------------------- +// The original overflow trigger. +// --------------------------------------------------------------------------- + +TEST(ParValidation, ParRejectsOverflowingTable) +{ + DdTableResults const tab = uniform_table(2000000000); + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + + EXPECT_EQ(Par(&tab, &resp, 0), RETURN_PAR_TABLE_FAULT); +} + +TEST(ParValidation, SidesParRejectsOverflowingTable) +{ + DdTableResults const tab = uniform_table(2000000000); + ParResultsDealer sides[2]; + std::memset(sides, 0, sizeof(sides)); + + EXPECT_EQ(SidesPar(&tab, sides, 0), RETURN_PAR_TABLE_FAULT); + // The pre-fix failure wrote 26 characters into this 10-byte field. + EXPECT_LT(std::strlen(sides[0].contracts[0]), sizeof(sides[0].contracts[0])); +} + +TEST(ParValidation, SidesParBinRejectsOverflowingTable) +{ + DdTableResults const tab = uniform_table(2000000000); + ParResultsMaster sides[2]; + std::memset(sides, 0, sizeof(sides)); + + EXPECT_EQ(SidesParBin(&tab, sides, 0), RETURN_PAR_TABLE_FAULT); +} + +TEST(ParValidation, DealerParRejectsOverflowingTable) +{ + DdTableResults const tab = uniform_table(2000000000); + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + + EXPECT_EQ(DealerPar(&tab, &resp, 0, 0), RETURN_PAR_TABLE_FAULT); +} + +TEST(ParValidation, DealerParBinRejectsOverflowingTable) +{ + DdTableResults const tab = uniform_table(2000000000); + ParResultsMaster resp; + std::memset(&resp, 0, sizeof(resp)); + + EXPECT_EQ(DealerParBin(&tab, &resp, 0, 0), RETURN_PAR_TABLE_FAULT); +} + +// --------------------------------------------------------------------------- +// Range boundaries. +// --------------------------------------------------------------------------- + +TEST(ParValidation, ThirteenTricksIsAccepted) +{ + // 13 is the largest legal trick count and must not be rejected. + DdTableResults tab = legal_table(); + tab.res_table[0][0] = 13; + tab.res_table[0][2] = 13; + tab.res_table[0][1] = 0; + tab.res_table[0][3] = 0; + + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(&tab, &resp, 0), RETURN_NO_FAULT); +} + +TEST(ParValidation, FourteenTricksIsRejected) +{ + DdTableResults tab = legal_table(); + tab.res_table[2][1] = 14; + + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(&tab, &resp, 0), RETURN_PAR_TABLE_FAULT); +} + +TEST(ParValidation, NegativeTrickCountIsRejected) +{ + DdTableResults tab = legal_table(); + tab.res_table[3][2] = -1; + + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(&tab, &resp, 0), RETURN_PAR_TABLE_FAULT); +} + +TEST(ParValidation, SingleBadEntryAnywhereIsRejected) +{ + // Every position is checked, not just the first. + for (int d = 0; d < DDS_STRAINS; d++) + { + for (int h = 0; h < DDS_HANDS; h++) + { + DdTableResults tab = legal_table(); + tab.res_table[d][h] = 99; + + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(&tab, &resp, 0), RETURN_PAR_TABLE_FAULT) + << "strain " << d << ", hand " << h; + } + } +} + +TEST(ParValidation, NullTableIsRejected) +{ + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(nullptr, &resp, 0), RETURN_PAR_TABLE_FAULT); +} + +// --------------------------------------------------------------------------- +// Legal input still works — the guard must not reject valid tables. +// --------------------------------------------------------------------------- + +TEST(ParValidation, LegalTableStillProducesAParResult) +{ + DdTableResults const tab = legal_table(); + + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + ASSERT_EQ(Par(&tab, &resp, 0), RETURN_NO_FAULT); + EXPECT_GT(std::strlen(resp.par_score[0]), 0u); + EXPECT_LT(std::strlen(resp.par_score[0]), sizeof(resp.par_score[0])); + EXPECT_LT(std::strlen(resp.par_contracts_string[0]), + sizeof(resp.par_contracts_string[0])); +} + +TEST(ParValidation, LegalTableAcceptedByAllEntryPoints) +{ + DdTableResults const tab = legal_table(); + + ParResultsDealer sides[2]; + std::memset(sides, 0, sizeof(sides)); + EXPECT_EQ(SidesPar(&tab, sides, 0), RETURN_NO_FAULT); + + ParResultsMaster sidesBin[2]; + std::memset(sidesBin, 0, sizeof(sidesBin)); + EXPECT_EQ(SidesParBin(&tab, sidesBin, 0), RETURN_NO_FAULT); + + ParResultsDealer dealerRes; + std::memset(&dealerRes, 0, sizeof(dealerRes)); + EXPECT_EQ(DealerPar(&tab, &dealerRes, 0, 0), RETURN_NO_FAULT); + + ParResultsMaster dealerBin; + std::memset(&dealerBin, 0, sizeof(dealerBin)); + EXPECT_EQ(DealerParBin(&tab, &dealerBin, 0, 0), RETURN_NO_FAULT); +} + +TEST(ParValidation, AllLegalVulnerabilitiesAccepted) +{ + DdTableResults const tab = legal_table(); + for (int vul = 0; vul <= 3; vul++) + { + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(DealerPar(&tab, &resp, 0, vul), RETURN_NO_FAULT) + << "vulnerable = " << vul; + } +} + +// --------------------------------------------------------------------------- +// DealerPar indexes VUL_LOOKUP[4][2] by `vulnerable`, so it must be +// range-checked rather than only compared against, as SidesParBin() does. +// --------------------------------------------------------------------------- + +TEST(ParValidation, DealerParRejectsOutOfRangeVulnerability) +{ + DdTableResults const tab = legal_table(); + + for (int vul : {-1, 4, 99}) + { + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(DealerPar(&tab, &resp, 0, vul), RETURN_UNKNOWN_FAULT) + << "vulnerable = " << vul; + } +} + +// --------------------------------------------------------------------------- +// DealerPar() propagates `dealer` into the par tables via pno_list[], where +// sacrifice_as_text() used to subscript with static_cast(pno) -- +// turning a negative index into a multi-gigabyte offset. Found by the par +// fuzz harness within 50000 runs, after the `vulnerable` fix above had +// guarded one parameter of the pair and missed the other. +// --------------------------------------------------------------------------- + +TEST(ParValidation, DealerParRejectsOutOfRangeDealer) +{ + DdTableResults const tab = legal_table(); + + for (int dealer : {-1, 4, 99, -2147483647}) + { + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(DealerPar(&tab, &resp, dealer, 0), RETURN_UNKNOWN_FAULT) + << "dealer = " << dealer; + } +} + +TEST(ParValidation, DealerParBinRejectsOutOfRangeDealer) +{ + DdTableResults const tab = legal_table(); + + for (int dealer : {-1, 4}) + { + ParResultsMaster resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(DealerParBin(&tab, &resp, dealer, 0), RETURN_UNKNOWN_FAULT) + << "dealer = " << dealer; + } +} + +TEST(ParValidation, AllLegalDealersAccepted) +{ + DdTableResults const tab = legal_table(); + + for (int dealer = 0; dealer <= 3; dealer++) + { + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(DealerPar(&tab, &resp, dealer, 0), RETURN_NO_FAULT) + << "dealer = " << dealer; + } +} + +TEST(ParValidation, SacrificeContractTextIsWellFormed) +{ + // A table where sacrificing is right, so the text path that used to index + // NUMBER_TO_PLAYER out of bounds actually runs. No "?" placeholder should + // appear: that would mean an index escaped DealerPar()'s range checks. + DdTableResults tab; + std::memset(&tab, 0, sizeof(tab)); + for (int d = 0; d < DDS_STRAINS; d++) + { + tab.res_table[d][0] = 12; + tab.res_table[d][1] = 1; + tab.res_table[d][2] = 12; + tab.res_table[d][3] = 1; + } + + for (int dealer = 0; dealer <= 3; dealer++) + { + ParResultsDealer resp; + std::memset(&resp, 0, sizeof(resp)); + ASSERT_EQ(DealerPar(&tab, &resp, dealer, 0), RETURN_NO_FAULT); + + for (int k = 0; k < resp.number; k++) + { + std::string const contract(resp.contracts[k]); + EXPECT_EQ(contract.find('?'), std::string::npos) + << "dealer " << dealer << " contract " << k << ": " << contract; + EXPECT_LT(contract.size(), sizeof(resp.contracts[k])); + } + } +} + +// --------------------------------------------------------------------------- +// SidesParBin() only compares against `vulnerable` rather than indexing with +// it, so an out-of-range value was memory-safe but silently computed the par +// result as "none vulnerable" -- and Par()/SidesPar() returned +// RETURN_NO_FAULT while DealerPar() rejected the same input. +// --------------------------------------------------------------------------- + +TEST(ParValidation, DirectEntryPointsRejectOutOfRangeVulnerability) +{ + DdTableResults const tab = legal_table(); + + for (int vul : {-1, 4, 99}) + { + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + EXPECT_EQ(Par(&tab, &resp, vul), RETURN_UNKNOWN_FAULT) + << "Par, vulnerable = " << vul; + + ParResultsDealer sides[2]; + std::memset(sides, 0, sizeof(sides)); + EXPECT_EQ(SidesPar(&tab, sides, vul), RETURN_UNKNOWN_FAULT) + << "SidesPar, vulnerable = " << vul; + + ParResultsMaster sides_bin[2]; + std::memset(sides_bin, 0, sizeof(sides_bin)); + EXPECT_EQ(SidesParBin(&tab, sides_bin, vul), RETURN_UNKNOWN_FAULT) + << "SidesParBin, vulnerable = " << vul; + } +} + +TEST(ParValidation, VulnerabilityActuallyChangesTheParScore) +{ + // Guards against the check above being satisfied by a stub: the four legal + // vulnerabilities must not all produce identical output. A sacrifice table + // is used because that is where doubled undertricks make vulnerability + // change the score; a flat partscore table scores the same either way. + DdTableResults tab; + std::memset(&tab, 0, sizeof(tab)); + for (int d = 0; d < DDS_STRAINS; d++) + { + tab.res_table[d][0] = 12; + tab.res_table[d][1] = 1; + tab.res_table[d][2] = 12; + tab.res_table[d][3] = 1; + } + + std::string first; + bool differs = false; + for (int vul = 0; vul <= 3; vul++) + { + ParResults resp; + std::memset(&resp, 0, sizeof(resp)); + ASSERT_EQ(Par(&tab, &resp, vul), RETURN_NO_FAULT) << "vulnerable " << vul; + + std::string const score(resp.par_score[0]); + if (vul == 0) + first = score; + else if (score != first) + differs = true; + } + EXPECT_TRUE(differs) << "par score identical across all vulnerabilities"; +} + +// --------------------------------------------------------------------------- +// The new code is wired into the error-message table. +// --------------------------------------------------------------------------- + +TEST(ParValidation, ErrorMessageDescribesTableFault) +{ + char line[80]; + std::memset(line, 0, sizeof(line)); + ErrorMessage(RETURN_PAR_TABLE_FAULT, line); + + EXPECT_STREQ(line, TEXT_PAR_TABLE_FAULT); + EXPECT_GT(std::strlen(line), 0u); +} + +} // namespace diff --git a/python/src/bindings.cpp b/python/src/bindings.cpp index 571effa67..c009c854a 100644 --- a/python/src/bindings.cpp +++ b/python/src/bindings.cpp @@ -44,6 +44,14 @@ auto throw_on_dds_error(const int code) -> void case RETURN_MODE_WRONG_HI: case RETURN_NO_SUIT: case RETURN_TOO_MANY_TABLES: + // Deal validation on the CalcDDtable*/CalcAllTables* paths, and the par + // table check. All describe malformed user data, and the docstrings for + // calc_dd_table and the par functions promise ValueError for exactly + // these cases. + case RETURN_CARD_COUNT: + case RETURN_DUPLICATE_CARDS: + case RETURN_SUIT_OR_RANK: + case RETURN_PAR_TABLE_FAULT: throw py::value_error(error_text); default: // All other errors are treated as solver/runtime failures. @@ -113,7 +121,8 @@ auto register_solve_bindings(py::module_& module) -> void "Returns:\n" " dict: Result dict with keys 'nodes', 'cards', 'suit', 'rank', 'equals', 'score'.\n\n" "Raises:\n" - " ValueError: If input validation fails (invalid suit/rank range).\n" + " ValueError: If input validation fails (invalid suit/rank range,\n" + " card count, or duplicate cards).\n" " RuntimeError: If DDS solver returns error code.\n\n" "Example (with context reuse for multiple boards):\n" " context = dds3.SolverContext()\n" diff --git a/python/tests/test_calc_par.py b/python/tests/test_calc_par.py index 4b37d3553..c65d80f2e 100644 --- a/python/tests/test_calc_par.py +++ b/python/tests/test_calc_par.py @@ -161,6 +161,36 @@ def test_calc_par_from_table_vulnerability_variations(self) -> None: self.assertTrue(isinstance(par_result, dict)) self.assertTrue("par_score" in par_result) + def test_calc_par_from_table_invalid_table_raises_value_error(self) -> None: + """An out-of-range table entry is bad user input, so ValueError. + + RETURN_PAR_TABLE_FAULT is returned natively by the par entry points. + Without it in the binding's input-validation mapping it surfaced as + RuntimeError, contradicting this function's documented contract. + """ + for bad in (14, -1, 2000000000): + table = {"res_table": [[7, 6, 7, 6] for _ in range(5)]} + table["res_table"][2][1] = bad + with self.assertRaises(ValueError): + calc_par_from_table(table, vulnerable=0) + + def test_calc_dd_table_invalid_deal_raises_value_error(self) -> None: + """A deal whose hands hold unequal numbers of cards is bad input. + + table_deal_checks() reports it as RETURN_CARD_COUNT, which must map to + ValueError to match the documented "invalid card distribution" case. + """ + table_deal = self._hand0_table_deal() + # Drop one card from north, leaving the other three hands untouched. + for suit in range(4): + if table_deal["cards"][0][suit]: + lowest = table_deal["cards"][0][suit] & -table_deal["cards"][0][suit] + table_deal["cards"][0][suit] &= ~lowest + break + + with self.assertRaises(ValueError): + calc_dd_table(table_deal) + def test_calc_par_from_table_invalid_vulnerability(self) -> None: """Test that invalid vulnerability raises ValueError.""" table_deal = self._hand0_table_deal() diff --git a/python/tests/test_solve_board.py b/python/tests/test_solve_board.py index 58102731e..8b3063884 100644 --- a/python/tests/test_solve_board.py +++ b/python/tests/test_solve_board.py @@ -43,12 +43,14 @@ def test_solve_board_with_defaults(self) -> None: "current_trick_suit": (0, 0, 0), "current_trick_rank": (0, 0, 0), } - # Should not raise, error handling is DDS-side + # This deal gives north all thirteen spades and the other hands + # nothing, so DDS rejects it with RETURN_CARD_COUNT. That is an + # input-validation error, so it surfaces as ValueError; earlier it + # fell through the binding's mapping to RuntimeError. try: result = solve_board(deal) self.assertIn("nodes", result) - except RuntimeError: - # Invalid deal may raise RuntimeError + except ValueError: pass def test_solve_board_invalid_trump(self) -> None: