When reporting that the game name in a save file isn't recognised,
don't include the name from the save file in the error message, partly
to avoid the complexity of freeing it properly on two different code
paths and partly because including unsanitized data from a
fuzzer-supplied save file in the error message just seems dangerous.
And properly sanitising it would waste the fuzzer's time exploring the
sanitising code.
Thanks to Ben Hutchings for reporting the bug.
Tracks allowed moves in execute_move() that shouldn't have been allowed,
like changing the state of the edges of the board. This moves couldn't
be generated by interpret_move(), but could be loaded from a save file.
Now execute_move() uses the same ui_can_flip_*() functions as
interpret_move() to decide whether a particular move is allowed. This
should prevent some assertion failures when loading corrupted save
files.
Towers' new_game() causes an assertion failure on game description
strings that contain spurious characters after a valid description, so
validate_desc() should also refuse such a description. The problem
could be demonstrated by editing the game description in the
"Specific" dialogue box to add a '!' at the end, which caused
"new_game: Assertion `!*p' failed.".
If a Mines save file contains a move after the player has already
died, this can lead to an assertion failure once there are more mines
that covered squares. Better to just reject any move after the
player's died.
It allowed V, W, X, Y, H, I, J, and K to appear in game descriptions
even though new_game() didn't ascribe any meaning to those letters and
would fail an assertion if they ever occurred. As far as I can tell,
those letters have never done anything, so I've just removed the
checks for them from validate_desc().
Without this, execute_move() can end up reading off the end of the
move string, which isn't very friendly. Also remove the comment
saying that the move string doesn't have to be null-terminated,
because now it does.
It can't signal an error, but it's worth documenting that it can
receive invalid input and should do what it can with it. I assume
that failing to decode game_ui data from a newer version generally
won't be disastrous the way failing to decode a description or move
string would be.
Previously if a move string starting with "M" contained anything else
other than a digit or a comma, execute_move() would spin trying to
parse it. Now it returns NULL.
Other games tolerate receiving an encoded game_ui even if they can
never generate one. This is sensible, since it means that if a new
version starts saving UI state, old versions can load save files
generated by those newer versions.
This makes sure that width * height <= INT_MAX, which it rather needs
to be. Also a similar check in decode_params when defaulting the
number of mines.
This makes sure that width * height <= INT_MAX, which it rather needs
to be. Also a similar check in decode_params when defaulting the
number of regions.
Every grid shape has its own limit, so this involved adding a new
interface between loopy.c and grid.c. The limits are based on ensuring
that the co-ordinate system of the grid doesn't overflow INT_MAX and
neither do the lengths of the face and dot arrays.
Though now I come to look at it I think the actual limits of grid.c are
much lower. Hmm.
Without this check, a corrupt save file can include a move that
removes an edge of the grid, and then is_solved() walks off the edge
of the grid causing a buffer over- or under-run.
To demonstrate the bug, load this save file in a build with
AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :8:Palisade
PARAMS :5:5x5n5
CPARAMS :5:5x5n5
DESC :0:
NSTATES :1:2
STATEPOS:1:2
MOVE :6:F0,0,1
If we start seeing state records in a save file (MOVE, SOLVE, or
RESTART), we should already have seen STATEPOS, so emit an error if not.
This avoids the situation where we overrun the end of the state array
because we're continuing loading states in the hope a STATEPOS will come
along. I've also added an assertion that we're not overrunning the
state array for added paranoia.
An earlier version of this fix just removed the test for data.statepos
at the head of the loop, but that's wrong for a file that only has the
initial state.
This bug can be demonstrated by building Bridges with AddressSanitizer
and loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Bridges
PARAMS :13:7x7i30e10m2d0
CPARAMS :13:7x7i30e10m2d0
DESC :24:a4b4a1g1a2a8a4a4m2b2b3e3
NSTATES :1:2
MOVE :10:L1,0,4,0,1
MOVE :10:L1,0,4,0,2
"1999999999999999999999999999999999999999999999999999" as a record
length should lead to an error, not a buffer overrun.
(fun fact that was less obvious to me than it should have been: very
large powers of ten are multiples of large powers of two, so that number
is -1 mod 2^32)
This bug can be demonstrated by building any puzzle with
AddressSanitizer and then loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1999999999999999999999999999999999999999999999999999:1
