Mines' "struct set" stores co-ordinates within the grid in a pair of
shorts, which leads to very bad behaviour (including heap-based buffer
overruns) if the grid is bigger than SHRT_MAX in either dimension. So
now we don't allow that.
The overrun can be demonstrated by loading this save file, though the
precise crash is quite variable. In particular, you seem to get
better crashes if the file doesn't have a trailing newline.
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
PARAMS :5:06000
CPARAMS :7:6x60000
NSTATES :1:3
STATEPOS:1:2
MOVE :5:C0,00
GAME :5:Mines
DESC :22:r8,u,00000000000000000
MOVE ::
If there are no white squares, then Range's check that all the white
squares form a connected component goes wrong. Skip the check in that
case to avoid an assretion violation ("edsf_canonify: Assertion `index
>= 0' failed."). This can be demonstrated by starting a game with no
clues (e.g. "range 3:i") and then filling in every square.
A corrupt save file can include an "S" move that doesn't give a valid
solution. An assertion failure ("execute_move: Assertion `rc > 0'
failed.") at that point is rude, so now we just don't set the
"completed" flag in that case. We still set the "cheated" flag, to
reward (lack of) effort.
Here's a trivial test case:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :7:Unequal
CPARAMS :1:3
PARAMS :1:3
DESC :17:0,0,0,0,0,0,0,0,0
NSTATES :1:2
STATEPOS:1:2
MOVE :10:S222222222
Similarly to the previous commit, if you started Pearl with at least
some kinds of invalid puzzle (e.g. "6x6:abBfWcWWrBa") and then pressed
'h' to get hints, you could provoke an assertion failure. But this
time the assertion wasn't in the solver itself; the solver gave up
gracefully and didn't crash, but it _did_ leave the links between
squares in the game_state in an inconsistent state, in that one square
was marked as linking to its neighbour without the neighbour also
linking back to it. This caused the /* should have reciprocal link */
assertion in dsf_update_completion to fail, when that was called from
check_completion after the solver had finished, to decide whether the
puzzle was now solved.
In this commit, I arrange that whether or not pearl_solve returns a
grid layout that's legal by the rules of the _puzzle_, it at least
returns one that's legal by the rules of the _data representation_, in
that every link between squares is either bidirectional or absent.
This is a better solution than just removing the assertion, because if
the inconsistent data were allowed to persist, it would lead to
further problems in gameplay. For example, if you just remove that
assertion instead of this fix and press 'h' on the example puzzle id
above, you'll find that the non-reciprocal links are actually visible,
in the form of several thick lines that stop at a grid square boundary
instead of connecting two square-centres. (It looks even sillier if
you set PEARL_GUI_LOOPY=y.)
That's a situation that can't be created by a normal move, and if you
try to make normal moves after it (e.g. click one of the weird edges),
you'll find that both sides of the half-link get toggled, so now it's
a half-link the other way round. So not only can't you _create_ this
situation in normal play, you can't get rid of it either!
That assertion in dsf_update_completion was commented out at one
point, and I put it back in commit c5500926bf7458a saying that if it
failed I'd like to know about it. And indeed, I'm glad I did, because
this kind of unfixable wrongness in the resulting game_state was worth
noticing and getting rid of!
In the setup phase of the centralised latin.c solver, we start by
going over the input grid containing already-placed clue numbers, and
calling latin_solver_place to enter each on into the solver's data
structure. This has the side effect of ruling out each number from the
rest of the row and column, and _also_ checking by assertion that the
number being placed is not ruled out.
Those are a bad combination, because it means that if you give an
obviously inconsistent input grid to latin_solver_alloc (e.g. with two
identical numbers in a row already), it will fail an assertion. In
that situation, you want the solver run as a whole to return
diff_impossible so that the error is reported cleanly.
This assertion failure could be provoked by giving either Towers or
Group a manually-constructed game description inconsistent in that
way, and hitting Solve. Worse, it could be provoked during live play
in Unequal, by filling in a number clashing with a clue and then
pressing 'h' to get hints.
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.
