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
The fix in e4112b3 was incomplete: there was another assertion that could be failed by a save file with an ill-formed solve move. That now gets rejected properly. Here's an example save file to demonstrate the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :5:Flood
PARAMS :7:6x6c6m0
CPARAMS :7:6x6c6m0
DESC :39:000000000000000000000000000000000000,00
NSTATES :1:2
STATEPOS:1:2
MOVE :1:S
A Pearl move string contains a sequence of sub-moves, each of which
can affect the state of the connection between the centre of a square
and one of its edges. interpret_move() generates these in pairs so
that the two halves of a connection between the centres of adjacent
squares stay in the same state.
If, however, a save file contains mismatched half-moves,
execute_move() should ideally return NULL rather than causing an
assertion failure. This has to be checked at the end of the whole
move string, so I've arranged for check_completion() to return a
boolean indicating whether the current state (and hence the move
preceding it) is valid. It now returns 'false' when a connection
stops at a square boundary or when it goes off the board. These
conditions used to be assertion failures, and now they just cause the
move to be rejected.
This supersedes the check for off-board connections added in 15f4fa8,
since now check_completion() can check for off-board links for the
whole board at once.
This save file trivially demonstrates the problem, causing
"dsf_update_completion: Assertion `state->lines[bc] & F(dir)' failed"
without this fix:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
GAME :5:Pearl
PARAMS :5:6x6t0
CPARAMS :5:6x6t0
DESC :17:BbBfWceBbWaBWWgWB
NSTATES :1:2
STATEPOS:1:2
MOVE :6:R1,0,0
For most puzzles, the manual tries to at least mention its origin.
Wikipedia suggests that the precise inventor of the 15-puzzle is a bit
uncertain, but US patent number 207124 definitely describes it in
1878, so "dates from the 1870s" seems pretty solid.
A game description with islands in adjacent grid squares, like
"3x3:11g", shouldn't be allowed. If it is, then bridges between the
islands are invisible and clicking one of them causes an assertion
failure: "Assertion `is_loop->adj.points[j].off > 1' failed."
The code to check this is really rather complex, but I think the
complexity is mostly necessary.
Specifically, a bridge or a non-bridge must connect two islands that
differ in precisely one co-ordinate. Without this, a save file that
tries to connect or disconnect two non-orthogonal islands will cause
"island_join: Assertion `!"island_join: islands not orthogonal."'
failed."
Here's a save file demonstrating the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Bridges
PARAMS :13:3x3i30e10m2d0
CPARAMS :13:3x3i30e10m2d0
DESC :6:b1c1a2
NSTATES :1:2
STATEPOS:1:2
MOVE :10:L0,2,2,0,1
In commit 5030d87903191d5 I gave latin_solver_alloc a return value,
and introduced a check of that value at every call site. One of the
checks was backwards, with the effect that Unequal game generation now
more or less always fails an assertion. For example:
$ unequal --generate 1 4#12345
unequal: unequal.c:1072: gg_best_clue: Assertion `best != -1' failed.
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.)
