The whole purpose of a dsf is that you can traverse the edges of your
graph in any order you feel like. So if you want to build the
connected components of a graph you can just loop over all the edges
once. There's no need to run a depth-first search.
In fact there were an amazing number of things wrong with this 10-line
function:
- As Ben points out in commit 21193eaf9308ace, it didn't bother with
bounds checking when searching the grid, instead relying on the
never-removed grid boundary to stop the search - which was fragile in
the face of other bugs.
- The recursion uses linear stack, which is much worse than linear
heap, since stacks are often much more limited. (And the dsf _also_
used linear heap.)
- The recursion was completely unnecessary.
- The function used internal knowledge about dsf.c in order to define
the value UNVISITED to match what would happen to work.
- The name 'dfs_dsf' is totally confusing and almost impossible to
type!
The "solve" operation in Inertia generates a proposed solution as a
move string. But if such a move string is loaded from a save file it
might not actually describe a solution. If that happens then it's
possible to reach the end of the "solution" without winning, and doing
so should probably cause a recalculation of the solution rather than
an assertion failure ("execute_move: Assertion `ret->solnpos <
ret->soln->len' failed.").
I am a little concerned by the way that normal solve operations end up
encoded in the save file, but the re-solvings caused by going off
course don't, but I haven't got a good answer to that.
Here's a save file that demonstrates the assertion failure:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :7:Inertia
PARAMS :3:8x8
CPARAMS :3:8x8
DESC :64:sbgwsmswwgggwggmmbwgwbssbwbsbwbbwsSmwbbsbbmggbmssgmgwbmmwmbmmwsw
NSTATES :2:3
STATEPOS:1:1
MOVE 000:2:S0
MOVE 000:2:00
While they couldn't be generated in normal play, execute_move() would
permit lines and marks across the edge of the grid that would then
generate assertion failures ("dsf_update_completion: Assertion
`INGRID(state, bx, by)' failed.").
I've added a check to execute_move() that after updating a square, the
square doesn't have any lines or marks that leave the grid.
This save file demonstrated the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSZON :1:1
GAME :5:Pearl
PARAMS :5:5x6dt
CPARAMS :5:5x6dt
DESC :6:eeeeee
NSTATES :1:2
STATEPOS:1:1
MOVE :6:F1,4,2
Also that a line indicating no domino be between adjacent squares.
Without this, execute_move would allow you to place dominos and edges
between any pair ot squares, and then generate assertion failures
("execute_move: Assertion `d2 - w >= 0' failed." and "execute_move:
Assertion `d1 - w >= 0' failed.") when a domino was placed over an
invalid edge. This example save file demonstrates the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :8:Dominosa
PARAMS :1:6
CPARAMS :1:6
DESC :56:55521461210004364611033535444421636022603153156422620503
NSTATES :1:3
STATEPOS:1:3
MOVE :4:E0,2
MOVE :4:D0,2
Specifically, TENT and NONTENT markers ('!' and '-') cannot appear as
the first or last character of a description, because that would
attempt to place them on squares outside the grid. This was caught by
assertions in new_game(), as can be demonstrated by feeding these
descriptions to older versions of Tents: "4:-p,0,0,0,0,0,0,0,0"
("new_game: Assertion `i >= 0 && i <= w*h' failed.") and
4:p-,0,0,0,0,0,0,0,0 ("new_game: Assertion `*desc == ','' failed.").
The maximum length of a column move in Netslide is the height of the
puzzle, and the maximum length of a row move is the width, not the
other way around.
Moves of absolute length more than 1 can't be generated by
interpret_move(), but they can come from save files. On non-square
grids, the incorrect check led to assertion failures: "0 <= tx && tx <
w" and "0 <= ty && ty < h". This save file demonstrates the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :8:Netslide
PARAMS :3:4x9
CPARAMS :3:4x9
DESC :39:0000000000000h0h0000000000000000000000v
NSTATES :1:2
STATEPOS:1:2
MOVE :4:R0,5
The solver, when it decided that an edge or square should be both TRACK
and NOTRACK, would correctly decide that the puzzle was insoluble, but
would also mark the edge with both flags in its working copy. This
could then lead to assertion violations if that working copy of the
board was used for something else, for instance if it was fed back into
the solver. This couldn't happen in normal play, since failed solutions
just cause the solve command to fail, but the diagnostic "H" command
could trigger it from a save file, causing an assertion failure:
"state->sflags[y*state->p.w + x] & S_CLUE".
Now when the solver runs into this situation, it marks the puzzle as
insoluble but doesn't set the invalid flag, so the board remains valid
and future solve operations are safe.
This save file is the one that demonstrated the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :12:Train Tracks
PARAMS :5:6x6t0
CPARAMS :5:6x6t0
DESC :31:b0t9l,,S0,00,0,0,4,0,0,S0,0,0,0
NSTATES :1:8
STATEPOS:1:2
MOVE :1:H
MOVE :1:H
MOVE :1:H
MOVE :1:H
MOVE :1:H
MOVE :1:H
MOVE :1:H
It can't happen in normal play, but if a save file had a "C" (clear
around) move that set off a mine, Mines could end up hitting an
assertion failure, "ncovered >= nmines". This was because it would
check if the player had won by counting covered squares and mines, and
of course an uncovered mine is no longer a covered square but is still a
mine.
Since winning after you're dead isn't possible (at least in Mines), we
now skip the check entirely if the player has already died.
This save file demonstrates the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :5:Mines
PARAMS :1:7
CPARAMS :1:7
DESC :22:r31,u,0000C000d0000020
NSTATES :1:2
STATEPOS:1:1
MOVE :4:C6,2
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.
