From f9fbe607663052f47a2179b89f6c96ca716995c3 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 6 Jan 2017 20:54:57 +0100 Subject: [PATCH 1/7] add a happy test for mt_new. refactor to not use strncpy. --- src/test_eressea.c | 1 + src/util/CMakeLists.txt | 2 +- src/util/message.c | 20 +++++++++++--------- src/util/message.test.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 src/util/message.test.c diff --git a/src/test_eressea.c b/src/test_eressea.c index 71cf008f2..6d2dead3f 100644 --- a/src/test_eressea.c +++ b/src/test_eressea.c @@ -65,6 +65,7 @@ int RunAllTests(int argc, char *argv[]) ADD_SUITE(direction); ADD_SUITE(skill); ADD_SUITE(keyword); + ADD_SUITE(message); ADD_SUITE(order); ADD_SUITE(race); /* util */ diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 7eda87132..842bcf58c 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -14,7 +14,7 @@ gamedata.test.c language.test.c # lists.test.c # log.test.c -# message.test.c +message.test.c # nrmessage.test.c parser.test.c password.test.c diff --git a/src/util/message.c b/src/util/message.c index f19fc3313..5b6f12eb8 100644 --- a/src/util/message.c +++ b/src/util/message.c @@ -87,19 +87,21 @@ message_type *mt_new(const char *name, const char *args[]) for (i = 0; args[i]; ++i) { const char *x = args[i]; const char *spos = strchr(x, ':'); - if (spos == NULL) { - mtype->pnames[i] = _strdup(x); - mtype->types[i] = NULL; + struct arg_type *atype = NULL; + if (spos != NULL) { + atype = find_argtype(spos + 1); + } + if (!atype) { + log_error("unknown argument type %s for message type %s\n", spos + 1, mtype->name); + assert(atype); } else { - char *cp = strncpy((char *)malloc(spos - x + 1), x, spos - x); + char *cp; + cp = malloc(spos - x + 1); + memcpy(cp, x, spos - x); cp[spos - x] = '\0'; mtype->pnames[i] = cp; - mtype->types[i] = find_argtype(spos + 1); - if (mtype->types[i] == NULL) { - log_error("unknown argument type %s for message type %s\n", spos + 1, mtype->name); - } - assert(mtype->types[i]); + mtype->types[i] = atype; } } } diff --git a/src/util/message.test.c b/src/util/message.test.c new file mode 100644 index 000000000..d114e33f2 --- /dev/null +++ b/src/util/message.test.c @@ -0,0 +1,29 @@ +#include +#include "message.h" + +#include +#include + +static void test_mt_new(CuTest *tc) +{ + message_type *mt; + test_setup(); + mt = mt_new_va("test", "name:string", "number:int", NULL); + CuAssertPtrNotNull(tc, mt); + CuAssertStrEquals(tc, "test", mt->name); + CuAssertIntEquals(tc, 2, mt->nparameters); + CuAssertPtrNotNull(tc, mt->pnames); + CuAssertStrEquals(tc, "name", mt->pnames[0]); + CuAssertStrEquals(tc, "number", mt->pnames[1]); + CuAssertPtrNotNull(tc, mt->types); + CuAssertStrEquals(tc, "string", mt->types[0]->name); + CuAssertStrEquals(tc, "int", mt->types[1]->name); + test_cleanup(); +} + +CuSuite *get_message_suite(void) +{ + CuSuite *suite = CuSuiteNew(); + SUITE_ADD_TEST(suite, test_mt_new); + return suite; +} From 946364268738ad3ad37af5014b594e789b17d2cd Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 6 Jan 2017 21:08:49 +0100 Subject: [PATCH 2/7] eliminate unnecessary strncpy use. --- src/report.c | 3 ++- src/reports.c | 6 +----- src/util/language.c | 25 ++++++++++++++++--------- src/util/language.test.c | 11 +++++++++++ 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/report.c b/src/report.c index 7f9ede7e6..0d6d283dd 100644 --- a/src/report.c +++ b/src/report.c @@ -413,7 +413,8 @@ void nr_spell_syntax(struct stream *out, spellbook_entry * sbe, const struct loc } else { char substr[32]; - strncpy(substr, syntaxp, cstr - syntaxp); + assert(sizeof(substr) > (cstr - syntaxp)); + memcpy(substr, syntaxp, cstr - syntaxp); substr[cstr - syntaxp] = 0; locp = LOC(lang, mkname("spellpar", substr)); syntaxp = substr + 1; diff --git a/src/reports.c b/src/reports.c index a8b9e5cfb..52278d270 100644 --- a/src/reports.c +++ b/src/reports.c @@ -849,10 +849,6 @@ const struct unit * u, struct skill * sv, int *dh, int days) void split_paragraph(strlist ** SP, const char *s, unsigned int indent, unsigned int width, char mark) { - - /* Die Liste SP wird mit dem String s aufgefuellt, mit indent und einer - * mark, falls angegeben. SP wurde also auf 0 gesetzt vor dem Aufruf. - * Vgl. spunit (). */ bool firstline; static char buf[REPORTWIDTH + 1]; // FIXME: static buffer, artificial limit size_t len = strlen(s); @@ -883,7 +879,7 @@ void split_paragraph(strlist ** SP, const char *s, unsigned int indent, unsigned if (!cut) { cut = s + _min(len, REPORTWIDTH); } - strncpy(buf + indent, s, cut - s); + memcpy(buf + indent, s, cut - s); buf[indent + (cut - s)] = 0; addstrlist(SP, buf); // TODO: too much string copying, cut out this function while (*cut == ' ') { diff --git a/src/util/language.c b/src/util/language.c index d8b899742..cb91d7419 100644 --- a/src/util/language.c +++ b/src/util/language.c @@ -101,16 +101,23 @@ locale *get_or_create_locale(const char *name) void make_locales(const char *str) { const char *tok = str; - while (*tok) { - char zText[32]; - while (*tok && *tok != ',') - ++tok; - strncpy(zText, str, tok - str); - zText[tok - str] = 0; - get_or_create_locale(zText); - if (*tok) { - str = ++tok; + while (tok) { + char zText[16]; + size_t len; + + tok = strchr(str, ','); + if (tok) { + len = tok - str; + assert(sizeof(zText) > len); + memcpy(zText, str, len); + str = tok + 1; } + else { + len = strlen(str); + memcpy(zText, str, len); + } + zText[len] = 0; + get_or_create_locale(zText); } } diff --git a/src/util/language.test.c b/src/util/language.test.c index 236eac497..40a6775fa 100644 --- a/src/util/language.test.c +++ b/src/util/language.test.c @@ -16,9 +16,20 @@ static void test_language(CuTest *tc) test_cleanup(); } +static void test_make_locales(CuTest *tc) +{ + test_setup(); + make_locales("aa,bb,cc"); + CuAssertPtrNotNull(tc, get_locale("aa")); + CuAssertPtrNotNull(tc, get_locale("bb")); + CuAssertPtrNotNull(tc, get_locale("cc")); + test_cleanup(); +} + CuSuite *get_language_suite(void) { CuSuite *suite = CuSuiteNew(); SUITE_ADD_TEST(suite, test_language); + SUITE_ADD_TEST(suite, test_make_locales); return suite; } From 8d02d5a5aa23b3260cd4e83569ad9ee09bb73ff7 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 6 Jan 2017 21:24:31 +0100 Subject: [PATCH 3/7] never use strncpy, anywhere. --- src/bind_config.c | 3 ++- src/helpers.c | 2 +- src/util/log.test.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bind_config.c b/src/bind_config.c index 3959cc22c..929e4cca0 100644 --- a/src/bind_config.c +++ b/src/bind_config.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -45,7 +46,7 @@ int config_parse(const char *json) if (xp >= ep) break; } xp = (ep > json + 10) ? ep - 10 : json; - strncpy(buffer, xp, sizeof(buffer)); + strlcpy(buffer, xp, sizeof(buffer)); buffer[9] = 0; log_error("json parse error in line %d, position %d, near `%s`\n", line, ep - lp, buffer); } diff --git a/src/helpers.c b/src/helpers.c index aa9bc12eb..e9502e075 100644 --- a/src/helpers.c +++ b/src/helpers.c @@ -167,7 +167,7 @@ static int lua_callspell(castorder * co) if (hashpos != NULL) { ptrdiff_t len = hashpos - fname; assert(len < (ptrdiff_t) sizeof(fbuf)); - strncpy(fbuf, fname, len); + memcpy(fbuf, fname, len); fbuf[len] = '\0'; fname = fbuf; } diff --git a/src/util/log.test.c b/src/util/log.test.c index 95cbefa96..55616c026 100644 --- a/src/util/log.test.c +++ b/src/util/log.test.c @@ -12,7 +12,7 @@ void log_string(void *data, int level, const char *module, const char *format, v unused_arg(format); unused_arg(module); unused_arg(level); - strncpy(str, arg, 32); + strcpy(str, arg); } static void test_logging(CuTest * tc) From 262580f1d5d70b258138926e1304d5d0c7e5f77d Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Fri, 6 Jan 2017 21:37:52 +0100 Subject: [PATCH 4/7] gcc warning eliminated --- src/report.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/report.c b/src/report.c index 0d6d283dd..0c7697fe5 100644 --- a/src/report.c +++ b/src/report.c @@ -413,9 +413,10 @@ void nr_spell_syntax(struct stream *out, spellbook_entry * sbe, const struct loc } else { char substr[32]; - assert(sizeof(substr) > (cstr - syntaxp)); - memcpy(substr, syntaxp, cstr - syntaxp); - substr[cstr - syntaxp] = 0; + size_t len = cstr - syntaxp; + assert(sizeof(substr) > len); + memcpy(substr, syntaxp, len); + substr[len] = 0; locp = LOC(lang, mkname("spellpar", substr)); syntaxp = substr + 1; } From 494643d65f399b1e7df8307e731382f92d52657e Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 7 Jan 2017 20:20:13 +0100 Subject: [PATCH 5/7] remove old & unused fix_famililar repair code. --- src/bind_monsters.c | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/src/bind_monsters.c b/src/bind_monsters.c index d7a3daab2..fd1130053 100644 --- a/src/bind_monsters.c +++ b/src/bind_monsters.c @@ -58,34 +58,6 @@ static int tolua_spawn_undead(lua_State * L) return 0; } -static int fix_familiars(struct lua_State *L) -{ - faction *f; - for (f = factions; f; f = f->next) { - unit *u; - for (u = f->units; u; u = u->nextF) { - struct sc_mage *mage = get_mage(u); - if (mage && is_familiar(u)) { - if (mage->spellbook && mage->magietyp == M_GRAY) { - equipment *eq; - char buffer[64]; - - spellbook_clear(mage->spellbook); - free(mage->spellbook); - mage->spellbook = 0; - - _snprintf(buffer, sizeof(buffer), "%s_familiar", u_race(u)->_name); - eq = get_equipment(buffer); - if (eq) { - equip_unit_mask(u, eq, EQUIP_SPELLS); - } - } - } - } - } - return 0; -} - void bind_monsters(struct lua_State *L) { tolua_module(L, NULL, 0); @@ -95,7 +67,6 @@ void bind_monsters(struct lua_State *L) tolua_function(L, TOLUA_CAST "plan_monsters", tolua_planmonsters); tolua_function(L, TOLUA_CAST "spawn_undead", tolua_spawn_undead); tolua_function(L, TOLUA_CAST "spawn_dragons", tolua_spawn_dragons); - tolua_function(L, TOLUA_CAST "fix_familiars", fix_familiars); tolua_function(L, TOLUA_CAST "get_monsters", tolua_get_monsters); } tolua_endmodule(L); From 3fb12d8f1ef0ee1a026d6d3ac76c393c89461d55 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 7 Jan 2017 21:09:39 +0100 Subject: [PATCH 6/7] replace snprintf and the like. you cannot trust _snprintf in MSVC (no zero-termination). --- src/bind_storage.c | 4 +++- src/gmtool.c | 2 +- src/json.c | 6 +++--- src/kernel/item.c | 5 ++++- src/kernel/jsonconf.c | 7 ++++++- src/util/bsdstring.c | 2 +- 6 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/bind_storage.c b/src/bind_storage.c index a81a3baeb..6b582e815 100644 --- a/src/bind_storage.c +++ b/src/bind_storage.c @@ -100,7 +100,9 @@ static int tolua_storage_tostring(lua_State * L) { gamedata *data = (gamedata *)tolua_tousertype(L, 1, 0); char name[64]; - _snprintf(name, sizeof(name), "", (void *)data, data->version); + // safe to use sprintf here, because: + // %p is at most 16 characters, %d 20, text is 16, comes to 53 with \0 + sprintf(name, "", (void *)data, data->version); lua_pushstring(L, name); return 1; } diff --git a/src/gmtool.c b/src/gmtool.c index 7aab2fea7..b1b9a1bc1 100644 --- a/src/gmtool.c +++ b/src/gmtool.c @@ -1167,7 +1167,7 @@ static void handlekey(state * st, int c) region *first = (mr && mr->r && mr->r->next) ? mr->r->next : regions; if (findmode == 'f') { - snprintf(sbuffer, sizeof(sbuffer), "find-faction: %s", locate); + slprintf(sbuffer, sizeof(sbuffer), "find-faction: %s", locate); statusline(st->wnd_status->handle, sbuffer); f = findfaction(atoi36(locate)); if (f == NULL) { diff --git a/src/json.c b/src/json.c index 10322fe3f..2c27b9947 100644 --- a/src/json.c +++ b/src/json.c @@ -67,13 +67,13 @@ int json_export(stream * out, int flags) { cJSON *json, *root = cJSON_CreateObject(); assert(out && out->api); if (regions && (flags & EXPORT_REGIONS)) { - char id[32]; + char id[32]; // TODO: static_assert(INT_MAX < 10^32) region * r; plane * p; cJSON_AddItemToObject(root, "planes", json = cJSON_CreateObject()); for (p = planes; p; p = p->next) { cJSON *data; - _snprintf(id, sizeof(id), "%d", p->id); + sprintf(id, "%d", p->id); // safe, unless int is bigger than 64 bit cJSON_AddItemToObject(json, id, data = cJSON_CreateObject()); cJSON_AddNumberToObject(data, "x", p->minx); cJSON_AddNumberToObject(data, "y", p->miny); @@ -85,7 +85,7 @@ int json_export(stream * out, int flags) { cJSON_AddItemToObject(root, "regions", json = cJSON_CreateObject()); for (r = regions; r; r = r->next) { cJSON *data; - _snprintf(id, sizeof(id), "%d", r->uid); + sprintf(id, "%d", r->uid); // safe, unless int is bigger than 64 bit cJSON_AddItemToObject(json, id, data = cJSON_CreateObject()); cJSON_AddNumberToObject(data, "x", r->x); cJSON_AddNumberToObject(data, "y", r->y); diff --git a/src/kernel/item.c b/src/kernel/item.c index 53fff79ab..9956f2a4a 100644 --- a/src/kernel/item.c +++ b/src/kernel/item.c @@ -156,7 +156,10 @@ const char *resourcename(const resource_type * rtype, int flags) } if (flags & NMF_PLURAL) { static char name[64]; // FIXME: static return value - _snprintf(name, sizeof(name), "%s_p", rtype->_name); + size_t len = strlen(rtype->_name); + assert(len <= sizeof(name) - 3); + memcpy(name, rtype->_name, len); + strcpy(name + len, "_p"); return name; } return rtype->_name; diff --git a/src/kernel/jsonconf.c b/src/kernel/jsonconf.c index d12238566..dccfbb68a 100644 --- a/src/kernel/jsonconf.c +++ b/src/kernel/jsonconf.c @@ -523,6 +523,8 @@ static void disable_feature(const char *str) { char name[32]; int k; skill_t sk; + size_t len; + sk = findskill(str); if (sk != NOSKILL) { enable_skill(sk, false); @@ -534,7 +536,10 @@ static void disable_feature(const char *str) { enable_keyword(k, false); return; } - _snprintf(name, sizeof(name), "%s.enabled", str); + len = strlen(str); + assert(len <= sizeof(name) - 9); + memcpy(name, str, len); + strcpy(name+len, ".enabled"); log_info("disable feature %s\n", name); config_set(name, "0"); } diff --git a/src/util/bsdstring.c b/src/util/bsdstring.c index 98d2d00f5..4fc56ce4e 100644 --- a/src/util/bsdstring.c +++ b/src/util/bsdstring.c @@ -13,7 +13,7 @@ int wrptr(char **ptr, size_t * size, int result) { size_t bytes = (size_t)result; if (result < 0) { - // _snprintf buffer was too small + // buffer was too small if (*size > 0) { **ptr = 0; *size = 0; From 6d60b48b3f95985aad55721bf275d309d84ac46d Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 7 Jan 2017 21:19:58 +0100 Subject: [PATCH 7/7] create_order takes variable arguments! more snprintf removal. --- src/give.test.c | 12 +++--------- src/kernel/jsonconf.c | 4 ++-- src/kernel/unit.test.c | 13 +++++-------- src/laws.test.c | 4 +--- 4 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/give.test.c b/src/give.test.c index f2b0d60a9..029ee7c44 100644 --- a/src/give.test.c +++ b/src/give.test.c @@ -233,7 +233,6 @@ static void test_give_men_requires_contact(CuTest * tc) { struct give env = { 0 }; message * msg; order *ord; - char cmd[32]; test_setup_ex(tc); env.f1 = test_create_faction(0); @@ -244,8 +243,7 @@ static void test_give_men_requires_contact(CuTest * tc) { CuAssertIntEquals(tc, 1, env.dst->number); CuAssertIntEquals(tc, 1, env.src->number); - _snprintf(cmd, sizeof(cmd), "%s ALLES PERSONEN", itoa36(env.dst->no)); - ord = create_order(K_GIVE, env.f1->locale, cmd); + ord = create_order(K_GIVE, env.f1->locale, "%s ALLES PERSONEN", itoa36(env.dst->no)); test_clear_messages(env.f1); give_cmd(env.src, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(env.f1->msgs, "give_person")); @@ -307,7 +305,6 @@ static void test_give(CuTest * tc) { static void test_give_cmd(CuTest * tc) { struct give env = { 0 }; struct order *ord; - char cmd[32]; test_setup_ex(tc); env.lang = test_create_locale(); @@ -316,8 +313,7 @@ static void test_give_cmd(CuTest * tc) { i_change(&env.src->items, env.itype, 10); - _snprintf(cmd, sizeof(cmd), "%s 5 %s", itoa36(env.dst->no), LOC(env.f1->locale, env.itype->rtype->_name)); - ord = create_order(K_GIVE, env.f1->locale, cmd); + ord = create_order(K_GIVE, env.f1->locale, "%s 5 %s", itoa36(env.dst->no), LOC(env.f1->locale, env.itype->rtype->_name)); assert(ord); give_cmd(env.src, ord); CuAssertIntEquals(tc, 5, i_get(env.src->items, env.itype)); @@ -330,7 +326,6 @@ static void test_give_cmd(CuTest * tc) { static void test_give_herbs(CuTest * tc) { struct give env = { 0 }; struct order *ord; - char cmd[32]; test_setup_ex(tc); test_create_world(); @@ -338,8 +333,7 @@ static void test_give_herbs(CuTest * tc) { setup_give(&env); i_change(&env.src->items, env.itype, 10); - _snprintf(cmd, sizeof(cmd), "%s %s", itoa36(env.dst->no), LOC(env.f1->locale, parameters[P_HERBS])); - ord = create_order(K_GIVE, env.f1->locale, cmd); + ord = create_order(K_GIVE, env.f1->locale, "%s %s", itoa36(env.dst->no), LOC(env.f1->locale, parameters[P_HERBS])); assert(ord); give_cmd(env.src, ord); diff --git a/src/kernel/jsonconf.c b/src/kernel/jsonconf.c index dccfbb68a..69e02c3a0 100644 --- a/src/kernel/jsonconf.c +++ b/src/kernel/jsonconf.c @@ -801,10 +801,10 @@ static void json_settings(cJSON *json) { else { char value[32]; if (child->type == cJSON_Number && child->valuedouble && child->valueintvaluedouble) { - _snprintf(value, sizeof(value), "%f", child->valuedouble); + sprintf(value, "%f", child->valuedouble); } else { - _snprintf(value, sizeof(value), "%d", child->valueint); + sprintf(value, "%d", child->valueint); } config_set(child->string, value); } diff --git a/src/kernel/unit.test.c b/src/kernel/unit.test.c index 752627144..1a481960d 100644 --- a/src/kernel/unit.test.c +++ b/src/kernel/unit.test.c @@ -140,37 +140,34 @@ static void test_scale_number(CuTest *tc) { static void test_unit_name(CuTest *tc) { unit *u; - char name[32]; test_cleanup(); test_create_world(); u = test_create_unit(test_create_faction(test_create_race("human")), findregion(0, 0)); + renumber_unit(u, 666); unit_setname(u, "Hodor"); - _snprintf(name, sizeof(name), "Hodor (%s)", itoa36(u->no)); - CuAssertStrEquals(tc, name, unitname(u)); + CuAssertStrEquals(tc, "Hodor (ii)", unitname(u)); test_cleanup(); } static void test_unit_name_from_race(CuTest *tc) { unit *u; - char name[32]; struct locale *lang; test_cleanup(); test_create_world(); u = test_create_unit(test_create_faction(test_create_race("human")), findregion(0, 0)); + renumber_unit(u, 666); unit_setname(u, NULL); lang = get_or_create_locale("de"); locale_setstring(lang, rc_name_s(u->_race, NAME_SINGULAR), "Mensch"); locale_setstring(lang, rc_name_s(u->_race, NAME_PLURAL), "Menschen"); - _snprintf(name, sizeof(name), "Mensch (%s)", itoa36(u->no)); - CuAssertStrEquals(tc, name, unitname(u)); + CuAssertStrEquals(tc, "Mensch (ii)", unitname(u)); CuAssertStrEquals(tc, "Mensch", unit_getname(u)); u->number = 2; - _snprintf(name, sizeof(name), "Menschen (%s)", itoa36(u->no)); - CuAssertStrEquals(tc, name, unitname(u)); + CuAssertStrEquals(tc, "Menschen (ii)", unitname(u)); CuAssertStrEquals(tc, "Menschen", unit_getname(u)); test_cleanup(); diff --git a/src/laws.test.c b/src/laws.test.c index 1bec0e6b4..d4753ec19 100644 --- a/src/laws.test.c +++ b/src/laws.test.c @@ -521,7 +521,6 @@ static void test_pay_cmd_other_building(CuTest *tc) { order *ord; faction *f; building *b; - char cmd[32]; test_setup(); setup_pay_cmd(&fix); @@ -531,8 +530,7 @@ static void test_pay_cmd_other_building(CuTest *tc) { config_set("rules.region_owner_pay_building", "lighthouse"); update_owners(b->region); - _snprintf(cmd, sizeof(cmd), "NOT %s", itoa36(b->no)); - ord = create_order(K_PAY, f->locale, cmd); + ord = create_order(K_PAY, f->locale, "NOT %s", itoa36(b->no)); assert(ord); CuAssertPtrEquals(tc, fix.u1, building_owner(b)); CuAssertIntEquals(tc, 0, pay_cmd(fix.u1, ord));