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.
Now there's a single version of the main loop that runs once in normal
mode and repeatedly in AFL++ persistent mode. In persistent mode,
fmemopen() allows the loop to read the shared-memory buffer as though
it were a stdio stream. fmemopen() is POSIX-only, but so is AFL++.
Rather than a save file from standard input and then exiting, this reads
it from a shared memory buffer and then loops. This makes fuzzing
_much_ faster: one core on my laptop can now load about 30,000 save
files per second.
As far as I can tell, nothing that uses nullfe.c depends on these
functions. On the other hand, they do make it impossible to include
nullfe.c and midend.c in the same program. If anything does turn out to
need them, they can be spun off into nullme.c or something.
The null get_random_seed required a little care. It's not OK for it to
return NULL and zero because NULL isn't a valid argument to memcpy()
even with a length of zero (thank you UBSan). So we return a single
zero byte instead.
I'd failed to initialise the row clue string to empty so it got the
contents of the previous row to be displayed. This could only happen
for imported descriptions, or for puzzles with one or two columns.
Thanks to Glen Sawyer for spotting and reporting the bug.
There are lots of places where Puzzles formats integers into
fixed-length buffers using sprintf() with a "%d" format. This isn't
very safe, since C doesn't guarantee any particular maximum size for an
"int". However, the restrictions on representations of integers means
we can infer an upper bound using sizeof(), CHAR_BIT, and an
approximation to the binary log of 10.
If all the column clues are single digits, then the font can be larger
without adjacent clues getting too close to each other. I've also
slightly increased the font size even with two-digit clues, from
TILE_SIZE/2 to TILE_SIZE/1.8.
Rather than centring each clue in an imaginary box, put equal amounts of
space between them (using space characters) and then right-align the
whole string. This makes for more efficient use of space, and I think
it also looks better when there are two-digit clues.
This does introduce a rather grotty (if large) fixed-size buffer for row
clues. Probably the row clues should be pre-formatted into a string
when the puzzle is set up.
Since the drawing API offers no guarantees about where text drawing
might have effects, it's unsafe to erase text other than by wiping the
clipping rectangle that was in effect when it was drawn. In practice,
this means that all variable (including colour) text should be drawn
with a narrow clipping rectangle.
It wasn't actually a practical problem that Pattern didn't clip its
clues, but I'll like to make its fonts less tiny, which will make it
more likely to need clipping in odd cases.
This also factors out some repeated computations in draw_numbers().
The square.pl script removed all pixels that were the same colour as the
edge ones, even if that meant removing all of the pixels. Now it stops
removing pixels at 1x1 so that there will be something left for
ImageMagick to work on.
All the not-quite-puzzle GUI programs (only galaxieseditor at this
point) were using the extra source files from Null Game, presumably as a
convenient way of asking not to have an icon on platforms where icons
are provided as extra source files. Unfortunately, this all went wrong
if Null Game did have a save file for an icon, causing CMake to complain
about multiple definitions of a target.
Now these programs look for extra files under their own names, which
will work just as well since those have no save files to generate icons
from either.
