For reasons now lost to history, Puzzles generally uses single-precision
floating point. However, C floating-point constants are by default
double-precision, and if they're then operated on along with a
single-precision variable the value of the variable gets promoted to
double precision, then the operation gets done, and then often the
result gets converted back to single precision again.
This is obviously silly, so I've used Clang's "-Wdouble-promotion" to
find instances of this and mark the constants as single-precision as
well. This is a bit awkward for PI, which ends up with a cast. Maybe
there should be a PIF, or maybe PI should just be single-precision.
This doesn't eliminate all warnings from -Wdouble-promotion. Some of
the others might merit fixing but adding explicit casts to double just
to shut the compiler up would be going too far, I feel.
Some headers used macros named like _THING_H for multiple-include
protection. That style of name is reserved in ISO C, though, so I've
replaced it with PUZZLES_THING_H which is my favourite of the other
styles in use.
Also -Wstrict-prototypes and -Wmissing-variable-declarations.
Functions that are called from JavaScript now have a separate
declaration at the top of the file with a comment reminding one to
update emcclib.js if they're changed.
Thanks to Larry Hastings for the patch (tweaked to include drag/release).
(cherry picked from Android port,
commit 377e61b144c518a3d9efba66be08bf00ff6596e8)
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.
