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 <armbru@pond.sub.org>

diff --git a/src/lib/commands/boar.c b/src/lib/commands/boar.c
index 72f95dd0d25a6aafe9375452d0b18a125b564b03..356eb3797d7b7bb8c391ed00d8520ce3cf453d9b 100644 (file)
--- a/src/lib/commands/boar.c
+++ b/src/lib/commands/boar.c
@@ -28,6 +28,7 @@
  *
  *  Known contributors to this file:
  *     Ken Stevens, 1995
  *
  *  Known contributors to this file:
  *     Ken Stevens, 1995

+ *     Markus Armbruster, 2011-2015

  */
 
 #include <config.h>
  */
 
 #include <config.h>


@@ -52,7 +53,7 @@ boar(void)
     struct sctstr sect;
     struct lndstr land;
     struct nstr_item ni;
     struct sctstr sect;
     struct lndstr land;
     struct nstr_item ni;

-    int foundland;
+    int foundland, def_uid;

     char *p;
     char buf[1024];
 
     char *p;
     char buf[1024];
 


@@ -155,11 +156,16 @@ boar(void)
      * Death, carnage, and destruction.
      */
 
      * 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))) {
     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');
        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 a3acccb326c045cf9d52b04f5a24c9517075c3ca..4248f19e31e9bbf628012fe727aa0359547750a9 100644 (file)
--- 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;
        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);
 }
 
     att_get_combat(com, com->own != player->cnum);
 }
 

diff --git a/tests/retreat/01-retreat-1 b/tests/retreat/01-retreat-1
index 5b9fcb683924c40390ee738bf71eb3d5949e06a1..d52c9e0b7a7e80eadcf9a5229b7c5c56ee86b474 100644 (file)
--- 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
 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
 __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 1a978003d61e381b1669d0a8b1239edb047ff24d..8509c993c16bb7e067282092608342c5cf71ca25 100644 (file)
--- 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
 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
 tester@127.0.0.1 logged out, country #1
 Connect from 127.0.0.1
 tester@127.0.0.1 using country #0