Bug#473673: boswars: fails to load a saved game

[Kalle Olavi Niemitalo]

The upstream patch r9536 does fix the SIGSEGV with duck2.sav.gz
attached to this bug report.

However, the problem with duck7.sav.gz is more insidious.  With a
debug build of boswars, that file causes this assertion to fail
in CclUnit when slot == 52:

               Assert(unit->Slot == slot);

unit->Slot actually has the correct value earlier but it gets
corrupted due to a buffer overflow in CclParseMove at unit ==
UnitSlots[51].  duck7.sav.gz contains this (whitespace changed):

  "orders", {{"action-die",
      "range", 0, "width", 0, "height", 0, "min-range", 0,
      "tile", {-1, -1}, "type", "unit-assault",},},
  "saved-order", {"action-still",
    "range", 0, "width", 0, "height", 0, "min-range", 0,
    "tile", {-1, -1},},
  "new-order", {"action-still",
    "range", 0, "width", 0, "height", 0, "min-range", 0,
    "tile", {-1, -1},},
  "data-move", {"fast", "path", {
      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, 105, 1, 0, 0, 1, 0, 0, 0, 52, 0, 0, 0, -112, 9,
      39, 8, 96, 105, 25, 8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, -93, 0, 0, 0, -84, 0, 0, 0, 40, 26, 99, 8, -56, 100,
      25, 8, -124, 96, 99, 8, },})

CUnit has room for at most MAX_PATH_LENGTH = 28 bytes in the path
but this list in the save file provides 102 bytes, so it
overwrites first the tail of the CUnit object and then also part
of the next CUnit and any heap-block headers in between.
(CUnitManager::AllocUnit uses "new CUnit" to allocate each object
separately from the heap.)

I suppose Bos Wars should be changed to detect the excessively
long path and reject the save file as corrupt, thus preventing
the buffer overflow.  However, it is also a bug that Bos Wars
saved this overlong path data in the fist place.  That bug
appears to be in SaveUnit, which saves a "data-move" section by
default even though UnitActionDie does not use any data.  This
data has just been left in the union by some other action and
then incorrectly treated as CUnit::_order_data_::_order_move_.
This has not yet been fixed in the upstream SVN repository.

Actually, I wonder if the "data-move" section could just always
be omitted.  If there's no precomputed path in the save file,
then the unit could just compute a new one when the file is
loaded.  That might cause the unit to treat other units as
obstacles and choose a far longer path, though.

Do you think it would make sense to clone this bug report, for
tracking the two different crashes separately?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-games-devel/attachments/20100114/cf887e91/attachment.pgp>