Merge pull request #327 from ennorehling/feature/coverity-scan

Fix a ton of coverity scan issues, we're now down to 0.88 bugs per 1000 LOC

quicklist

@@ -1 +1 @@
-Subproject commit 45f4577b8205d87b78d2b1f30b5c9baa25c86779
+Subproject commit f837dd31e5fcf13c706db1ac2c86b7de3e706578

s/coverity

@@ -0,0 +1,15 @@
+#!/bin/bash
+set -e
+VERSION="$1"
+shift
+DESC="$*"
+cd Debug
+make clean
+../../coverity/bin/cov-build --dir cov-int make eressea
+tar czf eressea.tgz cov-int
+curl --form token=IISXKH3A1ngZGfFmBz_aSA \
+  --form email=enno.rehling@gmail.com \
+  --form file=@eressea.tgz \
+  --form version="$VERSION" \
+  --form description="$DESC" \
+  https://scan.coverity.com/builds?project=eressea%2Fserver

src/battle.c

@@ -186,7 +186,7 @@ static const char *sideabkz(side * s, bool truename)
 #ifdef SIDE_ABKZ
     abkz(f->name, sideabkz_buf, sizeof(sideabkz_buf), 3);
 #else
-    strcpy(sideabkz_buf, itoa36(f->no));
+    strlcpy(sideabkz_buf, itoa36(f->no), sizeof(sideabkz_buf));
 #endif
     return sideabkz_buf;
 }
@@ -971,6 +971,7 @@ void drain_exp(struct unit *u, int n)
     }
     if (sk != NOSKILL) {
         skill *sv = unit_skill(u, sk);
+        if (sv) {
             while (n > 0) {
                 if (n >= 30 * u->number) {
                     reduce_skill(u, sv, 1);
@@ -983,6 +984,7 @@ void drain_exp(struct unit *u, int n)
                 }
             }
         }
+    }
 }
 
 const char *rel_dam(int dam, int hp)

src/bind_config.c

@@ -65,20 +65,26 @@ int config_read(const char *filename, const char * relpath)
         F = fopen(filename, "rt");
     }
     if (F) {
-        int result;
+        long size;
-        char *data;
-        size_t sz;
 
         fseek(F, 0, SEEK_END);
-        sz = ftell(F);
+        size = ftell(F);
         rewind(F);
-        data = malloc(sz);
+        if (size > 0) {
-        fread(data, 1, sz, F);
+            int result;
+            char *data;
+            size_t sz = (size_t)size;
+
+            data = malloc(sz+1);
+            sz = fread(data, 1, sz, F);
+            data[sz] = 0;
             fclose(F);
             result = config_parse(data);
             free(data);
             return result;
         }
+        fclose(F);
+    }
     return 1;
 }
 

src/bind_region.c

@@ -456,7 +456,7 @@ static int tolua_region_create(lua_State * L)
         assert(!pnormalize(&x, &y, pl));
         r = result = findregion(x, y);
 
-        if (terrain == NULL && r != NULL && r->units != NULL) {
+        if (r != NULL && r->units != NULL) {
             /* TODO: error message */
             result = NULL;
         }
@@ -465,10 +465,10 @@ static int tolua_region_create(lua_State * L)
         }
         if (result) {
             terraform_region(result, terrain);
-        }
             if (result->land) {
                 fix_demand(result);
             }
+        }
 
         tolua_pushusertype(L, result, TOLUA_CAST "region");
         return 1;

src/bind_storage.c

@@ -33,16 +33,13 @@ static int tolua_storage_create(lua_State * L)
 {
     const char *filename = tolua_tostring(L, 1, 0);
     const char *type = tolua_tostring(L, 2, "rb");
-    gamedata *data = (gamedata *)calloc(1, sizeof(gamedata));
-    storage *store = (storage *)calloc(1, sizeof(storage));
     FILE * F;
 
-    data->store = store;
-
     F = fopen(filename, type);
-    if (!F) {
+    if (F) {
-        return 0;
+        gamedata *data = (gamedata *)calloc(1, sizeof(gamedata));
-    }
+        storage *store = (storage *)calloc(1, sizeof(storage));
+        data->store = store;
         if (strchr(type, 'r')) {
             fread(&data->version, sizeof(int), 1, F);
             fseek(F, sizeof(int), SEEK_CUR);
@@ -57,6 +54,8 @@ static int tolua_storage_create(lua_State * L)
         binstore_init(store, &data->strm);
         tolua_pushusertype(L, (void *)data, TOLUA_CAST "storage");
         return 1;
+    }
+    return 0;
 }
 
 static int tolua_storage_read_unit(lua_State * L)

src/creport.c

@@ -637,8 +637,9 @@ faction * f)
         fprintf(F, "\"%s\";Beschr\n", b->display);
     if (b->size)
         fprintf(F, "%d;Groesse\n", b->size);
-    if (owner)
+    if (owner) {
-        fprintf(F, "%d;Besitzer\n", owner ? owner->no : -1);
+        fprintf(F, "%d;Besitzer\n", owner->no);
+    }
     if (fno >= 0)
         fprintf(F, "%d;Partei\n", fno);
     if (b->besieged)
@@ -667,8 +668,9 @@ const faction * f, const region * r)
             (sh->damage * 100 + DAMAGE_SCALE - 1) / (sh->size * DAMAGE_SCALE);
         fprintf(F, "%d;Schaden\n", percent);
     }
-    if (u)
+    if (u) {
-        fprintf(F, "%d;Kapitaen\n", u ? u->no : -1);
+        fprintf(F, "%d;Kapitaen\n", u->no);
+    }
     if (fcaptain >= 0)
         fprintf(F, "%d;Partei\n", fcaptain);
 
@@ -1703,6 +1705,7 @@ int crwritemap(const char *filename)
     FILE *F = fopen(filename, "w");
     region *r;
 
+    if (F) {
         fprintf(F, "VERSION %d\n", C_REPORT_VERSION);
         fputs("\"UTF-8\";charset\n", F);
 
@@ -1720,6 +1723,8 @@ int crwritemap(const char *filename)
         }
         fclose(F);
         return 0;
+    }
+    return EOF;
 }
 
 void register_cr(void)

src/give.c

@@ -289,7 +289,7 @@ message * give_men(int n, unit * u, unit * u2, struct order *ord)
     else {
         if (n > u->number)
             n = u->number;
-        if (u2 && n + u2->number > UNIT_MAXSIZE) {
+        if (n + u2->number > UNIT_MAXSIZE) {
             n = UNIT_MAXSIZE - u2->number;
             ADDMSG(&u->faction->msgs, msg_feedback(u, ord, "error_unit_size",
                 "maxsize", UNIT_MAXSIZE));

src/gmtool.c

@@ -54,6 +54,7 @@
 #include <util/lists.h>
 #include <util/rng.h>
 #include <util/base36.h>
+#include <util/bsdstring.h>
 
 #include <storage.h>
 #include <lua.h>
@@ -1086,7 +1087,7 @@ static void handlekey(state * st, int c)
         else if (findmode == 'F') {
             faction *f = select_faction(st);
             if (f != NULL) {
-                strcpy(locate, itoa36(f->no));
+                strlcpy(locate, itoa36(f->no), sizeof(locate));
                 findmode = 'f';
             }
             else {

src/kernel/build.c

@@ -494,7 +494,7 @@ int build(unit * u, const construction * ctype, int completed, int want)
          * type->improvement==type means build another object of the same time
          * while material lasts type->improvement==x means build x when type
          * is finished */
-        while (type->improvement != NULL &&
+        while (type && type->improvement &&
             type->improvement != type &&
             type->maxsize > 0 && type->maxsize <= completed) {
             completed -= type->maxsize;

src/kernel/config.c

@@ -307,7 +307,7 @@ const char *dbrace(const struct race *rc)
     char *zPtr = zText;
 
     /* the english names are all in ASCII, so we don't need to worry about UTF8 */
-    strcpy(zText, (const char *)LOC(get_locale("en"), rc_name_s(rc, NAME_SINGULAR)));
+    strlcpy(zText, (const char *)LOC(get_locale("en"), rc_name_s(rc, NAME_SINGULAR)), sizeof(zText));
     while (*zPtr) {
         *zPtr = (char)(toupper(*zPtr));
         ++zPtr;
@@ -1082,13 +1082,19 @@ int check_param(const struct param *p, const char *key, const char *searchvalue)
     return result;
 }
 
+const char * relpath(char *buf, size_t sz, const char *path) {
+    strlcpy(buf, basepath(), sz);
+    strlcat(buf, path, sz);
+    return buf;
+}
+
 static const char *g_datadir;
 const char *datapath(void)
 {
     static char zText[MAX_PATH]; // FIXME: static return value
     if (g_datadir)
         return g_datadir;
-    return strcat(strcpy(zText, basepath()), "/data");
+    return relpath(zText, sizeof(zText), "/data");
 }
 
 void set_datapath(const char *path)
@@ -1102,7 +1108,7 @@ const char *reportpath(void)
     static char zText[MAX_PATH]; // FIXME: static return value
     if (g_reportdir)
         return g_reportdir;
-    return strcat(strcpy(zText, basepath()), "/reports");
+    return relpath(zText, sizeof(zText), "/reports");
 }
 
 void set_reportpath(const char *path)

src/kernel/connection.c

@@ -117,11 +117,13 @@ void walk_connections(region *r, void(*cb)(connection *, void *), void *data) {
     walk_i(r, borders[key], cb, data);
     for (d = 0; d != MAXDIRECTIONS; ++d) {
         region *rn = r_connect(r, d);
+        if (rn) {
             int k = reg_hashkey(rn);
             if (k < key) {
                 walk_i(r, borders[k], cb, data);
             }
         }
+    }
 }
 
 static connection **get_borders_i(const region * r1, const region * r2)

src/kernel/item.c

@@ -925,7 +925,7 @@ struct order *ord)
         user->number);
 
     a_add(&f->attribs, make_key(atoi36("mbst")));
-    set_level(user, findskill("magic"), 3);
+    set_level(user, SK_MAGIC, 3);
 
     ADDMSG(&user->faction->msgs, msg_message("use_item",
         "unit item", user, itype->rtype));

src/kernel/region.c

@@ -751,7 +751,7 @@ void remove_region(region ** rlist, region * r)
     deleted_regions = r;
 }
 
-static void freeland(land_region * lr)
+void free_land(land_region * lr)
 {
     free(lr->ownership);
     while (lr->demands) {
@@ -817,7 +817,7 @@ void free_region(region * r)
         last = NULL;
     free(r->display);
     if (r->land)
-        freeland(r->land);
+        free_land(r->land);
 
     if (r->msgs) {
         free_messagelist(r->msgs->begin);
@@ -997,7 +997,7 @@ void terraform_region(region * r, const terrain_type * terrain)
         region_setinfo(r, NULL);
         if (r->land != NULL) {
             i_freeall(&r->land->items);
-            freeland(r->land);
+            free_land(r->land);
             r->land = NULL;
         }
         rsettrees(r, 0, 0);

src/kernel/region.h

@@ -245,6 +245,7 @@ extern "C" {
 #endif
 
     void free_regions(void);
+    void free_land(struct land_region * lr);
 
     int region_get_morale(const region * r);
     void region_set_morale(region * r, int morale, int turn);

src/kernel/save.c

@@ -840,8 +840,8 @@ static region *readregion(struct gamedata *data, int x, int y)
         while (r->attribs)
             a_remove(&r->attribs, r->attribs);
         if (r->land) {
-            free(r->land);            /* mem leak */
+            free_land(r->land);
-            r->land->demands = 0;     /* mem leak */
+            r->land = 0;
         }
         while (r->resources) {
             rawmaterial *rm = r->resources;

src/kernel/unit.c

@@ -116,9 +116,10 @@ unit *findunitr(const region * r, int n)
     return (u && u->region==r)?u:0;
 }
 
+// TODO: deprecated, replace with findunit(n)
 unit *findunitg(int n, const region * hint)
 {
-
+    unused_arg(hint);
     /* Abfangen von Syntaxfehlern. */
     if (n <= 0)
         return NULL;
@@ -735,6 +736,7 @@ variant read_unit_reference(struct storage * store)
 
 int get_level(const unit * u, skill_t id)
 {
+    assert(id != NOSKILL);
     if (skill_enabled(id)) {
         skill *sv = u->skills;
         while (sv != u->skills + u->skill_size) {
@@ -943,7 +945,7 @@ void move_unit(unit * u, region * r, unit ** ulist)
 /* ist mist, aber wegen nicht skalierender attribute notwendig: */
 #include "alchemy.h"
 
-void transfermen(unit * u, unit * u2, int n)
+void transfermen(unit * u, unit * dst, int n)
 {
     const attrib *a;
     int hp = u->hp;
@@ -954,22 +956,22 @@ void transfermen(unit * u, unit * u2, int n)
     assert(n > 0);
     /* "hat attackiert"-status wird uebergeben */
 
-    if (u2) {
+    if (dst) {
         skill *sv, *sn;
         skill_t sk;
         ship *sh;
 
-        assert(u2->number + n > 0);
+        assert(dst->number + n > 0);
 
         for (sk = 0; sk != MAXSKILLS; ++sk) {
             int weeks, level = 0;
 
             sv = unit_skill(u, sk);
-            sn = unit_skill(u2, sk);
+            sn = unit_skill(dst, sk);
 
             if (sv == NULL && sn == NULL)
                 continue;
-            if (sn == NULL && u2->number == 0) {
+            if (sn == NULL && dst->number == 0) {
                 /* new unit, easy to solve */
                 level = sv->level;
                 weeks = sv->weeks;
@@ -983,12 +985,12 @@ void transfermen(unit * u, unit * u2, int n)
                 }
                 if (sn && sn->level) {
                     dlevel +=
-                        (sn->level + 1 - sn->weeks / (sn->level + 1.0)) * u2->number;
+                        (sn->level + 1 - sn->weeks / (sn->level + 1.0)) * dst->number;
-                    level += sn->level * u2->number;
+                    level += sn->level * dst->number;
                 }
 
-                dlevel = dlevel / (n + u2->number);
+                dlevel = dlevel / (n + dst->number);
-                level = level / (n + u2->number);
+                level = level / (n + dst->number);
                 if (level <= dlevel) {
                     /* apply the remaining fraction to the number of weeks to go.
                      * subtract the according number of weeks, getting closer to the
@@ -1007,15 +1009,15 @@ void transfermen(unit * u, unit * u2, int n)
             }
             if (level) {
                 if (sn == NULL)
-                    sn = add_skill(u2, sk);
+                    sn = add_skill(dst, sk);
                 sn->level = (unsigned char)level;
                 sn->weeks = (unsigned char)weeks;
                 assert(sn->weeks > 0 && sn->weeks <= sn->level * 2 + 1);
-                assert(u2->number != 0 || (sn->level == sv->level
+                assert(dst->number != 0 || (sn->level == sv->level
                     && sn->weeks == sv->weeks));
             }
             else if (sn) {
-                remove_skill(u2, sk);
+                remove_skill(dst, sk);
                 sn = NULL;
             }
         }
@@ -1023,32 +1025,32 @@ void transfermen(unit * u, unit * u2, int n)
         while (a && a->type == &at_effect) {
             effect_data *olde = (effect_data *)a->data.v;
             if (olde->value)
-                change_effect(u2, olde->type, olde->value);
+                change_effect(dst, olde->type, olde->value);
             a = a->next;
         }
         sh = leftship(u);
         if (sh != NULL)
-            set_leftship(u2, sh);
+            set_leftship(dst, sh);
-        u2->flags |=
+        dst->flags |=
             u->flags & (UFL_LONGACTION | UFL_NOTMOVING | UFL_HUNGER | UFL_MOVED |
             UFL_ENTER);
         if (u->attribs) {
-            transfer_curse(u, u2, n);
+            transfer_curse(u, dst, n);
         }
     }
     scale_number(u, u->number - n);
-    if (u2) {
+    if (dst) {
-        set_number(u2, u2->number + n);
+        set_number(dst, dst->number + n);
         hp -= u->hp;
-        u2->hp += hp;
+        dst->hp += hp;
         /* TODO: Das ist schnarchlahm! und gehoert nicht hierhin */
-        a = a_find(u2->attribs, &at_effect);
+        a = a_find(dst->attribs, &at_effect);
         while (a && a->type == &at_effect) {
             attrib *an = a->next;
             effect_data *olde = (effect_data *)a->data.v;
             int e = get_effect(u, olde->type);
             if (e != 0)
-                change_effect(u2, olde->type, -e);
+                change_effect(dst, olde->type, -e);
             a = an;
         }
     }

src/kernel/unit.h

@@ -89,11 +89,11 @@ extern "C" {
         struct faction *faction;
         struct building *building;
         struct ship *ship;
-        unsigned short number;      /* persons */
+        int number;      /* persons */
-        short age;
+        int age;
 
         /* skill data */
-        short skill_size;
+        int skill_size;
         struct skill *skills;
         struct item *items;
         reservation *reservations;
@@ -167,7 +167,7 @@ extern "C" {
 
     void set_level(struct unit *u, skill_t id, int level);
     int get_level(const struct unit *u, skill_t id);
-    extern void transfermen(struct unit *u, struct unit *u2, int n);
+    extern void transfermen(struct unit *src, struct unit *dst, int n);
 
     int eff_skill(const struct unit *u, const struct skill *sv, const struct region *r);
     int effskill_study(const struct unit *u, skill_t sk, const struct region *r);

src/kernel/xmlreader.c

@@ -389,7 +389,8 @@ static int parse_calendar(xmlDocPtr doc)
                 int i;
 
                 weeks_per_month = nsetWeeks->nodeNr;
-                assert(!weeknames);
+                free(weeknames);
+                free(weeknames2);
                 weeknames = malloc(sizeof(char *) * weeks_per_month);
                 weeknames2 = malloc(sizeof(char *) * weeks_per_month);
                 for (i = 0; i != nsetWeeks->nodeNr; ++i) {
@@ -431,15 +432,16 @@ static int parse_calendar(xmlDocPtr doc)
                 int i;
 
                 months_per_year = nsetMonths->nodeNr;
-                assert(!monthnames);
+                free(monthnames);
                 monthnames = malloc(sizeof(char *) * months_per_year);
+                free(month_season);
                 month_season = malloc(sizeof(int) * months_per_year);
+                free(storms);
                 storms = malloc(sizeof(int) * months_per_year);
 
                 for (i = 0; i != nsetMonths->nodeNr; ++i) {
                     xmlNodePtr month = nsetMonths->nodeTab[i];
                     xmlChar *propValue = xmlGetProp(month, BAD_CAST "name");
-                    int j;
 
                     if (propValue) {
                         if (newyear
@@ -451,6 +453,8 @@ static int parse_calendar(xmlDocPtr doc)
                         monthnames[i] = _strdup(mkname("calendar", (const char *)propValue));
                         xmlFree(propValue);
                     }
+                    if (nsetSeasons) {
+                        int j;
                         for (j = 0; j != seasons; ++j) {
                             xmlNodePtr season = month->parent;
                             if (season == nsetSeasons->nodeTab[j]) {
@@ -459,6 +463,7 @@ static int parse_calendar(xmlDocPtr doc)
                             }
                         }
                         assert(j != seasons);
+                    }
                     storms[i] = xml_ivalue(nsetMonths->nodeTab[i], "storm", 0);
                 }
             }
@@ -929,12 +934,14 @@ static int parse_resources(xmlDocPtr doc)
             flags |= RTF_LIMITED;
 
         name = xmlGetProp(node, BAD_CAST "name");
-        assert(name != NULL);
+        if (!name) {
-
+            assert(name);
+            log_error("invalid resource %d has no name", i);
+            continue;
+        }
         rtype = rt_get_or_create((const char *)name);
         rtype->flags |= flags;
-
+        xmlFree(name);
-        if (name) xmlFree(name);
 
         name = xmlGetProp(node, BAD_CAST "material");
         if (name) {
@@ -1811,6 +1818,7 @@ static int parse_races(xmlDocPtr doc)
         if (result->nodesetval->nodeNr > MAXMAGIETYP) {
             log_error("race %s has %d potential familiars", rc->_name, result->nodesetval->nodeNr);
         }
+        else {
             for (k = 0; k != MAXMAGIETYP; ++k) {
                 if (k < result->nodesetval->nodeNr) {
                     xmlNodePtr node = result->nodesetval->nodeTab[k];
@@ -1826,10 +1834,12 @@ static int parse_races(xmlDocPtr doc)
                         rc->familiars[k] = frc;
                     }
                     xmlFree(propValue);
-            } else {
+                }
+                else {
                     rc->familiars[k] = frc;
                 }
             }
+        }
         xmlXPathFreeObject(result);
 
         /* reading eressea/races/race/precombatspell */

src/laws.c

@@ -4512,8 +4512,9 @@ void update_subscriptions(void)
 {
     FILE *F;
     char zText[MAX_PATH];
-    faction *f;
+
-    strcat(strcpy(zText, basepath()), "/subscriptions");
+    strlcpy(zText, basepath(), sizeof(zText));
+    strlcat(zText, "/subscriptions", sizeof(zText));
     F = fopen(zText, "r");
     if (F == NULL) {
         log_warning(0, "could not open %s.\n", zText);
@@ -4522,6 +4523,8 @@ void update_subscriptions(void)
     for (;;) {
         char zFaction[5];
         int subscription, fno;
+        faction *f;
+
         if (fscanf(F, "%d %s", &subscription, zFaction) <= 0)
             break;
         fno = atoi36(zFaction);
@@ -4534,11 +4537,14 @@ void update_subscriptions(void)
 
     sprintf(zText, "subscriptions.%u", turn);
     F = fopen(zText, "w");
+    if (F) {
+        faction *f;
         for (f = factions; f != NULL; f = f->next) {
             fprintf(F, "%s:%u:%s:%s:%u:\n",
                 itoa36(f->no), f->subscription, f->email, dbrace(f->race), f->lastorders);
         }
         fclose(F);
+    }
 }
 
 bool

src/magic.c

@@ -1281,19 +1281,20 @@ bool fumble(region * r, unit * u, const spell * sp, int cast_grade)
      * 20% Warscheinlichkeit nicht
      * */
 
-    int rnd = 0;
+    int fumble_chance, rnd = 0;
-    double x = (double)cast_grade / (double)effskill(u, SK_MAGIC, r);
+    int effsk = effskill(u, SK_MAGIC, r);
-    int fumble_chance = (int)(((double)x * 40.0) - 20.0);
     struct building *b = inside_building(u);
     const struct building_type *btype = b ? b->type : NULL;
     int fumble_enabled = get_param_int(global.parameters, "magic.fumble.enable", 1);
     sc_mage * mage;
 
-    if (!fumble_enabled) {
+    if (effsk<=0 || !fumble_enabled) {
         return false;
     }
-    if (btype)
+    fumble_chance = (int)((cast_grade * 40.0 / (double)effsk) - 20.0);
+    if (btype) {
         fumble_chance -= btype->fumblebonus;
+    }
 
     /* CHAOSPATZERCHANCE 10 : +10% Chance zu Patzern */
     mage = get_mage(u);

src/modules/autoseed.c

@@ -31,6 +31,7 @@
 /* util includes */
 #include <util/attrib.h>
 #include <util/base36.h>
+#include <util/bsdstring.h>
 #include <util/goodies.h>
 #include <util/language.h>
 #include <util/lists.h>
@@ -186,8 +187,9 @@ newfaction *read_newfactions(const char *filename)
         if (email[0] == '\0')
             break;
         if (password[0] == '\0') {
-            strcpy(password, itoa36(rng_int()));
+            size_t sz;
-            strcat(password, itoa36(rng_int()));
+            sz = strlcpy(password, itoa36(rng_int()), sizeof(password));
+            sz += strlcat(password, itoa36(rng_int()), sizeof(password));
         }
         for (f = factions; f; f = f->next) {
             if (strcmp(f->email, email) == 0 && f->subscription

src/modules/museum.c

@@ -323,14 +323,13 @@ order * ord)
     r = findregion(a->data.sa[0], a->data.sa[1]);
     assert(r);
     a_remove(&u->attribs, a);
-
     /* Übergebene Gegenstände zurückgeben */
 
     a = a_find(u->attribs, &at_museumgivebackcookie);
+    if (a) {
         unit_cookie = a->data.i;
         a_remove(&u->attribs, a);
 
-    if (a) {
         for (a = a_find(warden->attribs, &at_museumgiveback);
             a && a->type == &at_museumgiveback; a = a->next) {
             if (((museumgiveback *)(a->data.v))->cookie == unit_cookie)

src/move.c

@@ -1108,7 +1108,7 @@ static void cycle_route(order * ord, unit * u, int gereist)
 {
     int cm = 0;
     char tail[1024], *bufp = tail;
-    char neworder[2048];
+    char neworder[2048], *obuf = neworder;
     char token[128];
     direction_t d = NODIRECTION;
     bool paused = false;
@@ -1163,14 +1163,16 @@ static void cycle_route(order * ord, unit * u, int gereist)
             /* da PAUSE nicht in ein shortdirections[d] umgesetzt wird (ist
              * hier keine normale direction), muss jede PAUSE einzeln
              * herausgefiltert und explizit gesetzt werden */
-            if (neworder[0])
+            if (neworder != obuf) {
-                strcat(neworder, " ");
+                obuf += strlcat(obuf, " ", sizeof(neworder)-(obuf-neworder));
-            strcat(neworder, LOC(lang, parameters[P_PAUSE]));
+            }
+            obuf += strlcat(obuf, LOC(lang, parameters[P_PAUSE]), sizeof(neworder) - (obuf - neworder));
         }
         else {
-            if (neworder[0])
+            if (neworder != obuf) {
-                strcat(neworder, " ");
+                obuf += strlcat(obuf, " ", sizeof(neworder) - (obuf - neworder));
-            strcat(neworder, LOC(lang, shortdirections[d]));
+            }
+            obuf += strlcat(obuf, LOC(lang, shortdirections[d]), sizeof(neworder) - (obuf - neworder));
         }
     }
 
@@ -2379,7 +2381,7 @@ static int follow_ship(unit * u, order * ord)
             speed = maxspeed;
     }
     rc = rconnect(rc, dir);
-    while (moves < speed && (dir = hunted_dir(rc->attribs, id)) != NODIRECTION) {
+    while (rc && moves < speed && (dir = hunted_dir(rc->attribs, id)) != NODIRECTION) {
         const char *loc = LOC(u->faction->locale, directions[dir]);
         bufp = STRLCPY_EX(bufp, " ", &size, "hunt");
         bufp = STRLCPY_EX(bufp, loc, &size, "hunt");

src/names.c

@@ -103,22 +103,22 @@ static const char *make_names(const char *monster, int *num_postfix,
         sprintf(zText, "%s_prefix_%d", monster, uv);
         str = locale_getstring(default_locale, zText);
         if (str) {
-            strcat(name, (const char *)str);
+            size_t sz = strlcpy(name, (const char *)str, sizeof(name));
-            strcat(name, " ");
+            strlcpy(name + sz, " ", sizeof(name) - sz);
         }
     }
 
     sprintf(zText, "%s_name_%d", monster, uu);
     str = locale_getstring(default_locale, zText);
     if (str)
-        strcat(name, (const char *)str);
+        strlcat(name, (const char *)str, sizeof(name));
 
     if (un < *num_postfix) {
         sprintf(zText, "%s_postfix_%d", monster, un);
         str = locale_getstring(default_locale, zText);
         if (str) {
-            strcat(name, " ");
+            strlcat(name, " ", sizeof(name));
-            strcat(name, (const char *)str);
+            strlcat(name, (const char *)str, sizeof(name));
         }
     }
     return name;
@@ -281,22 +281,23 @@ static const char *dragon_name(const unit * u)
     }
     else {
         char n[32];
+        size_t sz;
 
-        strcpy(n, silbe1[rng_int() % SIL1]);
+        sz = strlcpy(n, silbe1[rng_int() % SIL1], sizeof(n));
-        strcat(n, silbe2[rng_int() % SIL2]);
+        sz += strlcat(n, silbe2[rng_int() % SIL2], sizeof(n));
-        strcat(n, silbe3[rng_int() % SIL3]);
+        sz += strlcat(n, silbe3[rng_int() % SIL3], sizeof(n));
         if (rng_int() % 5 > 2) {
             sprintf(name, "%s, %s", n, str);  /* "Name, der Titel" */
         }
         else {
-            strcpy(name, (const char *)str);  /* "Der Titel Name" */
+            sz = strlcpy(name, (const char *)str, sizeof(name));  /* "Der Titel Name" */
             name[0] = (char)toupper(name[0]); /* TODO: UNICODE - should use towupper() */
-            strcat(name, " ");
+            sz += strlcat(name, " ", sizeof(name));
-            strcat(name, n);
+            sz += strlcat(name, n, sizeof(name));
         }
         if (u && (rng_int() % 3 == 0)) {
-            strcat(name, " von ");
+            sz += strlcat(name, " von ", sizeof(name));
-            strcat(name, (const char *)rname(u->region, default_locale));
+            sz += strlcat(name, (const char *)rname(u->region, default_locale), sizeof(name));
         }
     }
 
@@ -356,21 +357,22 @@ static const char *dracoid_name(const unit * u)
 {
     static char name[NAMESIZE + 1]; // FIXME: static return value
     int mid_syllabels;
+    size_t sz;
 
     /* ignore u */
-    u = 0;
+    unused_arg(u);
     /* Wieviele Mittelteile? */
 
     mid_syllabels = rng_int() % 4;
 
-    strcpy(name, drac_pre[rng_int() % DRAC_PRE]);
+    sz = strlcpy(name, drac_pre[rng_int() % DRAC_PRE], sizeof(name));
     while (mid_syllabels > 0) {
         mid_syllabels--;
         if (rng_int() % 10 < 4)
-            strcat(name, "'");
+            strlcat(name, "'", sizeof(name));
-        strcat(name, drac_mid[rng_int() % DRAC_MID]);
+        sz += strlcat(name, drac_mid[rng_int() % DRAC_MID], sizeof(name));
     }
-    strcat(name, drac_suf[rng_int() % DRAC_SUF]);
+    sz += strlcat(name, drac_suf[rng_int() % DRAC_SUF], sizeof(name));
     return name;
 }
 

src/piracy.c

@@ -169,12 +169,13 @@ void piracy_cmd(unit * u, order *ord)
         if (saff != 0) {
             saff = rng_int() % saff;
             for (dir = 0; dir != MAXDIRECTIONS; ++dir) {
-                if (saff < aff[dir].value)
+                if (saff < aff[dir].value) {
-                    break;
-                saff -= aff[dir].value;
-            }
                     target_dir = dir;
                     a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir));
+                    break;
+                }
+                saff -= aff[dir].value;
+            }
         }
     }
 

src/report.c

@@ -1525,7 +1525,7 @@ report_template(const char *filename, report_context * ctx, const char *charset)
         }
     }
     newline(out);
-    strcpy(buf, LOC(f->locale, parameters[P_NEXT]));
+    strlcpy(buf, LOC(f->locale, parameters[P_NEXT]), sizeof(buf));
     rps_nowrap(out, buf);
     newline(out);
     fstream_done(&strm);
@@ -1578,7 +1578,7 @@ show_allies(const faction * f, const ally * allies, char *buf, size_t size)
                 WARN_STATIC_BUFFER();
         }
         else {
-            for (h = 1; h < HELP_ALL; h *= 2) {
+            for (h = 1; h <= HELP_TRAVEL; h *= 2) {
                 int p = MAXPARAMS;
                 if ((mode & h) == h) {
                     switch (h) {

src/reports.c

@@ -1621,9 +1621,10 @@ static void write_script(FILE * F, const faction * f)
     buf[0] = 0;
     for (rtype = report_types; rtype != NULL; rtype = rtype->next) {
         if (f->options & rtype->flag) {
-            if (buf[0])
+            if (buf[0]) {
-                strcat(buf, ",");
+                strlcat(buf, ",", sizeof(buf));
-            strcat(buf, rtype->extension);
+            }
+            strlcat(buf, rtype->extension, sizeof(buf));
         }
     }
     fputs(buf, F);
@@ -1772,7 +1773,10 @@ const char *trailinto(const region * r, const struct locale *lang)
     const char *s;
     if (r) {
         const char *tname = terrain_name(r);
-        strcat(strcpy(ref, tname), "_trail");
+        size_t sz;
+
+        sz = strlcpy(ref, tname, sizeof(ref));
+        sz += strlcat(ref+sz, "_trail", sizeof(ref)-sz);
         s = LOC(lang, ref);
         if (s && *s) {
             if (strstr(s, "%s"))

src/skill.c

@@ -45,6 +45,7 @@ const char *skillnames[MAXSKILLS] = {
 bool skill_disabled[MAXSKILLS];
 
 bool skill_enabled(skill_t sk) {
+    assert(sk != NOSKILL);
     return !skill_disabled[sk];
 }
 

src/skill.test.c

@@ -29,6 +29,7 @@ static void test_init_skill(CuTest *tc) {
 static void test_get_skill(CuTest *tc) {
     test_cleanup();
     CuAssertIntEquals(tc, SK_ALCHEMY, findskill("alchemy"));
+    CuAssertIntEquals(tc, SK_MAGIC, findskill("magic"));
     CuAssertIntEquals(tc, SK_CROSSBOW, findskill("crossbow"));
     CuAssertIntEquals(tc, NOSKILL, findskill(""));
     CuAssertIntEquals(tc, NOSKILL, findskill("potato"));

src/spells.c

@@ -571,11 +571,12 @@ static int sp_summon_familiar(castorder * co)
             region *rn = rconnect(r, dir);
             if (rn && fval(rn->terrain, SEA_REGION)) {
                 dh++;
-                if (dh == coasts)
+                if (dh == coasts) {
+                    r = rconnect(r, dir);
                     break;
                 }
             }
-        r = rconnect(r, dir);
+        }
     }
 
     msg = msg_message("familiar_name", "unit", mage);
@@ -5954,7 +5955,7 @@ int sp_movecastle(castorder * co)
 
     target_region = rconnect(r, dir);
 
-    if (!(target_region->terrain->flags & LAND_REGION)) {
+    if (!target_region || !(target_region->terrain->flags & LAND_REGION)) {
         ADDMSG(&mage->faction->msgs, msg_feedback(mage, co->order,
             "sp_movecastle_fail_1", "direction", dir));
         return cast_level;

src/spells/combatspells.c

@@ -622,11 +622,13 @@ int sp_mindblast(struct castorder * co)
                 skill_t sk = random_skill(du, false);
                 if (sk != NOSKILL) {
                     skill *sv = unit_skill(du, sk);
+                    if (sv) {
                         int n = 1 + rng_int() % 3;
 
                         reduce_skill(du, sv, n);
                         k += du->number;
                     }
+                }
                 else {
                     /* unit has no skill. kill it. */
                     kill_troop(dt);

src/spells/shipcurse.c

@@ -138,10 +138,12 @@ curse *shipcurse_flyingship(ship * sh, unit * mage, double power, int duration)
         /* mit C_SHIP_NODRIFT haben wir kein Problem */
         curse *c =
             create_curse(mage, &sh->attribs, ct_flyingship, power, duration, 0.0, 0);
+        if (c) {
             c->data.v = sh;
-        if (c && c->duration > 0) {
+            if (c->duration > 0) {
                 sh->flags |= SF_FLYING;
             }
+        }
         return c;
     }
 }

src/spy.c

@@ -428,7 +428,7 @@ static void sink_ship(region * r, ship * sh, unit * saboteur)
     else {
         for (d = 0; d != MAXDIRECTIONS; ++d) {
             region *rn = rconnect(r, d);
-            if (!fval(rn->terrain, SEA_REGION) && !move_blocked(NULL, r, rn)) {
+            if (rn && !fval(rn->terrain, SEA_REGION) && !move_blocked(NULL, r, rn)) {
                 safety = rn;
                 probability = OCEAN_SWIMMER_CHANCE;
                 break;

src/sqlite.c

@@ -123,7 +123,7 @@ static void update_faction(sqlite3 *db, const faction *f) {
         "INSERT INTO faction_data (faction_id, code, name, email, lang, turn)"
         " VALUES (?, ?, ?, ?, ?, ?)";
     sqlite3_stmt *stmt = 0;
-    strcpy(code, itoa36(f->no));
+    strncpy(code, itoa36(f->no), sizeof(code)-1);
     sqlite3_prepare_v2(db, sql, -1, &stmt, 0);
     sqlite3_bind_int(stmt, 1, f->subscription);
     sqlite3_bind_text(stmt, 2, code, -1, SQLITE_STATIC);

src/study.c

@@ -41,6 +41,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 /* util includes */
 #include <util/attrib.h>
 #include <util/base36.h>
+#include <util/bsdstring.h>
 #include <util/language.h>
 #include <util/log.h>
 #include <util/parser.h>
@@ -75,7 +76,7 @@ magic_t getmagicskill(const struct locale * lang)
             return (magic_t)token.i;
         }
         else {
-            char buffer[3];
+            char buffer[8];
             buffer[0] = s[0];
             buffer[1] = s[1];
             buffer[2] = '\0';
@@ -283,8 +284,8 @@ int teach_cmd(unit * u, struct order *ord)
     static const curse_type *gbdream_ct = NULL;
     plane *pl;
     region *r = u->region;
-    int teaching, i, j, count, academy = 0;
     skill_t sk = NOSKILL;
+    int teaching, i, j, count, academy = 0;
 
     if (gbdream_ct == 0)
         gbdream_ct = ct_find("gbdream");
@@ -325,26 +326,30 @@ int teach_cmd(unit * u, struct order *ord)
 
 #if TEACH_ALL
     if (getparam(u->faction->locale) == P_ANY) {
-        unit *student = r->units;
+        unit *student;
         skill_t teachskill[MAXSKILLS];
-        int i = 0;
+        int t = 0;
+
         do {
             sk = getskill(u->faction->locale);
-            teachskill[i++] = sk;
+            teachskill[t] = getskill(u->faction->locale);
         } while (sk != NOSKILL);
-        while (teaching && student) {
+
-            if (student->faction == u->faction) {
+        for (student = r->units; teaching && student; student = student->next) {
-                if (LongHunger(student))
+            if (LongHunger(student)) {
                 continue;
+            } else if (student->faction == u->faction) {
                 if (getkeyword(student->thisorder) == K_STUDY) {
                     /* Input ist nun von student->thisorder !! */
                     init_order(student->thisorder);
                     sk = getskill(student->faction->locale);
                     if (sk != NOSKILL && teachskill[0] != NOSKILL) {
-                        for (i = 0; teachskill[i] != NOSKILL; ++i)
+                        for (t = 0; teachskill[t] != NOSKILL; ++t) {
-                            if (sk == teachskill[i])
+                            if (sk == teachskill[t]) {
                                 break;
-                        sk = teachskill[i];
+                            }
+                        }
+                        sk = teachskill[t];
                     }
                     if (sk != NOSKILL
                         && effskill_study(u, sk, 0) - TEACHDIFFERENCE > effskill_study(student, sk, 0)) {
@@ -352,14 +357,8 @@ int teach_cmd(unit * u, struct order *ord)
                     }
                 }
             }
-            student = student->next;
-        }
 #ifdef TEACH_FRIENDS
-        while (teaching && student) {
+            else if (alliedunit(u, student->faction, HELP_GUARD)) {
-            if (student->faction != u->faction
-                && alliedunit(u, student->faction, HELP_GUARD)) {
-                if (LongHunger(student))
-                    continue;
                 if (getkeyword(student->thisorder) == K_STUDY) {
                     /* Input ist nun von student->thisorder !! */
                     init_order(student->thisorder);
@@ -370,14 +369,14 @@ int teach_cmd(unit * u, struct order *ord)
                     }
                 }
             }
-            student = student->next;
-        }
 #endif
         }
+    }
     else
 #endif
     {
         char zOrder[4096];
+        size_t sz = sizeof(zOrder);
         order *new_order;
 
         zOrder[0] = '\0';
@@ -429,9 +428,11 @@ int teach_cmd(unit * u, struct order *ord)
 
             /* Neuen Befehl zusammenbauen. TEMP-Einheiten werden automatisch in
              * ihre neuen Nummern uebersetzt. */
-            if (zOrder[0])
+            if (zOrder[0]) {
-                strcat(zOrder, " ");
+                strncat(zOrder, " ", sz - 1);
-            strcat(zOrder, unitid(u2));
+                --sz;
+            }
+            sz -= strlcpy(zOrder + 4096 - sz, unitid(u2), sz);
 
             if (getkeyword(u2->thisorder) != K_STUDY) {
                 ADDMSG(&u->faction->msgs,
@@ -505,7 +506,7 @@ static double study_speedup(unit * u, skill_t s, study_rule_t rule)
                 skill *sv = u->skills + i;
                 if (sv->id == s){
                     learnweeks = sv->level * (sv->level + 1) / 2.0;
-                    if (learnweeks < turn / 3) {
+                    if (learnweeks < turn / 3.0) {
                         return 2.0;
                     }
                 }
@@ -517,7 +518,7 @@ static double study_speedup(unit * u, skill_t s, study_rule_t rule)
                 skill *sv = u->skills + i;
                 learnweeks += (sv->level * (sv->level + 1) / 2.0);
             }
-            if (learnweeks < turn / 2) {
+            if (learnweeks < turn / 2.0) {
                 return 2.0;
             }
         }

src/summary.c

@@ -414,6 +414,7 @@ summary *make_summary(void)
             for (u = r->units; u; u = u->next)
                 freset(u->faction, FFL_SELECT);
             for (u = r->units; u; u = u->next) {
+                int orace;
                 f = u->faction;
                 if (!is_monsters(u->faction)) {
                     skill *sv;
@@ -452,7 +453,10 @@ summary *make_summary(void)
 
                 f->num_total += u->number;
                 f->money += get_money(u);
-                s->poprace[old_race(u_race(u))] += u->number;
+                orace = (int)old_race(u_race(u));
+                if (orace >= 0) {
+                    s->poprace[orace] += u->number;
+                }
             }
         }
     }

src/util/attrib.c

@@ -199,11 +199,17 @@ static void a_free(attrib * a)
 
 int a_remove(attrib ** pa, attrib * a)
 {
+    attrib *head = *pa;
     int ok;
+
     assert(a != NULL);
     ok = a_unlink(pa, a);
-    if (ok)
+    if (ok) {
+        if (head == a) {
+            *pa = a->next;
+        }
         a_free(a);
+    }
     return ok;
 }
 

src/util/attrib.test.c

@@ -39,6 +39,18 @@ static void test_attrib_add(CuTest * tc)
     a_removeall(&alist, &at_bar);
 }
 
+static void test_attrib_remove_self(CuTest * tc) {
+    attrib_type at_foo = { "foo" };
+    attrib *a, *alist = 0;
+
+    CuAssertPtrNotNull(tc, a_add(&alist, a_new(&at_foo)));
+    CuAssertPtrNotNull(tc, a = a_add(&alist, a_new(&at_foo)));
+    CuAssertPtrEquals(tc, a, alist->next);
+    CuAssertPtrEquals(tc, 0, alist->nexttype);
+    CuAssertIntEquals(tc, 1, a_remove(&alist, alist));
+    CuAssertPtrEquals(tc, a, alist);
+}
+
 static void test_attrib_remove(CuTest * tc)
 {
     attrib_type at_foo = { "foo" };
@@ -86,6 +98,7 @@ CuSuite *get_attrib_suite(void)
     SUITE_ADD_TEST(suite, test_attrib_new);
     SUITE_ADD_TEST(suite, test_attrib_add);
     SUITE_ADD_TEST(suite, test_attrib_remove);
+    SUITE_ADD_TEST(suite, test_attrib_remove_self);
     SUITE_ADD_TEST(suite, test_attrib_nexttype);
     return suite;
 }

src/util/bsdstring.c

@@ -119,12 +119,11 @@ size_t slprintf(char * dst, size_t size, const char * format, ...)
 
     va_start(args, format);
     result = vsnprintf(dst, size, format, args);
+    va_end(args);
     if (result < 0 || result >= (int)size) {
         dst[size - 1] = '\0';
         return size;
     }
-    va_start(args, format);
-    va_end(args);
 
     return (size_t)result;
 }

src/util/language.c

@@ -249,7 +249,7 @@ void add_translation(struct critbit_tree **cbp, const char *key, int i) {
         size_t len = strlen(str);
         if (!cb) {
             // TODO: this will leak, because we do not know how to clean it up */
-            *cbp = cb = (struct critbit_tree *)calloc(1, sizeof(struct critbit_tree *));
+            *cbp = cb = (struct critbit_tree *)calloc(1, sizeof(struct critbit_tree));
         }
         len = cb_new_kv(str, len, &i, sizeof(int), buffer);
         cb_insert(cb, buffer, len);

src/util/log.c

@@ -11,6 +11,7 @@ without prior permission by the authors of Eressea.
 */
 #include <platform.h>
 #include "log.h"
+#include "bsdstring.h"
 #include "unicode.h"
 
 #include <assert.h>
@@ -144,7 +145,7 @@ static int check_dupe(const char *format, const char *type)
         }
         dupes = 0;
     }
-    strncpy(last_message, format, sizeof(last_message));
+    strlcpy(last_message, format, sizeof(last_message));
     last_type = type;
     return 0;
 }

src/util/nrmessage.c

@@ -135,7 +135,7 @@ const char *string, int level, const char *section)
         for (i = 0; i != mtype->nparameters; ++i) {
             if (i != 0)
                 *c++ = ' ';
-            c += strlen(strcpy(c, mtype->pnames[i]));
+            c += strlcpy(c, mtype->pnames[i], sizeof(zNames)-(c-zNames));
         }
         nrt->vars = _strdup(zNames);
     }