From 0328e84c6c254e3d2a604b15a4a508983afc8cad Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:45:48 +0100 Subject: [PATCH 01/69] coverity scan CID 22555: sizeof not portable wrong type in sizeof (pointer instead of struct) --- src/util/language.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/language.c b/src/util/language.c index 05ddf50c9..ed43074dd 100644 --- a/src/util/language.c +++ b/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); From 6cccdec4a2b90c04a89bba3e879666b30f6dc14a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:46:35 +0100 Subject: [PATCH 02/69] coverity scan CID 22454 (logically dead code) --- src/report.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/report.c b/src/report.c index 688347eb4..54c089c7f 100644 --- a/src/report.c +++ b/src/report.c @@ -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) { From f72314e915b591471e520379d6ec6ff112ec76b2 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:47:58 +0100 Subject: [PATCH 03/69] coverity scan CID 22451 (logically dead code) --- src/creport.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/creport.c b/src/creport.c index 7ad0044e2..23f4f5671 100644 --- a/src/creport.c +++ b/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) - fprintf(F, "%d;Besitzer\n", owner ? owner->no : -1); + if (owner) { + 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) - fprintf(F, "%d;Kapitaen\n", u ? u->no : -1); + if (u) { + fprintf(F, "%d;Kapitaen\n", u->no); + } if (fcaptain >= 0) fprintf(F, "%d;Partei\n", fcaptain); From e60b739ea40bca40dafa1ee785a1610d2c125092 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:49:05 +0100 Subject: [PATCH 04/69] coverity scan CID 22448: logically dead code --- src/bind_region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bind_region.c b/src/bind_region.c index ec2585cc1..e114ea806 100644 --- a/src/bind_region.c +++ b/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; } From 70d4a6af03c07358a2bae9e6a624c2a82467e258 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:51:53 +0100 Subject: [PATCH 05/69] coverity scan CID 22553: dereference before null check --- src/kernel/xmlreader.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c index 4f6dbf0c0..63b0c1319 100644 --- a/src/kernel/xmlreader.c +++ b/src/kernel/xmlreader.c @@ -929,12 +929,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; - - if (name) xmlFree(name); + xmlFree(name); name = xmlGetProp(node, BAD_CAST "material"); if (name) { From 65429a12c31a74d4cbd266788b635b43fe8eaaa2 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:52:51 +0100 Subject: [PATCH 06/69] coverity scan CID 22585, 22586: result is not floating point --- src/study.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/study.c b/src/study.c index 3a0f38921..7c968637a 100644 --- a/src/study.c +++ b/src/study.c @@ -505,7 +505,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 +517,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; } } From 04bf07a526fb7cae899c045f4e4830047f293026 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 08:53:40 +0100 Subject: [PATCH 07/69] coverity scan (multiple CID) is confuced about a_remove, trying to help it. --- src/util/attrib.c | 8 +++++++- src/util/attrib.test.c | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/util/attrib.c b/src/util/attrib.c index 26869cfee..5c92b691e 100644 --- a/src/util/attrib.c +++ b/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; } diff --git a/src/util/attrib.test.c b/src/util/attrib.test.c index 479809d1d..691087477 100644 --- a/src/util/attrib.test.c +++ b/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; } From 78899ca801b844ae1eabe5c633acbbea879bb34a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:04:23 +0100 Subject: [PATCH 08/69] coverity scan CID 22433: buffer not null terminated, prefer strlcpy over strncpy --- src/util/log.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/log.c b/src/util/log.c index fadcb532a..33ca16abc 100644 --- a/src/util/log.c +++ b/src/util/log.c @@ -11,6 +11,7 @@ without prior permission by the authors of Eressea. */ #include #include "log.h" +#include "bsdstring.h" #include "unicode.h" #include @@ -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; } From 178a740ecf087e52a15ae225690db721853e6b78 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:15:29 +0100 Subject: [PATCH 09/69] coverity scan CID 22573: copy into fixed-size buffer in theory, all IDs in the game should be no longer than 4 characters, but coverity doesn't know this. --- src/sqlite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sqlite.c b/src/sqlite.c index fd46c4f1f..64819bca1 100644 --- a/src/sqlite.c +++ b/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)); sqlite3_prepare_v2(db, sql, -1, &stmt, 0); sqlite3_bind_int(stmt, 1, f->subscription); sqlite3_bind_text(stmt, 2, code, -1, SQLITE_STATIC); From 4da658584cc0e668a0969841a3d479594637b195 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:20:58 +0100 Subject: [PATCH 10/69] coverity scan CID 22511: dereference null return value --- src/spy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spy.c b/src/spy.c index eeca4e1d1..6249ab6b8 100644 --- a/src/spy.c +++ b/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; From c7aa8c89b194fcfd489ee2eef32587d323ac6d2d Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:22:00 +0100 Subject: [PATCH 11/69] coverity scan CID 22551: dereference before null check --- src/spells/shipcurse.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/spells/shipcurse.c b/src/spells/shipcurse.c index 29e4ad8f8..4b0dd9b30 100644 --- a/src/spells/shipcurse.c +++ b/src/spells/shipcurse.c @@ -138,9 +138,11 @@ 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); - c->data.v = sh; - if (c && c->duration > 0) { - sh->flags |= SF_FLYING; + if (c) { + c->data.v = sh; + if (c->duration > 0) { + sh->flags |= SF_FLYING; + } } return c; } From 6ebfd334789fb0fc654da1b7359aee81d0b25d5b Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:24:58 +0100 Subject: [PATCH 12/69] coverity scan CID 22503: negative array index write --- src/summary.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/summary.c b/src/summary.c index f38839db0..4b7e037a8 100644 --- a/src/summary.c +++ b/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; + } } } } From 55b3bfd90e86ba47caf3a1208ea2426a65c414a6 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:28:02 +0100 Subject: [PATCH 13/69] coverity scan CID 22574: copy into fixed size buffer zOrder *should* probably be big enough, but static analysis says safe is safe. --- src/study.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/study.c b/src/study.c index 7c968637a..9967719b3 100644 --- a/src/study.c +++ b/src/study.c @@ -430,8 +430,8 @@ int teach_cmd(unit * u, struct order *ord) /* Neuen Befehl zusammenbauen. TEMP-Einheiten werden automatisch in * ihre neuen Nummern uebersetzt. */ if (zOrder[0]) - strcat(zOrder, " "); - strcat(zOrder, unitid(u2)); + strncat(zOrder, " ", sizeof(zOrder)); + strncat(zOrder, unitid(u2), sizeof(zOrder)); if (getkeyword(u2->thisorder) != K_STUDY) { ADDMSG(&u->faction->msgs, From f4c32acd052fbf25f66f2f104bb803969efedc67 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:32:31 +0100 Subject: [PATCH 14/69] coverity scan CID 22515: out-of-bounds access increase buffer to stop coverity false positive --- src/study.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/study.c b/src/study.c index 9967719b3..f48b8176b 100644 --- a/src/study.c +++ b/src/study.c @@ -75,7 +75,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'; From e27c4b41641bf5ee1e59ed1d2a353c23201f59ff Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:39:33 +0100 Subject: [PATCH 15/69] coverity scan CID 22457: logically dead code the original TEACH_FRIENDS feature could not possibly have worked? --- src/study.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/src/study.c b/src/study.c index f48b8176b..c13b7818b 100644 --- a/src/study.c +++ b/src/study.c @@ -325,26 +325,29 @@ 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; - do { - sk = getskill(u->faction->locale); - teachskill[i++] = sk; - } while (sk != NOSKILL); - while (teaching && student) { - if (student->faction == u->faction) { - if (LongHunger(student)) - continue; + int t; + + for (t = 0; sk != NOSKILL; ++t) { + teachskill[t] = getskill(u->faction->locale); + }; + + for (student = r->units; teaching && student; student = student->next) { + 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) - if (sk == teachskill[i]) + for (t = 0; teachskill[t] != NOSKILL; ++t) { + 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 +355,8 @@ int teach_cmd(unit * u, struct order *ord) } } } - student = student->next; - } #ifdef TEACH_FRIENDS - while (teaching && student) { - if (student->faction != u->faction - && alliedunit(u, student->faction, HELP_GUARD)) { - if (LongHunger(student)) - continue; + else if (alliedunit(u, student->faction, HELP_GUARD)) { if (getkeyword(student->thisorder) == K_STUDY) { /* Input ist nun von student->thisorder !! */ init_order(student->thisorder); @@ -370,9 +367,8 @@ int teach_cmd(unit * u, struct order *ord) } } } - student = student->next; - } #endif + } } else #endif From 8b92003ffffa0f9c20fa0d987243bb8a05a11860 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:43:51 +0100 Subject: [PATCH 16/69] coverity scan CID 22487: dereference after null check we don't use many differnt calendars, so this has never happened. --- src/kernel/xmlreader.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c index 63b0c1319..52a0b8824 100644 --- a/src/kernel/xmlreader.c +++ b/src/kernel/xmlreader.c @@ -439,7 +439,6 @@ static int parse_calendar(xmlDocPtr doc) 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,14 +450,17 @@ static int parse_calendar(xmlDocPtr doc) monthnames[i] = _strdup(mkname("calendar", (const char *)propValue)); xmlFree(propValue); } - for (j = 0; j != seasons; ++j) { - xmlNodePtr season = month->parent; - if (season == nsetSeasons->nodeTab[j]) { - month_season[i] = j; - break; + if (nsetSeasons) { + int j; + for (j = 0; j != seasons; ++j) { + xmlNodePtr season = month->parent; + if (season == nsetSeasons->nodeTab[j]) { + month_season[i] = j; + break; + } } + assert(j != seasons); } - assert(j != seasons); storms[i] = xml_ivalue(nsetMonths->nodeTab[i], "storm", 0); } } From 2372d3aacd1c04ebd9c46495fe0a0c45e5ab39b3 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:49:26 +0100 Subject: [PATCH 17/69] coverity scan CID 22516: out-of-bounds write potentially a bug with bad XML files? only matters if we have those, but we shouldn't --- src/kernel/xmlreader.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c index 52a0b8824..12406dff6 100644 --- a/src/kernel/xmlreader.c +++ b/src/kernel/xmlreader.c @@ -1815,23 +1815,26 @@ static int parse_races(xmlDocPtr doc) if (result->nodesetval->nodeNr > MAXMAGIETYP) { log_error("race %s has %d potential familiars", rc->_name, result->nodesetval->nodeNr); } - for (k = 0; k != MAXMAGIETYP; ++k) { - if (k < result->nodesetval->nodeNr) { - xmlNodePtr node = result->nodesetval->nodeTab[k]; + else { + for (k = 0; k != MAXMAGIETYP; ++k) { + if (k < result->nodesetval->nodeNr) { + xmlNodePtr node = result->nodesetval->nodeTab[k]; - propValue = xmlGetProp(node, BAD_CAST "race"); - assert(propValue != NULL); - frc = rc_get_or_create((const char *)propValue); - if (xml_bvalue(node, "default", false)) { - rc->familiars[k] = rc->familiars[0]; - rc->familiars[0] = frc; + propValue = xmlGetProp(node, BAD_CAST "race"); + assert(propValue != NULL); + frc = rc_get_or_create((const char *)propValue); + if (xml_bvalue(node, "default", false)) { + rc->familiars[k] = rc->familiars[0]; + rc->familiars[0] = frc; + } + else { + rc->familiars[k] = frc; + } + xmlFree(propValue); } else { rc->familiars[k] = frc; } - xmlFree(propValue); - } else { - rc->familiars[k] = frc; } } xmlXPathFreeObject(result); From 8675002e687d09387fc787b77ccb98ca5abf287e Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 09:56:53 +0100 Subject: [PATCH 18/69] coverity scan CIDs 22540 22541 22546: resource leak the calendar should only ever be initialized once, but based on static analysis, I agree that this was sloppy. --- src/kernel/xmlreader.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c index 12406dff6..fdaad4b51 100644 --- a/src/kernel/xmlreader.c +++ b/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,9 +432,11 @@ 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) { From c298b7fd741be163ac2f815923496dc7bb4a538a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 11:06:25 +0100 Subject: [PATCH 19/69] coverity CID 22501 imroper use of negative value explicitly use SK_MAGIC instead of a lookup. Faster, easier to reason about. --- src/kernel/item.c | 2 +- src/skill.test.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/kernel/item.c b/src/kernel/item.c index ba6504746..63ba87a53 100644 --- a/src/kernel/item.c +++ b/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)); diff --git a/src/skill.test.c b/src/skill.test.c index 451f95d7c..248228a53 100644 --- a/src/skill.test.c +++ b/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")); From 976b6aaea171bf79191a92d546a9f7f53e4e5a95 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 11:17:31 +0100 Subject: [PATCH 20/69] coverity CID 22593: write to pointer after free github issue #324 use free_land where we need it. --- src/kernel/region.c | 6 +++--- src/kernel/region.h | 1 + src/kernel/save.c | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/kernel/region.c b/src/kernel/region.c index 4b57b9e51..779b45e4a 100644 --- a/src/kernel/region.c +++ b/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); diff --git a/src/kernel/region.h b/src/kernel/region.h index 93453364a..b0672e758 100644 --- a/src/kernel/region.h +++ b/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); diff --git a/src/kernel/save.c b/src/kernel/save.c index 300555180..c47e10e20 100644 --- a/src/kernel/save.c +++ b/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 */ - r->land->demands = 0; /* mem leak */ + free_land(r->land); + r->land = 0; } while (r->resources) { rawmaterial *rm = r->resources; From 4384183ab856a5087586ae283a4e25671af782cb Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 11:20:09 +0100 Subject: [PATCH 21/69] coverity scan CID 22504: argument cannot be negative handle error cases for ftell, just because they could happen, I guess? --- src/bind_config.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/bind_config.c b/src/bind_config.c index 66f3cb47f..a80cd279f 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -67,14 +67,16 @@ int config_read(const char *filename, const char * relpath) if (F) { int result; char *data; - size_t sz; fseek(F, 0, SEEK_END); - sz = ftell(F); + result = ftell(F); rewind(F); - data = malloc(sz); - fread(data, 1, sz, F); - fclose(F); + if (result > 0) { + size_t sz = (size_t)result; + data = malloc(sz); + fread(data, 1, sz, F); + fclose(F); + } result = config_parse(data); free(data); return result; From 4f25831407e4583a671b0d28e088ea33b7ae5a6a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 11:24:01 +0100 Subject: [PATCH 22/69] coverity scan CID 22576: arguments in wrong order I believe that was a false positive, and will try to aquelch it by naming the arguments better. --- src/kernel/unit.c | 42 +++++++++++++++++++++--------------------- src/kernel/unit.h | 2 +- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/kernel/unit.c b/src/kernel/unit.c index 064bdd87e..d922efcd5 100644 --- a/src/kernel/unit.c +++ b/src/kernel/unit.c @@ -943,7 +943,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 +954,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 +983,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; - level += sn->level * u2->number; + (sn->level + 1 - sn->weeks / (sn->level + 1.0)) * dst->number; + level += sn->level * dst->number; } - dlevel = dlevel / (n + u2->number); - level = level / (n + u2->number); + dlevel = dlevel / (n + dst->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 +1007,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 +1023,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); - u2->flags |= + set_leftship(dst, sh); + 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) { - set_number(u2, u2->number + n); + if (dst) { + 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; } } diff --git a/src/kernel/unit.h b/src/kernel/unit.h index 3aef7bd0f..86d4c08e4 100644 --- a/src/kernel/unit.h +++ b/src/kernel/unit.h @@ -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); From 8b6da79984986ea16fb73c224147a1f91f437793 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 11:32:04 +0100 Subject: [PATCH 23/69] 64 bit compatibility, thank you gcc. --- src/bind_config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bind_config.c b/src/bind_config.c index a80cd279f..c4140427f 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -65,7 +65,7 @@ int config_read(const char *filename, const char * relpath) F = fopen(filename, "rt"); } if (F) { - int result; + long result; char *data; fseek(F, 0, SEEK_END); From 5f4c31af1c8f3c626c63cc333b879f0e1970f3ec Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 12:04:52 +0100 Subject: [PATCH 24/69] bad variable reuse leads to gcc confusion --- src/bind_config.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/bind_config.c b/src/bind_config.c index c4140427f..68e5da9a1 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -65,14 +65,15 @@ int config_read(const char *filename, const char * relpath) F = fopen(filename, "rt"); } if (F) { - long result; + long size; + int result; char *data; fseek(F, 0, SEEK_END); - result = ftell(F); + size = ftell(F); rewind(F); - if (result > 0) { - size_t sz = (size_t)result; + if (size > 0) { + size_t sz = (size_t)size; data = malloc(sz); fread(data, 1, sz, F); fclose(F); From db6e9444d6895609d6abbdffe810a4044f79aa62 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:25:52 +0100 Subject: [PATCH 25/69] CID 26263: Memory - illegal accesses (UNINIT) --- src/bind_config.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/bind_config.c b/src/bind_config.c index 68e5da9a1..fd256a0cd 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -66,21 +66,23 @@ int config_read(const char *filename, const char * relpath) } if (F) { long size; - int result; - char *data; fseek(F, 0, SEEK_END); size = ftell(F); rewind(F); if (size > 0) { + int result; + char *data; size_t sz = (size_t)size; + data = malloc(sz); fread(data, 1, sz, F); fclose(F); + result = config_parse(data); + free(data); + return result; } - result = config_parse(data); - free(data); - return result; + fclose(F); } return 1; } From 59069ae3423b6a2ed937bb501a79054629649079 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:28:59 +0100 Subject: [PATCH 26/69] CID 26260: Resource leaks (RESOURCE_LEAK) --- src/bind_storage.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/bind_storage.c b/src/bind_storage.c index 726d2c14f..d82ccd07f 100644 --- a/src/bind_storage.c +++ b/src/bind_storage.c @@ -33,30 +33,29 @@ 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) { - 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); + } + else if (strchr(type, 'w')) { + int n = STREAM_VERSION; + data->version = RELEASE_VERSION; + fwrite(&data->version, sizeof(int), 1, F); + fwrite(&n, sizeof(int), 1, F); + } + fstream_init(&data->strm, F); + binstore_init(store, &data->strm); + tolua_pushusertype(L, (void *)data, TOLUA_CAST "storage"); + return 1; } - if (strchr(type, 'r')) { - fread(&data->version, sizeof(int), 1, F); - fseek(F, sizeof(int), SEEK_CUR); - } - else if (strchr(type, 'w')) { - int n = STREAM_VERSION; - data->version = RELEASE_VERSION; - fwrite(&data->version, sizeof(int), 1, F); - fwrite(&n, sizeof(int), 1, F); - } - fstream_init(&data->strm, F); - 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) From b7dce8071ca1f1afee281cb1163dbb37a0b3dd53 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:36:20 +0100 Subject: [PATCH 27/69] CID 26259: Memory - corruptions I apparently don't know how strncat works. --- src/study.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/study.c b/src/study.c index c13b7818b..4da9df4eb 100644 --- a/src/study.c +++ b/src/study.c @@ -41,6 +41,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. /* util includes */ #include #include +#include #include #include #include @@ -374,6 +375,7 @@ int teach_cmd(unit * u, struct order *ord) #endif { char zOrder[4096]; + size_t sz = sizeof(zOrder); order *new_order; zOrder[0] = '\0'; @@ -425,9 +427,11 @@ int teach_cmd(unit * u, struct order *ord) /* Neuen Befehl zusammenbauen. TEMP-Einheiten werden automatisch in * ihre neuen Nummern uebersetzt. */ - if (zOrder[0]) - strncat(zOrder, " ", sizeof(zOrder)); - strncat(zOrder, unitid(u2), sizeof(zOrder)); + if (zOrder[0]) { + strncat(zOrder, " ", sz - 1); + --sz; + } + sz -= strlcpy(zOrder + 4096 - sz, unitid(u2), sz); if (getkeyword(u2->thisorder) != K_STUDY) { ADDMSG(&u->faction->msgs, From 9d9994811a6de72a26813327519864b0a0049671 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:38:29 +0100 Subject: [PATCH 28/69] CID 26258: (NULL_RETURNS) deal with bad connection data, static analysis doen't knwo that we never have that. --- src/move.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/move.c b/src/move.c index 8eaf052ca..8a1e850ed 100644 --- a/src/move.c +++ b/src/move.c @@ -2379,7 +2379,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"); From 833a1e70be6e0f845be58928d5fa86149b549c84 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:40:18 +0100 Subject: [PATCH 29/69] CID 26257: Null pointer dereferences (NULL_RETURNS) this test should never fire, but it shuts up coverity --- src/spells/combatspells.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/spells/combatspells.c b/src/spells/combatspells.c index b20c6b42f..4b497d689 100644 --- a/src/spells/combatspells.c +++ b/src/spells/combatspells.c @@ -622,10 +622,12 @@ int sp_mindblast(struct castorder * co) skill_t sk = random_skill(du, false); if (sk != NOSKILL) { skill *sv = unit_skill(du, sk); - int n = 1 + rng_int() % 3; + if (sv) { + int n = 1 + rng_int() % 3; - reduce_skill(du, sv, n); - k += du->number; + reduce_skill(du, sv, n); + k += du->number; + } } else { /* unit has no skill. kill it. */ From 0a67d53264f235a075f6586e3e74410b8b8e6698 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:41:53 +0100 Subject: [PATCH 30/69] CID 26256: (NULL_RETURNS) --- src/battle.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/battle.c b/src/battle.c index 319aa4151..e8f076b9c 100644 --- a/src/battle.c +++ b/src/battle.c @@ -971,15 +971,17 @@ void drain_exp(struct unit *u, int n) } if (sk != NOSKILL) { skill *sv = unit_skill(u, sk); - while (n > 0) { - if (n >= 30 * u->number) { - reduce_skill(u, sv, 1); - n -= 30; - } - else { - if (rng_int() % (30 * u->number) < n) + if (sv) { + while (n > 0) { + if (n >= 30 * u->number) { reduce_skill(u, sv, 1); - n = 0; + n -= 30; + } + else { + if (rng_int() % (30 * u->number) < n) + reduce_skill(u, sv, 1); + n = 0; + } } } } From 7a01b58e8dfad056729818f5636f6833fc7991a4 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:43:09 +0100 Subject: [PATCH 31/69] CID 26255: Null pointer dereferences (FORWARD_NULL) --- src/bind_region.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bind_region.c b/src/bind_region.c index e114ea806..b7eaea9e7 100644 --- a/src/bind_region.c +++ b/src/bind_region.c @@ -465,9 +465,9 @@ static int tolua_region_create(lua_State * L) } if (result) { terraform_region(result, terrain); - } - if (result->land) { - fix_demand(result); + if (result->land) { + fix_demand(result); + } } tolua_pushusertype(L, result, TOLUA_CAST "region"); From 6113bc214412cd0d48451ac8759f6d8bd8ebd859 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:46:43 +0100 Subject: [PATCH 32/69] CID 26254: Incorrect expression (DIVIDE_BY_ZERO) magic should never be zero, but coverity doesn't know that. --- src/magic.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/magic.c b/src/magic.c index 4d801e7ff..52d85f435 100644 --- a/src/magic.c +++ b/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; - double x = (double)cast_grade / (double)effskill(u, SK_MAGIC, r); - int fumble_chance = (int)(((double)x * 40.0) - 20.0); + int fumble_chance, rnd = 0; + int effsk = effskill(u, SK_MAGIC, r); 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); From 1a252bc99471ca7f7a233180b0ff016baf40ef4d Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:50:55 +0100 Subject: [PATCH 33/69] CID 26253: Control flow issues (DEADCODE) transformations gone wrong in previous commit --- src/study.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/study.c b/src/study.c index 4da9df4eb..5edba9168 100644 --- a/src/study.c +++ b/src/study.c @@ -284,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"); @@ -328,11 +328,12 @@ int teach_cmd(unit * u, struct order *ord) if (getparam(u->faction->locale) == P_ANY) { unit *student; skill_t teachskill[MAXSKILLS]; - int t; + int t = 0; - for (t = 0; sk != NOSKILL; ++t) { + do { + sk = getskill(u->faction->locale); teachskill[t] = getskill(u->faction->locale); - }; + } while (sk != NOSKILL); for (student = r->units; teaching && student; student = student->next) { if (LongHunger(student)) { From 3bf5ba14bbc62d7dab2e97fddf5b21a97e169f93 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 16:51:39 +0100 Subject: [PATCH 34/69] CID 26252: Memory - illegal accesses (BUFFER_SIZE_WARNING) Calling strncpy with a maximum size argument of 5 bytes on destination array "code" of size 5 bytes might leave the destination string unterminated. --- src/sqlite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sqlite.c b/src/sqlite.c index 64819bca1..50b4024c8 100644 --- a/src/sqlite.c +++ b/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; - strncpy(code, itoa36(f->no), sizeof(code)); + 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); From b3edd8dc5ac574b79c930cd476f81c501c11683b Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 18:55:47 +0100 Subject: [PATCH 35/69] anerror in this bugfix was found by a lua test. sweet! --- src/bind_storage.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bind_storage.c b/src/bind_storage.c index d82ccd07f..dd67d93a8 100644 --- a/src/bind_storage.c +++ b/src/bind_storage.c @@ -36,7 +36,7 @@ static int tolua_storage_create(lua_State * L) FILE * F; F = fopen(filename, type); - if (!F) { + if (F) { gamedata *data = (gamedata *)calloc(1, sizeof(gamedata)); storage *store = (storage *)calloc(1, sizeof(storage)); data->store = store; From 4eb6a89821f5355076338a0dcc77d5ddf6f1c67b Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 20:39:44 +0100 Subject: [PATCH 36/69] CID 26262: short and unsigned types are the devil, use int everywhere for fewer headaches --- src/kernel/unit.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/kernel/unit.h b/src/kernel/unit.h index 86d4c08e4..606bccbcf 100644 --- a/src/kernel/unit.h +++ b/src/kernel/unit.h @@ -89,11 +89,11 @@ extern "C" { struct faction *faction; struct building *building; struct ship *ship; - unsigned short number; /* persons */ - short age; + int number; /* persons */ + int age; /* skill data */ - short skill_size; + int skill_size; struct skill *skills; struct item *items; reservation *reservations; From f581999dde1beca5a61ab8b43c06cef1d1e8053c Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 20:40:40 +0100 Subject: [PATCH 37/69] add a script to automate coverity scans --- s/coverity | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100755 s/coverity diff --git a/s/coverity b/s/coverity new file mode 100755 index 000000000..8a7748e31 --- /dev/null +++ b/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 From d3f7bd7b100756a217b04e619a5e6a52c731e78d Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 20:43:37 +0100 Subject: [PATCH 38/69] CID 22597: va_start/va_end mismatch --- src/util/bsdstring.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/bsdstring.c b/src/util/bsdstring.c index 96b71b595..98d2d00f5 100644 --- a/src/util/bsdstring.c +++ b/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; } From 4e431b41e2abac11fa8007bfc79c2f03f4697f83 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 20:45:36 +0100 Subject: [PATCH 39/69] CID 22575 (#2-1 of 2): Copy into fixed size buffer (STRING_OVERFLOW) --- src/report.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/report.c b/src/report.c index 54c089c7f..a8a347689 100644 --- a/src/report.c +++ b/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); From 05ef6deb26bf2d6c274101c02ce16299ceaa0ce2 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Thu, 29 Oct 2015 20:47:07 +0100 Subject: [PATCH 40/69] CID 22572: Copy into fixed size buffer (STRING_OVERFLOW) --- src/gmtool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/gmtool.c b/src/gmtool.c index dcdef8f45..5929eea0b 100644 --- a/src/gmtool.c +++ b/src/gmtool.c @@ -54,6 +54,7 @@ #include #include #include +#include #include #include @@ -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 { From 2f82cccea13503f54eae6a114a58cf810b1fea30 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 10:55:06 +0100 Subject: [PATCH 41/69] CID 22571: Copy into fixed size buffer (STRING_OVERFLOW) replace strcat with strlcat --- src/move.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/move.c b/src/move.c index 8a1e850ed..951fd33f3 100644 --- a/src/move.c +++ b/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]) - strcat(neworder, " "); - strcat(neworder, LOC(lang, parameters[P_PAUSE])); + if (neworder != obuf) { + obuf += strlcat(obuf, " ", sizeof(neworder)-(obuf-neworder)); + } + obuf += strlcat(obuf, LOC(lang, parameters[P_PAUSE]), sizeof(neworder) - (obuf - neworder)); } else { - if (neworder[0]) - strcat(neworder, " "); - strcat(neworder, LOC(lang, shortdirections[d])); + if (neworder != obuf) { + obuf += strlcat(obuf, " ", sizeof(neworder) - (obuf - neworder)); + } + obuf += strlcat(obuf, LOC(lang, shortdirections[d]), sizeof(neworder) - (obuf - neworder)); } } From 4ee0f76927449b58ff37c2cec73634312bb47b5c Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 10:57:52 +0100 Subject: [PATCH 42/69] CID 22570: Copy into fixed size buffer (STRING_OVERFLOW) not only that, but strlcpy is the faster option here, too. --- src/util/nrmessage.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/nrmessage.c b/src/util/nrmessage.c index 640cb81e6..e3b0bcc0b 100644 --- a/src/util/nrmessage.c +++ b/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); } From e69635d697311e9684e892b2c97616d623266c0b Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:02:51 +0100 Subject: [PATCH 43/69] CID 22569: Copy into fixed size buffer (STRING_OVERFLOW) bsdstring functions are the best. --- src/names.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/names.c b/src/names.c index df2e5e0a6..503d3f5ea 100644 --- a/src/names.c +++ b/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); - strcat(name, " "); + size_t sz = strlcpy(name, (const char *)str, sizeof(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, " "); - strcat(name, (const char *)str); + strlcat(name, " ", sizeof(name)); + strlcat(name, (const char *)str, sizeof(name)); } } return name; From 51f66b8da856fd3fc3a82071903712c6e2d6bb5e Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:06:24 +0100 Subject: [PATCH 44/69] CID 22568: Copy into fixed size buffer (STRING_OVERFLOW) more bsdstring replacements --- src/names.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/names.c b/src/names.c index 503d3f5ea..40bc9cbb2 100644 --- a/src/names.c +++ b/src/names.c @@ -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]); - strcat(n, silbe2[rng_int() % SIL2]); - strcat(n, silbe3[rng_int() % SIL3]); + sz = strlcpy(n, silbe1[rng_int() % SIL1], sizeof(n)); + sz += strlcat(n, silbe2[rng_int() % SIL2], sizeof(n)); + 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, " "); - strcat(name, n); + sz += strlcat(name, " ", sizeof(name)); + sz += strlcat(name, n, sizeof(name)); } if (u && (rng_int() % 3 == 0)) { - strcat(name, " von "); - strcat(name, (const char *)rname(u->region, default_locale)); + sz += strlcat(name, " von ", sizeof(name)); + sz += strlcat(name, (const char *)rname(u->region, default_locale), sizeof(name)); } } From b8d7fa5bcc5c43ca052ed6b1d21f7fdb39ff0c92 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:09:28 +0100 Subject: [PATCH 45/69] CID 22567: Copy into fixed size buffer (STRING_OVERFLOW) bsdstring replacements, again --- src/names.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/names.c b/src/names.c index 40bc9cbb2..7a0855ef4 100644 --- a/src/names.c +++ b/src/names.c @@ -357,6 +357,7 @@ 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; @@ -364,14 +365,14 @@ static const char *dracoid_name(const unit * u) 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, "'"); - strcat(name, drac_mid[rng_int() % DRAC_MID]); + strlcat(name, "'", sizeof(name)); + 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; } From df82a9795d15ad8eb584c5799cf62398900debfa Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:11:54 +0100 Subject: [PATCH 46/69] CID 22565: Copy into fixed size buffer (STRING_OVERFLOW) bsdstring to the rescue --- src/modules/autoseed.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/modules/autoseed.c b/src/modules/autoseed.c index dd73943f2..4ba679439 100644 --- a/src/modules/autoseed.c +++ b/src/modules/autoseed.c @@ -31,6 +31,7 @@ /* util includes */ #include #include +#include #include #include #include @@ -186,8 +187,9 @@ newfaction *read_newfactions(const char *filename) if (email[0] == '\0') break; if (password[0] == '\0') { - strcpy(password, itoa36(rng_int())); - strcat(password, itoa36(rng_int())); + size_t sz; + 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 From 0de6e5ecb8bbf4f19d6d646d64d469d139b22cd9 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:13:48 +0100 Subject: [PATCH 47/69] CID 22564: Copy into fixed size buffer (STRING_OVERFLOW) buffer sizes => bsdstring.h --- src/reports.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/reports.c b/src/reports.c index ce66d273f..ae965c938 100644 --- a/src/reports.c +++ b/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]) - strcat(buf, ","); - strcat(buf, rtype->extension); + if (buf[0]) { + strlcat(buf, ",", sizeof(buf)); + } + strlcat(buf, rtype->extension, sizeof(buf)); } } fputs(buf, F); From ca7d25858e8e57a88912524375d14f029cd8e23e Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:16:09 +0100 Subject: [PATCH 48/69] CID 22563: Copy into fixed size buffer (STRING_OVERFLOW) bsdstring to the rescue, once again --- src/laws.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/laws.c b/src/laws.c index 2fd9ce1d8..ca2e8a8ef 100755 --- a/src/laws.c +++ b/src/laws.c @@ -4513,7 +4513,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); From 1fb04179dccc001d837cc601fce50106b6770438 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:39:47 +0100 Subject: [PATCH 49/69] CID 22557: Copy into fixed size buffer (STRING_OVERFLOW) eliminate code duplication. bsdstring is better than libc strings. --- src/kernel/config.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/kernel/config.c b/src/kernel/config.c index 19390799d..0ff8542bb 100644 --- a/src/kernel/config.c +++ b/src/kernel/config.c @@ -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) From 5d273e475e28aac7844bf997b212eeaaab2e3334 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:41:59 +0100 Subject: [PATCH 50/69] CID 22558: Copy into fixed size buffer (STRING_OVERFLOW) bsdstring buffer size check --- src/kernel/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/config.c b/src/kernel/config.c index 0ff8542bb..e6a2b9b09 100644 --- a/src/kernel/config.c +++ b/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; From be0563c1f04b3afde3000a3dd31d87fe08168560 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:43:45 +0100 Subject: [PATCH 51/69] CID 22560: Copy into fixed size buffer (STRING_OVERFLOW) --- src/battle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/battle.c b/src/battle.c index e8f076b9c..50357c7bf 100644 --- a/src/battle.c +++ b/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; } From 68f8f0830e9c70bcbce0f340eb38ecd5a6a9305a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:45:20 +0100 Subject: [PATCH 52/69] CID 22561: Copy into fixed size buffer (STRING_OVERFLOW) --- src/reports.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/reports.c b/src/reports.c index ae965c938..792f8c7fb 100644 --- a/src/reports.c +++ b/src/reports.c @@ -1773,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")) From 7fa12ae3af8ced7e7e0271bed7d98351844fc585 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 11:59:05 +0100 Subject: [PATCH 53/69] CID 22556: String not null terminated (STRING_NULL) add missing null-termination --- src/bind_config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bind_config.c b/src/bind_config.c index fd256a0cd..be3495dea 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -76,7 +76,8 @@ int config_read(const char *filename, const char * relpath) size_t sz = (size_t)size; data = malloc(sz); - fread(data, 1, sz, F); + sz = fread(data, 1, sz, F); + data[sz] = 0; fclose(F); result = config_parse(data); free(data); From 54304af182a226250a9c6431e30102dd0e879045 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:08:09 +0100 Subject: [PATCH 54/69] allocate space for null terminator --- src/bind_config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bind_config.c b/src/bind_config.c index be3495dea..945c7f603 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -75,7 +75,7 @@ int config_read(const char *filename, const char * relpath) char *data; size_t sz = (size_t)size; - data = malloc(sz); + data = malloc(sz+1); sz = fread(data, 1, sz, F); data[sz] = 0; fclose(F); From 93b7bacff50b929ed93bef6a51e12ef9d37b71f4 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:37:27 +0100 Subject: [PATCH 55/69] CID 22552: Dereference before null check (REVERSE_INULL) --- src/kernel/build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/build.c b/src/kernel/build.c index bc5df861d..924dccf49 100644 --- a/src/kernel/build.c +++ b/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; From e1eb5098d49bedcca1592d6677f9440d52fbcc1c Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:40:48 +0100 Subject: [PATCH 56/69] CID 22554: Improper use of negative value (REVERSE_NEGATIVE) I believe this was a false positive, but more asserts can never hurt. --- src/kernel/unit.c | 1 + src/skill.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/kernel/unit.c b/src/kernel/unit.c index d922efcd5..ba2543e2b 100644 --- a/src/kernel/unit.c +++ b/src/kernel/unit.c @@ -735,6 +735,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) { diff --git a/src/skill.c b/src/skill.c index 3bee18005..3d5b77a96 100644 --- a/src/skill.c +++ b/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]; } From 1d204b12f1890b95c79e039d04305fc319066240 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:49:37 +0100 Subject: [PATCH 57/69] CID 22549: Dereference before null check (REVERSE_INULL) --- src/modules/museum.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/modules/museum.c b/src/modules/museum.c index 4914e11bd..05e446f1f 100644 --- a/src/modules/museum.c +++ b/src/modules/museum.c @@ -319,18 +319,18 @@ order * ord) } a = a_find(u->attribs, &at_museumexit); - assert(a); - r = findregion(a->data.sa[0], a->data.sa[1]); - assert(r); - a_remove(&u->attribs, a); - + if (a) { + 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); - unit_cookie = a->data.i; - a_remove(&u->attribs, a); - if (a) { + unit_cookie = a->data.i; + a_remove(&u->attribs, 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) From 1df0afc58a49d49ea9571dce8d866a57b7a58e40 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:50:32 +0100 Subject: [PATCH 58/69] CID 22550: Dereference before null check (REVERSE_INULL) --- src/give.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/give.c b/src/give.c index f9879b9dd..6d1d525e8 100644 --- a/src/give.c +++ b/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)); From 204b4d6b934a45c9e10362e77b5828ff08a25fab Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:52:18 +0100 Subject: [PATCH 59/69] CID 22505: Dereference null return value (NULL_RETURNS) potential bugs at the edge of the map --- src/spells.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spells.c b/src/spells.c index d9f7f4b10..abb938780 100644 --- a/src/spells.c +++ b/src/spells.c @@ -5954,7 +5954,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; From d6bc1c3119ef3732990a3cd2d1c53f66ffe68a7a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:54:07 +0100 Subject: [PATCH 60/69] CID 22506: Dereference null return value (NULL_RETURNS) always check fopen success --- src/laws.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/laws.c b/src/laws.c index ca2e8a8ef..d3cdcaba4 100755 --- a/src/laws.c +++ b/src/laws.c @@ -4512,7 +4512,6 @@ void update_subscriptions(void) { FILE *F; char zText[MAX_PATH]; - faction *f; strlcpy(zText, basepath(), sizeof(zText)); strlcat(zText, "/subscriptions", sizeof(zText)); @@ -4524,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); @@ -4536,11 +4537,14 @@ void update_subscriptions(void) sprintf(zText, "subscriptions.%u", turn); F = fopen(zText, "w"); - 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); + 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); } - fclose(F); } bool From 8f7f182c91124b3726b5bb76884381e9d9dbcb3b Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:55:30 +0100 Subject: [PATCH 61/69] CID 22507: Dereference null return value (NULL_RETURNS) checking fopen results --- src/creport.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/creport.c b/src/creport.c index 23f4f5671..9c2d2c423 100644 --- a/src/creport.c +++ b/src/creport.c @@ -1705,23 +1705,26 @@ int crwritemap(const char *filename) FILE *F = fopen(filename, "w"); region *r; - fprintf(F, "VERSION %d\n", C_REPORT_VERSION); - fputs("\"UTF-8\";charset\n", F); + if (F) { + fprintf(F, "VERSION %d\n", C_REPORT_VERSION); + fputs("\"UTF-8\";charset\n", F); - for (r = regions; r; r = r->next) { - plane *pl = rplane(r); - int plid = plane_id(pl); - if (plid) { - fprintf(F, "REGION %d %d %d\n", r->x, r->y, plid); + for (r = regions; r; r = r->next) { + plane *pl = rplane(r); + int plid = plane_id(pl); + if (plid) { + fprintf(F, "REGION %d %d %d\n", r->x, r->y, plid); + } + else { + fprintf(F, "REGION %d %d\n", r->x, r->y); + } + fprintf(F, "\"%s\";Name\n\"%s\";Terrain\n", rname(r, default_locale), + LOC(default_locale, terrain_name(r))); } - else { - fprintf(F, "REGION %d %d\n", r->x, r->y); - } - fprintf(F, "\"%s\";Name\n\"%s\";Terrain\n", rname(r, default_locale), - LOC(default_locale, terrain_name(r))); + fclose(F); + return 0; } - fclose(F); - return 0; + return EOF; } void register_cr(void) From e7ca5345d7c54b075435076d3400045a44bf8f88 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:57:07 +0100 Subject: [PATCH 62/69] CID 22509: Dereference null return value (NULL_RETURNS) potential problems at the edge of the map. --- src/kernel/connection.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/kernel/connection.c b/src/kernel/connection.c index 5bccfafd6..eecbd6c59 100644 --- a/src/kernel/connection.c +++ b/src/kernel/connection.c @@ -117,9 +117,11 @@ 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); - int k = reg_hashkey(rn); - if (k < key) { - walk_i(r, borders[k], cb, data); + if (rn) { + int k = reg_hashkey(rn); + if (k < key) { + walk_i(r, borders[k], cb, data); + } } } } From 674bc2b4c788bbdc026f8cd8877e4161d0f34ad5 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 12:58:27 +0100 Subject: [PATCH 63/69] nope, the ticket is actually required. --- src/modules/museum.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/modules/museum.c b/src/modules/museum.c index 05e446f1f..e1e7d33e5 100644 --- a/src/modules/museum.c +++ b/src/modules/museum.c @@ -319,11 +319,10 @@ order * ord) } a = a_find(u->attribs, &at_museumexit); - if (a) { - r = findregion(a->data.sa[0], a->data.sa[1]); - assert(r); - a_remove(&u->attribs, a); - } + assert(a); + 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); From 6713a7541e3f06a613368657da225cdf3a3500db Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 13:06:57 +0100 Subject: [PATCH 64/69] CID 22512: Out-of-bounds read (OVERRUN) upgrade to quicklist version that has this bug fixed --- quicklist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quicklist b/quicklist index 45f4577b8..66cfcd457 160000 --- a/quicklist +++ b/quicklist @@ -1 +1 @@ -Subproject commit 45f4577b8205d87b78d2b1f30b5c9baa25c86779 +Subproject commit 66cfcd45780ed8fd37217675a4a0795e0f5b995c From 1f45553cb5ba47e40024a31266750b635b4a2f75 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 14:25:17 +0100 Subject: [PATCH 65/69] quicklist update, again. --- quicklist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quicklist b/quicklist index 66cfcd457..f837dd31e 160000 --- a/quicklist +++ b/quicklist @@ -1 +1 @@ -Subproject commit 66cfcd45780ed8fd37217675a4a0795e0f5b995c +Subproject commit f837dd31e5fcf13c706db1ac2c86b7de3e706578 From 0da10ea490b1faeb08d3b58fb3f8826c3c50d903 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 14:41:52 +0100 Subject: [PATCH 66/69] CID 22513: Out-of-bounds read (OVERRUN) --- src/spells.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/spells.c b/src/spells.c index abb938780..9498a5a1f 100644 --- a/src/spells.c +++ b/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); From a91a0f6a535294246158062ea15f4d6667e5159e Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 15:17:39 +0100 Subject: [PATCH 67/69] CID 22514: Out-of-bounds read (OVERRUN) --- src/piracy.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/piracy.c b/src/piracy.c index ab68e4dae..8911f0882 100644 --- a/src/piracy.c +++ b/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) { + target_dir = dir; + a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); break; + } saff -= aff[dir].value; } - target_dir = dir; - a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); } } From 21a2313e2a79e066716669e550392620a2458551 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 15:20:06 +0100 Subject: [PATCH 68/69] CID 22517: Parse warning (PW.PARAM_SET_BUT_NOT_USED) --- src/kernel/unit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/kernel/unit.c b/src/kernel/unit.c index ba2543e2b..5e9b39705 100644 --- a/src/kernel/unit.c +++ b/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; From fbf483fb4eb82de366d4735d67e67e5eab4ca591 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 30 Oct 2015 15:21:06 +0100 Subject: [PATCH 69/69] CID 22518: Parse warning (PW.PARAM_SET_BUT_NOT_USED) --- src/names.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/names.c b/src/names.c index 7a0855ef4..e32d553d7 100644 --- a/src/names.c +++ b/src/names.c @@ -360,7 +360,7 @@ static const char *dracoid_name(const unit * u) size_t sz; /* ignore u */ - u = 0; + unused_arg(u); /* Wieviele Mittelteile? */ mid_syllabels = rng_int() % 4;