Improve CLI error handling a bit

Reduce the number of *easy* ways to hit UB or an uncaught exception. This builds on Sopel's work on FEN validation.

Obviously SF is not and will never be a security boundary, but at least we can reduce the number of reports about uncaught `std::stoi` out of range, etc.

I also added a gentle fallback to `NumaConfig` specifically because that's something that (as a dev) I frequently set by hand in the CLI, but happy to revert if that's too annoying of a change.

closes https://github.com/official-stockfish/Stockfish/pull/6844

No functional change

src/engine.cpp

@@ -75,7 +75,8 @@ Engine::Engine(std::optional<std::filesystem::path> 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<std::filesystem::path> path) :
     resize_threads();
 }
 
-u64 Engine::perft(const std::string& fen, Depth depth, bool isChess960) {
+std::variant<u64, PositionSetError>
+Engine::perft(const std::string& fen, Depth depth, bool isChess960) {
     verify_network();
 
     return Benchmark::perft(fen, depth, isChess960);
@@ -214,7 +216,7 @@ std::optional<PositionSetError> 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<PositionSetError> Engine::flip() { return pos.flip(); }
 
 std::string Engine::visualize() const {
     std::stringstream ss;

src/engine.h

@@ -27,6 +27,7 @@
 #include <string>
 #include <string_view>
 #include <utility>
+#include <variant>
 #include <vector>
 
 #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<u64, PositionSetError> 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<PositionSetError>      flip();
     std::string                          visualize() const;
     std::vector<std::pair<usize, usize>> get_bound_thread_count_by_numa_node() const;
     std::string                          get_numa_config_as_string() const;

src/misc.cpp

@@ -23,6 +23,7 @@
 #include <cassert>
 #include <cctype>
 #include <cstring>
+#include <cerrno>
 #include <cmath>
 #include <cstdlib>
 #include <filesystem>
@@ -543,10 +544,14 @@ CommandLine::CommandLine(int _argc, char** _argv) :
 }
 
 
-usize str_to_size_t(const std::string& s) {
+std::optional<usize> str_to_size_t(const std::string& s) {
-    unsigned long long value = std::stoull(s);
+    if (s.empty() || s[0] == '-')
-    if (value > std::numeric_limits<usize>::max())
+        return std::nullopt;
-        std::exit(EXIT_FAILURE);
+    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<usize>::max())
+        return std::nullopt;
     return static_cast<usize>(value);
 }
 

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<usize> 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);

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<NumaConfig> 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]))};
+                auto c = str_to_size_t(std::string(parts[0]));
-                indices.emplace_back(c);
+                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]))};
+                constexpr usize MaxIndices = 1 << 20;  // prevent oom
-                const CpuIndex clast  = CpuIndex{str_to_size_t(std::string(parts[1]))};
+
-                for (usize c = cfirst; c <= clast; ++c)
+                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);
+                    }
                 }
             }
         }

src/perft.h

@@ -20,6 +20,7 @@
 #define PERFT_H_INCLUDED
 
 #include <cstdint>
+#include <variant>
 
 #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<u64, PositionSetError>
+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<true>(p, depth);
 }

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<PositionSetError> 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);
+    return set(f, is_chess960(), st);
-
-    assert(pos_is_ok());
 }
 
 

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                            pos_is_ok() const;
-    bool material_key_is_ok() const;
+    bool                            material_key_is_ok() const;
-    void flip();
+    std::optional<PositionSetError> flip();
 
     StateInfo* state() const;
 

src/uci.cpp

@@ -28,6 +28,7 @@
 #include <string_view>
 #include <filesystem>
 #include <utility>
+#include <variant>
 #include <vector>
 
 #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<PositionSetError>(&result))
+        terminate_on_critical_error(err->what());
+
+    auto nodes = std::get<u64>(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,
+void UCIEngine::terminate_on_critical_error(const std::string& message) {
-                                            const std::string& message) {
+    sync_cout << "info string CRITICAL ERROR: Command `" << currentCmd
-    sync_cout << "info string CRITICAL ERROR: Command `" << fullCommand
               << "` failed. Reason: " << message << '\n'
               << sync_endl;
     std::exit(1);

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,
+    [[noreturn]] void terminate_on_critical_error(const std::string& message);
-                                                  const std::string& message);
 };
 
 }  // namespace Stockfish

src/ucioption.cpp

@@ -21,6 +21,7 @@
 #include <algorithm>
 #include <cassert>
 #include <cctype>
+#include <cerrno>
 #include <cstdlib>
 #include <iostream>
 #include <sstream>
@@ -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")

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()