herzbube / littlego

Little Go. An iOS application that lets you play the game of Go on the iPhone or iPad.
https://littlego.herzbube.ch/
Apache License 2.0
138 stars 53 forks source link

App crashes with stack overflow error when game with many moves is archived or unarchived #357

Closed herzbube closed 3 years ago

herzbube commented 3 years ago

GoMove includes the previous and next moves in the data that is archived or unarchived with NSCoding. As soon as the first move is archived or unarchived, a chain reaction is set into motion that archives or unarchives the next move of the next move of the next move ...

If a game has a large number of moves this eventually leads to the app crashing with a stack overflow error. For archiving this can happen when a game is loaded from an .sgf file, or when the player is playing out a game within the app. For unarchiving this is less likely to happen because the app would probably have crashed already when the archive was created (unless unarchiving begins at a point where the call stack is already deeper than when when the archiving occurs).

The issue was first seen in a crash report in Xcode, sent from an iPhone 11 Pro. The crash occurred within a large number of GoMove::initWithCoder calls (i.e. during unarchiving), at a call stack depth of 510.

In the simulator the issue can be successfully reproduced when loading an .sgf file with 1231 moves (arbitrary number).

herzbube commented 3 years ago

Fuego supports up to 1394 moves when playing a move or generating a move via GTP.

GO_MAX_NUM_MOVES - RESERVE

See GoGtpEngine::CheckMoveStackOverflow() and GoBoard::StackOverflowLikely().

When the limit is reached Fuego refuses the GTP command with an error "too many moves". When it loads data from an .sgf file Fuego apparently does not perform any checks because in a macOS environment it can load games with a higher number of moves. I've seen the first segmentation faults occurring at around 1482 moves.

Anyway, it appears that the call stack limit on a real iOS device is much lower than what Fuego assumes.