search

miguel-ambrona / D3-Chess

Chess Unwinnability Analyzer is an implementation of a decision procedure for checking whether there exists a sequence of legal moves that allows a certain player to checkmate their opponent in a given chess position.
https://chasolver.org
GNU General Public License v3.0
51 stars 8 forks source link

Invalid pointers to StateInfo #8

Closed vonaka closed 3 years ago

vonaka commented 3 years ago

At the different places in your code (here, here, here and maybe somewhere else) you call do_move with a locally allocated StateInfo. Internally do_move stores the pointer to this state. It looks like in the most cases you never reuse the same position without first resetting its state, but still this behavior is not very safe. For example, in the main loop at the moment of execution of line 479 stateinfo variable of pos is most likely already deallocated:

==9138== Invalid read of size 4
==9138==    at 0x12EEE1: can_castle (position.h:268)
==9138==    by 0x12EEE1: Position::fen[abi:cxx11]() const (position.cpp:423)
==9138==    by 0x133D54: operator<<(std::ostream&, Position const&) (position.cpp:69)
==9138==    by 0x168B56: CHA::loop(int, char**) (dynamic.cpp:479)
==9138==    by 0x10F133: main (main.cpp:35)
==9138==  Address 0x1ffeffdd98 is on thread 1's stack
==9138==  2424 bytes below stack pointer

If the problem occurs only when cha prints the position, then I believe it's fine, you can simply remove this peace of code and don't print the position (in any case my guess is that this printing is useful only for some sort of debugging). But I'm not sure, maybe all this affects the main program in some unsuspected way.

miguel-ambrona commented 3 years ago

Thank you again! As you say, this may only be a problem for printing the position. But is StateInfo used in the output of printing? To the best of my understanding, StateInfo may be used (in the Stockfish library) to undo moves in the position. I believe every time we apply do_move, we also call undo (later) while StateInfo is still allocated in the local scope. With the exception of function is_unwinnable_after_one_move, which is a (temporary) attempt of making the program more complete, but I am planning to revisit this part of the code soon and approach it more elegantly. As I comment here, is_unwinnable_after_one_move may change the position passed as argument. This is no problem since the position will never be used again (except possibly for printing). Maybe that is what is causing the warning.

Do you have any suggestions to address StateInfo in a better way? Should we allocate this variable globally?

vonaka commented 3 years ago

Answering your question: yes, StateInfo is used in printing (for example here there is a call to fen, that calls can_castle, that uses position's st). Then, I trust you when you say that after each do you do undo, so it shouldn't be hard to get rid of this warning. I don't know how tho. At the begging I thought that the way to go is to dynamically allocate those states (and then use some sort of pool), but now I'm not sure, maybe you're right, maybe one global state is enough. Honestly, I don't really know how the whole thing works, so I'm afraid of giving bad advises, I need to look at the code more carefully to be able to help.

miguel-ambrona commented 3 years ago

Thanks, @vonaka. Could you check whether the same warning is triggered now?

vonaka commented 3 years ago

Nope, no warnings. If you don't plan to uncomment the printing, I guess we can close this issue.

miguel-ambrona commented 3 years ago

Thanks, yeah I am not going to print the position anymore. I edited that line to just print the FEN given as input. (And this print was only used for debugging in the past, I may drop it at some point.)

I checked Stockfish code and they also allocate StateInfo locally, so the current approach seems fine for now.