From e38f5b2f53efef035e6396bbd9888d91c05db434 Mon Sep 17 00:00:00 2001 From: Tomasz Sobczyk Date: Wed, 18 Mar 2026 20:41:31 +0100 Subject: [PATCH] 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 | 13 ++- src/engine.h | 3 +- src/position.cpp | 241 ++++++++++++++++++++++++++++++++--------- src/position.h | 14 ++- src/syzygy/tbprobe.cpp | 23 +++- src/uci.cpp | 17 ++- src/uci.h | 3 + 7 files changed, 254 insertions(+), 60 deletions(-) diff --git a/src/engine.cpp b/src/engine.cpp index be0fe3c40..c26567854 100644 --- a/src/engine.cpp +++ b/src/engine.cpp @@ -195,21 +195,26 @@ void Engine::set_on_verify_networks(std::function&& 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& moves) { +std::optional Engine::set_position(const std::string& fen, + const std::vector& moves) { // Drop the old state and create a new one - states = StateListPtr(new std::deque(1)); - pos.set(fen, options["UCI_Chess960"], &states->back()); + states = StateListPtr(new std::deque(1)); + 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 diff --git a/src/engine.h b/src/engine.h index 92d6282dc..e22466991 100644 --- a/src/engine.h +++ b/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& moves); + std::optional set_position(const std::string& fen, + const std::vector& moves); // modifiers diff --git a/src/position.cpp b/src/position.cpp index daadf59ec..f9b94b5dd 100644 --- a/src/position.cpp +++ b/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, -// this is assumed to be the responsibility of the GUI. -Position& Position::set(const string& fenStr, bool isChess960, StateInfo* si) { +// The FEN string is strictly validated; if it is invalid or inconsistent, +// a PositionSetError describing the problem is returned, otherwise std::nullopt. +std::optional +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; - size_t idx; - Square sq = SQ_A8; + unsigned char token; std::istringstream ss(fenStr); std::memset(reinterpret_cast(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(WHITE) != 1 || count(BLACK) != 1) + return PositionSetError("Unsupported position. Incorrect number of kings."); + + const int wPawns = count(WHITE); + const int bPawns = count(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(WHITE) - 2, 0); + const int bAdditionalKnights = std::max((int) count(BLACK) - 2, 0); + const int wAdditionalBishops = std::max((int) count(WHITE) - 2, 0); + const int bAdditionalBishops = std::max((int) count(BLACK) - 2, 0); + const int wAdditionalRooks = std::max((int) count(WHITE) - 2, 0); + const int bAdditionalRooks = std::max((int) count(BLACK) - 2, 0); + const int wAdditionalQueens = std::max((int) count(WHITE) - 1, 0); + const int bAdditionalQueens = std::max((int) count(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') - for (rsq = relative_square(c, SQ_H1); piece_on(rsq) != rook; --rsq) - {} - - else if (token == 'Q') - for (rsq = relative_square(c, SQ_A1); piece_on(rsq) != rook; ++rsq) - {} - + if (token == 'K' || token == 'Q') + { + const int dir = token == 'K' ? -1 : 1; + Square sq = relative_square(c, token == 'K' ? SQ_H1 : SQ_A1); + // Look for a rook and a king for the castling. King must come later. + // 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; - - if (((ss >> col) && (col >= 'a' && col <= 'h')) - && ((ss >> row) && (row == (sideToMove == WHITE ? '6' : '3')))) + bool enpassant = false, legalEP = false; + unsigned char col = '-', row; + ss >> col; + 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(st->epSquare, ~sideToMove) & pieces(sideToMove, PAWN); - Bitboard target = (pieces(~sideToMove, PAWN) & (st->epSquare + pawn_push(~sideToMove))); - Bitboard occ = pieces() ^ target ^ st->epSquare; + if ((col >= 'a' && col <= 'h') && (row == (sideToMove == WHITE ? '6' : '3'))) + { + st->epSquare = make_square(File(col - 'a'), Rank(row - '1')); - // En passant square will be considered only if - // a) side to move have a pawn threatening epSquare - // b) there is an enemy pawn in front of epSquare - // c) there is no piece on epSquare or behind epSquare - enpassant = - pawns && target && !(pieces() & (st->epSquare | (st->epSquare + pawn_push(sideToMove)))); + Bitboard pawns = attacks_bb(st->epSquare, ~sideToMove) & pieces(sideToMove, PAWN); + Bitboard target = (pieces(~sideToMove, PAWN) & (st->epSquare + pawn_push(~sideToMove))); + Bitboard occ = pieces() ^ target ^ st->epSquare; - // 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(sideToMove), occ ^ pop_lsb(pawns)) - & pieces(~sideToMove) & ~target); + // En passant square will be considered only if + // a) side to move have a pawn threatening epSquare + // b) there is an enemy pawn in front of 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 + while (pawns) + legalEP |= !(attackers_to(square(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(~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 Position::set(const string& code, Color c, StateInfo* si) { assert(code[0] == 'K'); diff --git a/src/position.h b/src/position.h index e02a400d3..693207aaa 100644 --- a/src/position.h +++ b/src/position.h @@ -25,6 +25,8 @@ #include #include #include +#include +#include #include #include "bitboard.h" @@ -70,6 +72,12 @@ struct StateInfo { // elements are not invalidated upon list resizing. using StateListPtr = std::unique_ptr>; +// 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); - Position& set(const std::string& code, Color c, StateInfo* si); - std::string fen() const; + std::optional set(const std::string& fenStr, bool isChess960, StateInfo* si); + std::optional set(const std::string& code, Color c, StateInfo* si); + std::string fen() const; // Position representation Bitboard pieces() const; // All pieces diff --git a/src/syzygy/tbprobe.cpp b/src/syzygy/tbprobe.cpp index 9fe6df9dc..ba808ee4b 100644 --- a/src/syzygy/tbprobe.cpp +++ b/src/syzygy/tbprobe.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -386,7 +387,16 @@ TBTable::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(); hasPawns = pos.pieces(PAWN); @@ -404,7 +414,16 @@ TBTable::TBTable(const std::string& code) : pawnCount[0] = pos.count(c ? WHITE : BLACK); pawnCount[1] = pos.count(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<> diff --git a/src/uci.cpp b/src/uci.cpp index 927007686..54305bc31 100644 --- a/src/uci.cpp +++ b/src/uci.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -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 diff --git a/src/uci.h b/src/uci.h index ebc04fc3c..85c74afd8 100644 --- a/src/uci.h +++ b/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