Strict FEN parsing. Exit on setting invalid position via UCI.

fixes #6663, #6664, and a million others issues raised over the years.

This is similar to
https://github.com/official-stockfish/Stockfish/pull/4563/changes but
more conservative and errors are split between "Unsupported position"
and "Invalid FEN".

The FEN parser needs to be strict as a foundation for safety. It does
not specify much of the semantics, so this step is fairly simple. Parts
after the ep square are optional, however, since it's common, for
example in EPD notation.

Errors arising from positional semantics that were previously bucketed
under invalid FENs are now reported as "Unsupported position". Only
positions that are potentially problematic are designated as
unsupported. It is NOT guided by illegality of the position, but instead
by the ability of the engine to handle them correctly. This means that
some checks from the previous PR were removed.

std::exit is used instead of std::terminate so atexit handles will be
called.

Probably wise to run copilot on this or smth because I scribbled it
without much thought.

With these small changes and reduced, less controversial, scope I hope
this PR will finally make it and we will be done with these weekly
issues. I'll wait with putting it on fishtest until there's approval.

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

No functional change

src/engine.cpp

@@ -195,21 +195,26 @@ void Engine::set_on_verify_networks(std::function<void(std::string_view)>&& f) {
 
 void Engine::wait_for_search_finished() { threads.main_thread()->wait_for_search_finished(); }
 
-void Engine::set_position(const std::string& fen, const std::vector<std::string>& moves) {
+std::optional<PositionSetError> Engine::set_position(const std::string&              fen,
+                                                     const std::vector<std::string>& moves) {
     // Drop the old state and create a new one
-    states = StateListPtr(new std::deque<StateInfo>(1));
+    states   = StateListPtr(new std::deque<StateInfo>(1));
-    pos.set(fen, options["UCI_Chess960"], &states->back());
+    auto err = pos.set(fen, options["UCI_Chess960"], &states->back());
+    if (err.has_value())
+        return err;
 
     for (const auto& move : moves)
     {
         auto m = UCIEngine::to_move(pos, move);
 
         if (m == Move::none())
-            break;
+            return PositionSetError("Illegal move: " + move);
 
         states->emplace_back();
         pos.do_move(m, states->back());
     }
+
+    return std::nullopt;
 }
 
 // modifiers

src/engine.h

@@ -68,7 +68,8 @@ class Engine {
     // blocking call to wait for search to finish
     void wait_for_search_finished();
     // set a new position, moves are in UCI format
-    void set_position(const std::string& fen, const std::vector<std::string>& moves);
+    std::optional<PositionSetError> set_position(const std::string&              fen,
+                                                 const std::vector<std::string>& moves);
 
     // modifiers
 

src/position.cpp

@@ -162,9 +162,10 @@ void Position::init() {
 
 
 // Initializes the position object with the given FEN string.
-// This function is not very robust - make sure that input FENs are correct,
+// The FEN string is strictly validated; if it is invalid or inconsistent,
-// this is assumed to be the responsibility of the GUI.
+// a PositionSetError describing the problem is returned, otherwise std::nullopt.
-Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
+std::optional<PositionSetError>
+Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
     /*
    A FEN string defines a particular position using only the ASCII character set.
 
@@ -200,9 +201,7 @@ Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
       incremented after Black's move.
 */
 
-    unsigned char      col, row, token;
+    unsigned char      token;
-    size_t             idx;
-    Square             sq = SQ_A8;
     std::istringstream ss(fenStr);
 
     std::memset(reinterpret_cast<char*>(this), 0, sizeof(Position));
@@ -211,81 +210,214 @@ Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
 
     ss >> std::noskipws;
 
+    int numPieces = 0;
+    int file      = FILE_A;
+    int rank      = RANK_8;
+
     // 1. Piece placement
-    while ((ss >> token) && !isspace(token))
+    for (;;)
     {
+        if (!(ss >> token))
+            return PositionSetError("Invalid FEN. Unexpected end of stream.");
+
+        if (isspace(token))
+            break;
+
         if (isdigit(token))
-            sq += (token - '0') * EAST;  // Advance the given number of files
-
-        else if (token == '/')
-            sq += 2 * SOUTH;
-
-        else if ((idx = PieceToChar.find(token)) != string::npos)
         {
+            const int diff = (token - '0');
+            if (diff < 1 || diff > 8)
+                return PositionSetError("Invalid FEN. Invalid number of squares to skip.");
+
+            file += diff;
+            if (file > FILE_NB)
+                return PositionSetError("Invalid FEN. Invalid file reached.");
+        }
+        else if (token == '/')
+        {
+            if (file != FILE_NB)
+                return PositionSetError(
+                  "Invalid FEN. Trying to end rank when not at the end of it.");
+
+            --rank;
+            file = FILE_A;
+
+            if (rank < RANK_1)
+                return PositionSetError("Invalid FEN. Invalid rank reached.");
+        }
+        else
+        {
+            if (file >= FILE_NB)
+                return PositionSetError("Invalid FEN. Invalid file reached.");
+
+            const size_t idx = PieceToChar.find(token);
+            if (idx == string::npos)
+                return PositionSetError(std::string("Invalid FEN. Invalid piece: ")
+                                        + std::string(1, token));
+
+            if (++numPieces > 32)
+                return PositionSetError("Invalid FEN. More than 32 pieces on the board.");
+
+            const Square sq = make_square(File(file), Rank(rank));
             put_piece(Piece(idx), sq);
-            ++sq;
+
+            ++file;
         }
     }
 
+    if (rank != RANK_1 || file != FILE_NB)
+        return PositionSetError("Invalid FEN. Board state encoding ended but cursor not at end.");
+
+    if (pieces(PAWN) & (RANK_1 | RANK_8))
+        return PositionSetError("Unsupported position. Pawns on the first or eighth rank.");
+
+    if (count<KING>(WHITE) != 1 || count<KING>(BLACK) != 1)
+        return PositionSetError("Unsupported position. Incorrect number of kings.");
+
+    const int wPawns = count<PAWN>(WHITE);
+    const int bPawns = count<PAWN>(BLACK);
+    if (wPawns > 8)
+        return PositionSetError("Unsupported position. WHITE has more than 8 pawns.");
+    if (bPawns > 8)
+        return PositionSetError("Unsupported position. BLACK has more than 8 pawns.");
+
+    const int wAdditionalKnights = std::max((int) count<KNIGHT>(WHITE) - 2, 0);
+    const int bAdditionalKnights = std::max((int) count<KNIGHT>(BLACK) - 2, 0);
+    const int wAdditionalBishops = std::max((int) count<BISHOP>(WHITE) - 2, 0);
+    const int bAdditionalBishops = std::max((int) count<BISHOP>(BLACK) - 2, 0);
+    const int wAdditionalRooks   = std::max((int) count<ROOK>(WHITE) - 2, 0);
+    const int bAdditionalRooks   = std::max((int) count<ROOK>(BLACK) - 2, 0);
+    const int wAdditionalQueens  = std::max((int) count<QUEEN>(WHITE) - 1, 0);
+    const int bAdditionalQueens  = std::max((int) count<QUEEN>(BLACK) - 1, 0);
+    if (wAdditionalKnights + wAdditionalBishops + wAdditionalRooks + wAdditionalQueens > 8 - wPawns)
+        return PositionSetError("Unsupported position. Too many major pieces for WHITE.");
+    if (bAdditionalKnights + bAdditionalBishops + bAdditionalRooks + bAdditionalQueens > 8 - bPawns)
+        return PositionSetError("Unsupported position. Too many major pieces for BLACK.");
+
     // 2. Active color
-    ss >> token;
+    if (!(ss >> token))
+        return PositionSetError("Invalid FEN. Unexpected end of stream.");
+    if (token != 'w' && token != 'b')
+        return PositionSetError(std::string("Invalid FEN. Invalid side to move: ")
+                                + std::string(1, token));
     sideToMove = (token == 'w' ? WHITE : BLACK);
-    ss >> token;
+    if (!(ss >> token) || !isspace(token) || ss.eof())
+        return PositionSetError("Invalid FEN. Expected whitespace after side to move.");
 
     // 3. Castling availability. Compatible with 3 standards: Normal FEN standard,
     // Shredder-FEN that uses the letters of the columns on which the rooks began
     // the game instead of KQkq and also X-FEN standard that, in case of Chess960,
     // if an inner rook is associated with the castling right, the castling tag is
     // replaced by the file letter of the involved rook, as for the Shredder-FEN.
-    while ((ss >> token) && !isspace(token))
+    //
+    // NOTE: Due to the prevalnce of incorrect (or missing) castling rights the
+    // validation is less strict. However, incorrect castling rights are still sanitized.
+    int num_castling_rights = 0;
+    for (;;)
     {
-        Square rsq;
+        if (!(ss >> token))
+            break;
+
+        if (isspace(token))
+            break;
+
+        if (num_castling_rights == 0 && token == '-')
+        {
+            ss >> std::ws;
+            break;
+        }
+
+        if (++num_castling_rights > 4)
+            return PositionSetError("Invalid FEN. Maximum of 4 castling rights can be specified.");
+
+        Square rsq  = SQ_NONE;
+        Square ksq  = SQ_NONE;
         Color  c    = islower(token) ? BLACK : WHITE;
         Piece  rook = make_piece(c, ROOK);
+        Piece  king = make_piece(c, KING);
 
         token = char(toupper(token));
 
-        if (token == 'K')
+        if (token == 'K' || token == 'Q')
-            for (rsq = relative_square(c, SQ_H1); piece_on(rsq) != rook; --rsq)
+        {
-            {}
+            const int dir = token == 'K' ? -1 : 1;
-
+            Square    sq  = relative_square(c, token == 'K' ? SQ_H1 : SQ_A1);
-        else if (token == 'Q')
+            // Look for a rook and a king for the castling. King must come later.
-            for (rsq = relative_square(c, SQ_A1); piece_on(rsq) != rook; ++rsq)
+            // Only the first rook is noted.
-            {}
+            // If the castling rights are available the king must always be between files 2 and 7 inclusive
-
+            // so there is no need to check the last square.
+            for (int i = 0; i < 7; ++i, sq = Square(sq + dir))
+            {
+                const Piece pc = piece_on(sq);
+                if (pc == king)
+                {
+                    ksq = sq;
+                    break;
+                }
+                else if (pc == rook && rsq == SQ_NONE)
+                {
+                    rsq = sq;
+                }
+            }
+        }
         else if (token >= 'A' && token <= 'H')
-            rsq = make_square(File(token - 'A'), relative_rank(c, RANK_1));
+        {
+            const Square rsqCandidate = make_square(File(token - 'A'), relative_rank(c, RANK_1));
+            ;
+            if (piece_on(rsqCandidate) == rook)
+                rsq = rsqCandidate;
 
+            // If the castling rights are available the king must always be between files 2 and 7 inclusive.
+            Square sq = relative_square(c, SQ_B1);
+            for (int i = 0; i < 6; ++i, ++sq)
+            {
+                if (piece_on(sq) == king)
+                    ksq = sq;
+            }
+        }
         else
-            continue;
+        {
+            return PositionSetError(std::string("Invalid FEN. Expected castling rights. Got: ")
+                                    + std::string(1, token));
+        }
 
-        set_castling_right(c, rsq);
+        // Only apply castling rights if they can be valid.
+        if (ksq != SQ_NONE && rsq != SQ_NONE)
+            set_castling_right(c, rsq);
     }
 
     // 4. En passant square.
     // Ignore if square is invalid or not on side to move relative rank 6.
-    bool enpassant = false, legalEP = false;
+    bool          enpassant = false, legalEP = false;
-
+    unsigned char col = '-', row;
-    if (((ss >> col) && (col >= 'a' && col <= 'h'))
+    ss >> col;
-        && ((ss >> row) && (row == (sideToMove == WHITE ? '6' : '3'))))
+    if (col != '-')
     {
-        st->epSquare = make_square(File(col - 'a'), Rank(row - '1'));
+        if (!(ss >> row))
+            return PositionSetError("Invalid FEN. Unexpected end of stream.");
 
-        Bitboard pawns  = attacks_bb<PAWN>(st->epSquare, ~sideToMove) & pieces(sideToMove, PAWN);
+        if ((col >= 'a' && col <= 'h') && (row == (sideToMove == WHITE ? '6' : '3')))
-        Bitboard target = (pieces(~sideToMove, PAWN) & (st->epSquare + pawn_push(~sideToMove)));
+        {
-        Bitboard occ    = pieces() ^ target ^ st->epSquare;
+            st->epSquare = make_square(File(col - 'a'), Rank(row - '1'));
 
-        // En passant square will be considered only if
+            Bitboard pawns = attacks_bb<PAWN>(st->epSquare, ~sideToMove) & pieces(sideToMove, PAWN);
-        // a) side to move have a pawn threatening epSquare
+            Bitboard target = (pieces(~sideToMove, PAWN) & (st->epSquare + pawn_push(~sideToMove)));
-        // b) there is an enemy pawn in front of epSquare
+            Bitboard occ    = pieces() ^ target ^ st->epSquare;
-        // c) there is no piece on epSquare or behind epSquare
-        enpassant =
-          pawns && target && !(pieces() & (st->epSquare | (st->epSquare + pawn_push(sideToMove))));
 
-        // If no pawn can execute the en passant capture without leaving the king in check, don't record the epSquare
+            // En passant square will be considered only if
-        while (pawns)
+            // a) side to move have a pawn threatening epSquare
-            legalEP |= !(attackers_to(square<KING>(sideToMove), occ ^ pop_lsb(pawns))
+            // b) there is an enemy pawn in front of epSquare
-                         & pieces(~sideToMove) & ~target);
+            // c) there is no piece on epSquare or behind epSquare
+            enpassant = pawns && target
+                     && !(pieces() & (st->epSquare | (st->epSquare + pawn_push(sideToMove))));
+
+            // If no pawn can execute the en passant capture without leaving the king in check, don't record the epSquare
+            while (pawns)
+                legalEP |= !(attackers_to(square<KING>(sideToMove), occ ^ pop_lsb(pawns))
+                             & pieces(~sideToMove) & ~target);
+        }
+        else
+            return PositionSetError("Invalid FEN. Invalid en-passant square.");
     }
 
     if (!enpassant || !legalEP)
@@ -294,6 +426,14 @@ Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
     // 5-6. Halfmove clock and fullmove number
     ss >> std::skipws >> st->rule50 >> gamePly;
 
+    // Normally values larger than 99 would be pointless but we do support ignoring 50 move rule for TB purposes.
+    // Limit at 2**15 as it's used multiplicativly with position evaluation during search.
+    if (st->rule50 < 0 || st->rule50 > 32767)
+        return PositionSetError("Unsupported position. Rule50 counter out of range.");
+
+    if (gamePly < 0 || gamePly > 100000)
+        return PositionSetError("Unsupported position. Game ply out of range.");
+
     // Convert from fullmove starting from 1 to gamePly starting from 0,
     // handle also common incorrect FEN with fullmove = 0.
     gamePly = std::max(2 * (gamePly - 1), 0) + (sideToMove == BLACK);
@@ -301,9 +441,12 @@ Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) {
     chess960 = isChess960;
     set_state();
 
+    if (attackers_to_exist(square<KING>(~sideToMove), pieces(), sideToMove))
+        return PositionSetError("Unsupported position. King can be captured.");
+
     assert(pos_is_ok());
 
-    return *this;
+    return std::nullopt;
 }
 
 
@@ -401,7 +544,7 @@ Key Position::compute_material_key() const {
 
 // Overload to initialize the position object with the given endgame code string
 // like "KBPKN". It's mainly a helper to get the material key out of an endgame code.
-Position& Position::set(const string& code, Color c, StateInfo* si) {
+std::optional<PositionSetError> Position::set(const string& code, Color c, StateInfo* si) {
 
     assert(code[0] == 'K');
 

src/position.h

@@ -25,6 +25,8 @@
 #include <iosfwd>
 #include <memory>
 #include <new>
+#include <optional>
+#include <stdexcept>
 #include <string>
 
 #include "bitboard.h"
@@ -70,6 +72,12 @@ struct StateInfo {
 // elements are not invalidated upon list resizing.
 using StateListPtr = std::unique_ptr<std::deque<StateInfo>>;
 
+// This error should be used whenever a position is suspected to be unsupported
+// by the engine. In particular positions that may cause hard errors like segmentation fault.
+struct PositionSetError: std::runtime_error {
+    using std::runtime_error::runtime_error;
+};
+
 // Position class stores information regarding the board representation as
 // pieces, side to move, hash keys, castling info, etc. Important methods are
 // do_move() and undo_move(), used by the search to update node info when
@@ -83,9 +91,9 @@ class Position {
     Position& operator=(const Position&) = delete;
 
     // FEN string input/output
-    Position&   set(const std::string& fenStr, bool isChess960, StateInfo* si);
+    std::optional<PositionSetError> set(const std::string& fenStr, bool isChess960, StateInfo* si);
-    Position&   set(const std::string& code, Color c, StateInfo* si);
+    std::optional<PositionSetError> set(const std::string& code, Color c, StateInfo* si);
-    std::string fen() const;
+    std::string                     fen() const;
 
     // Position representation
     Bitboard pieces() const;  // All pieces

src/syzygy/tbprobe.cpp

@@ -29,6 +29,7 @@
 #include <initializer_list>
 #include <iostream>
 #include <mutex>
+#include <optional>
 #include <sstream>
 #include <string_view>
 #include <sys/stat.h>
@@ -386,7 +387,16 @@ TBTable<WDL>::TBTable(const std::string& code) :
     StateInfo st;
     Position  pos;
 
-    key        = pos.set(code, WHITE, &st).material_key();
+    auto err = pos.set(code, WHITE, &st);
+    // IMPORTANT: We cannot assert here because it WILL produce validation errors
+    // on some TB7 and higher positions due to the black king being attacked
+    // while white is to move. This is not fixable without significant changes.
+    // As using pos.set here is already a very hacky way to achieve the desired
+    // result here so we leave it for now. The validation checks that fail are
+    // done after the position is fully set up, so it's fine for now.
+    // assert(!err.has_value());
+    (void) err;
+    key        = pos.material_key();
     pieceCount = pos.count<ALL_PIECES>();
     hasPawns   = pos.pieces(PAWN);
 
@@ -404,7 +414,16 @@ TBTable<WDL>::TBTable(const std::string& code) :
     pawnCount[0] = pos.count<PAWN>(c ? WHITE : BLACK);
     pawnCount[1] = pos.count<PAWN>(c ? BLACK : WHITE);
 
-    key2 = pos.set(code, BLACK, &st).material_key();
+    err = pos.set(code, BLACK, &st);
+    // IMPORTANT: We cannot assert here because it WILL produce validation errors
+    // on some TB7 and higher positions due to the black king being attacked
+    // while white is to move. This is not fixable without significant changes.
+    // As using pos.set here is already a very hacky way to achieve the desired
+    // result here so we leave it for now. The validation checks that fail are
+    // done after the position is fully set up, so it's fine for now.
+    // assert(!err.has_value());
+    (void) err;
+    key2 = pos.material_key();
 }
 
 template<>

src/uci.cpp

@@ -22,6 +22,7 @@
 #include <cctype>
 #include <cmath>
 #include <cstdint>
+#include <cstdlib>
 #include <iterator>
 #include <optional>
 #include <sstream>
@@ -466,6 +467,8 @@ std::uint64_t UCIEngine::perft(const Search::LimitsType& limits) {
 }
 
 void UCIEngine::position(std::istringstream& is) {
+    const std::string fullCommand = is.str();
+
     std::string token, fen;
 
     is >> token;
@@ -488,7 +491,11 @@ void UCIEngine::position(std::istringstream& is) {
         moves.push_back(token);
     }
 
-    engine.set_position(fen, moves);
+    auto err = engine.set_position(fen, moves);
+    if (err.has_value())
+    {
+        terminate_on_critical_error(fullCommand, err->what());
+    }
 }
 
 namespace {
@@ -658,4 +665,12 @@ 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
+              << "` failed. Reason: " << message << '\n'
+              << sync_endl;
+    std::exit(1);
+}
+
 }  // namespace Stockfish

src/uci.h

@@ -73,6 +73,9 @@ class UCIEngine {
     static void on_bestmove(std::string_view bestmove, std::string_view ponder);
 
     void init_search_update_listeners();
+
+    [[noreturn]] void terminate_on_critical_error(const std::string& fullCommand,
+                                                  const std::string& message);
 };
 
 }  // namespace Stockfish