Segmentation fault

[tinbugger]

In the last stable build, and also in the last development build (stockfish_20092822_x64_modern), the following UCI commands cause a segmentation fault more than 50% of the times: position fen r7/3R2kQ/5p2/4p3/8/7P/8/5K2 b - - 2 3 go depth 14 (It does not matter if I type "uci" "isready", "setoption name Threads value 2" before that or not)

[vondele]

invalid fen, user/GUI mistake.

[tinbugger]

It is true that this position cannot be reached in the course of a game - there is no move that can lead to it - but it is not invalid by itself and it can be used in puzzles, for example. The same position was handled correctly in Stockfish 8 (I haven't checked other versions), and the move generator code (which is what causes the issue, as far as I can see) seems to implement the same algorithm in both versions. The problem is that there are two sliding attackers on the king, and both attack on the same line. The move generator creates an illegal move (king takes queen g7h7) due to a hidden assumption that two attackers cannot be aligned this way. Is it that hard/costly to avoid such a move?

[tinbugger]

Coming to think of it - it is trivial to add a test if the king is still under check after an evasion move, and if so, avoid generating the move. This will not affect run time significantly because evasion moves are the minority.

[ddugovic]

Considering that #905 was closed, without ample discussion in the fishcooking forum I can't imagine the policy (input sanitization is the consumer's responsibility) changing.

[tinbugger]

But is this really an invalid fen? Not handling it correctly precludes the use of Stockfish in some puzzles.

[Sopel97]

Maybe this could be solved by removing the king from the bitboard when computing the defended pieces during check. But I stand with "not a bug".

[vondele]

no, not a bug. I'll close the issue

[joergoster]

@vondele It IS a BUG! See https://github.com/joergoster/Stockfish/commit/a3c35cacd5bf188894b0742c8fda0910f35b84e3 and https://github.com/joergoster/Stockfish/commit/3aedd2712ccf264293437c88733d1c79b4575b53

It can be verified quite easily with Stockfish 11 which still treats the position without problems:

Stockfish 11 64 POPCNT by T. Romstad, M. Costalba, J. Kiiski, G. Linscott uci id name Stockfish 11 64 POPCNT id author T. Romstad, M. Costalba, J. Kiiski, G. Linscott

option name Debug Log File type string default option name Contempt type spin default 24 min -100 max 100 option name Analysis Contempt type combo default Both var Off var White var Black var Both option name Threads type spin default 1 min 1 max 512 option name Hash type spin default 16 min 1 max 131072 option name Clear Hash type button option name Ponder type check default false option name MultiPV type spin default 1 min 1 max 500 option name Skill Level type spin default 20 min 0 max 20 option name Move Overhead type spin default 30 min 0 max 5000 option name Minimum Thinking Time type spin default 20 min 0 max 5000 option name Slow Mover type spin default 84 min 10 max 1000 option name nodestime type spin default 0 min 0 max 10000 option name UCI_Chess960 type check default false option name UCI_AnalyseMode type check default false option name UCI_LimitStrength type check default false option name UCI_Elo type spin default 1350 min 1350 max 2850 option name SyzygyPath type string default option name SyzygyProbeDepth type spin default 1 min 1 max 100 option name Syzygy50MoveRule type check default true option name SyzygyProbeLimit type spin default 7 min 0 max 7 uciok position fen r7/3R2kQ/5p2/4p3/8/7P/8/5K2 b - - 2 3 go depth 12 info depth 1 seldepth 1 multipv 1 score mate -1 nodes 15 nps 15000 tbhits 0 time 1 pv g7f8 h7f7 info depth 2 seldepth 3 multipv 1 score mate -1 nodes 17 nps 17000 tbhits 0 time 1 pv g7f8 h7f7 info depth 3 seldepth 3 multipv 1 score mate -1 nodes 19 nps 19000 tbhits 0 time 1 pv g7f8 h7f7 info depth 4 seldepth 3 multipv 1 score mate -1 nodes 21 nps 21000 tbhits 0 time 1 pv g7f8 h7f7 info depth 5 seldepth 3 multipv 1 score mate -1 nodes 23 nps 23000 tbhits 0 time 1 pv g7f8 h7f7 info depth 6 seldepth 3 multipv 1 score mate -1 nodes 25 nps 25000 tbhits 0 time 1 pv g7f8 h7f7 info depth 7 seldepth 3 multipv 1 score mate -1 nodes 27 nps 27000 tbhits 0 time 1 pv g7f8 h7f7 info depth 8 seldepth 3 multipv 1 score mate -1 nodes 29 nps 29000 tbhits 0 time 1 pv g7f8 h7f7 info depth 9 seldepth 3 multipv 1 score mate -1 nodes 31 nps 31000 tbhits 0 time 1 pv g7f8 h7f7 info depth 10 seldepth 3 multipv 1 score mate -1 nodes 33 nps 33000 tbhits 0 time 1 pv g7f8 h7f7 info depth 11 seldepth 3 multipv 1 score mate -1 nodes 35 nps 35000 tbhits 0 time 1 pv g7f8 h7f7 info depth 12 seldepth 3 multipv 1 score mate -1 nodes 37 nps 37000 tbhits 0 time 1 pv g7f8 h7f7 bestmove g7f8 ponder h7f7 quit

[ddugovic]

According to the laws of chess section 7.1, 7.4, and 7.5, that position is invalid.

[joergoster]

@ddugovic LOL, that bug gets also triggered on valid chess positions!

[MichaelB7]

@ddugovic LOL, that bug gets also triggered on valid chess positions!

I don't understand , black was already in check when white move to put the black king in check again - I have seen moves like this happen in OTB games - but those are just oversights. Whatever white's last move , it was an illegal move. If we care for this position , then where does it stop? Do we care for this one too?

r7/3R2kQ/5p2/4p3/8/6RP/8/5K2 b - - 2 1

I believe the maintainers are correct for not addressing this "bug" that is not really a "bug".

The bug here is that any GUI that accepts this position is in fact buggy. Th only time that a King have two pieces checking it , is when the second check is a discovered check when the pieces that is moving also checks. N and any piece, , R & B, Q with R. B or N — a pwn is never involved in a discovered check, since it must be in contact to deliver check and if it moves to deliver a check , there is no piece that can deliver a second check when the pawn is moving.

[joergoster]

Don't you realize that this is not about the fact that the position is illegal, but that it is in fact triggering a bug? What do you think is the reason SF 11 is not crashing at this position? Nothing has been changed in treating illegal positions since then to my knowledge.

However, you can reproduce the bug by adding the assertion as in the above mentioned commit. Run a default bench with a debug build and report back!

[vondele]

so I'll reopen this to have it investigated further, since seemingly the discussion is not finished.

[MichaelB7]

FWIW, I have found multiple "illegal" positions that do not cause SF to crash. This is one strictly because the two pieces are on the same line - but one can create many positions with two checks that are not result of a discovered check and SF treats them as legal. So the new behavior was clearly unintentional and I'm going to reverse my position and say we probably should look at this and restore previous behavior.

[vondele]

there is obviously no requirement to crash on illegal positions.. it just so happens. What I would need to understand is if there is a legal position where the current code is problematic.

[MichaelB7]

The usual culprits are en passant moves and maybe a few of the weird 960 castling moves. The is one 960 castling move where you say you castle and you don't move a piece. Another is the first move of the game and you castle. Not all GUIs handle 960 correctly.

[Sopel97]

So according to Jorg assert(!MoveList<LEGAL>(pos).size()); exposes the bug. The bug is seemingly that mate scores are claimed not only when there is no legal moves but also when all legal moves are pruned. This could be fixed by introducing a boolean that indicates whether any loop iteration was done and use that in the condition instead of value == -VALUE_INFINITE (or use moveCount). Warrants a fishtest run?

[vondele]

ah, there were some (2,3, snicolet, voyagerOne) tests of that IIRC.

[vondele]

https://tests.stockfishchess.org/tests/view/5f6e7be34bdee0c232ab4bfb actually, @VoyagerOne

[joergoster]

There are 2 critical commits:

https://github.com/official-stockfish/Stockfish/commit/6596f0eac0c1d25a12bfd923907bfc78beedbc90 by @protonspring and https://github.com/official-stockfish/Stockfish/commit/843a961a8c10d5949e04718b829e3b3d5adeedb4 by @Vizvezdenec

I didn't look into details of the 1st commit (I leave this to @protonspring) but simply reverted it. The latter is probably only safe when not in check.

I really, really wish all devs/maintainers/participants would be a bit more open-minded to such issues before tossing them away as irrelevant. Sometimes a 2nd and more thorough look is worth it.

[Vizvezdenec]

This bugfix was tested and I actually thought it's already in master but @VoyagerOne never created a PR (it seems?).

[VoyagerOne]

Hi. Just realized I was mentioned here. Does my patch fix this bug? https://tests.stockfishchess.org/tests/view/5f6e7be34bdee0c232ab4bfb

If so I can do a PR.

[snicolet]

So according to Jorg assert(!MoveList(pos).size()); exposes the bug. The bug is seemingly that mate scores are claimed not only when there is no legal moves but also when all legal moves are pruned. This could be fixed by introducing a boolean that indicates whether any loop iteration was done and use that in the condition instead of value == -VALUE_INFINITE (or use moveCount). Warrants a fishtest run?

ah, there were some (2,3, snicolet, voyagerOne) tests of that IIRC.

Indeed, one bug in the current code is that qsearch can prune some moves when in check, via the "CounterMove based pruning" heuristic of line 1567 of search.cpp, see https://github.com/official-stockfish/Stockfish/blob/6328135264d3b13a2cef3f0c835a27192cae0f40/src/search.cpp#L1567

This may lead to erroneous mate scores in qsearch when in check if the first defense move generated is not a good defense of the check, but the following defenses are pruned.

I tried the most obvious bugfix (not allowing this heuristic when in check) in the following test, but it failed (edit: with lots of losses on time, however): https://github.com/snicolet/Stockfish/compare/9a64e737cf...7ca68b8841 https://tests.stockfishchess.org/tests/view/5f6b5b62c7759d4ee307d006

Maybe a better bugfix would be to allow the "CounterMove based pruning" heuristic in qsearch only if bestValue > -VALUE_MATE.

[vondele]

The discussion is now spread over a few issues / PRs (https://github.com/official-stockfish/Stockfish/pull/3199 and https://github.com/official-stockfish/Stockfish/pull/3198 and https://github.com/official-stockfish/Stockfish/issues/3196), also because there are two issues.

The qsearch pruning part was retested here https://tests.stockfishchess.org/tests/view/5f9918796a2c112b60691b80 and with a similar check as bestValue > -MATE here: https://tests.stockfishchess.org/tests/view/5f992f346a2c112b60691b89

It might be that this is just not fixable without a small Elo regression, and we might have to go with a revert of the patch that caused this in the first place.

[syzygy1]

I could be wrong, but I'm not convinced that the original illegal position has much to do with the real problems that this discussion has usefully brought to our attention ;-)