[RFC] Handle *some* out-of-spec behaviour explicitly

[Sopel97]

This is a preliminary set of changes to explicitly handle some cases that resulted in undefined behaviour previously:

1. UCI is given a fen that has more than 32 pieces
2. UCI is given a fen that has invalid number of kings
3. UCI is given a fen that has a piece configuration definitely unreachable piece counts (prevents positions with >256 moves)
4. UCI is given a fen that would write outside of a board (due to implementation details)
5. UCI is given a fen that otherwise fails pos_is_ok check
6. UCI is given a fen with rule50 counter >100

Additional validation changes:

1. During FEN parsing, error on invalid castling rights (previously ignored)
2. During FEN parsing, error on invalid ep-square (previously ignored)
3. During FEN parsing, error on invalid side to move (previously assumed white)
4. During FEN + moves parsing, error on illegal move (previously was being ignored)

TODO (at least what I have on mind right now, there's definitely more):

1. When parsing FEN, ensure that the file cannot change by means other than '/'

Any above errors are handled by calling std::terminate. The motivation for such behaviour is lack of specification from the UCI protocol. UCI allows ignoring errors, but ignoring errors never a good idea as it leads to differences between expected and actual state. Crashing is the only responsible thing we can do in this case.

The test under https://tests.stockfishchess.org/tests/view/645b9b0a175801c38d5c4e85 was mostly a sanity check (with a slightly older version), I don't expect any performance degradation. The current state of this PR is that the changes are slapped on top of whatever was there. It may be better to rewrite some parts completely, but first I'd like to know if this is desired, and if I'm not missing something important.

[noobpwnftw]

but first I'd like to know if this is desired, and if I'm not missing something important.

No, for two reasons, 1) to the professional intensive care unit people, your abort() is equivalent to a segfault, so none of this solves their problem(until they write to agree that we can perform a controlled crash with invalid inputs now), 2) before, people can yolo through some of the less harmful invalids without any problem, now they can't and they will anyways never figure out how to write a proper sanitizer, so in a sense this just made it more punishing to the noobs.

You did not miss anything "important", as it is obvious that it is nothing complicated to filter out positions that will crash SF, however it is complicated to define what is allowed afterwards, do you allow for anything that passes your filter, or is it still "legal positions reachable from startpos"? If the former, are the conditions subject to change or permanent?

[peregrineshahin]

I think this is hardly an issue since a user can work around this and submit a fen of counter 99, 2 plies earlier and then get his way through to the same fen we rejected.
On a different note If this is really relevant I think what should reject is negative values for integers instead.

[Sopel97]

I don't think that's possible. I think it depends on whether movegen generates an empty list when hitting rule50 or not, and I don't remember the behaviour there right now. If it generates moves normally then additional check would be required for the 'moves' part.

[vdbergh]

Any above errors are handled by calling std::terminate. The motivation for such behaviour is lack of specification from the UCI protocol. UCI allows ignoring errors, but ignoring errors never a good idea as it leads to differences between expected and actual state. Crashing is the only responsible thing we can do in this case.

You are not ignoring the error if you are sending an info string message to the user (which can contain an explanation of what is wrong with the FEN).

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

The UCI protocol is not totally immutable and can evolve. For example UCI_Variant has been added in this way.

[Sopel97]

before, people can yolo through some of the less harmful invalids without any problem, now they can't

Why do we suddenly care about usage that violates preconditions?

You did not miss anything "important", as it is obvious that it is nothing complicated to filter out positions that will crash SF, however it is complicated to define what is allowed afterwards, do you allow for anything that passes your filter, or is it still "legal positions reachable from startpos"? If the former, are the conditions subject to change or permanent?

Crashing aside, this PR's purpose is to ensure there is no differences in observed and assumed state of the engine by the user - where sometimes input is ignored and sometimes everything can happen.

[Sopel97]

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

[noobpwnftw]

Why do we suddenly care about usage that violates preconditions?

Which precondition? I'm kinda lost in the chain of cause and consequences.

[Sopel97]

Why do we suddenly care about usage that violates preconditions?

Which precondition? I'm kinda lost in the chain of cause and consequences.

The precondition that positions passed to the engine must be reachable from startpos

[noobpwnftw]

The precondition that positions passed to the engine must be reachable from startpos

And why are we starting to care about usage that violates it?

[Sopel97]

The precondition that positions passed to the engine must be reachable from startpos

And why are we starting to care about usage that violates it?

you suggested such a use case as a reason to dismiss this PR

[noobpwnftw]

you suggested such a use case as a reason to dismiss this PR

If you argue that the outcome of such violations of precondition is not of concern, then I think we also won't need this PR to deal with the outcome of such violations of precondition to begin with. Whether it resulted in "happens to work" or "crashing" is irrelevant to the problem.

[vdbergh]

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.

This is BTW the way the xboard protocol handles it.

[Sopel97]

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.

This is BTW the way the xboard protocol handles it.

And what happens? What's the state after the error?

[XInTheDark]

And what happens? What's the state after the error?

I would presume that the program gracefully exits, or at least this is the most accepted opinion in #4558. To the GUI and user itself it won't make much of a difference though.

[vdbergh]

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.
This is BTW the way the xboard protocol handles it.

And what happens? What's the state after the error?

Currently the GUI and the engine would be out of sync (which is not a fundamental problem since the GUI should not trust the engine output anyway, but it might be confusing for the user, although likely not since there is the info string message which the GUI has shown). Once GUIs parse the error message sent by the engines (like in the xboard case) this issue would go away,

I don't think this is currently a serious issue though since at this time GUIs should be somewhat conservative in the positions they send to UCI engines, since they have no way of knowing what positions the engine can handle. Indeed the party that knows best which positions an engine can handle is of course the engine itself and not the GUI. This is the philosophy of the xboard protocol and it makes a lot of sense IMHO.

[ddobbelaere]

The best way to change the UCI protocol is to set the tone and return info error INVALID_FEN. Maybe everyone else follows :)

[vdbergh]

The best way to change the UCI protocol is to set the tone and return info error INVALID_FEN. Maybe everyone else follows :)

I think it is not a good idea to make an incompatible change. The GUIs will ignore the info error message (as it is not valid UCI) and so no one will see it.

[vondele]

As I noted elsewhere, we already have similar actions relating to the net:

Stockfish/src/evaluate.cpp

Line 140 in 65e2150

exit(EXIT_FAILURE);

[TheBlackPlague]

Crashing aside, this PR's purpose is to ensure there is no differences in observed and assumed state of the engine by the user - where sometimes input is ignored and sometimes everything can happen.

To be very honest, I agree that removing undefined behavior is a good idea. I'm for this proposed change.

However, might I suggest this be released as part of a major release, being a backwards compatibility breaking change (it can't be considered as a bug fix)? I'm not sure if Stockfish even cares about semantic versioning — but it's a nice thing for the end users.

[Sopel97]

Stockfish doesn't use semantic versioning so there's no way to indicate a breaking change like that. It is also not a breaking change, in a sense, since inputs that would be experiencing a change would have been already violating the precondition.

now. exit(EXIT_FAILURE) or std::terminate?

[TheBlackPlague]

Stockfish doesn't use semantic versioning so there's no way to indicate a breaking change like that. It is also not a breaking change, in a sense, since inputs that would be experiencing a change would have been already violating the precondition.

now. exit(EXIT_FAILURE) or std::terminate?

std::terminate is obviously the right way to go, and the modern C++ way.

However, such a change should be reflected in all parts of Stockfish's code. If that cannot be done and the other parts of the code use exit(EXIT_FAILURE), then it's best to follow the project's convention.

[Disservin]

why not throw a runtime error?

[TheBlackPlague]

why not throw a runtime error?

You mean without crashing?

[Disservin]

why not throw a runtime error?

You mean without crashing?

throw std::runtime_error("Unexpected fen, failed to parse: ....");

kills the program unless a try and catch block surrends it.

[bftjoe]

I think it is worth noting that restricting the engine to positions with 16 pieces for each side does limit some use cases for it. I've seen many crazy chess puzzles involving more than 32 pieces, and it would be a bit unfortunate to not be able to run Stockfish on those special cases.

Stockfish already can't guarantee quality analysis of such positions due to limitations imposed for performance reasons. Not allowing people to feed such positions to Stockfish and then have them make clickbait YouTube videos about how stockfish is blind is actually a good thing. Use fairy sf or stockfish mv for such positions, anyway.

[vdbergh]

Probably this comment has been made before. Although UCI is supposed to be stateless, there is still a little bit of state introduced by the setposition command.

So it is seems to be ok that the engine, after receiving a setposition command it does not want to handle, refuses to execute certain commands (e.g. returning bestmove null on go) until a new valid setposition command is received.

People have argued (including me) that UCI mandates that the engine should simply ignore an invalid setposition commands. Now I don't support this opinion anymore.

[Sopel97]

Moved the actual error handling to the UCI layer. Position::set now returns the error. Updated the comment on Position::set.

This is close to the final form. Sanity check: https://tests.stockfishchess.org/tests/view/6461f8ae87f6567dd4df2ad8

I settle on crashing. Per UCI spec:

• if the engine or the GUI receives an unknown command or token it should just ignore it and try to
  parse the rest of the string in this line.
  Examples: "joho debug on\n" should switch the debug mode on given that joho is not defined,
  "debug joho on\n" will be undefined however.

incorrect arguments result in undefined behaviour. The "ignore" clause only applies to commands.

[vdbergh]

One little issue I have is that the final error message does not seem to say which command is actually the culprit of the error. I would prefer something like Invalid command (setposition): <message> instead of the generic "CRITICAL ERROR".

Although it seems the idea is regarded as controversial, I would still like to make it possible for the GUI to parse the error message in a simple way if it wants to do this (rather than just relaying the message to the user).

[Sopel97]

One little issue I have is that the final error message does not seem to say which command is actually the culprit of the error. I would prefer something like Invalid command (setposition): instead of the generic "CRITICAL ERROR".

It does have a somewhat granular error message, though for batched commands it could be even more precise. I'll see if it can be done in a reasonable way.

Although it seems the idea is regarded as controversial, I would still like to make it possible for the GUI to parse the error message in a simple way if it wants to do this (rather than just relaying the message to the user).

I'd like to see UCI specification first. Then we can implement it in Stockfish.

[vdbergh]

I'd like to see UCI specification first. Then we can implement it in Stockfish.

Sure but the UCI specification is essentially frozen. The only way to make it move is to implement something reasonable and then hope it is copied (this is what happened with UCI_variant). Anyway my opinion on this is known.

[Sopel97]

The only way to make it move is to implement something reasonable and then hope it is copied

I'd like to avoid implementation defined behaviour

The whole command that failed is now printed along the error message.

[miguel-l]

Not a big deal, and maybe even a little bit nitpicky, but from my limited testing:

• when compiled using make build

position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 
info string CRITICAL ERROR: Command `position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 ` failed. Reason: Invalid FEN.

terminate called without an active exception
Aborted

• when compiled using make build debug=yes

position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 
stockfish: position.cpp:1428: bool Stockfish::Position::pos_is_ok() const: Assertion `0 && "pos_is_ok: King count"' failed.
Aborted

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

[Sopel97]

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

With debug=yes it leaks implementation details and suggests to the user it is a bug in Stockfish.

[miguel-l]

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

With debug=yes it leaks implementation details and suggests to the user it is a bug in Stockfish.

I agree we shouldn't leak implementation details. I was thinking more of, for example, adding the "king count" part to the info string message rather than "it's in pos.is_ok()". Although I don't immediately see an easy way to change it.

From the user's point of view, it seems inconsistent that some info string errors give details about what's wrong with the position, and some just say it's incorrect.

[robertnurnberg]

Tiny nit: one could rather check difference of number of DSBs and LSBs to 1, for both white and black. (not sure how easy the number of dark squared bishops can be obtained, for example)

This would help catch a board with two white LSBs, no white DSB and 8 white pawns, say.

[robertnurnberg]

If one defines 8 - wPawns as wMaxPromotions, the latter could be set to zero if all opposing pawns are still on their original rank. (plus some other cases, where they form an impenetrable wall, that could never have been overcome by an opposing pawn). But these kind of complicated checks may already go beyond the original scope of this PR.

[snicolet]

So, what's the consensus about this pull request?

[vondele]

I think we should merge something along these lines, but was holding off until after the SF16 release, and busy afterwards. Probably, this could be revisited now as this gives some time to fix-up eventual fallout.

In the back of my mind, I have been thinking also that there is possibly some overlap with #4626 or said differently, if we move in the direction of having a shared library of SF, we will have to think a bit more about error handling (i.e. a library should not abort).

[Disservin]

I agree with merging, though we should keep it rather simple in the future otherwise people will constantly try to add new rules to prohibit non legal chess positions. I think all we really care about is avoiding positions where a crash can happen (or which could later cause a crash).
Btw needs a rebase ; )

[DemiMarie]

While the 50-move rule and 75-move rule do apply in over-the-board play, they do not apply for certain purposes in endgame theory. In particular, endgame tablebases will allow one to step through a forced with that takes far longer, and this would (presulably) result in a larger count here. Instead of refusing to import the FEN, I think it would be better for Stockfish to clamp rule50 to 150.

[DemiMarie]

Exiting is an appropriate action here, but I don’t think std::terminate() is the right way to do it. std::terminate() typically results in a core dump, so it should only be used when there is a bug in Stockfish itself. It isn’t the appropriate reaction to invalid input. In this case, I would just call exit() if there are no other threads running, or quick_exit() or _Exit otherwise.