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
Normal moves shouldn't be allowed to write outside the board. This
buffer overrun can be demonstrated by building Undead with
AddressSanitizer and loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :6:Undead
PARAMS :5:4x4dn
CPARAMS :5:4x4dn
DESC :48:5,0,5,cRRaLRcLRc,0,2,1,3,1,0,0,3,4,3,2,3,4,2,1,1
NSTATES :1:2
STATEPOS:1:2
MOVE :3:Z10
The guessable squares are numbered up to num_total, not "wh". The
latter includes mirror squares that aren't included in the various
arrays describing the game state.
To reproduce the problem, build Undead with AddressSanitizer and press
"M".
Previously, Undead's execute_move would go into a spin when it
encountered an unexpected command character in a move string. Now it
rejects the move instead.
The code that actually executes the moves can only cope with moves of
at most the width (or height as appropriate) of the grid. Reject any
longer move, and for symmetry also negative moves of the same
magnitude.
Without this, the tile-moving code tends to access off the start of the
tile array. To demonstrate this, build Sixteen with AddressSanitizer
and load this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Sixteen
PARAMS :3:4x4
CPARAMS :3:4x4
DESC :38:2,16,3,10,13,8,7,4,9,14,12,11,15,1,5,6
NSTATES :1:2
STATEPOS:1:2
MOVE :4:C1,9
Also add a corresponding assertion to the underlying move primitive.
Without this limit, long moves cause a buffer overrun.
To demonstrate the problem, build Netslide with AddressSanitizer and
load this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :8:Netslide
PARAMS :3:4x4
CPARAMS :3:4x4
DESC :16:49b59aca247714b4
NSTATES :1:2
STATEPOS:1:2
MOVE :5:R3,51
Only numbers and lower-case letters are allowed. Without this
restriction, a buffer overrun is possible.
To demonstrate the problem, load this save file into a build of Mosaic
with AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :6:Mosaic
PARAMS :7:8x8a0h1
CPARAMS :7:8x8a0h1
DESC :41:b2c3b~~2a5c6e3a55c6a5a4244e0c3a64d4b4232b
NSTATES :1:1
STATEPOS:1:1
Peg colours in the current guess must be within the range of colours
for the current game, just as they must be for completed moves.
Otherwise is_markable() can cause a buffer overrun.
Since there's no way for decode_ui() to report an error, it just ignores
the bad peg colours. I've also added an assertion to catch this problem
in is_markable().
The following save file demonstrates the problem when loaded in a build
with AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Guess
PARAMS :9:c6p4g10Bm
CPARAMS :9:c6p4g10Bm
DESC :8:2de917c0
UI :7:7,7,7,7
NSTATES :1:1
STATEPOS:1:1
If the game is solved (either by a win or a loss), interpret_move()
can never return a move, but execute_move() should also reject any
moves in case we're loading a corrupt or malicious save file.
Otherwise a save file with more guesses than the maximum allowed can
cause a buffer overrun.
This save file demonstrates the problem when loaded into a build of
Puzzles with AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Guess
PARAMS :9:c6p4g1Bm
CPARAMS :9:c6p4g1Bm
DESC :8:b5f3faed
NSTATES :1:3
STATEPOS:1:3
MOVE :8:G1,1,2,2
MOVE :8:G4,3,1,1
The call to cliprogram() doesn't actually add the target 'fuzzpuzz' on
that platform, so the subsequent target_include_directories fails. Fix
is to condition target_include_directories on the build_cli_programs
flag.
It consists of two parts. One is the list of all record types used by
the serialiser, to make it easy for AFL++ to find them. The other is
the "interesting" integers used by AFL++ converted to ASCII decimal
form because Puzzles save files are coded in decimal and this will
help AFL++ to guess good values. I hope.
