diff --git a/src/engine.cpp b/src/engine.cpp index db38ef200..c84e91742 100644 --- a/src/engine.cpp +++ b/src/engine.cpp @@ -75,7 +75,8 @@ Engine::Engine(std::optional path) : options.add( // "NumaPolicy", Option("auto", [this](const Option& o) { - set_numa_config_from_option(o); + if (!set_numa_config_from_option(o)) + return "NumaPolicy: invalid value '" + std::string(o) + "', keeping previous config."; return numa_config_information_as_string() + "\n" + thread_allocation_information_as_string(); })); @@ -144,7 +145,8 @@ Engine::Engine(std::optional path) : resize_threads(); } -u64 Engine::perft(const std::string& fen, Depth depth, bool isChess960) { +std::variant +Engine::perft(const std::string& fen, Depth depth, bool isChess960) { verify_network(); return Benchmark::perft(fen, depth, isChess960); @@ -214,7 +216,7 @@ std::optional Engine::set_position(const std::string& // modifiers -void Engine::set_numa_config_from_option(const std::string& o) { +bool Engine::set_numa_config_from_option(const std::string& o) { if (o == "auto" || o == "system") { numaContext.set_numa_config(NumaConfig::from_system(DefaultNumaPolicy)); @@ -230,12 +232,16 @@ void Engine::set_numa_config_from_option(const std::string& o) { } else { - numaContext.set_numa_config(NumaConfig::from_string(o)); + auto parsed = NumaConfig::from_string(o); + if (!parsed.has_value()) + return false; + numaContext.set_numa_config(std::move(*parsed)); } // Force reallocation of threads in case affinities need to change. resize_threads(); threads.ensure_network_replicated(); + return true; } void Engine::resize_threads() { @@ -330,7 +336,7 @@ OptionsMap& Engine::get_options() { return options; } std::string Engine::fen() const { return pos.fen(); } -void Engine::flip() { pos.flip(); } +std::optional Engine::flip() { return pos.flip(); } std::string Engine::visualize() const { std::stringstream ss; diff --git a/src/engine.h b/src/engine.h index e07630277..52c14e978 100644 --- a/src/engine.h +++ b/src/engine.h @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "misc.h" @@ -59,7 +60,7 @@ class Engine { ~Engine() { wait_for_search_finished(); } - u64 perft(const std::string& fen, Depth depth, bool isChess960); + std::variant perft(const std::string& fen, Depth depth, bool isChess960); // non blocking call to start searching void go(Search::LimitsType&); @@ -74,7 +75,7 @@ class Engine { // modifiers - void set_numa_config_from_option(const std::string& o); + bool set_numa_config_from_option(const std::string& o); void resize_threads(); void set_tt_size(usize mb); void set_ponderhit(bool); @@ -103,7 +104,7 @@ class Engine { int get_hashfull(int maxAge = 0) const; std::string fen() const; - void flip(); + std::optional flip(); std::string visualize() const; std::vector> get_bound_thread_count_by_numa_node() const; std::string get_numa_config_as_string() const; diff --git a/src/misc.cpp b/src/misc.cpp index 765a035c7..64f67cff3 100644 --- a/src/misc.cpp +++ b/src/misc.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -543,10 +544,14 @@ CommandLine::CommandLine(int _argc, char** _argv) : } -usize str_to_size_t(const std::string& s) { - unsigned long long value = std::stoull(s); - if (value > std::numeric_limits::max()) - std::exit(EXIT_FAILURE); +std::optional str_to_size_t(const std::string& s) { + if (s.empty() || s[0] == '-') + return std::nullopt; + errno = 0; + char* endptr = nullptr; + const unsigned long long value = std::strtoull(s.c_str(), &endptr, 10); + if (errno == ERANGE || *endptr != '\0' || value > std::numeric_limits::max()) + return std::nullopt; return static_cast(value); } diff --git a/src/misc.h b/src/misc.h index db2c03f1c..9c65d95ca 100644 --- a/src/misc.h +++ b/src/misc.h @@ -133,7 +133,7 @@ void prefetch(const void* addr) { void start_logger(const std::filesystem::path& fname); -usize str_to_size_t(const std::string& s); +std::optional str_to_size_t(const std::string& s); std::string utf8_from_wstring(std::wstring_view s); std::filesystem::path path_from_utf8(const std::string& path); diff --git a/src/numa.h b/src/numa.h index 35f7383a2..732d7a8fb 100644 --- a/src/numa.h +++ b/src/numa.h @@ -664,7 +664,7 @@ class NumaConfig { // ','-separated cpu indices // supports "first-last" range syntax for cpu indices // For example "0-15,128-143:16-31,144-159:32-47,160-175:48-63,176-191" - static NumaConfig from_string(const std::string& s) { + static std::optional from_string(const std::string& s) { NumaConfig cfg = empty(); NumaIndex n = 0; @@ -676,13 +676,16 @@ class NumaConfig { for (auto idx : indices) { if (!cfg.add_cpu_to_node(n, CpuIndex(idx))) - std::exit(EXIT_FAILURE); + return std::nullopt; } n += 1; } } + if (n == 0) // failed to parse any nodes + return std::nullopt; + cfg.customAffinity = true; return cfg; @@ -1041,16 +1044,22 @@ class NumaConfig { auto parts = split(ss, "-"); if (parts.size() == 1) { - const CpuIndex c = CpuIndex{str_to_size_t(std::string(parts[0]))}; - indices.emplace_back(c); + auto c = str_to_size_t(std::string(parts[0])); + if (c.has_value()) + indices.emplace_back(*c); } else if (parts.size() == 2) { - const CpuIndex cfirst = CpuIndex{str_to_size_t(std::string(parts[0]))}; - const CpuIndex clast = CpuIndex{str_to_size_t(std::string(parts[1]))}; - for (usize c = cfirst; c <= clast; ++c) + constexpr usize MaxIndices = 1 << 20; // prevent oom + + auto cfirst = str_to_size_t(std::string(parts[0])); + auto clast = str_to_size_t(std::string(parts[1])); + if (cfirst.has_value() && clast.has_value() && *clast - *cfirst < MaxIndices) { - indices.emplace_back(c); + for (usize c = *cfirst; c <= *clast; ++c) + { + indices.emplace_back(c); + } } } } diff --git a/src/perft.h b/src/perft.h index e3369ff0e..2c18c8b81 100644 --- a/src/perft.h +++ b/src/perft.h @@ -20,6 +20,7 @@ #define PERFT_H_INCLUDED #include +#include #include "movegen.h" #include "position.h" @@ -55,10 +56,13 @@ u64 perft(Position& pos, Depth depth) { return nodes; } -inline u64 perft(const std::string& fen, Depth depth, bool isChess960) { +inline std::variant +perft(const std::string& fen, Depth depth, bool isChess960) { StateInfo st; Position p; - p.set(fen, isChess960, &st); + + if (auto err = p.set(fen, isChess960, &st)) + return {*err}; return perft(p, depth); } diff --git a/src/position.cpp b/src/position.cpp index 0c89e6417..4279f89d4 100644 --- a/src/position.cpp +++ b/src/position.cpp @@ -1593,7 +1593,7 @@ bool Position::upcoming_repetition(int ply) const { // Flips position with the white and black sides reversed. This // is only useful for debugging e.g. for finding evaluation symmetry bugs. -void Position::flip() { +std::optional Position::flip() { string f, token; std::stringstream ss(fen()); @@ -1622,9 +1622,7 @@ void Position::flip() { std::getline(ss, token); // Half and full moves f += token; - set(f, is_chess960(), st); - - assert(pos_is_ok()); + return set(f, is_chess960(), st); } diff --git a/src/position.h b/src/position.h index 7316075cb..f216be4fa 100644 --- a/src/position.h +++ b/src/position.h @@ -180,9 +180,9 @@ class Position { bool dtz_is_dtm() const; // Pawnless && (3-men || 4-men-minors-only) // Position consistency check, for debugging - bool pos_is_ok() const; - bool material_key_is_ok() const; - void flip(); + bool pos_is_ok() const; + bool material_key_is_ok() const; + std::optional flip(); StateInfo* state() const; diff --git a/src/uci.cpp b/src/uci.cpp index c9941b7cf..2e35fe6d5 100644 --- a/src/uci.cpp +++ b/src/uci.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "benchmark.h" @@ -98,6 +99,7 @@ void UCIEngine::loop() { && !getline(std::cin, cmd)) // Wait for an input or an end-of-file (EOF) indication cmd = "quit"; + currentCmd = cmd; std::istringstream is(cmd); token.clear(); // Avoid a stale if getline() returns nothing or a blank line @@ -138,9 +140,13 @@ void UCIEngine::loop() { sync_cout << "readyok" << sync_endl; // Add custom non-UCI commands, mainly for debugging purposes. - // These commands must not be used during a search! else if (token == "flip") - engine.flip(); + { + if (auto err = engine.flip()) + { + terminate_on_critical_error(err->what()); + } + } else if (token == "bench") bench(is); else if (token == BenchmarkCommand) @@ -184,9 +190,13 @@ Search::LimitsType UCIEngine::parse_limits(std::istream& is) { limits.startTime = now(); // The search starts as early as possible while (is >> token) + { if (token == "searchmoves") // Needs to be the last command on the line + { while (is >> token) limits.searchmoves.push_back(to_lower(token)); + break; + } else if (token == "wtime") is >> limits.time[WHITE]; @@ -213,6 +223,10 @@ Search::LimitsType UCIEngine::parse_limits(std::istream& is) { else if (token == "ponder") limits.ponderMode = true; + if (is.fail()) + terminate_on_critical_error("Invalid argument for '" + token + "'"); + } + return limits; } @@ -459,7 +473,11 @@ void UCIEngine::setoption(std::istringstream& is) { } u64 UCIEngine::perft(const Search::LimitsType& limits) { - auto nodes = engine.perft(engine.fen(), limits.perft, engine.get_options()["UCI_Chess960"]); + auto result = engine.perft(engine.fen(), limits.perft, engine.get_options()["UCI_Chess960"]); + if (auto err = std::get_if(&result)) + terminate_on_critical_error(err->what()); + + auto nodes = std::get(result); sync_cout << "\nNodes searched: " << nodes << "\n" << sync_endl; return nodes; } @@ -492,7 +510,7 @@ void UCIEngine::position(std::istringstream& is) { auto err = engine.set_position(fen, moves); if (err.has_value()) { - terminate_on_critical_error(fullCommand, err->what()); + terminate_on_critical_error(err->what()); } } @@ -600,7 +618,8 @@ std::string UCIEngine::move(Move m, bool chess960) { std::string UCIEngine::to_lower(std::string str) { - std::transform(str.begin(), str.end(), str.begin(), [](auto c) { return std::tolower(c); }); + std::transform(str.begin(), str.end(), str.begin(), + [](unsigned char c) { return std::tolower(c); }); return str; } @@ -662,9 +681,8 @@ void UCIEngine::on_bestmove(std::string_view bestmove, std::string_view ponder) std::cout << sync_endl; } -void UCIEngine::terminate_on_critical_error(const std::string& fullCommand, - const std::string& message) { - sync_cout << "info string CRITICAL ERROR: Command `" << fullCommand +void UCIEngine::terminate_on_critical_error(const std::string& message) { + sync_cout << "info string CRITICAL ERROR: Command `" << currentCmd << "` failed. Reason: " << message << '\n' << sync_endl; std::exit(1); diff --git a/src/uci.h b/src/uci.h index 50ce6b013..4d3c4cbbd 100644 --- a/src/uci.h +++ b/src/uci.h @@ -51,13 +51,14 @@ class UCIEngine { static std::string to_lower(std::string str); static Move to_move(const Position& pos, std::string str); - static Search::LimitsType parse_limits(std::istream& is); + Search::LimitsType parse_limits(std::istream& is); auto& engine_options() { return engine.get_options(); } private: Engine engine; CommandLine cli; + std::string currentCmd; static void print_info_string(std::string_view str); @@ -75,8 +76,7 @@ class UCIEngine { void init_search_update_listeners(); - [[noreturn]] void terminate_on_critical_error(const std::string& fullCommand, - const std::string& message); + [[noreturn]] void terminate_on_critical_error(const std::string& message); }; } // namespace Stockfish diff --git a/src/ucioption.cpp b/src/ucioption.cpp index df03338d6..9fa90d175 100644 --- a/src/ucioption.cpp +++ b/src/ucioption.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -34,7 +35,7 @@ bool CaseInsensitiveLess::operator()(const std::string& s1, const std::string& s return std::lexicographical_compare( s1.begin(), s1.end(), s2.begin(), s2.end(), - [](char c1, char c2) { return std::tolower(c1) < std::tolower(c2); }); + [](unsigned char c1, unsigned char c2) { return std::tolower(c1) < std::tolower(c2); }); } void OptionsMap::add_info_listener(InfoListener&& message_func) { info = std::move(message_func); } @@ -144,6 +145,16 @@ bool Option::operator==(const char* s) const { bool Option::operator!=(const char* s) const { return !(*this == s); } +static bool value_in_range(const std::string& v, int min, int max) { + if (v.empty()) + return false; + errno = 0; + char* end = nullptr; + const long long result = std::strtoll(v.c_str(), &end, 10); + if (errno == ERANGE || *end != '\0') + return false; + return result >= min && result <= max; +} // Updates currentValue and triggers on_change() action. It's up to // the GUI to check for option's limits, but we could receive the new value @@ -154,7 +165,7 @@ Option& Option::operator=(const std::string& v) { if ((type != "button" && type != "string" && v.empty()) || (type == "check" && v != "true" && v != "false") - || (type == "spin" && (std::stoi(v) < min || std::stoi(v) > max))) + || (type == "spin" && !value_in_range(v, min, max))) return *this; if (type == "combo") diff --git a/tests/instrumented.py b/tests/instrumented.py index 900151340..7c43cefa4 100644 --- a/tests/instrumented.py +++ b/tests/instrumented.py @@ -563,6 +563,115 @@ class TestEnPassantSanitization(metaclass=OrderedClassMembers): self.stockfish.check_output(check_output) self.stockfish.expect("bestmove d8d7*") +class TestInvalidFEN(metaclass=OrderedClassMembers): + def beforeEach(self): + self.stockfish = None + + def afterEach(self): + assert postfix_check(self.stockfish.get_output()) == True + self.stockfish.clear_output() + + def _expect_critical(self, fen): + self.stockfish = Stockfish(f"position fen {fen}".split(" "), True) + assert self.stockfish.process.returncode != 0 + assert "CRITICAL ERROR" in self.stockfish.process.stdout + + def test_no_kings(self): + self._expect_critical("8/8/8/8/8/8/8/8 w - - 0 1") + + def test_invalid_piece(self): + self._expect_critical("rnbqkbnr/pppXpppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1") + + def test_invalid_side_to_move(self): + self._expect_critical("rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR x KQkq - 0 1") + + def test_pawns_on_back_rank(self): + self._expect_critical("pppppppp/8/8/8/8/8/8/4K2k w - - 0 1") + + def test_invalid_skip_count(self): + self._expect_critical("9/8/8/8/8/8/8/8 w - - 0 1") + + +class TestInvalidOptions(metaclass=OrderedClassMembers): + def beforeAll(self): + self.stockfish = Stockfish() + + def afterAll(self): + self.stockfish.quit() + assert self.stockfish.close() == 0 + + def afterEach(self): + assert postfix_check(self.stockfish.get_output()) == True + self.stockfish.clear_output() + + # Ignore bogus spin values + def test_spin_non_numeric(self): + self.stockfish.send_command("setoption name Threads value abc") + self.stockfish.send_command("isready") + self.stockfish.equals("readyok") + + def test_spin_out_of_range(self): + self.stockfish.send_command("setoption name Threads value 999999999999") + self.stockfish.send_command("isready") + self.stockfish.equals("readyok") + + def test_spin_negative(self): + self.stockfish.send_command("setoption name Threads value -5") + self.stockfish.send_command("isready") + self.stockfish.equals("readyok") + + # Warn on bogus NUMA configs + def test_numa_garbage(self): + self.stockfish.send_command("setoption name NumaPolicy value zzz") + self.stockfish.expect("*NumaPolicy: invalid value 'zzz', keeping previous config.*") + + def test_numa_malformed_range(self): + self.stockfish.send_command("setoption name NumaPolicy value 0-") + self.stockfish.expect("*NumaPolicy: invalid value '0-', keeping previous config.*") + + def test_numa_overflow(self): + self.stockfish.send_command( + "setoption name NumaPolicy value 99999999999999999999999" + ) + self.stockfish.expect("*NumaPolicy: invalid value*keeping previous config.*") + self.stockfish.send_command("isready") + self.stockfish.equals("readyok") + + +class TestBenchFile(metaclass=OrderedClassMembers): + def beforeEach(self): + self.stockfish = None + + def afterEach(self): + assert postfix_check(self.stockfish.get_output()) == True + self.stockfish.clear_output() + + def _bench(self, name, content): + with open(name, "w") as f: + f.write(content) + self.stockfish = Stockfish(f"bench 16 1 4 {name} depth".split(" "), True) + + def test_valid_file(self): + self._bench("good.epd", "4k3/8/4K3/8/8/8/8/8 w - - 0 1\n") + assert self.stockfish.process.returncode == 0 + assert "Nodes searched" in self.stockfish.process.stderr + + def test_empty_file(self): + self._bench("empty.epd", "") + assert self.stockfish.process.returncode == 0 + + def test_malformed_fen(self): + self._bench("bad.epd", "not a valid fen\n") + assert self.stockfish.process.returncode != 0 + assert "CRITICAL ERROR" in self.stockfish.process.stdout + + def test_missing_file(self): + self.stockfish = Stockfish( + "bench 16 1 4 does_not_exist.epd depth".split(" "), True + ) + assert self.stockfish.process.returncode != 0 + + def parse_args(): parser = argparse.ArgumentParser(description="Run Stockfish with testing options") parser.add_argument("--valgrind", action="store_true", help="Run valgrind testing") @@ -595,7 +704,17 @@ if __name__ == "__main__": framework = MiniTestFramework() # Each test suite will be run inside a temporary directory - framework.run([TestCLI, TestInteractive, TestSyzygy, TestEnPassantSanitization]) + framework.run( + [ + TestCLI, + TestInteractive, + TestSyzygy, + TestEnPassantSanitization, + TestInvalidFEN, + TestInvalidOptions, + TestBenchFile, + ] + ) EPD.delete_bench_epd()