Unlike AFL, Honggfuzz's compiler wrapper doesn't provide a convenient
preprocessor macro, so we have to have CMake detect the existence of
HF_ITER. Also the resulting program can't run outside of Honggfuzz, so
maybe some additional cleverness is called for there as well. Still, it
makes Honggfuzz go ten times faster, which is nice.
I've just enabled a warning that only fires in that mode, so we need
to keep running the build in that configuration to ensure further
instances of the warning aren't introduced.
After Ben fixed all the unwanted global functions by using gcc's
-Wmissing-declarations to spot any that were not predeclared, I
remembered that clang has -Wmissing-variable-declarations, which does
the same job for global objects. Enabled it in -DSTRICT=ON, and made
the code clean under it.
Mostly this was just a matter of sticking 'static' on the front of
things. One variable was outright removed ('verbose' in signpost.c)
because after I made it static clang was then able to spot that it was
also unused.
The more interesting cases were the ones where declarations had to be
_added_ to header files. In particular, in COMBINED builds, puzzles.h
now arranges to have predeclared each 'game' structure defined by a
puzzle backend. Also there's a new tiny header file gtk.h, containing
the declarations of xpm_icons and n_xpm_icons which are exported by
each puzzle's autogenerated icon source file and by no-icon.c. Happily
even the real XPM icon files were generated by our own Perl script
rather than being raw xpm output from ImageMagick, so there was no
difficulty adding the corresponding #include in there.
If you enable -DSTRICT=ON in cmake and also build with clang, it
reports a couple of variables set but not otherwise used. One was
genuinely unused ('loop_found' in loop_deductions in Loopy); the other
is used by debug statements that are usually but not always compiled
out.
Ben added -Wmissing-declarations in commit 3a3e491a8bc624e for gcc
builds, and observed that clang's option of the same name doesn't do
the same job. But clang does _have_ an option to do the same job: it's
just spelled differently. Added -Wmissing-prototypes in clang builds,
so that those will check the same thing.
Clang's -Wmissing-declarations means something quite different, so we
only enable it on GCC. This is slightly silly since Clang's
-Wmissing-declarations is enabled by default, but it makes it clear that
we know they're different.
I noticed commit db3b531e2cab765a00475054d2e9046c9d0437d3 in the history
where Simon added a bunch of "static" qualifiers. That suggested that
consistently marking internal functions "static" is desirable, so I
tried a build using GCC's -Wmissing-declarations, which requires prior
declaration (presumed to be in a header file) of all global functions.
This commit makes the GTK build clean under GCC's
-Wmissing-declarations. I've also adding "static" to a few obviously
internal objects, but GCC doesn't complain about those so I certainly
haven't got them all.
This is mostly so that the function is used at all, but I've also
removed another memory leak from --autotest mode to make it apparently
leak-free. The testing from standard input mode has more leaks than I
want to fix.
GCC warns about that character sequence in a comment. I shouldn't have
assumed that having only edited a comment meant I could get away without
a test build.
Otherwise, if subsequent mouse/finger movement lines up with the previous
drag attempt's start, then suddenly a drag is in progress from there, which
is confusing.
Fixes#588
(cherry picked from Android port,
commit 8ce1bbe460d70a915caf2dbeb30354d22dc8a8ef)
There's now a function, fuzz_one(), that processes a single save file,
and main() arranges to call this a suitable number of times depending
on whether we're in AFL persistent mode or not. This makes things a
bit cleaner, and will probably make adding good support for other
fuzzers, or just switching entirely to the horrible but popular
libFuzzer interface, simpler.
Solo's layout calculations for pencil marks could fail with a tilesize
of 1, generating an assertion failure: "draw_number: Assertion `pbest
> 0' failed." This was reported as Debian bug #905852.
My solution is slightly silly, namely to change a ">" in the test for
whether a new layout is the best so far to ">=". This allows for
finding a (terrible) layout even for tilesize == 1, and also has the
side-effect of slightly preserring wide layouts over tall ones.
Personally, I think that's an improvement.
Mines uses random_upto() to decide where to place mines, and
random_upto() takes a maximum limit of 2^28-1, so limit the number of
grid squares to that (or INT_MAX if someone's still trying to build on
a 16-bit system).
This avoids an assertion failure: "random_upto: Assertion `bits < 32'
failed." which can be demonstrated by this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Mines
PARAMS :5:18090
CPARAMS :5:18090
DESC :11:r9,u,MEdff6
UI :2:D0
TIME :1:0
NSTATES :1:2
STATEPOS:1:2
MOVE :4:O2,1
If execute_move() receieves a move that uses a colour beyond the range
for the current game, it now rejects it. Without this a solve string
containing an invalid colour would cause an assertion failure: "fill:
Assertion `oldcolour != newcolour' failed." While I was in the area I
put a range check on colours for normal moves as well. To demonstrate
the problem, load this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Flood
PARAMS :7:6x6c6m5
CPARAMS :7:6x6c6m3
DESC :39:432242034203340350204502505323231342,17
NSTATES :1:2
STATEPOS:1:2
MOVE :2:S6
To avoid assertion failures while painting it, we need to ensure that
the purported solution in a solve move doesn't include filling with the
current top-left colour at any point. That means checking the first
entry against the current top-left colours, and each later one against
its predecessor.
Apart from "SEED" records, all values in save files generated by Puzzles
should be printable ASCII. This is enforced by assertion in the saving
code. However, if a save file with non-ASCII move strings (for
instance) manages to get loaded then these non-ASCII values can cause an
assertion failure on saving. Instead, the loading code now checks
values for ASCIIness.
This will not only avoid problems when re-saving files, but will also
defend the various internal parsers from at least some evil strings. It
shouldn't invalidate any save files actually generated by Puzzles, but
it will sadly invalidate some of my fuzzing corpus.
Now if the input save file loads correctly, fuzzpuzz asks the back-end
to draw the puzzle. All the drawing operations are no-ops, but this
tests the drawing code in the back-end.
On a triangular grid, Cube allows either d1 or d2 (but not both) to be
zero, so it's important to check that each one is not zero before
dividing by it.
The crash could be triggered by, for instance "cube t0x2".
Mosaic's validate_desc() doesn't write to the description string, so
it has no need to make a copy of it. And if it doesn't copy it, it
can't leak the copy.
Using sfree() rather than free_game() in the error paths meant that
various arrays referenced from the game_state weren't properly freed.
Also one error path didn't free the game_state at all.
Untangle game descriptions are allowed to contain duplicate edges, and
add234() can handle deduping them. However, when add234() reports that
your newly-allocated edge is a duplicate, it's important to free it
again.
sscanf() assigns its output in order, so if a conversion specifier fails
to match, a later "%n" specifier will also not get its result assigned.
In Undead's execute_move(), this led to the result of "%n" being used
without being initialised. That could cause it to try to parse
arbitrary memory as part of the move string, which shouldn't be a
security problem (since execute_move() handles untrusted input anyway),
but could lead to a crash and certainly wasn't helpful.
I found a plausible looking Web page claiming that various different
sizes of cross are soluble, and some of them are quite widespread. I've
enabled the ones that are symmetric enough that the existing game
generator can lay them out.
Unequal has a flags word per cell. Some of those flags are fixed,
like the locations of the ">" signs, but others indicate errors and
are used to allow the player to mark clues as "spent". Move strings
beginning with "F" allow the user to change the "spent" flags, but
they shouldn't allow the user to change any other flags, especially
those marking the constraints.
Without this fix, the following save file gives a "solver_nminmax:
Assertion `x >= 0 && y >= 0 && x < o && y < o' failed" after it adds a
clue that points off the board:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :7:Unequal
PARAMS :3:3e0
CPARAMS :3:3e0
DESC :17:0,0,0,0,0,0,0,0,0
NSTATES :2:3
STATEPOS:1:3
MOVE :6:F2,0,4
MOVE :1:H
The fix in e4112b3 was incomplete: there was another assertion that could be failed by a save file with an ill-formed solve move. That now gets rejected properly. Here's an example save file to demonstrate the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :5:Flood
PARAMS :7:6x6c6m0
CPARAMS :7:6x6c6m0
DESC :39:000000000000000000000000000000000000,00
NSTATES :1:2
STATEPOS:1:2
MOVE :1:S
A Pearl move string contains a sequence of sub-moves, each of which
can affect the state of the connection between the centre of a square
and one of its edges. interpret_move() generates these in pairs so
that the two halves of a connection between the centres of adjacent
squares stay in the same state.
If, however, a save file contains mismatched half-moves,
execute_move() should ideally return NULL rather than causing an
assertion failure. This has to be checked at the end of the whole
move string, so I've arranged for check_completion() to return a
boolean indicating whether the current state (and hence the move
preceding it) is valid. It now returns 'false' when a connection
stops at a square boundary or when it goes off the board. These
conditions used to be assertion failures, and now they just cause the
move to be rejected.
This supersedes the check for off-board connections added in 15f4fa8,
since now check_completion() can check for off-board links for the
whole board at once.
This save file trivially demonstrates the problem, causing
"dsf_update_completion: Assertion `state->lines[bc] & F(dir)' failed"
without this fix:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :5:Pearl
PARAMS :5:6x6t0
CPARAMS :5:6x6t0
DESC :17:BbBfWceBbWaBWWgWB
NSTATES :1:2
STATEPOS:1:2
MOVE :6:R1,0,0
For most puzzles, the manual tries to at least mention its origin.
Wikipedia suggests that the precise inventor of the 15-puzzle is a bit
uncertain, but US patent number 207124 definitely describes it in
1878, so "dates from the 1870s" seems pretty solid.
A game description with islands in adjacent grid squares, like
"3x3:11g", shouldn't be allowed. If it is, then bridges between the
islands are invisible and clicking one of them causes an assertion
failure: "Assertion `is_loop->adj.points[j].off > 1' failed."
The code to check this is really rather complex, but I think the
complexity is mostly necessary.
Specifically, a bridge or a non-bridge must connect two islands that
differ in precisely one co-ordinate. Without this, a save file that
tries to connect or disconnect two non-orthogonal islands will cause
"island_join: Assertion `!"island_join: islands not orthogonal."'
failed."
Here's a save file demonstrating the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Bridges
PARAMS :13:3x3i30e10m2d0
CPARAMS :13:3x3i30e10m2d0
DESC :6:b1c1a2
NSTATES :1:2
STATEPOS:1:2
MOVE :10:L0,2,2,0,1
In commit 5030d87903191d5 I gave latin_solver_alloc a return value,
and introduced a check of that value at every call site. One of the
checks was backwards, with the effect that Unequal game generation now
more or less always fails an assertion. For example:
$ unequal --generate 1 4#12345
unequal: unequal.c:1072: gg_best_clue: Assertion `best != -1' failed.
Mines' "struct set" stores co-ordinates within the grid in a pair of
shorts, which leads to very bad behaviour (including heap-based buffer
overruns) if the grid is bigger than SHRT_MAX in either dimension. So
now we don't allow that.
The overrun can be demonstrated by loading this save file, though the
precise crash is quite variable. In particular, you seem to get
better crashes if the file doesn't have a trailing newline.
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
PARAMS :5:06000
CPARAMS :7:6x60000
NSTATES :1:3
STATEPOS:1:2
MOVE :5:C0,00
GAME :5:Mines
DESC :22:r8,u,00000000000000000
MOVE ::
If there are no white squares, then Range's check that all the white
squares form a connected component goes wrong. Skip the check in that
case to avoid an assretion violation ("edsf_canonify: Assertion `index
>= 0' failed."). This can be demonstrated by starting a game with no
clues (e.g. "range 3:i") and then filling in every square.
A corrupt save file can include an "S" move that doesn't give a valid
solution. An assertion failure ("execute_move: Assertion `rc > 0'
failed.") at that point is rude, so now we just don't set the
"completed" flag in that case. We still set the "cheated" flag, to
reward (lack of) effort.
Here's a trivial test case:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :7:Unequal
CPARAMS :1:3
PARAMS :1:3
DESC :17:0,0,0,0,0,0,0,0,0
NSTATES :1:2
STATEPOS:1:2
MOVE :10:S222222222
Similarly to the previous commit, if you started Pearl with at least
some kinds of invalid puzzle (e.g. "6x6:abBfWcWWrBa") and then pressed
'h' to get hints, you could provoke an assertion failure. But this
time the assertion wasn't in the solver itself; the solver gave up
gracefully and didn't crash, but it _did_ leave the links between
squares in the game_state in an inconsistent state, in that one square
was marked as linking to its neighbour without the neighbour also
linking back to it. This caused the /* should have reciprocal link */
assertion in dsf_update_completion to fail, when that was called from
check_completion after the solver had finished, to decide whether the
puzzle was now solved.
In this commit, I arrange that whether or not pearl_solve returns a
grid layout that's legal by the rules of the _puzzle_, it at least
returns one that's legal by the rules of the _data representation_, in
that every link between squares is either bidirectional or absent.
This is a better solution than just removing the assertion, because if
the inconsistent data were allowed to persist, it would lead to
further problems in gameplay. For example, if you just remove that
assertion instead of this fix and press 'h' on the example puzzle id
above, you'll find that the non-reciprocal links are actually visible,
in the form of several thick lines that stop at a grid square boundary
instead of connecting two square-centres. (It looks even sillier if
you set PEARL_GUI_LOOPY=y.)
That's a situation that can't be created by a normal move, and if you
try to make normal moves after it (e.g. click one of the weird edges),
you'll find that both sides of the half-link get toggled, so now it's
a half-link the other way round. So not only can't you _create_ this
situation in normal play, you can't get rid of it either!
That assertion in dsf_update_completion was commented out at one
point, and I put it back in commit c5500926bf7458a saying that if it
failed I'd like to know about it. And indeed, I'm glad I did, because
this kind of unfixable wrongness in the resulting game_state was worth
noticing and getting rid of!
