In all front ends other than JavaScript, and in JavaScript prior to my
recent changes, MOD_NUM_KEYPAD can only be set on ASCII digits. For
now, enforce this in midend_process_key() so as to avoid inconsistency
between front ends. It might be useful to be able to distinguish
keypad versions of more keys, but that should be co-ordinated across
the code-base.
The two Return/Enter keys have always been treated the same in the
past, but a user complained today that Enter was no longer functioning
as CURSOR_SELECT in the web puzzles.
This happened in commit 9dbcfa765ba59a8, apparently because the web
front end is now translating the Enter key as MOD_NUM_KEYPAD | '\r'
instead of just '\r', and the new code in midend.c is only stripping
off MOD_NUM_KEYPAD for values >= 0x80.
Now it strips MOD_NUM_KEYPAD off C0 control characters as well, so
that only _printable_ ASCII characters can still have that modifier
when they get to the backend - i.e. you can tell numpad digits from
normal digits, and ditto +-* etc. But keypad Enter is now turned into
plain '\r' by the modifier removal code, and then into CURSOR_SELECT.
Other front ends still aren't even bothering to set MOD_NUM_KEYPAD on
the code sent by Enter. But that's fine, because now midend.c
officially doesn't care whether they do or not.
Just checking for the getKaiAd() function by doing "if (!getKaiAd ..."
throws a ReferenceError if it's not defined, and now my error box
catches that. Using "if (getKaiAd === undefined ..." seems to be
acceptable, though.
Element.setPointerCapture() captures both pointer and mouse events, so
we can use our existing mouse handlers and just have a minimal
pointerdown handler that calls setPointerCapture(). This saves (or at
least postpones) all the tedious rewriting referred to in ecd868ac.
This means that now drags beyond the puzzle area work in WebKit-based
browsers, and we don't get deprecation warnings in current Gecko-based
ones. Older Gecko-based browsers continue to use Element.setCapture()
and hence still work correctly.
Any solution actually generated by the solver will contain at least one
move, because it refuses to solve games that are already solved.
However, a save file might contain an empty "solve" move. This causes
an uninitialised read when execute_move() then tries to check if the
next move is in accordance with the solution, because the check for
running off the end of the solution happens after that.
We now avoid this by treating a zero-length "solution" as an invalid
move.
A save file that ended in the middle of a value before the "SAVEFILE"
field had been loaded would cause a read from uninitialised memory.
While technically undefined behaviour this was practically pretty
harmless. Fixed by handling unexpected EOF here the same an
unexpected EOF anywhere else.
This bug could be demonstrated by loading a truncated save file like
this in a build with MemorySanitizer enabled:
SAVEFILE:41:Simo
On a 32-bit system, evaluating 1<<31 causes undefined behaviour because
1 is signed and so it produces signed overflow. UBSan has spotted a
couple of occasions where this happens in Puzzles, so in each case I've
converted the left operand to the unsigned result type we actually want.
It now strips off modifier flags from keys that shouldn't have them and
maps printable characters with MOD_CTRL to the corresponding control
characters. It also catches Ctrl+Shift+Z because that obviously belongs
in the midend.
I've updated the JavaScript front-end to take advantage of these
changes. Other front ends are unchanged and should work just as they
did before.
I've changed my mind already. The other versions of fuzzpuzz all have
different command-line interfaces anyway, so I think the best approach
is to just accept that and decide that precisely how fuzzpuzz works
isn't a defined API. Fuzzing is inherently not an end-user activity, so
I think it's acceptable to make it a bit inconsistent.
This means that in Clang builds you get the non-libFuzzer version of
fuzzpuzz by default (so you can use it with other fuzzers), but if you
turn on WITH_LIBFUZZER then you'll get the libFuzzer version instead.
I've separated out the various versions of main(), which has helped a
little bit. I've also stopped using fmemopen() since libFuzzer might
work on Windows. But I think I probably still have something
fundamentally wrong in my approach.
For AFL++ and Honggfuzz, our approach is to build a standard fuzzpuzz
binary with extra hooks for interacting with an external fuzzer. This
works well for AFL++ and tolerably for Honggfuzz. LibFuzzer, though,
provides its own main() so that the resulting program has a very
different command-line interface from the normal one. Also, since
libFuzzer is a standard part of Clang, we can't decide whether to use it
based on the behaviour of the compiler.
So what I've done, at least for now, is to have CMake detect when we're
using Clang and in that case build a separate binary called
"fuzzpuzz-libfuzzer" which is built with -fsanitize=fuzzer, while the
ordinary fuzzpuzz is built without. I'm not sure if this is the right
approach, though.
This reverts commit 9d7c2b8c83506c1f239c840e372058fac603b255.
I thought that switching from the JS 'mousedown', 'mousemove' and
'mouseup' events to the corresponding 'pointer*' events would make
essentially no difference except that the pointer events would come
with more information. But in fact it turns out that there's a
fundamental change of semantics.
If you press one mouse button down and then, without releasing it,
press a second one, then the mouse API will send you this information
in the form of two 'mousedown' events, one for each button. But the
pointer API will only send you a 'pointerdown' for the first event,
when the state of the pointer changes from 'no buttons down' to 'at
least one button down'. The second button press will be delivered as a
'pointermove', in which the 'buttons' field is different from its
previous value.
I'm backing out the migration to PointerEvent for the moment, because
that's too complicated for a trivial fix. In simple cases we could
easily detect the changed buttons field in the pointermove handler and
generate a call to the C side of this front end's mousedown()
function, effectively converting the changed JS representation to the
one the C was already expecting. But this also has to interact with
our one-button support (converting Ctrl and Shift clicks into a
different logical button) _and_ with the ad-hoc mechanism we use to
avoid delivering buttonless mouse movements to the C side. So getting
it right in all cases at once isn't trivial, and I'd rather revert the
attempt now and think about it later than commit to getting it all
perfect on short notice.
After commit 1470c9530b1cff3 enabled assertions, I found that my build
scripts were complaining that the Windows binaries built from the same
source twice were not generating the same output, and it turned out to
be because the use of __FILE__ in every assert was baking in a
pathname from my build setup containing a mkstemp()-randomised path
component.
I've found the '-fmacro-prefix-map' option, available in both gcc and
clang (except the archaic gcc used by my NestedVM build, alas), which
lets you remap pathnames for purpose of what __FILE__ expands to. So
now our assertion statements should look as if the puzzle source just
lived in /puzzles, and (just in case a pathname in a generated header
ever becomes relevant) the cmake build directory is /build.
If the browser knows what 'PointerEvent' means, then we switch our
'onmousefoo' event handlers to the 'onpointerfoo' events, for both the
puzzle canvas and the resize handle.
The immediate effect of this is that we get to use the
setPointerCapture method on the puzzle canvas, in preference to the
deprecated Firefox-only setCapture.
A pointer event also contains extra fields compared to a mouse event:
as well as telling you which pointing device the event comes from, it
can also provide extra information, such as pressure, or the angle of
a stylus if the hardware can detect it. I don't have any immediate
ideas about what those could be used for, but it can't hurt to have
them available just in case we think of something in future.
element.setCapture only seems to exist in Firefox. On most other
browsers, our attempt to call it must have been generating a whinge in
the console log all along. But Ben's commit bb16b5a70ddf77d turned
that into a prominent alert box, triggered on every mouse click in the
puzzle canvas.
Worked around by wrapping both calls to setCapture in a local
subroutine which checks if it's there before calling it.
Also, setCapture turns out to be deprecated in any case, according to
https://developer.mozilla.org/en-US/docs/Web/API/Element/setCapture .
It looks as if the non-deprecated version is element.setPointerCapture:
https://developer.mozilla.org/en-US/docs/Web/API/Element/setPointerCapture
But it also looks as if that needs the 'pointerId' field that's only
found in 'onpointerdown' events and not 'onmousedown' ones. So
including that as an alternative will be a bigger job.
This is essentially the same fix as 73c7bc090155ab8c was for Twiddle.
The new code is less clever but more correct (and more obviously
correct). The bug could be demonstrated by using a parameter string
of "c" or "m" with an AddressSanitizer build of Flood.
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.
