Despite the name, COMPILE_DEFINITIONS was only ever used to set a
single definition, and as far as I can tell that's all it could do
even when I tried to put them in a single word separated by
semicolons. Turning COMPILE_DEFINITIONS into a multi-valued argument
seems to make it work much better.
Assertion failures are ugly, but they're better than the alternative.
Defensive coding is a general principle throughout Puzzles and I don't
think it's sensible to selectively turn that off.
The mechanism by which we re-enable assertions is stolen from PuTTY
(with an enhancement to cover MinSizeRel builds as well) and is pretty
ugly because CMake doesn't seem to have a good way to do it.
I'm not quite sure how useful it will be, but it does at least catch
an assertion failure in main() and present an opaque message in a box,
which is better than stopping and putting a message in the console
where no-one will see it.
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
