]> git.pond.sub.org Git - empserver/commit
navigate march: Fix use-after-free and other bugs
authorMarkus Armbruster <armbru@pond.sub.org>
Mon, 5 Jan 2015 15:32:25 +0000 (16:32 +0100)
committerMarkus Armbruster <armbru@pond.sub.org>
Mon, 2 Mar 2015 07:20:47 +0000 (08:20 +0100)
commit24000b4855d2025c8f5de78a0286ff7ddb17fb7f
treee6ee0399830133e8067fe532793071f15128ea05
parent198e2dd076beac3b80bf713c7c8142aadbb02423
navigate march: Fix use-after-free and other bugs

unit_move() is too big and has too many paths through its loop.
Maintenance of the (unspoken) loop invariant isn't obvious.  In fact,
it isn't maintained on some paths.  I found several bugs:

* We check prerequisite conditions for moving before the first move
  and around prompts.  When a condition becomes wrong on the move,
  movement continues all the same until the next prompt.  I believe
  the only way this can happen is loss of crew due to hitting a mine.

* We cache ships and land units in a list of struct ulist.  When a
  ship or land unit gets left behind, its node is removed from the
  list and freed.

  We keep pointer flg pointing to the flagship in that list for
  convenience.  However, the pointer isn't updated until the next
  prompt.  It's referenced for automatic radar and all sub-commands
  other than the six directions and 'h'.  Use after free when such a
  sub-command gets processed after a flagship change without a prompt.
  Same for land units.  For instance, navigating a pair of ships "jh"
  where the flagship has no mobility leaves the flagship behind, then
  attempts to radar automatically using the ship in the freed list
  node.  Likewise, marching a similar pair of land units "jl" examines
  the land unit in the freed list node to figure out how to look.

* We cache mobility in the same list to support fractional mobility
  during movement.  Movement deducts from cached mobility and writes
  the result back to the ship or land unit.

  If something else charges it mobility while it's in this list, the
  cache becomes stale.  shp_nav() and lnd_nav() reload stale caches,
  but don't run often enough.  For instance, when a ship hits mines,
  the mine damage makes the cache stale.  If a direction or 'h'
  follows directly, the stale mobility is written back, clobbering the
  mine hit's mobility loss.

This mess dates back to Empire 2, where it replaced a different mess.
There may be more bugs.

unit_move()'s complex control flow makes reasoning about its loop
invariant too error-prone.  Rewrite the mess instead, splitting off
sensible subroutines.

Also fixes a couple of minor annoyances:

* White-space can confuse the parser.  For instance, "jg l" is
  interpreted like "jgll".  Fix to reject the space.  Broken in commit
  0c12d83, v4.3.7.

* The flagship uses radar automatically before any sub-command (since
  Chainsaw), and all ships use it automatically after a move (since
  4.2.2).  Make them all use it before and after each sub-command,
  whether it's a move or not.

* Land units don't use radar automatically.  Make them use it just
  like ships.

* Always report a flagship / leader change right when it happens, not
  only before and after a prompt.

Left for another day, marked FIXME: BTU charging is unclean.

Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
13 files changed:
include/land.h
include/ship.h
include/unit.h
src/lib/commands/marc.c
src/lib/commands/navi.c
src/lib/subs/lndsub.c
src/lib/subs/shpsub.c
src/lib/subs/unitsub.c
tests/navi-march/01-navigate-1
tests/navi-march/02-march-1
tests/navi-march/final.xdump
tests/navi-march/journal.log
tests/smoke/journal.log