From 2abb260e5f22cd6597f2e4c5f9e721e36139428a Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 11:09:27 +0100 Subject: [PATCH 01/11] Validate double dummy table in the par API The par entry points derived contract levels directly from DdTableResults::res_table and formatted them into fixed-size character buffers without ever checking that the trick counts were legal. A table containing large out-of-range values overflowed those buffers: - Par() smashed the stack via strcat into `char temp[8]` (par.cpp:121), reproducible under ASan as a stack-buffer-overflow in Par+0xccc. - SidesPar() silently wrote a 26-character string into the 10-byte field ParResultsDealer::contracts[0] -- an intra-object overflow ASan does not instrument by default. Both returned RETURN_NO_FAULT. Legal tables (0-13) were unaffected, and CalcDDtable() only ever emits legal tables, so the normal CalcDDtable -> Par flow could not reach this. Par, SidesPar, SidesParBin, DealerPar and DealerParBin are all exported, however, so a caller that hand-builds or deserialises a table could. Add par_table_checks() and call it from the two chokepoints the par entry points funnel through: both SidesParBin variants and DealerPar. Introduce RETURN_PAR_TABLE_FAULT (-401) following the existing RETURN_*/TEXT_* convention, wire it into ErrorMessage() and mirror it in DdsStatus.java. This makes the par API consistent with SolveBoard, which already validates its input thoroughly. Also fix an out-of-bounds read found alongside it: DealerPar indexed VUL_LOOKUP[4][2] with an unvalidated `vulnerable`. SidesParBin only compares against that parameter, so DealerPar was the sole indexing site. Regression tests in library/tests/par_validation_test.cpp cover the original trigger through all five entry points, the 13/14 and negative boundaries, every res_table position, a null table, the vulnerability range, and that legal tables still produce par results. Verified by reverting the source fix with the tests in place: the suite aborts with the original ASan overflow, and passes with the fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- jni/java/org/dds/ffm/DdsStatus.java | 3 + library/src/api/dll.h | 4 + library/src/dealer_par.cpp | 9 + library/src/init.cpp | 3 + library/src/par.cpp | 6 + library/src/par_validate.hpp | 41 +++++ library/tests/BUILD.bazel | 16 ++ library/tests/par_validation_test.cpp | 244 ++++++++++++++++++++++++++ 8 files changed, 326 insertions(+) create mode 100644 library/src/par_validate.hpp create mode 100644 library/tests/par_validation_test.cpp 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/api/dll.h b/library/src/api/dll.h index dc83dc836..2be61dfa2 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 "Double dummy table entry outside the range 0 to 13" + /** diff --git a/library/src/dealer_par.cpp b/library/src/dealer_par.cpp index 5bf204765..7b9892061 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,14 @@ 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; + + /* vulnerable indexes VUL_LOOKUP below, so it must be range-checked and + not merely compared against, as it is in SidesParBin(). */ + if (vulnerable < 0 || vulnerable > 3) + return RETURN_UNKNOWN_FAULT; + int const * vul_by_side = VUL_LOOKUP[vulnerable]; data_type data; list_type list[2][DDS_STRAINS]; diff --git a/library/src/init.cpp b/library/src/init.cpp index c9900ad11..cf2d87a0a 100644 --- a/library/src/init.cpp +++ b/library/src/init.cpp @@ -488,6 +488,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..9c52e80d3 100644 --- a/library/src/par.cpp +++ b/library/src/par.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -227,6 +228,8 @@ 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; int denom_conv[5] = { 4, 0, 1, 2, 3 }; /* Preallocate for efficiency. These hold result from last direction @@ -698,6 +701,9 @@ int STDCALL SidesParBin( int vulnerable) { + if (int const check = par_table_checks(tablep); 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..ac56b7f76 --- /dev/null +++ b/library/src/par_validate.hpp @@ -0,0 +1,41 @@ +/* + 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; +} diff --git a/library/tests/BUILD.bazel b/library/tests/BUILD.bazel index ff30e5092..0a4ddb45b 100644 --- a/library/tests/BUILD.bazel +++ b/library/tests/BUILD.bazel @@ -18,6 +18,7 @@ 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 ], ), ) @@ -79,6 +80,21 @@ cc_test( ], ) +# 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/par_validation_test.cpp b/library/tests/par_validation_test.cpp new file mode 100644 index 000000000..0c72ed553 --- /dev/null +++ b/library/tests/par_validation_test.cpp @@ -0,0 +1,244 @@ +/// @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 + +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; + } +} + +// --------------------------------------------------------------------------- +// 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 From 90ecaa0648b19017f68d538688460013cd761ba6 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 11:34:14 +0100 Subject: [PATCH 02/11] Add fuzz harnesses for the input-handling surfaces Fuzzing was the one gap in the project's memory-safety tooling: ASan, TSan and UBSan already run in CI, but nothing generated inputs to drive them. Add libFuzzer harnesses for the four surfaces that consume caller- or file-supplied data: convert_from_pbn(), CalcDDtablePBN(), SolveBoard() and the par entry points. Each harness is exposed two ways. A cc_test replays the checked-in seed corpus through it, which needs no libFuzzer and so runs on every platform and under --config=asan/ubsan -- that is what keeps a fixed bug fixed, since a reproducer added to corpus/ becomes a permanent regression seed. A libFuzzer cc_binary, tagged manual and built only under the new --config=fuzz, is for actual campaigns. --config=fuzz uses the registered hermetic LLVM toolchain, which ships libclang_rt.fuzzer. Apple's does not, so the config deliberately does not chain --config=asan: on macOS that switches to the Xcode toolchain and the link fails. On Linux the two combine. The harnesses found three defects on their first runs. None is fixed here; reproducers and analysis are in library/tests/fuzz/findings/README.md, kept out of corpus/ so the replay tests stay green. 01 CalcDDtable()/CalcDDtablePBN() do not check that the four hands hold equal numbers of cards, so a 51-card deal reaches the search and reads 14248 bytes past rel_rank_storage. SolveBoard() rejects the same deal via board_value_checks(). Reachable from an ordinary truncated PBN file containing only legal characters; convert_from_pbn() silently skipping unrecognised characters is a second way in. 02 DealerPar() does not validate `dealer`. A negative value reaches sacrifice_as_text(), where static_cast(pno) turns -1 into 4294967295 and indexes a std::string array far out of bounds. 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. 03 DumpInput() indexes card_suit[], card_hand[] and card_rank[] with the very values board_range_checks() is rejecting as out of range, so the error path itself reads out of bounds. Compiled in unless DDS_NO_DUMP_ON_ERROR is defined, which the build does not define. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- .bazelrc | 15 ++ library/src/BUILD.bazel | 1 + library/tests/fuzz/BUILD.bazel | 54 +++++++ library/tests/fuzz/README.md | 72 +++++++++ library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp | 45 ++++++ .../corpus/calc_dd_table_pbn/bad_compass.txt | 1 + .../corpus/calc_dd_table_pbn/east_first.txt | 1 + .../fuzz/corpus/calc_dd_table_pbn/empty.txt | 0 .../corpus/calc_dd_table_pbn/extra_dots.txt | 1 + .../corpus/calc_dd_table_pbn/lowercase.txt | 1 + .../corpus/calc_dd_table_pbn/no_colon.txt | 1 + .../corpus/calc_dd_table_pbn/north_first.txt | 1 + .../corpus/calc_dd_table_pbn/south_first.txt | 1 + .../corpus/calc_dd_table_pbn/truncated.txt | 1 + .../corpus/calc_dd_table_pbn/void_suits.txt | 1 + .../corpus/calc_dd_table_pbn/west_first.txt | 1 + .../tests/fuzz/corpus/par/all_thirteen.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/par/all_zero.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/par/int_min.bin | Bin 0 -> 81 bytes .../tests/fuzz/corpus/par/legal_balanced.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/par/legal_slam.bin | Bin 0 -> 81 bytes .../tests/fuzz/corpus/par/legal_vul_both.bin | Bin 0 -> 81 bytes .../tests/fuzz/corpus/par/legal_vul_ew.bin | Bin 0 -> 81 bytes .../tests/fuzz/corpus/par/legal_vul_ns.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/par/mixed_edge.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/par/negative.bin | 1 + .../fuzz/corpus/par/overflow_regression.bin | Bin 0 -> 81 bytes library/tests/fuzz/corpus/pbn/bad_compass.txt | 1 + .../fuzz/corpus/pbn/bad_rank_parser_only.txt | 1 + library/tests/fuzz/corpus/pbn/east_first.txt | 1 + library/tests/fuzz/corpus/pbn/empty.txt | 0 library/tests/fuzz/corpus/pbn/extra_dots.txt | 1 + library/tests/fuzz/corpus/pbn/lowercase.txt | 1 + library/tests/fuzz/corpus/pbn/no_colon.txt | 1 + library/tests/fuzz/corpus/pbn/north_first.txt | 1 + library/tests/fuzz/corpus/pbn/south_first.txt | 1 + library/tests/fuzz/corpus/pbn/truncated.txt | 1 + library/tests/fuzz/corpus/pbn/void_suits.txt | 1 + library/tests/fuzz/corpus/pbn/west_first.txt | 1 + .../fuzz/corpus/solve_board/all_zero.bin | Bin 0 -> 97 bytes .../fuzz/corpus/solve_board/legal_notrump.bin | Bin 0 -> 97 bytes .../solve_board/legal_partial_trick.bin | Bin 0 -> 97 bytes .../fuzz/corpus/solve_board/legal_spades.bin | Bin 0 -> 97 bytes .../fuzz/corpus/solve_board/one_card_each.bin | Bin 0 -> 97 bytes .../findings/01_unbalanced_deal_51_cards.txt | 1 + .../findings/01_unbalanced_deal_bad_rank.txt | 1 + .../02_dealer_par_negative_dealer.bin | Bin 0 -> 81 bytes .../03_dump_input_out_of_range_deal.bin | Bin 0 -> 97 bytes library/tests/fuzz/findings/README.md | 142 ++++++++++++++++++ library/tests/fuzz/fuzz.bzl | 55 +++++++ library/tests/fuzz/fuzz_corpus_main.cpp | 108 +++++++++++++ library/tests/fuzz/par_fuzz.cpp | 91 +++++++++++ library/tests/fuzz/pbn_fuzz.cpp | 42 ++++++ library/tests/fuzz/solve_board_fuzz.cpp | 52 +++++++ 54 files changed, 701 insertions(+) create mode 100644 library/tests/fuzz/BUILD.bazel create mode 100644 library/tests/fuzz/README.md create mode 100644 library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/bad_compass.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/east_first.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/empty.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/extra_dots.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/lowercase.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/no_colon.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/north_first.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/south_first.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/truncated.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/void_suits.txt create mode 100644 library/tests/fuzz/corpus/calc_dd_table_pbn/west_first.txt create mode 100644 library/tests/fuzz/corpus/par/all_thirteen.bin create mode 100644 library/tests/fuzz/corpus/par/all_zero.bin create mode 100644 library/tests/fuzz/corpus/par/int_min.bin create mode 100644 library/tests/fuzz/corpus/par/legal_balanced.bin create mode 100644 library/tests/fuzz/corpus/par/legal_slam.bin create mode 100644 library/tests/fuzz/corpus/par/legal_vul_both.bin create mode 100644 library/tests/fuzz/corpus/par/legal_vul_ew.bin create mode 100644 library/tests/fuzz/corpus/par/legal_vul_ns.bin create mode 100644 library/tests/fuzz/corpus/par/mixed_edge.bin create mode 100644 library/tests/fuzz/corpus/par/negative.bin create mode 100644 library/tests/fuzz/corpus/par/overflow_regression.bin create mode 100644 library/tests/fuzz/corpus/pbn/bad_compass.txt create mode 100644 library/tests/fuzz/corpus/pbn/bad_rank_parser_only.txt create mode 100644 library/tests/fuzz/corpus/pbn/east_first.txt create mode 100644 library/tests/fuzz/corpus/pbn/empty.txt create mode 100644 library/tests/fuzz/corpus/pbn/extra_dots.txt create mode 100644 library/tests/fuzz/corpus/pbn/lowercase.txt create mode 100644 library/tests/fuzz/corpus/pbn/no_colon.txt create mode 100644 library/tests/fuzz/corpus/pbn/north_first.txt create mode 100644 library/tests/fuzz/corpus/pbn/south_first.txt create mode 100644 library/tests/fuzz/corpus/pbn/truncated.txt create mode 100644 library/tests/fuzz/corpus/pbn/void_suits.txt create mode 100644 library/tests/fuzz/corpus/pbn/west_first.txt create mode 100644 library/tests/fuzz/corpus/solve_board/all_zero.bin create mode 100644 library/tests/fuzz/corpus/solve_board/legal_notrump.bin create mode 100644 library/tests/fuzz/corpus/solve_board/legal_partial_trick.bin create mode 100644 library/tests/fuzz/corpus/solve_board/legal_spades.bin create mode 100644 library/tests/fuzz/corpus/solve_board/one_card_each.bin create mode 100644 library/tests/fuzz/findings/01_unbalanced_deal_51_cards.txt create mode 100644 library/tests/fuzz/findings/01_unbalanced_deal_bad_rank.txt create mode 100644 library/tests/fuzz/findings/02_dealer_par_negative_dealer.bin create mode 100644 library/tests/fuzz/findings/03_dump_input_out_of_range_deal.bin create mode 100644 library/tests/fuzz/findings/README.md create mode 100644 library/tests/fuzz/fuzz.bzl create mode 100644 library/tests/fuzz/fuzz_corpus_main.cpp create mode 100644 library/tests/fuzz/par_fuzz.cpp create mode 100644 library/tests/fuzz/pbn_fuzz.cpp create mode 100644 library/tests/fuzz/solve_board_fuzz.cpp 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/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/tests/fuzz/BUILD.bazel b/library/tests/fuzz/BUILD.bazel new file mode 100644 index 000000000..fdfa6c452 --- /dev/null +++ b/library/tests/fuzz/BUILD.bazel @@ -0,0 +1,54 @@ +# 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", +) diff --git a/library/tests/fuzz/README.md b/library/tests/fuzz/README.md new file mode 100644 index 000000000..a1898d897 --- /dev/null +++ b/library/tests/fuzz/README.md @@ -0,0 +1,72 @@ +# Fuzz harnesses + +Coverage-guided fuzzing for the four DDS surfaces that consume caller- or +file-supplied data: + +| 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. | + +## 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 +``` + +## 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/` currently holds two open defects — see `findings/README.md`. 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..6366186a0 --- /dev/null +++ b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp @@ -0,0 +1,45 @@ +/* + 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(1); + 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; + std::memcpy(table_deal.cards, data, n); + table_deal.cards[n] = '\0'; + + DdTableResults table; + std::memset(&table, 0, sizeof(table)); + + CalcDDtablePBN(table_deal, &table); + + return 0; +} 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/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/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 0000000000000000000000000000000000000000..d8ed76e320c6f7334e4947314de3be7d01e5b499 GIT binary patch literal 81 Pcmd;OU|`^-A{GV!ED!-0 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..149420a230a2c044c619db6be7ffec88b5ad0a4f GIT binary patch literal 81 McmZQzpf2D7003J63jhEB literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..4e1d0062137c2a5de65de5b336fff0ea349ad117 GIT binary patch literal 81 PcmZQzU}#{VB31 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..40a7d94c1c3ebfdd43aaab816025b7078f067f7e GIT binary patch literal 81 RcmZQ)U|?VaVsAPvLGKsF2mr6>acMd3zS literal 0 HcmV?d00001 diff --git a/library/tests/fuzz/findings/README.md b/library/tests/fuzz/findings/README.md new file mode 100644 index 000000000..88d59b5e3 --- /dev/null +++ b/library/tests/fuzz/findings/README.md @@ -0,0 +1,142 @@ +# Open findings + +Reproducers for defects the harnesses have found that are **not yet fixed**. +They live here rather than in `corpus/` so the corpus-replay tests stay green; +a fuzzing campaign will rediscover them immediately, which is expected. + +Move a file into the matching `corpus/` directory once its defect is fixed, so +it becomes a permanent regression seed. + +## 01 — unbalanced deal reaches the search (CalcDDtable path) + +**Reproduce** + +``` +bazel build --config=asan //library/tests/fuzz:calc_dd_table_pbn_fuzz_corpus_test +./bazel-bin/library/tests/fuzz/calc_dd_table_pbn_fuzz_corpus_test \ + library/tests/fuzz/findings/01_unbalanced_deal_51_cards.txt +``` + +``` +AddressSanitizer: global-buffer-overflow +READ of size 2 ... in QuickTricksPartnerHandNT / weight_alloc_trump0 +0x... is located 14248 bytes after global variable + '(anonymous namespace)::rel_rank_storage' + defined in 'library/src/lookup_tables/lookup_tables.cpp' of size 122880 +``` + +**Cause.** `CalcDDtable()` / `CalcDDtablePBN()` do not validate that the four +hands hold equal numbers of cards. `SolveBoard()` does — `board_value_checks()` +in `solver_if.cpp` returns `RETURN_CARD_COUNT` for exactly this — so the same +malformed deal is rejected safely on that path and only the CalcDDtable path +reaches the search, where the relative-rank index runs off the end of +`rel_rank_storage`. It is an out-of-bounds *read*, not a write. + +**Two ways in, both from an ordinary PBN file:** + +- `01_unbalanced_deal_51_cards.txt` contains **only legal PBN characters** and + is simply one card short (north's spades are `T8`, not `T98`) — the shape a + hand-edited or truncated PBN file naturally takes. +- `01_unbalanced_deal_bad_rank.txt` is the same deal with a card replaced by + `Z`. `convert_from_pbn()` silently ignores any character that is not a card, + `.`, ` ` or a compass letter (`pbn.cpp:113-116`), so the invalid rank is + skipped and the deal is short by one card while the parser still returns + success. + +**Fix sketch.** Two independent changes, either of which closes the crash: + +1. Validate card counts in `CalcDDtable()`, mirroring `board_value_checks()`. + This is the load-bearing fix, since case 1 uses only legal characters. +2. Make `convert_from_pbn()` reject unrecognised characters instead of + skipping them. Do this carefully: PBN text from files often carries + trailing newlines or `\r`, which the current loop tolerates, so tightening + it needs an explicit whitespace allowance to avoid rejecting valid input. + +## 02 — `DealerPar()` does not validate `dealer` + +**Reproduce** + +``` +bazel build --config=asan //library/tests/fuzz:par_fuzz_corpus_test +mkdir -p /tmp/f && cp library/tests/fuzz/findings/02_dealer_par_negative_dealer.bin /tmp/f/ +./bazel-bin/library/tests/fuzz/par_fuzz_corpus_test /tmp/f +``` + +``` +AddressSanitizer: BUS on unknown address (READ) + #5 sacrifice_as_text(int, int, int) + #6 sacrifices_as_text(...) + #7 DealerPar +``` + +**Cause.** `DealerPar()` validates its table and (since `2abb260e`) its +`vulnerable` argument, but not `dealer`. A negative `dealer` propagates into +`pno_list[]` and reaches `dealer_par.cpp:648`: + +```cpp +return NUMBER_TO_CONTRACT[static_cast(no)] + "-" + + NUMBER_TO_PLAYER[static_cast(pno)] + "-" + ... +``` + +The `static_cast` turns `pno == -1` into 4294967295, indexing far +outside the `std::string` array and reading a garbage string object. The +crashing input uses a **legal** `res_table`; only `dealer` is out of range. + +**Fix sketch.** Range-check `dealer` to 0-3 in `DealerPar()` alongside the +existing `vulnerable` check — the header already documents 0 = North .. 3 = +West. The `static_cast` in `sacrifice_as_text()` is worth removing +too: it converts a bounds bug into a wild read rather than a negative index +that ASan or UBSan would flag more clearly. + +**Note.** This is the same class as the `vulnerable` bug fixed by hand in +`2abb260e`; that fix guarded one parameter of the pair and missed the other. +The fuzzer found it within 50000 runs. + +## 03 — `DumpInput()` reads out of bounds while reporting invalid input + +**Reproduce** + +``` +bazel build --config=asan //library/tests/fuzz:solve_board_fuzz_corpus_test +mkdir -p /tmp/f && cp library/tests/fuzz/findings/03_dump_input_out_of_range_deal.bin /tmp/f/ +./bazel-bin/library/tests/fuzz/solve_board_fuzz_corpus_test /tmp/f +``` + +``` +AddressSanitizer: global-buffer-overflow +READ of size 1 + #0 DumpInput(int, Deal const&, int, int, int) + #1 board_range_checks(Deal const&, int, int, int) + #2 solve_board_internal(...) + #4 SolveBoard +``` + +**Cause.** `board_range_checks()` correctly *detects* an out-of-range deal, then +calls `DumpInput()` to log it — and `DumpInput()` indexes the character tables +with the very values it is reporting as invalid (`dump.cpp:288-298`): + +```cpp +fout << card_suit[dl.trump] << "\n"; // card_suit[DDS_STRAINS] == [5] +fout << "first=" << card_hand[dl.first] << "\n"; // card_hand[4] + ... card_suit[dl.currentTrickSuit[k]] + ... card_rank[dl.currentTrickRank[k]] // card_rank[16] +``` + +The reproducer uses `currentTrickSuit = {7,7,7}` and `currentTrickRank = +{99,99,99}`, so `card_rank[99]` reads well past a 16-byte array. `trump` and +`first` are indexed the same way on the lines above. + +**Reach.** `DumpInput()` is compiled in unless `DDS_NO_DUMP_ON_ERROR` is +defined, and the build does not define it — so this is present in release +builds, on the error path of the main solver entry point. Every `SolveBoard()` +rejection with an out-of-range `trump`, `first`, `currentTrickSuit` or +`currentTrickRank` goes through it. It is an out-of-bounds *read*. + +Worth noting separately: `DumpInput()` also writes `dump.txt` into the process +working directory whenever any input is rejected, which is surprising behaviour +for a library and is a side effect a caller cannot disable at runtime. + +**Fix sketch.** Bounds-check each index in `DumpInput()` before using it as a +table subscript, printing the raw integer when it is out of range — the value +is being reported *because* it is invalid, so it must never be trusted as an +index. Consider also making the `dump.txt` side effect opt-in. 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..a2b8bce3e --- /dev/null +++ b/library/tests/fuzz/fuzz_corpus_main.cpp @@ -0,0 +1,108 @@ +/* + 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 + +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 { + +auto run_one(std::filesystem::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. SetMaxThreads) 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::filesystem::path const root(argv[i]); + std::error_code ec; + + if (std::filesystem::is_directory(root, ec)) + { + for (auto const & entry : + std::filesystem::recursive_directory_iterator(root, ec)) + { + if (!entry.is_regular_file()) + continue; + ok = run_one(entry.path()) && ok; + files++; + } + } + else if (std::filesystem::is_regular_file(root, ec)) + { + ok = run_one(root) && ok; + files++; + } + else + { + std::fprintf(stderr, "no such corpus path: %s\n", argv[i]); + ok = false; + } + } + + // 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..3a218704c --- /dev/null +++ b/library/tests/fuzz/par_fuzz.cpp @@ -0,0 +1,91 @@ +/* + 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; + 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..263206eb2 --- /dev/null +++ b/library/tests/fuzz/pbn_fuzz.cpp @@ -0,0 +1,42 @@ +/* + 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; + + std::string const deal(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..7835c2fac --- /dev/null +++ b/library/tests/fuzz/solve_board_fuzz.cpp @@ -0,0 +1,52 @@ +/* + 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 +{ + // One worker keeps runs deterministic and avoids per-input thread setup. + SetMaxThreads(1); + 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; +} From bc32e072ed9a0c8f4f75161bd8c00009f96c106c Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 11:35:37 +0100 Subject: [PATCH 03/11] Add SECURITY.md documenting the input trust model DDS is an in-process library with no network surface, and it assumes trusted, well-formed input. That assumption has always been implicit. For a library with Python, Java, .NET and WebAssembly bindings, whose deployments the maintainers do not control, stating it explicitly is a real control rather than a substitute for one: it lets consumers judge their own exposure and design against a contract instead of a guess. Document what is actually validated and what is not -- SolveBoard() validates thoroughly, the par entry points partially, CalcDDtable() not at all for card counts, and convert_from_pbn() skips unrecognised characters -- along with guidance for anyone exposing the library to untrusted input, and the sanitizer and fuzzing tooling available. Note the WebAssembly build as the one deployment where the sandbox contains memory errors by construction, and point at library/tests/fuzz/findings/README.md for the open defects. Those are documented openly because consumers need them to assess their own risk and because all are out-of-bounds reads reachable only through the input paths described, not remote code execution in any supported deployment. Reporting goes through GitHub's private vulnerability advisories, so no new contact address is introduced. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- SECURITY.md | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 SECURITY.md diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..bcc366eea --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,114 @@ +# Security Policy + +## What DDS is, for threat-modelling purposes + +DDS is an **in-process library**, not a service. It opens no sockets, crosses no +privilege boundary, and keeps no persistent state between calls. 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. + +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()`, added after an out-of-range table was found to + overflow a fixed character buffer) but not every scalar parameter. +- `CalcDDtable()` and `CalcDDtablePBN()` do **not** check that the four hands + hold equal numbers of cards, so a malformed deal can reach the search. +- `convert_from_pbn()` silently ignores characters it does not recognise + rather than rejecting the string. + +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 + +Open memory-safety defects found by the fuzz harnesses are tracked, with +reproducers and analysis, in +[`library/tests/fuzz/findings/README.md`](library/tests/fuzz/findings/README.md). + +They are documented openly because DDS is a library whose consumers need the +information to judge their own exposure, and because all of them are +out-of-bounds *reads* reachable only through the input paths described above — +not remote code execution in any supported deployment. If that assessment is +wrong for your deployment, please tell us. + +## 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. +2. **Prefer `SolveBoard()`** over the `CalcDDtable*` entry points where you + have the choice: its validation is the most complete. +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. +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 the four +input-handling surfaces: + +``` +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. From f2793eb2469305a5eebf7efc14f4464bb1287b8e Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 12:59:12 +0100 Subject: [PATCH 04/11] Fix four out-of-bounds reads found by the fuzz harnesses All four are out-of-bounds reads reachable from the documented input paths. Three were reported when the harnesses landed; the fourth was found while fuzzing the fixes for the first three. Each reproducer moves from library/tests/fuzz/findings/ into the matching corpus/ directory, so it is now a permanent regression seed, and each gains a unit test. 01 CalcDDtable(), CalcDDtablePBN() and CalcAllTables*() 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. Reachable from an ordinary PBN file short of a card -- the reproducer contains only legal PBN characters. Add table_deal_checks(), enforcing the same three rules SolveBoard() already applied via board_value_checks(), so nothing is rejected that the solver would have accepted. 02 DealerPar() did not validate `dealer`. A negative value reached sacrifice_as_text(), where NUMBER_TO_PLAYER[static_cast(pno)] turned -1 into 4294967295. Range-check `dealer` alongside `vulnerable`, and replace the unsigned casts with guarded contract_text()/player_text() helpers: the cast is what turned a detectable bug into a wild read. 03 DumpInput() indexed card_suit[5], card_hand[4] and card_rank[16] with the very values board_range_checks() was rejecting as out of range, so the error path read out of bounds. Present in release builds, since DumpInput() is compiled in unless DDS_NO_DUMP_ON_ERROR is defined. Add suit_text()/hand_text()/rank_text(), which fall back to printing the raw integer -- more useful in a diagnostic than a wrong character. 04 board_range_checks() validates currentTrickSuit[k] only when the matching rank is non-zero, but hand_rel_first derives from the card count, not from the trick entries. A five-card deal with all trick ranks zero gives hand_rel_first == 3, so board_value_checks() indexed remainCards with an unchecked suit. Validate it inside that loop, where it is used as a subscript, so only inputs that would genuinely have been read out of bounds are rejected. Verified by reverting the source fixes with the tests in place: the suite fails with the original ASan reports and passes with the fixes. Campaigns after the fixes total roughly 1.35 million executions across the four harnesses with no crashes. SECURITY.md is updated -- its account of what each entry point validates was written before these fixes and no longer described the code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- SECURITY.md | 33 +-- library/src/calc_tables.cpp | 20 ++ library/src/dealer_par.cpp | 39 ++- library/src/dump.cpp | 41 ++- library/src/solver_if.cpp | 11 + library/src/table_deal_validate.hpp | 74 ++++++ library/tests/BUILD.bazel | 16 ++ library/tests/deal_input_validation_test.cpp | 235 ++++++++++++++++ library/tests/fuzz/README.md | 10 +- .../calc_dd_table_pbn/bad_rank.txt} | 0 .../unbalanced_51_cards.txt} | 0 .../par/regression_negative_dealer.bin} | Bin .../regression_out_of_range_deal.bin} | Bin .../regression_unchecked_trick_suit.bin | Bin 0 -> 97 bytes library/tests/fuzz/findings/README.md | 251 ++++++++---------- library/tests/par_validation_test.cpp | 79 ++++++ 16 files changed, 640 insertions(+), 169 deletions(-) create mode 100644 library/src/table_deal_validate.hpp create mode 100644 library/tests/deal_input_validation_test.cpp rename library/tests/fuzz/{findings/01_unbalanced_deal_bad_rank.txt => corpus/calc_dd_table_pbn/bad_rank.txt} (100%) rename library/tests/fuzz/{findings/01_unbalanced_deal_51_cards.txt => corpus/calc_dd_table_pbn/unbalanced_51_cards.txt} (100%) rename library/tests/fuzz/{findings/02_dealer_par_negative_dealer.bin => corpus/par/regression_negative_dealer.bin} (100%) rename library/tests/fuzz/{findings/03_dump_input_out_of_range_deal.bin => corpus/solve_board/regression_out_of_range_deal.bin} (100%) create mode 100644 library/tests/fuzz/corpus/solve_board/regression_unchecked_trick_suit.bin diff --git a/SECURITY.md b/SECURITY.md index bcc366eea..8d5edd1e3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -35,27 +35,29 @@ is uneven across entry points: 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()`, added after an out-of-range table was found to - overflow a fixed character buffer) but not every scalar parameter. -- `CalcDDtable()` and `CalcDDtablePBN()` do **not** check that the four hands - hold equal numbers of cards, so a malformed deal can reach the search. + (`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()` and `CalcAllTables*()` 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. - `convert_from_pbn()` silently ignores characters it does not recognise - rather than rejecting the string. + 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 -Open memory-safety defects found by the fuzz harnesses are tracked, with -reproducers and analysis, in -[`library/tests/fuzz/findings/README.md`](library/tests/fuzz/findings/README.md). +There are currently **no known unfixed memory-safety defects**. -They are documented openly because DDS is a library whose consumers need the -information to judge their own exposure, and because all of them are -out-of-bounds *reads* reachable only through the input paths described above — -not remote code execution in any supported deployment. If that assessment is -wrong for your deployment, please tell us. +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 @@ -63,8 +65,9 @@ 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. -2. **Prefer `SolveBoard()`** over the `CalcDDtable*` entry points where you - have the choice: its validation is the most complete. +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 diff --git a/library/src/calc_tables.cpp b/library/src/calc_tables.cpp index 901163878..ce345ab74 100644 --- a/library/src/calc_tables.cpp +++ b/library/src/calc_tables.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -191,6 +192,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; @@ -281,6 +285,13 @@ 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; @@ -463,6 +474,15 @@ int STDCALL CalcAllTablesX( 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. diff --git a/library/src/dealer_par.cpp b/library/src/dealer_par.cpp index 7b9892061..11ef3329f 100644 --- a/library/src/dealer_par.cpp +++ b/library/src/dealer_par.cpp @@ -190,11 +190,15 @@ int STDCALL DealerPar( if (int const check = par_table_checks(tablep); check != RETURN_NO_FAULT) return check; - /* vulnerable indexes VUL_LOOKUP below, so it must be range-checked and - not merely compared against, as it is in SidesParBin(). */ + /* Both parameters reach array subscripts: vulnerable indexes VUL_LOOKUP + below, and dealer propagates into the par tables via pno_list[]. Neither + can be merely compared against, as vulnerable is in SidesParBin(). */ if (vulnerable < 0 || vulnerable > 3) return RETURN_UNKNOWN_FAULT; + 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]; @@ -624,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, @@ -636,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)); } @@ -650,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 eb7419c70..c9fc1cda4 100644 --- a/library/src/dump.cpp +++ b/library/src/dump.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include "dump.hpp" #include @@ -267,6 +268,38 @@ 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. */ + +auto suit_text(const int suit) -> std::string +{ + if (suit < 0 || suit >= DDS_STRAINS) + return "?(" + std::to_string(suit) + ")"; + return std::string(1, static_cast(card_suit[suit])); +} + +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])); +} + +auto rank_text(const int rank) -> std::string +{ + constexpr int card_rank_size = 16; + if (rank < 0 || rank >= card_rank_size) + return "?(" + std::to_string(rank) + ")"; + return std::string(1, static_cast(card_rank[rank])); +} + +} // namespace + + int DumpInput( const int errCode, const Deal& dl, @@ -285,8 +318,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 << suit_text(dl.trump) << "\n"; + fout << "first=" << hand_text(dl.first) << "\n"; unsigned short ranks[4][4]; @@ -294,8 +327,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=" << 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/solver_if.cpp b/library/src/solver_if.cpp index f8fa05533..078fd2bf4 100644 --- a/library/src/solver_if.cpp +++ b/library/src/solver_if.cpp @@ -1141,6 +1141,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..fe3829cc9 --- /dev/null +++ b/library/src/table_deal_validate.hpp @@ -0,0 +1,74 @@ +/* + 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. + * + * Reachable through CalcDDtable(), CalcDDtablePBN() and CalcAllTables*(), so + * in particular through any PBN file that is short of a card. + * + * @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 0a4ddb45b..43cd9b6f3 100644 --- a/library/tests/BUILD.bazel +++ b/library/tests/BUILD.bazel @@ -19,6 +19,7 @@ filegroup( "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 ], ), ) @@ -80,6 +81,21 @@ 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", + "@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( diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp new file mode 100644 index 000000000..7d4e62bab --- /dev/null +++ b/library/tests/deal_input_validation_test.cpp @@ -0,0 +1,235 @@ +/// @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 + +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); +} + +// --------------------------------------------------------------------------- +// 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, 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); +} + +} // namespace diff --git a/library/tests/fuzz/README.md b/library/tests/fuzz/README.md index a1898d897..98c08e7fd 100644 --- a/library/tests/fuzz/README.md +++ b/library/tests/fuzz/README.md @@ -43,6 +43,13 @@ 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. + ## Harness contract Each harness defines `LLVMFuzzerTestOneInput()` and `LLVMFuzzerInitialize()`. @@ -69,4 +76,5 @@ harness bug as a library bug. `findings/README.md` so the corpus tests stay green. 4. Once fixed, move it into the matching `corpus/` directory. -`findings/` currently holds two open defects — see `findings/README.md`. +`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/findings/01_unbalanced_deal_bad_rank.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/bad_rank.txt similarity index 100% rename from library/tests/fuzz/findings/01_unbalanced_deal_bad_rank.txt rename to library/tests/fuzz/corpus/calc_dd_table_pbn/bad_rank.txt diff --git a/library/tests/fuzz/findings/01_unbalanced_deal_51_cards.txt b/library/tests/fuzz/corpus/calc_dd_table_pbn/unbalanced_51_cards.txt similarity index 100% rename from library/tests/fuzz/findings/01_unbalanced_deal_51_cards.txt rename to library/tests/fuzz/corpus/calc_dd_table_pbn/unbalanced_51_cards.txt diff --git a/library/tests/fuzz/findings/02_dealer_par_negative_dealer.bin b/library/tests/fuzz/corpus/par/regression_negative_dealer.bin similarity index 100% rename from library/tests/fuzz/findings/02_dealer_par_negative_dealer.bin rename to library/tests/fuzz/corpus/par/regression_negative_dealer.bin diff --git a/library/tests/fuzz/findings/03_dump_input_out_of_range_deal.bin b/library/tests/fuzz/corpus/solve_board/regression_out_of_range_deal.bin similarity index 100% rename from library/tests/fuzz/findings/03_dump_input_out_of_range_deal.bin rename to library/tests/fuzz/corpus/solve_board/regression_out_of_range_deal.bin 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 0000000000000000000000000000000000000000..97ccb27d5ca3930739ce539c4a9bcde76c7b3cc1 GIT binary patch literal 97 ecmZQ!Kmv(KY;-0gLcjsRgtIWj6o67d!HEDi0s+_n literal 0 HcmV?d00001 diff --git a/library/tests/fuzz/findings/README.md b/library/tests/fuzz/findings/README.md index 88d59b5e3..94749793f 100644 --- a/library/tests/fuzz/findings/README.md +++ b/library/tests/fuzz/findings/README.md @@ -1,142 +1,109 @@ -# Open findings - -Reproducers for defects the harnesses have found that are **not yet fixed**. -They live here rather than in `corpus/` so the corpus-replay tests stay green; -a fuzzing campaign will rediscover them immediately, which is expected. - -Move a file into the matching `corpus/` directory once its defect is fixed, so -it becomes a permanent regression seed. - -## 01 — unbalanced deal reaches the search (CalcDDtable path) - -**Reproduce** - -``` -bazel build --config=asan //library/tests/fuzz:calc_dd_table_pbn_fuzz_corpus_test -./bazel-bin/library/tests/fuzz/calc_dd_table_pbn_fuzz_corpus_test \ - library/tests/fuzz/findings/01_unbalanced_deal_51_cards.txt -``` - -``` -AddressSanitizer: global-buffer-overflow -READ of size 2 ... in QuickTricksPartnerHandNT / weight_alloc_trump0 -0x... is located 14248 bytes after global variable - '(anonymous namespace)::rel_rank_storage' - defined in 'library/src/lookup_tables/lookup_tables.cpp' of size 122880 -``` - -**Cause.** `CalcDDtable()` / `CalcDDtablePBN()` do not validate that the four -hands hold equal numbers of cards. `SolveBoard()` does — `board_value_checks()` -in `solver_if.cpp` returns `RETURN_CARD_COUNT` for exactly this — so the same -malformed deal is rejected safely on that path and only the CalcDDtable path -reaches the search, where the relative-rank index runs off the end of -`rel_rank_storage`. It is an out-of-bounds *read*, not a write. - -**Two ways in, both from an ordinary PBN file:** - -- `01_unbalanced_deal_51_cards.txt` contains **only legal PBN characters** and - is simply one card short (north's spades are `T8`, not `T98`) — the shape a - hand-edited or truncated PBN file naturally takes. -- `01_unbalanced_deal_bad_rank.txt` is the same deal with a card replaced by - `Z`. `convert_from_pbn()` silently ignores any character that is not a card, - `.`, ` ` or a compass letter (`pbn.cpp:113-116`), so the invalid rank is - skipped and the deal is short by one card while the parser still returns - success. - -**Fix sketch.** Two independent changes, either of which closes the crash: - -1. Validate card counts in `CalcDDtable()`, mirroring `board_value_checks()`. - This is the load-bearing fix, since case 1 uses only legal characters. -2. Make `convert_from_pbn()` reject unrecognised characters instead of - skipping them. Do this carefully: PBN text from files often carries - trailing newlines or `\r`, which the current loop tolerates, so tightening - it needs an explicit whitespace allowance to avoid rejecting valid input. - -## 02 — `DealerPar()` does not validate `dealer` - -**Reproduce** - -``` -bazel build --config=asan //library/tests/fuzz:par_fuzz_corpus_test -mkdir -p /tmp/f && cp library/tests/fuzz/findings/02_dealer_par_negative_dealer.bin /tmp/f/ -./bazel-bin/library/tests/fuzz/par_fuzz_corpus_test /tmp/f -``` - -``` -AddressSanitizer: BUS on unknown address (READ) - #5 sacrifice_as_text(int, int, int) - #6 sacrifices_as_text(...) - #7 DealerPar -``` - -**Cause.** `DealerPar()` validates its table and (since `2abb260e`) its -`vulnerable` argument, but not `dealer`. A negative `dealer` propagates into -`pno_list[]` and reaches `dealer_par.cpp:648`: - -```cpp -return NUMBER_TO_CONTRACT[static_cast(no)] + "-" + - NUMBER_TO_PLAYER[static_cast(pno)] + "-" + ... -``` - -The `static_cast` turns `pno == -1` into 4294967295, indexing far -outside the `std::string` array and reading a garbage string object. The -crashing input uses a **legal** `res_table`; only `dealer` is out of range. - -**Fix sketch.** Range-check `dealer` to 0-3 in `DealerPar()` alongside the -existing `vulnerable` check — the header already documents 0 = North .. 3 = -West. The `static_cast` in `sacrifice_as_text()` is worth removing -too: it converts a bounds bug into a wild read rather than a negative index -that ASan or UBSan would flag more clearly. - -**Note.** This is the same class as the `vulnerable` bug fixed by hand in -`2abb260e`; that fix guarded one parameter of the pair and missed the other. -The fuzzer found it within 50000 runs. - -## 03 — `DumpInput()` reads out of bounds while reporting invalid input - -**Reproduce** - -``` -bazel build --config=asan //library/tests/fuzz:solve_board_fuzz_corpus_test -mkdir -p /tmp/f && cp library/tests/fuzz/findings/03_dump_input_out_of_range_deal.bin /tmp/f/ -./bazel-bin/library/tests/fuzz/solve_board_fuzz_corpus_test /tmp/f -``` - -``` -AddressSanitizer: global-buffer-overflow -READ of size 1 - #0 DumpInput(int, Deal const&, int, int, int) - #1 board_range_checks(Deal const&, int, int, int) - #2 solve_board_internal(...) - #4 SolveBoard -``` - -**Cause.** `board_range_checks()` correctly *detects* an out-of-range deal, then -calls `DumpInput()` to log it — and `DumpInput()` indexes the character tables -with the very values it is reporting as invalid (`dump.cpp:288-298`): - -```cpp -fout << card_suit[dl.trump] << "\n"; // card_suit[DDS_STRAINS] == [5] -fout << "first=" << card_hand[dl.first] << "\n"; // card_hand[4] - ... card_suit[dl.currentTrickSuit[k]] - ... card_rank[dl.currentTrickRank[k]] // card_rank[16] -``` - -The reproducer uses `currentTrickSuit = {7,7,7}` and `currentTrickRank = -{99,99,99}`, so `card_rank[99]` reads well past a 16-byte array. `trump` and -`first` are indexed the same way on the lines above. - -**Reach.** `DumpInput()` is compiled in unless `DDS_NO_DUMP_ON_ERROR` is -defined, and the build does not define it — so this is present in release -builds, on the error path of the main solver entry point. Every `SolveBoard()` -rejection with an out-of-range `trump`, `first`, `currentTrickSuit` or -`currentTrickRank` goes through it. It is an out-of-bounds *read*. - -Worth noting separately: `DumpInput()` also writes `dump.txt` into the process -working directory whenever any input is rejected, which is surprising behaviour -for a library and is a side effect a caller cannot disable at runtime. - -**Fix sketch.** Bounds-check each index in `DumpInput()` before using it as a -table subscript, printing the raw integer when it is out of range — the value -is being reported *because* it is invalid, so it must never be trusted as an -index. Consider also making the `dump.txt` side effect opt-in. +# 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`). diff --git a/library/tests/par_validation_test.cpp b/library/tests/par_validation_test.cpp index 0c72ed553..890ac2bf4 100644 --- a/library/tests/par_validation_test.cpp +++ b/library/tests/par_validation_test.cpp @@ -15,6 +15,7 @@ #include #include +#include #include namespace { @@ -227,6 +228,84 @@ TEST(ParValidation, DealerParRejectsOutOfRangeVulnerability) } } +// --------------------------------------------------------------------------- +// 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])); + } + } +} + // --------------------------------------------------------------------------- // The new code is wired into the error-message table. // --------------------------------------------------------------------------- From 861a57be0a77b8a8feee4eb92db48755968a906b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 14:04:59 +0100 Subject: [PATCH 05/11] Fix fuzz corpus tests on Windows and under UBSan Two CI failures, both in the new harnesses rather than in the library. Windows: all four corpus tests reported "corpus resolved to 0 files". The replay driver resolved the corpus directory relative to the working directory, which only works where Bazel builds a runfiles symlink tree. Windows disables those by default and supplies RUNFILES_MANIFEST_FILE instead, so nothing was found. Resolve through the runfiles the same way library/tests/test_dtest_nothing_makes.py already does: prefer RUNFILES_DIR or TEST_SRCDIR, fall back to the manifest, and keep the plain filesystem path last so running a harness by hand from the repository root still works. The manifest branch is verified by pointing RUNFILES_MANIFEST_FILE at a generated manifest with the runfiles tree variables unset and the working directory outside the repository -- the Windows configuration. The "resolved to 0 files" guard is what turned this into a clear CI failure rather than a test that silently checked nothing, so it stays. UBSan on Linux: calc_dd_table_pbn_fuzz.cpp called memcpy with a null source and a zero length, which libFuzzer produces and glibc declares nonnull. Guard the empty case. Two other harnesses had the same latent issue -- constructing a std::string from (nullptr, 0) in pbn_fuzz.cpp, and the reader in par_fuzz.cpp -- so guard those too. macOS does not mark these arguments nonnull, which is why the local UBSan run passed. Also merges upstream/develop, which has gained CalcDDtable support for deals with fewer than 13 cards. table_deal_checks() requires the four hands to hold *equal* numbers of cards rather than exactly 13, so the new calc_dd_table_partial_test (one card per hand) passes unchanged. Verified on the merged tree: 56/56 //library/... plain, ASan and UBSan; 19 python, 5 jni, 1 utilities; 550000 further fuzz executions with no crashes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp | 6 +- library/tests/fuzz/fuzz_corpus_main.cpp | 145 +++++++++++++++--- library/tests/fuzz/par_fuzz.cpp | 2 + library/tests/fuzz/pbn_fuzz.cpp | 6 +- 4 files changed, 138 insertions(+), 21 deletions(-) diff --git a/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp index 6366186a0..db84d6d46 100644 --- a/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp +++ b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp @@ -33,7 +33,11 @@ extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int size_t const n = size < sizeof(table_deal.cards) - 1 ? size : sizeof(table_deal.cards) - 1; - std::memcpy(table_deal.cards, data, n); + // 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; diff --git a/library/tests/fuzz/fuzz_corpus_main.cpp b/library/tests/fuzz/fuzz_corpus_main.cpp index a2b8bce3e..a462b06cc 100644 --- a/library/tests/fuzz/fuzz_corpus_main.cpp +++ b/library/tests/fuzz/fuzz_corpus_main.cpp @@ -17,8 +17,10 @@ #include #include #include +#include #include #include +#include #include extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int; @@ -30,7 +32,122 @@ extern "C" auto LLVMFuzzerInitialize(int * argc, char *** argv) -> int; namespace { -auto run_one(std::filesystem::path const & path) -> bool +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) @@ -70,30 +187,20 @@ auto main(int argc, char ** argv) -> int for (int i = 1; i < argc; i++) { - std::filesystem::path const root(argv[i]); - std::error_code ec; + std::vector const found = corpus_files(argv[i]); - if (std::filesystem::is_directory(root, ec)) + if (found.empty()) { - for (auto const & entry : - std::filesystem::recursive_directory_iterator(root, ec)) - { - if (!entry.is_regular_file()) - continue; - ok = run_one(entry.path()) && ok; - files++; - } + std::fprintf(stderr, "no corpus files under: %s\n", argv[i]); + ok = false; + continue; } - else if (std::filesystem::is_regular_file(root, ec)) + + for (fs::path const & path : found) { - ok = run_one(root) && ok; + ok = run_one(path) && ok; files++; } - else - { - std::fprintf(stderr, "no such corpus path: %s\n", argv[i]); - ok = false; - } } // A corpus that silently resolves to nothing would make this test vacuous. diff --git a/library/tests/fuzz/par_fuzz.cpp b/library/tests/fuzz/par_fuzz.cpp index 3a218704c..1c02df27d 100644 --- a/library/tests/fuzz/par_fuzz.cpp +++ b/library/tests/fuzz/par_fuzz.cpp @@ -31,6 +31,8 @@ class Reader { 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; diff --git a/library/tests/fuzz/pbn_fuzz.cpp b/library/tests/fuzz/pbn_fuzz.cpp index 263206eb2..367e15d70 100644 --- a/library/tests/fuzz/pbn_fuzz.cpp +++ b/library/tests/fuzz/pbn_fuzz.cpp @@ -33,7 +33,11 @@ extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int if (size > 4096) return 0; - std::string const deal(reinterpret_cast(data), size); + // 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); From 65286a9436ba0398043efaf057fa4e39163dc384 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 16:50:23 +0100 Subject: [PATCH 06/11] address PR review feedback All six inline comments on #349. Two identified real gaps in the earlier fixes, one of them a stack-buffer-overflow *write*. - SECURITY.md: "keeps no persistent state between calls" was wrong. DDS carries process-local solver resources that outlive a call -- the transposition table and per-thread memory behind SetResources()/ FreeMemory(), the SetMaxThreads() budget, and a worker pool held in a function-local static (parallel_boards.cpp:230). Describe those, and note the two consequences for a threat model: calls are not isolated from one another, and the budgets are process-wide. - doc/dll-description.{md,html}: add the -401 RETURN_PAR_TABLE_FAULT row; the table still ended at -301 while dll.h points consumers at it. - calc_tables.cpp: bound no_of_tables before it is used. CalcAllTablesPBNN() converted that many records into a fixed MAXNOOFTABLES * DDS_STRAINS local before any validation ran -- confirmed under ASan as a stack-buffer-overflow WRITE in convert_from_pbn(), the first write among these findings rather than a read. CalcAllTablesN()'s capacity check multiplied by count first, which can overflow signed int and wrap past the check, so bound the count ahead of the multiply; CalcAllTablesX() is heap-backed and uncapped by design, so guard only the product. - par.cpp: SidesParBin() accepted out-of-range `vulnerable`. It only compares against the value, so this was memory-safe, but Par() and SidesPar() returned RETURN_NO_FAULT with a result computed as "none vulnerable" while DealerPar() rejected the same input. Both variants now validate, through a shared par_vulnerable_checks() that DealerPar() also uses. - calc_dd_table.cpp: the C++ calc_dd_table(ctx, ...) overload built Boards directly and never called table_deal_checks(), so a one-card-short deal still reached the search through it and through the dds_c_* shims. It is the chokepoint the context-free and PBN overloads delegate to, so the guard goes there; tests cover the C++ and shim paths. - table_deal_validate.hpp: the doc comment claimed CalcAllTables* coverage without noting that it validates one deal and not the batch count. Say which entry points apply it, and that bounding no_of_tables is separate. Tests: 10 new cases across deal_input_validation_test.cpp and par_validation_test.cpp, each verified by reverting its guard and confirming the failure. 56/56 //library/... plain, ASan and UBSan; 19 python, 5 jni, 1 utilities. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- SECURITY.md | 26 ++-- doc/dll-description.html | 3 + doc/dll-description.md | 3 + library/src/calc_dd_table.cpp | 7 ++ library/src/calc_tables.cpp | 27 ++++ library/src/dealer_par.cpp | 8 +- library/src/par.cpp | 8 ++ library/src/par_validate.hpp | 22 ++++ library/src/table_deal_validate.hpp | 12 +- library/tests/BUILD.bazel | 1 + library/tests/deal_input_validation_test.cpp | 125 +++++++++++++++++++ library/tests/par_validation_test.cpp | 63 ++++++++++ 12 files changed, 292 insertions(+), 13 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 8d5edd1e3..a7f89c558 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,10 +2,20 @@ ## What DDS is, for threat-modelling purposes -DDS is an **in-process library**, not a service. It opens no sockets, crosses no -privilege boundary, and keeps no persistent state between calls. 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. +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 `SetResources()` and `FreeMemory()`, a thread budget set by +`SetMaxThreads()`, 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. In practice this means two things for a threat model: +corruption caused by one call can be observed by a later one, and the memory +and thread budgets are process-wide, so one component's `SetResources()` choice +applies to every other user of the library in that process. 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 @@ -38,9 +48,11 @@ is uneven across entry points: (`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()` and `CalcAllTables*()` 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. +- `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. The batch entry points also + range-check `no_of_tables` against the fixed capacity of their arrays. - `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 diff --git a/doc/dll-description.html b/doc/dll-description.html index 7fb9ac01c..1cfa75ffa 100644 --- a/doc/dll-description.html +++ b/doc/dll-description.html @@ -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 a double dummy table entry is outside the range 0 to 13. + diff --git a/doc/dll-description.md b/doc/dll-description.md index 8b5726f0b..2911e500a 100644 --- a/doc/dll-description.md +++ b/doc/dll-description.md @@ -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 a double dummy table entry is 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 49d2976c1..360430ab0 100644 --- a/library/src/calc_tables.cpp +++ b/library/src/calc_tables.cpp @@ -9,6 +9,7 @@ #include "calc_tables.hpp" #include +#include #include #include #include @@ -321,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; @@ -441,6 +453,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) @@ -555,6 +577,11 @@ int STDCALL CalcAllTablesX( // 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. + // Uncapped by design (this is the large-batch path), but the product + // must not overflow signed int before it sizes the vectors below. + if (numDeals > std::numeric_limits::max() / included) + return RETURN_TOO_MANY_TABLES; + const int nboards = numDeals * included; std::vector boards(static_cast(nboards)); std::vector> scores(static_cast(nboards)); diff --git a/library/src/dealer_par.cpp b/library/src/dealer_par.cpp index 11ef3329f..d110ea8a6 100644 --- a/library/src/dealer_par.cpp +++ b/library/src/dealer_par.cpp @@ -191,10 +191,10 @@ int STDCALL DealerPar( return check; /* Both parameters reach array subscripts: vulnerable indexes VUL_LOOKUP - below, and dealer propagates into the par tables via pno_list[]. Neither - can be merely compared against, as vulnerable is in SidesParBin(). */ - if (vulnerable < 0 || vulnerable > 3) - return RETURN_UNKNOWN_FAULT; + 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; diff --git a/library/src/par.cpp b/library/src/par.cpp index 9c52e80d3..a40f4d3ec 100644 --- a/library/src/par.cpp +++ b/library/src/par.cpp @@ -231,6 +231,10 @@ int STDCALL SidesParBin( 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 (N-S or E-W) examined. */ @@ -704,6 +708,10 @@ int STDCALL SidesParBin( 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 index ac56b7f76..d324b57e3 100644 --- a/library/src/par_validate.hpp +++ b/library/src/par_validate.hpp @@ -39,3 +39,25 @@ inline auto par_table_checks(DdTableResults const * tablep) -> int 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/table_deal_validate.hpp b/library/src/table_deal_validate.hpp index fe3829cc9..3c8e28c49 100644 --- a/library/src/table_deal_validate.hpp +++ b/library/src/table_deal_validate.hpp @@ -26,8 +26,16 @@ * - no card appears in more than one hand, * - all four hands hold the same number of cards. * - * Reachable through CalcDDtable(), CalcDDtablePBN() and CalcAllTables*(), so - * in particular through any PBN file that is short of a card. + * 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 diff --git a/library/tests/BUILD.bazel b/library/tests/BUILD.bazel index 43cd9b6f3..0abcf766b 100644 --- a/library/tests/BUILD.bazel +++ b/library/tests/BUILD.bazel @@ -92,6 +92,7 @@ cc_test( local_defines = DDS_LOCAL_DEFINES, deps = [ "//library/src:dds", + "//library/src/api:dds_c_api", "@googletest//:gtest_main", ], ) diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp index 7d4e62bab..5463a756a 100644 --- a/library/tests/deal_input_validation_test.cpp +++ b/library/tests/deal_input_validation_test.cpp @@ -18,8 +18,11 @@ #include #include +#include #include #include +#include +#include namespace { @@ -151,6 +154,128 @@ TEST(CalcTableValidation, CalcAllTablesRejectsUnbalancedDeal) 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, 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. // --------------------------------------------------------------------------- diff --git a/library/tests/par_validation_test.cpp b/library/tests/par_validation_test.cpp index 890ac2bf4..15f5fb158 100644 --- a/library/tests/par_validation_test.cpp +++ b/library/tests/par_validation_test.cpp @@ -306,6 +306,69 @@ TEST(ParValidation, SacrificeContractTextIsWellFormed) } } +// --------------------------------------------------------------------------- +// 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. // --------------------------------------------------------------------------- From 30b021bdaa0e8abf4d9bc2e3dac8db0aafb4dc40 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 17:28:00 +0100 Subject: [PATCH 07/11] Add a fuzz harness for the batch table entry points The four existing harnesses each drive a single deal, so none of them exercised how CalcAllTables*() handles a caller-supplied deal count. That is exactly where CalcAllTablesPBNN() copied no_of_tables records into a fixed-size local before validating it -- a stack-buffer-overflow write that code review caught and fuzzing did not. This closes that gap. The harness drives CalcAllTablesN(), CalcAllTablesPBNN() and CalcAllTablesX(). It distinguishes the two kinds of count involved: no_of_tables is a field inside a fixed-capacity struct, so any value is legitimate fuzzer input and the library must bound it, and the harness passes it through verbatim; CalcAllTablesX()'s count describes a caller-allocated array, so the harness allocates exactly what it declares and caps it. Two details are load-bearing, and both cost a round to get right: - 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 left them zeroed and did not catch the bug it exists for. - The fill deal holds one card per hand rather than a full 52. This harness targets count and batch handling, not search depth, and a full deal in every slot drops throughput from ~75000 executions in four minutes to ~1700. Verified by removing each count guard in turn: without the CalcAllTablesPBNN bound the harness reports the original stack-buffer-overflow WRITE in convert_from_pbn(), and without the CalcAllTablesN bound it reports a heap-buffer-overflow READ inside table_deal_checks() itself -- the read past dealsp->deals that review predicted. Both restore to green. 17 seeds covering hostile, negative and capacity-boundary counts, legal batches, filter edge cases and a real PBN batch. A 240s campaign completed 75487 executions with no crashes. 57/57 //library/... plain, ASan and UBSan; 25 python/jni/utilities. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- library/tests/fuzz/BUILD.bazel | 6 + library/tests/fuzz/README.md | 21 ++ library/tests/fuzz/calc_all_tables_fuzz.cpp | 209 ++++++++++++++++++ .../calc_all_tables/all_strains_filtered.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_at_capacity.bin | Bin 0 -> 12 bytes .../corpus/calc_all_tables/bin_count_huge.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_count_int_min.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_count_just_over.bin | Bin 0 -> 12 bytes .../bin_count_mul_overflow.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_count_negative.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_one_legal_deal.bin | Bin 0 -> 12 bytes .../calc_all_tables/bin_two_legal_deals.bin | Bin 0 -> 12 bytes .../corpus/calc_all_tables/bin_with_par.bin | Bin 0 -> 12 bytes .../calc_all_tables/pbn_count_just_over.bin | Bin 0 -> 12 bytes .../calc_all_tables/pbn_count_overflow.bin | Bin 0 -> 12 bytes .../calc_all_tables/pbn_two_real_deals.bin | Bin 0 -> 170 bytes .../corpus/calc_all_tables/x_count_huge.bin | Bin 0 -> 12 bytes .../calc_all_tables/x_count_negative.bin | Bin 0 -> 12 bytes .../corpus/calc_all_tables/x_two_deals.bin | Bin 0 -> 12 bytes .../corpus/calc_all_tables/zero_tables.bin | Bin 0 -> 12 bytes 20 files changed, 236 insertions(+) create mode 100644 library/tests/fuzz/calc_all_tables_fuzz.cpp create mode 100644 library/tests/fuzz/corpus/calc_all_tables/all_strains_filtered.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_at_capacity.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_count_huge.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_count_int_min.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_count_just_over.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_count_mul_overflow.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_count_negative.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_one_legal_deal.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_two_legal_deals.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/bin_with_par.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/pbn_count_just_over.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/pbn_count_overflow.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/pbn_two_real_deals.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/x_count_huge.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/x_count_negative.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/x_two_deals.bin create mode 100644 library/tests/fuzz/corpus/calc_all_tables/zero_tables.bin diff --git a/library/tests/fuzz/BUILD.bazel b/library/tests/fuzz/BUILD.bazel index fdfa6c452..3ff33e914 100644 --- a/library/tests/fuzz/BUILD.bazel +++ b/library/tests/fuzz/BUILD.bazel @@ -52,3 +52,9 @@ dds_fuzz_harness( 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 index 98c08e7fd..5f9de6cfe 100644 --- a/library/tests/fuzz/README.md +++ b/library/tests/fuzz/README.md @@ -9,6 +9,7 @@ file-supplied data: | `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 @@ -50,6 +51,26 @@ bazel run --config=fuzz --config=asan //library/tests/fuzz:solve_board_fuzz -- \ > 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. + +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. + ## Harness contract Each harness defines `LLVMFuzzerTestOneInput()` and `LLVMFuzzerInitialize()`. 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..70b6721b0 --- /dev/null +++ b/library/tests/fuzz/calc_all_tables_fuzz.cpp @@ -0,0 +1,209 @@ +/* + 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 +{ + // One worker keeps runs deterministic and avoids per-input thread setup. + SetMaxThreads(1); + 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/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 0000000000000000000000000000000000000000..a373a3fd2be458f3f8a325f0609181a30e1130b4 GIT binary patch literal 12 PcmZQ#U|?Vb0|o{F07U=^ literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..eac653455c961c1fe4a1a1b484a60c2658e4c312 GIT binary patch literal 12 RcmX@Xz`(!=1PqJ}3;+p<0L}ma literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..3bffd268c5e0a7072e7d02b47e5612d441a51c42 GIT binary patch literal 12 QcmZRWVp`4s0*nld01T}GLjV8( literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..3e51ac2bb059b89226743c0e04acecc8e4d07353 GIT binary patch literal 12 QcmZQzU}#_f0Y(N!00fW#g8%>k literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..8747d0166aaa31a0532a6bd0fb6c82da08021e09 GIT binary patch literal 12 NcmX@f00oQ;OaKXe0L=gZ literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..58a9520cdf4d8dd307d36cc03b63e155a4cd84cc GIT binary patch literal 12 QcmZQbJuAxq0*nld01a*dQUCw| literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..0af02cc3b3800b2cbea20a30b34d371aeba70e48 GIT binary patch literal 12 RcmezW|Nnmm5MX3r1OO@K1OEU3 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..2e6c8a5ae49da5e5766ca0322992c513e387f11d GIT binary patch literal 12 RcmZQ%U|?Vb0tQ9~1^@sx00#g7 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..72f559d1da83299e5f27c8da09c4fa382598d84a GIT binary patch literal 12 RcmZQ#U|?Vb0tQ9~1^@s-00;m8 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..a581cc8578996102f750bc7415545922717f8d7d GIT binary patch literal 12 NcmZQ#fC4541^@sm00jU5 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..653cf394191706c9841bc80133df5d7cda3954e1 GIT binary patch literal 12 NcmX@f00oSUOaKXg0L}ma literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..d2c689e207da88eca7fa1021a8de3a6cac012332 GIT binary patch literal 12 PcmZ3`#>fB$jEqbG4POB) literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..7c302a3a01a8d64f64723a6b4d05c3a9e879b759 GIT binary patch literal 170 zcmb`9u?@f=3DA7cOj literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..418021d40a0fc86008e9efc234d789f9cd145d45 GIT binary patch literal 12 QcmZRWVp`4s0*p+I01UAKMF0Q* literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..46adce0c0f2d5f9d626f2d99cdf9c1098d3eef1b GIT binary patch literal 12 RcmezU|Nnmm5MX3t1OO?z1N{I1 literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..adee929e172ca4aaa41d0361c95cd1837816b2ff GIT binary patch literal 12 RcmZQ#U|?Vb0tQAV1^@s>015yA literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..e7f1dd580a3dc018751250c4b9040bd9d0d653a2 GIT binary patch literal 12 NcmZQzfC5Ga1^@sL00IC2 literal 0 HcmV?d00001 From d3271b76aad2d0c75703e50a5a2e9f486cc0a845 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 17:44:51 +0100 Subject: [PATCH 08/11] Fix CalcAllTablesN solving an uninitialised board when given no deals The new calc_all_tables harness found this on its first CI run, under MemorySanitizer. Boards bo is an uninitialised stack local. The board count came from a lastIndex variable initialised to 0 and assigned only inside the board-building loop, so bo.no_of_boards = lastIndex + 1 claimed one board even when no_of_tables was 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. Return RETURN_NO_FAULT early for zero deals, matching what CalcAllTablesX() already did, and take the count from ind -- the number of boards actually written -- rather than from a last-index variable that starts at a valid-looking 0. ASan and UBSan do not detect uninitialised reads and MSan is Linux x86_64 only, so this cannot be reproduced on macOS; the local sweep is 57/57 under plain, ASan and UBSan, and CI's msan job is the check that matters here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- library/src/calc_tables.cpp | 13 ++++++++--- library/tests/deal_input_validation_test.cpp | 20 ++++++++++++++++ library/tests/fuzz/README.md | 5 +++- library/tests/fuzz/findings/README.md | 24 ++++++++++++++++++++ 4 files changed, 58 insertions(+), 4 deletions(-) diff --git a/library/src/calc_tables.cpp b/library/src/calc_tables.cpp index 360430ab0..126fb8ba4 100644 --- a/library/src/calc_tables.cpp +++ b/library/src/calc_tables.cpp @@ -361,9 +361,14 @@ int STDCALL CalcAllTablesN( } 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--) @@ -387,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) diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp index 5463a756a..28b312fe9 100644 --- a/library/tests/deal_input_validation_test.cpp +++ b/library/tests/deal_input_validation_test.cpp @@ -255,6 +255,26 @@ TEST(CalcTableValidation, CalcAllTablesRejectsNegativeTableCount) 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 diff --git a/library/tests/fuzz/README.md b/library/tests/fuzz/README.md index 5f9de6cfe..72a9d833b 100644 --- a/library/tests/fuzz/README.md +++ b/library/tests/fuzz/README.md @@ -58,7 +58,10 @@ 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. +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: diff --git a/library/tests/fuzz/findings/README.md b/library/tests/fuzz/findings/README.md index 94749793f..f18790e64 100644 --- a/library/tests/fuzz/findings/README.md +++ b/library/tests/fuzz/findings/README.md @@ -107,3 +107,27 @@ 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`). From 95aea5f52df704724a88a78cae0ea6307791efea Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 24 Aug 2026 22:02:45 +0100 Subject: [PATCH 09/11] address PR review feedback Seven inline comments from the second review round. One is a real behaviour bug in the fuzz harnesses; the rest are accuracy fixes to documentation and a diagnostic. - dump.cpp: suit_text() was shared between dl.trump, where 4 is a legal no-trump, and currentTrickSuit, where board_range_checks() accepts only 0..3. An invalid trick suit of 4 therefore printed as "N" and hid the value that had been rejected -- the opposite of the helper's purpose. Split into trump_text() and trick_suit_text() over a shared bound-taking helper, with a regression test. - Fuzz harnesses: SetMaxThreads(1) capped nothing. It is a deprecated alias of InitializeStaticMemory() whose argument is ignored, and CalcDDtablePBN() delegates with maxThreads = 0, which selects hardware concurrency -- so every input fanned out across all cores, contrary to the harnesses' own comments. Call InitializeStaticMemory() directly and use CalcDDtablePBNN(..., 1). This makes calc_dd_table_pbn compute-bound: a full deal is 20 solves, and under coverage instrumentation on one worker the hardest corpus seed takes ~36s, which -max_total_time cannot interrupt because it is only checked between runs. That is the right trade for a fuzzer, which parallelises across processes rather than within one input, so the cap stays and the README documents the cost and recommends an explicit -timeout. - fuzz_corpus_main.cpp: include for std::istreambuf_iterator rather than relying on it arriving transitively, which MSVC may not do. - SECURITY.md: SetMaxThreads() was described as setting a thread budget. It sets nothing. Separate the process-wide legacy memory settings from the shared worker pool and the per-call maxThreads of the *N and *X entry points, and say plainly that SetMaxThreads() is deprecated and ignored. - SECURITY.md: the batch entry points do not all bound their count. CalcAllTablesX() and CalcAllTablesPBNX() accept an arbitrary numDeals by design and guard only integer overflow, so their count is caller-bounded. Say so, and add it to the untrusted-input guidance rather than leaving readers to assume a fixed-count limit. - SECURITY.md and library/tests/fuzz/README.md: both said four harnesses; there are five since calc_all_tables landed. 57/57 //library/... plain, ASan and UBSan; 25 python/jni/utilities. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- SECURITY.md | 51 ++++++++++++++----- library/src/dump.cpp | 26 ++++++++-- library/tests/deal_input_validation_test.cpp | 17 +++++++ library/tests/fuzz/README.md | 27 +++++++++- library/tests/fuzz/calc_all_tables_fuzz.cpp | 6 ++- library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp | 9 +++- library/tests/fuzz/fuzz_corpus_main.cpp | 3 +- library/tests/fuzz/solve_board_fuzz.cpp | 6 ++- 8 files changed, 119 insertions(+), 26 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index a7f89c558..9617886ad 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -9,13 +9,24 @@ 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 `SetResources()` and `FreeMemory()`, a thread budget set by -`SetMaxThreads()`, 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. In practice this means two things for a threat model: -corruption caused by one call can be observed by a later one, and the memory -and thread budgets are process-wide, so one component's `SetResources()` choice -applies to every other user of the library in that process. +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 @@ -51,8 +62,15 @@ is uneven across entry points: - `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. The batch entry points also - range-check `no_of_tables` against the fixed capacity of their arrays. + 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 @@ -76,14 +94,20 @@ whose consumers need the information to judge their own exposure. 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. + 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. + 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 @@ -93,8 +117,9 @@ The library was not designed for this. If you must: ## Testing and tooling The project runs AddressSanitizer, ThreadSanitizer and UndefinedBehaviorSanitizer -in CI on Linux and macOS, and carries libFuzzer harnesses for the four -input-handling surfaces: +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/... diff --git a/library/src/dump.cpp b/library/src/dump.cpp index d6a5722a4..80b03898c 100644 --- a/library/src/dump.cpp +++ b/library/src/dump.cpp @@ -278,13 +278,31 @@ namespace { 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. */ -auto suit_text(const int suit) -> std::string +/* 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 >= DDS_STRAINS) + 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) @@ -321,7 +339,7 @@ int DumpInput( if (dl.trump == DDS_NOTRUMP) fout << "N\n"; else - fout << suit_text(dl.trump) << "\n"; + fout << trump_text(dl.trump) << "\n"; fout << "first=" << hand_text(dl.first) << "\n"; unsigned short ranks[4][4]; @@ -330,7 +348,7 @@ int DumpInput( if (dl.currentTrickRank[k] != 0) { fout << "index=" << k << - " currentTrickSuit=" << suit_text(dl.currentTrickSuit[k]) << + " currentTrickSuit=" << trick_suit_text(dl.currentTrickSuit[k]) << " currentTrickRank= " << rank_text(dl.currentTrickRank[k]) << "\n"; } diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp index 28b312fe9..57fc57b66 100644 --- a/library/tests/deal_input_validation_test.cpp +++ b/library/tests/deal_input_validation_test.cpp @@ -317,6 +317,23 @@ TEST(DumpInputSafety, OutOfRangeTrickSuitAndRankAreReportedNotIndexed) 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; diff --git a/library/tests/fuzz/README.md b/library/tests/fuzz/README.md index 72a9d833b..080894837 100644 --- a/library/tests/fuzz/README.md +++ b/library/tests/fuzz/README.md @@ -1,7 +1,7 @@ # Fuzz harnesses -Coverage-guided fuzzing for the four DDS surfaces that consume caller- or -file-supplied data: +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 | |---|---|---| @@ -74,6 +74,29 @@ Two details in that harness are load-bearing: 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()`. diff --git a/library/tests/fuzz/calc_all_tables_fuzz.cpp b/library/tests/fuzz/calc_all_tables_fuzz.cpp index 70b6721b0..b42c82a7a 100644 --- a/library/tests/fuzz/calc_all_tables_fuzz.cpp +++ b/library/tests/fuzz/calc_all_tables_fuzz.cpp @@ -102,8 +102,10 @@ auto legal_deal() -> DdTableDeal extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int { - // One worker keeps runs deterministic and avoids per-input thread setup. - SetMaxThreads(1); + // 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; } diff --git a/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp index db84d6d46..54e0dec7c 100644 --- a/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp +++ b/library/tests/fuzz/calc_dd_table_pbn_fuzz.cpp @@ -21,7 +21,10 @@ extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int { - SetMaxThreads(1); + // 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; } @@ -43,7 +46,9 @@ extern "C" auto LLVMFuzzerTestOneInput(const uint8_t * data, size_t size) -> int DdTableResults table; std::memset(&table, 0, sizeof(table)); - CalcDDtablePBN(table_deal, &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/fuzz_corpus_main.cpp b/library/tests/fuzz/fuzz_corpus_main.cpp index a462b06cc..1abbda63a 100644 --- a/library/tests/fuzz/fuzz_corpus_main.cpp +++ b/library/tests/fuzz/fuzz_corpus_main.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -168,7 +169,7 @@ auto run_one(fs::path const & path) -> bool 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. SetMaxThreads) run unconfigured. + // or harnesses relying on it (e.g. InitializeStaticMemory) run unconfigured. LLVMFuzzerInitialize(&argc, &argv); // Degenerate inputs every harness must survive, independent of the corpus. diff --git a/library/tests/fuzz/solve_board_fuzz.cpp b/library/tests/fuzz/solve_board_fuzz.cpp index 7835c2fac..41cd52770 100644 --- a/library/tests/fuzz/solve_board_fuzz.cpp +++ b/library/tests/fuzz/solve_board_fuzz.cpp @@ -21,8 +21,10 @@ extern "C" auto LLVMFuzzerInitialize(int * /*argc*/, char *** /*argv*/) -> int { - // One worker keeps runs deterministic and avoids per-input thread setup. - SetMaxThreads(1); + // 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; } From 7ab338aeb98ea181387ad86d1c552bef808f1ac5 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Tue, 25 Aug 2026 10:19:05 +0100 Subject: [PATCH 10/11] address suppressed PR review comments All five comments Copilot suppressed as low-confidence. Each was checked against the code and each was correct. - python/src/bindings.cpp: RETURN_PAR_TABLE_FAULT fell through throw_on_dds_error()'s input-validation cases to the default branch, so the par functions raised RuntimeError for exactly the invalid-table inputs whose docstrings promise ValueError. RETURN_CARD_COUNT, RETURN_DUPLICATE_CARDS and RETURN_SUIT_OR_RANK had the same problem and this branch is what made them reachable from calc_dd_table, whose docstring promises ValueError for an invalid card distribution. Map all four. This also reclassifies them for solve_board, where they were already reachable: a malformed deal now raises ValueError rather than RuntimeError. That is a deliberate behaviour change, agreed with the maintainer, and test_solve_board.py's catch is widened to match. The solve_board docstring now names card count and duplicate cards alongside suit/rank. - library/src/api/dll.h: par_table_checks() returns RETURN_PAR_TABLE_FAULT for a null table as well as an out-of-range entry, but TEXT_PAR_TABLE_FAULT described only the entry case, so a null table produced a misleading diagnostic. Reworded, and the error tables in doc/dll-description.{md,html} kept in sync. - library/src/calc_tables.cpp: the deal-by-deal validation scan ran before the board-count overflow check, so a numDeals that was already guaranteed to be rejected still cost O(numDeals) work -- and CalcAllTablesPBNX() allocated and converted that many records first, a memory-exhaustion path. Factor the check into batch_count_preflight(), run it ahead of any per-deal work, and share it with the PBN variant. The regression test drops from 10.8s to 0.9s. - library/src/dump.cpp: rank_text() was bounded by the size of card_rank[] rather than by the legal trick-rank range, so rejected ranks of 1 and 15 rendered as the sentinel characters 'x' and '-' instead of the raw value -- hiding in dump.txt exactly the input that had been refused. Bound by 2..14, with a test that reads dump.txt back. - doc/dll-description.{md,html} and docs/dotnet_interface.md still described SetMaxThreads as limiting the worker count, and even as returning the actual number of threads. It returns void and ignores its argument. Correcting this matters because SECURITY.md now tells readers not to rely on it, and conflicting guidance in the same repository defeats that. 57/57 //library/... plain, ASan and UBSan; 25 python/jni/utilities. Each fix verified by reverting it and confirming the new tests fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0182npkiFY2DCbdranYEhs38 --- doc/dll-description.html | 14 ++--- doc/dll-description.md | 14 ++--- docs/dotnet_interface.md | 11 +++- library/src/api/dll.h | 2 +- library/src/calc_tables.cpp | 54 ++++++++++++++---- library/src/dump.cpp | 10 +++- library/tests/deal_input_validation_test.cpp | 58 ++++++++++++++++++++ python/src/bindings.cpp | 11 +++- python/tests/test_calc_par.py | 30 ++++++++++ python/tests/test_solve_board.py | 8 ++- 10 files changed, 176 insertions(+), 36 deletions(-) diff --git a/doc/dll-description.html b/doc/dll-description.html index 1cfa75ffa..9bb575381 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 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

@@ -1218,7 +1218,7 @@

Return codes

-301RETURN_CHUNK_SIZESolveAllChunks\*(), returned when the chunk size is < 1. --401RETURN_PAR_TABLE_FAULTPar(), SidesPar(), SidesParBin(), DealerPar(), DealerParBin(), returned when a double dummy table entry is outside the range 0 to 13. +-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 2911e500a..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. @@ -1307,7 +1307,7 @@ 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 a double dummy table entry is outside the range 0 to 13. +-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/library/src/api/dll.h b/library/src/api/dll.h index 2be61dfa2..9cd6ce1ba 100644 --- a/library/src/api/dll.h +++ b/library/src/api/dll.h @@ -154,7 +154,7 @@ // Par(), SidesPar(), SidesParBin(), DealerPar(), DealerParBin() #define RETURN_PAR_TABLE_FAULT -401 -#define TEXT_PAR_TABLE_FAULT "Double dummy table entry outside the range 0 to 13" +#define TEXT_PAR_TABLE_FAULT "Missing double dummy table, or an entry outside the range 0 to 13" diff --git a/library/src/calc_tables.cpp b/library/src/calc_tables.cpp index 126fb8ba4..7e69f831f 100644 --- a/library/src/calc_tables.cpp +++ b/library/src/calc_tables.cpp @@ -538,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, @@ -560,13 +591,9 @@ 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) @@ -584,11 +611,7 @@ int STDCALL CalcAllTablesX( // 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. - // Uncapped by design (this is the large-batch path), but the product - // must not overflow signed int before it sizes the vectors below. - if (numDeals > std::numeric_limits::max() / included) - return RETURN_TOO_MANY_TABLES; - + // 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)); @@ -704,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/dump.cpp b/library/src/dump.cpp index 80b03898c..b9cff22c8 100644 --- a/library/src/dump.cpp +++ b/library/src/dump.cpp @@ -310,10 +310,16 @@ auto hand_text(const int hand) -> std::string 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 card_rank_size = 16; - if (rank < 0 || rank >= card_rank_size) + 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])); } diff --git a/library/tests/deal_input_validation_test.cpp b/library/tests/deal_input_validation_test.cpp index 57fc57b66..0cf0703ad 100644 --- a/library/tests/deal_input_validation_test.cpp +++ b/library/tests/deal_input_validation_test.cpp @@ -18,6 +18,9 @@ #include #include +#include +#include +#include #include #include #include @@ -394,4 +397,59 @@ TEST(DumpInputSafety, UncheckedTrickSuitIsNotUsedAsSubscript) 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/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: From 802ac03d0cfcbd7624457659226cf3dc08dbba80 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 27 Aug 2026 13:03:49 +0100 Subject: [PATCH 11/11] fix: clarifies that internal, global threading has been removed. --- doc/dll-description.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/dll-description.html b/doc/dll-description.html index 9bb575381..2348dcf45 100644 --- a/doc/dll-description.html +++ b/doc/dll-description.html @@ -1125,7 +1125,7 @@

Functions

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.

-

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.

+

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.