From 1e669472a6f9c2317b9c5d6e28574d81a9e66679 Mon Sep 17 00:00:00 2001
From: Enno Rehling <enno.rehling@gmail.com>
Date: Tue, 7 Jul 2015 09:29:43 +0200
Subject: [PATCH] add a test and fix potential crashes in SABOTAGE command.
 sometimes even a simple test will uncover a ton of small errors.

---
 src/spy.c      | 37 +++++++++++++++++++++++--------------
 src/spy.test.c | 27 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/src/spy.c b/src/spy.c
index 7d9c6529a..73af0148f 100644
--- a/src/spy.c
+++ b/src/spy.c
@@ -394,7 +394,7 @@ static int try_destruction(unit * u, unit * u2, const ship * sh, int skilldiff)
     return 1;                     /* success */
 }
 
-static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur)
+static void sink_ship(region * r, ship * sh, unit * saboteur)
 {
     unit **ui, *u;
     region *safety = r;
@@ -404,6 +404,9 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur)
     message *sink_msg = NULL;
     faction *f;
 
+    assert(r);
+    assert(sh);
+    assert(saboteur);
     for (f = NULL, u = r->units; u; u = u->next) {
         /* slight optimization to avoid dereferencing u->faction each time */
         if (f != u->faction) {
@@ -426,7 +429,7 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur)
             }
         }
     }
-    for (ui = &r->units; *ui; ui = &(*ui)->next) {
+    for (ui = &r->units; *ui;) {
         unit *u = *ui;
 
         /* inform this faction about the sinking ship: */
@@ -471,12 +474,13 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur)
             add_message(&u->faction->msgs, msg);
             msg_release(msg);
             if (dead == u->number) {
-                /* the poor creature, she dies */
-                if (remove_unit(ui, u) != 0) {
-                    ui = &u->next;
+                if (remove_unit(ui, u) == 0) {
+                    /* ui is already pointing at u->next */
+                    continue;
                 }
             }
         }
+        ui = &u->next;
     }
     if (sink_msg)
         msg_release(sink_msg);
@@ -487,19 +491,21 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur)
 int sabotage_cmd(unit * u, struct order *ord)
 {
     const char *s;
-    int i;
+    param_t p;
     ship *sh;
     unit *u2;
-    char buffer[DISPLAYSIZE];
-    region *r = u->region;
-    int skdiff;
+    region *r;
+    int skdiff = INT_MAX;
+
+    assert(u);
+    assert(ord);
 
     init_order(ord);
     s = getstrtoken();
 
-    i = findparam(s, u->faction->locale);
+    p = findparam(s, u->faction->locale);
 
-    switch (i) {
+    switch (p) {
     case P_SHIP:
         sh = u->ship;
         if (!sh) {
@@ -507,10 +513,13 @@ int sabotage_cmd(unit * u, struct order *ord)
             return 0;
         }
         u2 = ship_owner(sh);
-        skdiff =
-            eff_skill(u, SK_SPY, r) - crew_skill(r, u2->faction, sh, SK_PERCEPTION);
+        r = u->region;
+        if (u2->faction != u->faction) {
+            skdiff =
+                eff_skill(u, SK_SPY, r) - crew_skill(r, u2->faction, sh, SK_PERCEPTION);
+        }
         if (try_destruction(u, u2, sh, skdiff)) {
-            sink_ship(r, sh, buffer, u);
+            sink_ship(r, sh, u);
         }
         break;
     default:
diff --git a/src/spy.test.c b/src/spy.test.c
index e0f8c2043..7e0f715c0 100644
--- a/src/spy.test.c
+++ b/src/spy.test.c
@@ -5,9 +5,11 @@
 #include <kernel/region.h>
 #include <kernel/unit.h>
 #include <kernel/faction.h>
+#include <kernel/order.h>
 #include <kernel/item.h>
 #include <kernel/messages.h>
 #include <util/attrib.h>
+#include <util/language.h>
 #include <util/message.h>
 #include <util/crmessage.h>
 #include <tests.h>
@@ -16,6 +18,7 @@
 
 #include "spy.h"
 
+#include <assert.h>
 #include <stdio.h>
 
 #include <CuTest.h>
@@ -98,12 +101,36 @@ static void test_all_spy_message(CuTest *tc) {
     test_cleanup();
 }
 
+static void test_sabotage_self(CuTest *tc) {
+    unit *u;
+    region *r;
+    order *ord;
+    struct locale *lang;
 
+    test_cleanup();
+    lang = get_or_create_locale("de");
+    locale_setstring(lang, parameters[P_SHIP], "SCHIFF");
+    test_create_world();
+    init_locales();
+
+    r = test_create_region(0, 0, NULL);
+    assert(r);
+    u = test_create_unit(test_create_faction(NULL), r);
+    assert(u && u->faction && u->region==r);
+    u->ship = test_create_ship(r, test_create_shiptype("boat"));
+    assert(u->ship);
+    ord = create_order(K_SABOTAGE, lang, "SCHIFF");
+    assert(ord);
+    CuAssertIntEquals(tc, 0, sabotage_cmd(u, ord));
+    CuAssertPtrEquals(tc, 0, r->ships);
+    test_cleanup();
+}
 
 CuSuite *get_spy_suite(void)
 {
   CuSuite *suite = CuSuiteNew();
   SUITE_ADD_TEST(suite, test_simple_spy_message);
   SUITE_ADD_TEST(suite, test_all_spy_message);
+  SUITE_ADD_TEST(suite, test_sabotage_self);
   return suite;
 }