From: Markus Armbruster Date: Mon, 12 Jan 2015 19:07:17 +0000 (+0100) Subject: board: Don't retreat ship#0 after failed board sinks ship X-Git-Tag: v4.3.33~61 X-Git-Url: http://git.pond.sub.org/?p=empserver;a=commitdiff_plain;h=ebe4f05d8730852c47abd05200b6a2203842faa9 board: Don't retreat ship#0 after failed board sinks ship The root cause is in put_combat(): after it sinks the ship, it calls att_get_combat(), which treats a combat object with a dead ship as an error, tells the attacker "not in the same sector!", and "recovers" by putting the combat object into an error state. Too hard for me to fix right now, so put in a FIXME comment. The error state trips up retreat. boar() uses the victim's ship number in the combat object to find the ship it may have to retreat. Putting the combat object into an error state sets this number to zero. If that ship exists, and isn't owned by the attacker, and has RET_BOARDED set, it retreats. Oops. Broken when Empire 2 factored out common combat code. Fix by saving the ship number while it's still valid. This uncovers the next bug: we now pass a dead ship to retreat_ship(). Oopses since commit f743f37. Its commit message says "Harmless, but avoid it anyway." Going to revert. Signed-off-by: Markus Armbruster --- diff --git a/src/lib/commands/boar.c b/src/lib/commands/boar.c index 72f95dd0d..356eb3797 100644 --- a/src/lib/commands/boar.c +++ b/src/lib/commands/boar.c @@ -28,6 +28,7 @@ * * Known contributors to this file: * Ken Stevens, 1995 + * Markus Armbruster, 2011-2015 */ #include @@ -52,7 +53,7 @@ boar(void) struct sctstr sect; struct lndstr land; struct nstr_item ni; - int foundland; + int foundland, def_uid; char *p; char buf[1024]; @@ -155,11 +156,16 @@ boar(void) * Death, carnage, and destruction. */ + /* + * Careful: when the fight sinks the ship, putcombat() clobbers + * *def (see FIXME there). + */ + def_uid = def->shp_uid; if (!(att_fight(A_BOARD, off, &olist, 1.0, def, &dlist, 1.0))) { - getship(def->shp_uid, &ship); + getship(def_uid, &ship); if (ship.shp_rflags & RET_BOARDED) { retreat_ship(&ship, 'u'); - putship(def->shp_uid, &ship); + putship(def_uid, &ship); } } diff --git a/src/lib/subs/attsub.c b/src/lib/subs/attsub.c index a3acccb32..4248f19e3 100644 --- a/src/lib/subs/attsub.c +++ b/src/lib/subs/attsub.c @@ -415,6 +415,11 @@ put_combat(struct combat *com) putship(com->shp_uid, &ship); } com->mobcost = 0; + /* + * FIXME if we just sank the ship att_get_combat() will report + * "not in the same sector", and proceed to clobber *com. See + * also the workaround in boar(). + */ att_get_combat(com, com->own != player->cnum); } diff --git a/tests/retreat/01-retreat-1 b/tests/retreat/01-retreat-1 index 5b9fcb683..d52c9e0b7 100644 --- a/tests/retreat/01-retreat-1 +++ b/tests/retreat/01-retreat-1 @@ -146,6 +146,7 @@ board 130 5 board 132 5 50 | BUG: group does not retreat +| BUG: oopses! __cmd added 1 4 0 ||| Land units | BUG: condition b triggers only on hit diff --git a/tests/retreat/server.log b/tests/retreat/server.log index 1a978003d..8509c993c 100644 --- a/tests/retreat/server.log +++ b/tests/retreat/server.log @@ -8,6 +8,8 @@ Connect from 127.0.0.1 Connect from 127.0.0.1 tester@127.0.0.1 using country #1 tester@127.0.0.1 logged in as country #1 +Oops: !sp->shp_own in ../src/lib/subs/retreat.c:79 +Crash dump complete tester@127.0.0.1 logged out, country #1 Connect from 127.0.0.1 tester@127.0.0.1 using country #0