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.
Simon points out that if you have an ambiguous puzzle then it might
need different flash data for different solutions, so if you solve it
one way, than manually unsolve it and solve it another way, the old
flash data in the game state need to be cleared out when the new flash
data are written.
Tested by solving the hugely ambiguous "5x5:CwC,5,5,5,5,S5,S5,5,5,5,5".
COL_GRID used to be mid-way between the usual COL_BACKGROUND and
COL_TRACK_BACKGROUND. But then I changed mkhighlight() so that it
didn't make COL_BACKGROUND so dark and that made the grid lines
indistinguishable from the track background.
Now COL_GRID is generated from COL_BACKGROUND and COL_TRACK_BACKGROUND
so as long as those are sufficiently distinct from each other, COL_GRID
will be distinct from both of them.
Quite a few backends currently generate colours by multiplying the
foreground colour by a fraction, effectively mixing it with black. On a
black background, this might be reasonably replaced by mixing the
background colour with white, but that's rather arithmetically fiddly.
Happily, I already have a function for that and just need to expose it.
Normally, Tracks puts error marks on row and column clues only when one
of the existing track or no-track marks will have to be removed to
satisfy the clue. This could lead to a situation where the player had
built a track from A to B and got neither a win nor a highlighted error
because the only error was in a row or column having too few track
segments.
This commit re-arranges the completion checking so that if there's a
complete track from A to B and no spurious track then the game will
highlight any clue that isn't matched by the actually laid track. This
should mean that any solution with a track from A to B will either be a
win or have a highlighted error.
This should fix Android issue #266.
This applies to various square-filling games: Keen, Solo, Towers,
Undead, and Unequal. In all cases, selecting a square and pressing the
number that was already in it, or selecting an empty square and pressing
Backspace, would add a move to the undo chain that did nothing. This
also meant that the convention where Backspace from the top level of an
application in KaiOS leaves the application didn't work.
Now the various interpret_move() functions check the current state of
the grid, and return NULL or UI_UPDATE where a move wouldn't change the
board. UI_UPDATE is returned in the case where the cursor was put in
place using the mouse, because in those cases I think the cursor should
still be hidden again. NULL is returned when the cursor was put in
place by keyboard, because then there's really nothing to do.
Before it would not only generate an invalid guess, but also override
the usual rules to allow you to submit it. Now guesses are only
provided if they're valid, and I've adjusted the maximum-colour finder
so that the code can actually find the correct guess. This should have
no effect on the behaviour with "Allow duplicates" turned on.
(2x2 with four-way symmetry must be either all-black (trivial) or
all-white (ambiguous). --bjh21)
(cherry picked from Android port, commit
27ae898e118b0a31a98d393bf56aa138845123e6)
