interpret_move() couldn't generate them, but execute_move() also needs
to forbid them to defend against corrupt save files. I don't think this
actually caused any crashes, but it did cause unexpected "1" squares not
adjacent to mines.
There are various functions pointed to by the game structure that are
only called if certain flags are set. Some of them were documented as
not being called when the flags were clear, but there was no explicit
statement that the pointers could simply be NULL in that case. Now
there is.
If can_configure is false, then the game's configure() and
custom_params() functions will never be called. If can_solve is false,
solve() will never be called. If can_format_as_text_ever is false,
can_format_as_text_now() and text_format() will never be called. If
can_print is false, print_size() and print() will never be called. If
is_timed is false, timing_state() will never be called.
In each case, almost all puzzles provided a function nonetheless. I
think this is because in Puzzles' early history there was no "game"
structure, so the functions had to be present for linking to work. But
now that everything indirects through the "game" structure, unused
functions can be left unimplemented and the corresponding pointers set
to NULL.
So now where the flags mentioned above are false, the corresponding
functions are omitted and the function pointers in the "game" structures
are NULL.
Without this, "1:O" causes an assertion violation, '!"new_ui found
nowhere for cursor"'. We may as well require two pegs and one hole,
since that's the minimum for a game in which there are any moves to
make.
Without this, it's possible to specify a description that has more
mines than there are places on the board to place them, which
eventually leads to a division by zero. This can be demonstrated by
entering a game description of "3:r8,u," and then clicking anywhere on
the board.
If a save file contained a solve move as the first move, Mines would
dereference a null pointer trying to look up the (at that point
undetermined) mine locations. Now execute_move() politely returns
NULL instead.
This save file demonstrates the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Mines
PARAMS :5:3x3n0
CPARAMS :5:3x3n0
DESC :127:r0,u,7a142789cabddc3fc4dcb7d2baa4a4937b33c9613ea870ac098e217981ad339930af585557d62048ea745d05b01475d9699596b394cc0adeebf0440a02
UI :2:D0
TIME :1:0
NSTATES :1:2
STATEPOS:1:2
SOLVE :1:S
A solve move containing characters other than digits and commas would
cause an assertion failure, "*p == ','", in execute_move(). Such a move
can't as far as I know be generated in play, but can be read from a
corrupt save file.
Here's a sample of such a save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Flood
PARAMS :7:3x3c6m5
CPARAMS :7:3x3c6m5
DESC :12:403011503,10
NSTATES :1:2
STATEPOS:1:2
SOLVE :2:SA
This avoids an assertion failure, "oldcolour != newcolour" in fill(),
by catching it it execute_move(). As far as I know this couldn't be
triggered from the UI, but it could be demonstrated with this save
file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :5:Flood
PARAMS :1:3
CPARAMS :1:3
DESC :12:231353400,11
NSTATES :1:3
STATEPOS:1:3
MOVE :2:M3
MOVE :2:M3
Technically, a game with no islands is always solved, but it causes a
null-pointer dereference at startup because there's nowhere to put the
cursor. Games with one island are always insoluble because the island
must have at least one bridge and there's nowhere for it to go. So
the minimum playable game has two islands.
To demonstrate the segfault, try loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Bridges
PARAMS :1:3
CPARAMS :1:3
DESC :1:i
NSTATES :1:1
STATEPOS:1:1
It's a bit of a nuisance at the moment if you're trying to get to the
documentation by pressing the up arrow, so I've moved it above the
documentation links, which is where I keep expecting to find it. I
probably want to do some larger-scale menu re-organisation on KaiOS,
though.
This means that you'll get the right font if you one the app page on a
desktop browser (assuming you have Open Sans installed). Also my
Nokia 800 Tough seems to default to a different font (maybe Noto
Sans?) and I'd prefer to have the same font everywhere.
Kai recommend five to ten seconds, and five seconds is short enough
that I get timeouts when testing that are hard to distinguish from
having screwed up my content security policy.
Ben tells me that his recent work in this area was entirely driven by
fuzzing: he added bounds checks in validate_params when the fuzzer had
managed to prove that the lack of them allowed something buggy to
happen.
It seemed worth doing an eyeball-review pass to complement that
strategy, so in this commit I've gone through and added a few more
checks that restrict the area of the grid to be less than INT_MAX.
Notable in this commit: cube.c had to do something complicated because
in the triangular-grid modes the area isn't calculated as easily as
w*h, and Range's existing check that w+h-1 < SCHAR_MAX is sufficient
to rule out w*h being overlarge _but_ should be done before w*h is
ever computed.
You can inject one via a game desc string such as "10x10M5m-1", and
it's clearly silly.
_Zero_ balls, on the other hand, are a perfectly fine number: there's
nothing incoherent about a BB puzzle in which the possible numbers of
balls vary from (say) 0 to 5 inclusive, so that part of the challenge
is to work out as efficiently as possible whether there are even any
balls at all.
(We only need to check minballs, because once we know minballs >= 0,
the subsequent check ensures that maxballs >= minballs, and hence, by
transitivity, maxballs >= 0 too.)
This is for defence in depth against security holes either in Puzzles or
in the KaiAds API. I haven't found any documentation of what KaiAds'
CSP requirements are, but allowing scripts and frames from *.kaiads.com
seems to be enough to let the test adverts work.
The default behaviour of JSON::PP's encode_json output is to write the
keys of a hash in Perl's natural hash order, which isn't consistent
between runs of the same script due to hash function randomisation.
This causes my build system to complain when successive builds from
the same source revision don't produce the same output.
Easily fixed: JSON::PP already has a switch to ensure consistent
ordering, it's just a matter of finding it and turning it on.
Rather than installing the documentation only when Halibut is
available, install the documentation whenever it exists. This allows
for the way that Buildscr injects the documentation into official
KaiOS builds.
Simon's build infrastructure doesn't have Halibut available on the same
system as Emscripten, which means that the normal KaiOS build can't
build the documentation. At present, though, the version of the
documentation that's built for the Web page is perfectly acceptable for
KaiOS as well, so we may as well seed the KaiOS build directory with
that.
This is slightly unfortunate because it means that it's difficult to
make changes only to the KaiOS documentation, and I'd like to be able to
do that eventually. We can cross that bridge once Halibut has the
features I want.
The Kai Store makes display of advertisements provided by the KaiAds API
mandatory. I don't want such adverts to be inconvenient for the users,
so I've just gone for adding a menu item that will display one. This is
probably a little too crude, but it's good for testing things.
The actual KaiAds API code is not free software, so it's not included
here. My intention is to add it by hand to the Zip files for Kai Store
uploads. Without it, the advertising code does nothing.
Now pressing "1" to "9" or "0" activate the first ten items of the
current menu, to save so much pressing of the arrow keys. There isn't
any visible indication of this, so it's a bit of an Easter egg.
This is no use on KaiOS itself, but provides a way to open the menu from
the keyboard on desktop browsers for testing (and for
keyboard-accessibility in general).
It wants a 240x130 pixel JPEG. I've gone for rotating the screenshot
a bit because the store overlays text on the picture and I don't want
horizontal lines in the picture confusing the text. I think the store
handles dimming the image, so the picture we produce is at full
brightness.
Even when the virtual pointer is hidden in KaiOS, it still causes :hover
on whatever element is nominally under it, which is confusing. Disable
all the :hover effects as a workaround.
This only happens if something edits manifest.pl to provide a version
number, but Buildscr can do that. KaiOS manifests are documented as
requiring dotted-decimal version numbers. In fact, the restrictions are
much stricter than that, and unacceptable version numbers can break the
KaiStore developer portal. YY.MM.DD version numbers seem to be
acceptable.
They're proving to be a right nuisance and will probably require a
substantial overhaul to how they work across the entire JavaScript
front-end. I don't think any of the functionality provided by the
dialogue boxes is critical, so in the interests of getting a minimum
viable product actually released I've disabled those features.
In most cases, this just involves commenting out bits of HTML. The
"Custom..." menu item is added by C code, though, so there I've fallen
back to the standard Puzzles way to implement a nasty hack: an
environment variable.
Now that our script is loaded using <script defer>, we can rely on the
DOM's being complete as soon as it starts running. So when we declare a
variable to point to a DOM element, we can initialise it with that
element. This saves having these odd initialisations scattered around
the code, usually but not always at the site of first use.
I'd like to be able to do the same thing with the variables that point
to C functions, but the Module.cwrap() call isn't entirely safe before
Emscripten has finished loading the C code.
Previously, we initialised all of the JavaScript event handlers as soon
at the DOM was loaded, and then called main() ourselves once the
Emscripten runtime was ready. This was slightly dangerous since it
depended on none of those event handlers' being called before main().
In practice this was difficult because most of the elements the event
handlers were attached to were invisible, but it did limit what event
handlers could safely be used.
Now, the event handlers are initialised from main(). This makes things
work in a sufficiently conventional way that we can just let the
Emscripten run-time call main() in its usual way, rather than involving
ourselves in the minutiae of Emscripten's startup.
KaiOS (which is based on Firefox OS, formerly Boot to Gecko) runs its
"native" apps in a Web browser, so this is essentially a rather
specialised version of the JavaScript front-end. Indeed, the JavaScript
and C parts are the same as the Web version.
There are three major parts that are specific to the KaiOS build.
First, there's manifest.pl, which generates a KaiOS-specific JSON
manifest describing each puzzle.
Second, there's a new HTML page generator, apppage.pl, that generates an
HTML page that is much less like a Web page, and much more like an
application, than the one generated by jspage.pl. It expects to build a
single HTML page at a time and gets all its limited knowledge of the
environment from its command line. This makes it gratuitously different
from jspage.pl and javapage.pl, but makes it easier to run from the
build system.
And finally, there's the CMake glue that assembles the necessary parts
for each application in a directory. This includes the manifest, the
HTML, the JavaScript, the KaiOS-specific icons (generated as part of the
GTK build) and a copy of the HTML documentation. The directory is
assembled using CMake's install() function, and can be installed on a
KaiOS device using the developer tools.
These are built alongside other icons as part of the GTK build. It
builds new icon sizes of 44 and 88 pixels and then uses ImageMagick to
round off the corners and add a shadow in accordance with the KaiOS
design guide.
Since using the "solve" option doesn't consume a guess, it's safe to
allow it to occur multiple times. Without this, selecting "solve" a
second time causes an assertion failure because solve() returns a move
string that's rejected by execute_move().
Possible solve() could instead refuse to solve an already-solved
puzzle, but that seems needlessly pedantic.
[fixes c84af670b52f09e9e47587584c0559c508d4a37d]
Chris mentioned in the commit message that there was a risk that
illegal moves might be permitted when playing on after a solve. So
I've changed the condition so that it depends only on whether the move
_currently being executed_ is a solve, rather than whether there was a
solve action anywhere in the undo history.
(Also, wrapped overlong lines while I was here.)
Not only does it set the outer edges to NOTRACK, but it may also overwrite
any mistakes the user has previously made elsewhere. Otherwise, the entire
solve is rejected ("Solve unavailable" error on Android) if the user has
made a single mistake, which is inconsistent with the other games.
This may be giving a free pass to corrupted moves that occur after a solve,
so this may still want tightening up in some way, but it's still limited to
squares within the grid, so I agree with Ben's assessment that this is
likely not to be exploitable.
Fixes#584
(cherry picked from Android port, commit
33bd14fb6f7cd760e7218fffd90f3a266b1f4123)
If I re-run cmake in a Unix build directory, it unconditionally
rewrites generated-games.h, which causes fuzzpuzz to be rebuilt. This
is a waste of effort in the extremely common case where the rewritten
generated-games.h is identical to the old one.
Now we write the data to a temporary file first, and use cmake's
'configure_file' command to copy that to generated-games.h, because it
so happens that configure_file checks if the two files are identical
and avoids updating the timestamp on the destination file if so.
(This will presumably also be a beneficial change on any other
platform that uses generated_games.h in the build, such as OS X. I
just hadn't noticed until it hit the build I most often re-run in an
existing build directory.)
cmake 3.21 has a more intuitively spelled command I could have used,
called 'file(COPY_FILE src dst ONLY_IF_DIFFERENT)'. But we currently
permit cmake all the way back to 3.5, so I can't use that.
