From 82501b3fc40dae2660a86ab07462f33fe26347ad Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 18 Feb 2016 17:33:10 -0500 Subject: basic/strbuf: do not call bsearch with a null argument Das ist verboten! src/basic/strbuf.c:162:23: runtime error: null pointer passed as argument 2, which is declared to never be null --- src/basic/strbuf.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/basic/strbuf.c b/src/basic/strbuf.c index 77220c0251..dac2881603 100644 --- a/src/basic/strbuf.c +++ b/src/basic/strbuf.c @@ -156,6 +156,10 @@ ssize_t strbuf_add_string(struct strbuf *str, const char *s, size_t len) { return off; } + /* bsearch is not allowed on a NULL sequence */ + if (node->children_count == 0) + break; + /* lookup child node */ c = s[len - 1 - depth]; search.c = c; -- cgit v1.2.3-54-g00ecf From 06466a7f039240b4c75cd5920401a7699c263d6e Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 18 Feb 2016 17:37:17 -0500 Subject: journal/catalog: fix memory leaks Various buffers were lost because finish_item() either consumed the buffer or allocated a new one (if an entry with the same key existed). The caller would simply forget the buffer in either case. Also add a check for the case when a valid identifier is followed by an empty body. We should not allow this. Also be more consistent in error handling and always print an error message. --- src/journal/catalog.c | 81 +++++++++++++++++++++++----------------------- src/journal/test-catalog.c | 2 ++ 2 files changed, 43 insertions(+), 40 deletions(-) diff --git a/src/journal/catalog.c b/src/journal/catalog.c index 164a3a15f2..72c2da10f1 100644 --- a/src/journal/catalog.c +++ b/src/journal/catalog.c @@ -164,14 +164,14 @@ static int finish_item( Hashmap *h, sd_id128_t id, const char *language, - char *payload) { + char *payload, size_t payload_size) { _cleanup_free_ CatalogItem *i = NULL; - _cleanup_free_ char *combined = NULL, *prev = NULL; - int r; + _cleanup_free_ char *prev = NULL, *combined = NULL; assert(h); assert(payload); + assert(payload_size > 0); i = new0(CatalogItem, 1); if (!i) @@ -184,23 +184,25 @@ static int finish_item( } prev = hashmap_get(h, i); - - /* Already have such an item, combine them */ if (prev) { + /* Already have such an item, combine them */ combined = combine_entries(payload, prev); if (!combined) return log_oom(); - r = hashmap_update(h, i, combined); - if (r < 0) - return r; - combined = NULL; - /* A new item */ + if (hashmap_update(h, i, combined) < 0) + return log_oom(); + combined = NULL; } else { - r = hashmap_put(h, i, payload); - if (r < 0) - return r; + /* A new item */ + combined = memdup(payload, payload_size + 1); + if (!combined) + return log_oom(); + + if (hashmap_put(h, i, combined) < 0) + return log_oom(); i = NULL; + combined = NULL; } return 0; @@ -262,6 +264,7 @@ static int catalog_entry_lang(const char* filename, int line, int catalog_import_file(Hashmap *h, const char *path) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *payload = NULL; + size_t payload_size = 0, payload_allocated = 0; unsigned n = 0; sd_id128_t id; _cleanup_free_ char *deflang = NULL, *lang = NULL; @@ -283,8 +286,7 @@ int catalog_import_file(Hashmap *h, const char *path) { for (;;) { char line[LINE_MAX]; - size_t a, b, c; - char *t; + size_t line_len; if (!fgets(line, sizeof(line), f)) { if (feof(f)) @@ -323,17 +325,23 @@ int catalog_import_file(Hashmap *h, const char *path) { if (sd_id128_from_string(line + 2 + 1, &jd) >= 0) { if (got_id) { - r = finish_item(h, id, lang ?: deflang, payload); + if (payload_size == 0) { + log_error("[%s:%u] No payload text.", path, n); + return -EINVAL; + } + + r = finish_item(h, id, lang ?: deflang, payload, payload_size); if (r < 0) return r; - payload = NULL; lang = mfree(lang); + payload_size = 0; } if (with_language) { - t = strstrip(line + 2 + 1 + 32 + 1); + char *t; + t = strstrip(line + 2 + 1 + 32 + 1); r = catalog_entry_lang(path, n, t, deflang, &lang); if (r < 0) return r; @@ -343,9 +351,6 @@ int catalog_import_file(Hashmap *h, const char *path) { empty_line = false; id = jd; - if (payload) - payload[0] = '\0'; - continue; } } @@ -356,34 +361,30 @@ int catalog_import_file(Hashmap *h, const char *path) { return -EINVAL; } - a = payload ? strlen(payload) : 0; - b = strlen(line); - - c = a + (empty_line ? 1 : 0) + b + 1 + 1; - t = realloc(payload, c); - if (!t) + line_len = strlen(line); + if (!GREEDY_REALLOC(payload, payload_allocated, + payload_size + (empty_line ? 1 : 0) + line_len + 1 + 1)) return log_oom(); - if (empty_line) { - t[a] = '\n'; - memcpy(t + a + 1, line, b); - t[a+b+1] = '\n'; - t[a+b+2] = 0; - } else { - memcpy(t + a, line, b); - t[a+b] = '\n'; - t[a+b+1] = 0; - } + if (empty_line) + payload[payload_size++] = '\n'; + memcpy(payload + payload_size, line, line_len); + payload_size += line_len; + payload[payload_size++] = '\n'; + payload[payload_size] = '\0'; - payload = t; empty_line = false; } if (got_id) { - r = finish_item(h, id, lang ?: deflang, payload); + if (payload_size == 0) { + log_error("[%s:%u] No payload text.", path, n); + return -EINVAL; + } + + r = finish_item(h, id, lang ?: deflang, payload, payload_size); if (r < 0) return r; - payload = NULL; } return 0; diff --git a/src/journal/test-catalog.c b/src/journal/test-catalog.c index da6fcbca4d..898c876450 100644 --- a/src/journal/test-catalog.c +++ b/src/journal/test-catalog.c @@ -103,6 +103,8 @@ static void test_catalog_import_one(void) { assert_se(hashmap_size(h) == 1); HASHMAP_FOREACH(payload, h, j) { + printf("expect: %s\n", expect); + printf("actual: %s\n", payload); assert_se(streq(expect, payload)); } } -- cgit v1.2.3-54-g00ecf From d09139e1879960bd2ed81466d2f13d6cf35d0a5b Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 18 Feb 2016 18:59:27 -0500 Subject: test-hashmap: fix undefined behaviour on string constants The test was failing at -O2+ with gcc 5.3 and 6.0. "val1" == "val1" and "val1" != "val1" are both valid. http://stackoverflow.com/questions/4843640/why-is-a-a-in-c --- src/test/test-hashmap-plain.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/test/test-hashmap-plain.c b/src/test/test-hashmap-plain.c index 6bf33306a9..1bd5c02f87 100644 --- a/src/test/test-hashmap-plain.c +++ b/src/test/test-hashmap-plain.c @@ -323,26 +323,29 @@ static void test_hashmap_remove_value(void) { _cleanup_hashmap_free_ Hashmap *m = NULL; char *r; - r = hashmap_remove_value(NULL, "key 1", (void*) "val 1"); + char val1[] = "val 1"; + char val2[] = "val 2"; + + r = hashmap_remove_value(NULL, "key 1", val1); assert_se(r == NULL); m = hashmap_new(&string_hash_ops); assert_se(m); - r = hashmap_remove_value(m, "key 1", (void*) "val 1"); + r = hashmap_remove_value(m, "key 1", val1); assert_se(r == NULL); - hashmap_put(m, "key 1", (void*) "val 1"); - hashmap_put(m, "key 2", (void*) "val 2"); + hashmap_put(m, "key 1", val1); + hashmap_put(m, "key 2", val2); - r = hashmap_remove_value(m, "key 1", (void*) "val 1"); + r = hashmap_remove_value(m, "key 1", val1); assert_se(streq(r, "val 1")); r = hashmap_get(m, "key 2"); assert_se(streq(r, "val 2")); assert_se(!hashmap_get(m, "key 1")); - r = hashmap_remove_value(m, "key 2", (void*) "val 1"); + r = hashmap_remove_value(m, "key 2", val1); assert_se(r == NULL); r = hashmap_get(m, "key 2"); -- cgit v1.2.3-54-g00ecf From 240a7ba9d8eaf76e1f3fb7a5b8424ab6eaead59d Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 16 Feb 2016 13:15:34 -0500 Subject: time-util: rewrite check in a way that does not confuse gcc gcc thinks that multiplier might be unitialized. Split out the inner loop to make the function easier to grok. --- src/basic/time-util.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/basic/time-util.c b/src/basic/time-util.c index 0b4f5ab5b9..130acaa9de 100644 --- a/src/basic/time-util.c +++ b/src/basic/time-util.c @@ -705,8 +705,7 @@ finish: return 0; } -int parse_time(const char *t, usec_t *usec, usec_t default_unit) { - +static char* extract_multiplier(char *p, usec_t *multiplier) { static const struct { const char *suffix; usec_t usec; @@ -740,7 +739,22 @@ int parse_time(const char *t, usec_t *usec, usec_t default_unit) { { "usec", 1ULL }, { "us", 1ULL }, }; + unsigned i; + + for (i = 0; i < ELEMENTSOF(table); i++) { + char *e; + e = startswith(p, table[i].suffix); + if (e) { + *multiplier = table[i].usec; + return e; + } + } + + return p; +} + +int parse_time(const char *t, usec_t *usec, usec_t default_unit) { const char *p, *s; usec_t r = 0; bool something = false; @@ -765,8 +779,8 @@ int parse_time(const char *t, usec_t *usec, usec_t default_unit) { for (;;) { long long l, z = 0; char *e; - unsigned i, n = 0; - usec_t multiplier, k; + unsigned n = 0; + usec_t multiplier = default_unit, k; p += strspn(p, WHITESPACE); @@ -779,10 +793,8 @@ int parse_time(const char *t, usec_t *usec, usec_t default_unit) { errno = 0; l = strtoll(p, &e, 10); - if (errno > 0) return -errno; - if (l < 0) return -ERANGE; @@ -806,18 +818,7 @@ int parse_time(const char *t, usec_t *usec, usec_t default_unit) { return -EINVAL; e += strspn(e, WHITESPACE); - - for (i = 0; i < ELEMENTSOF(table); i++) - if (startswith(e, table[i].suffix)) { - multiplier = table[i].usec; - p = e + strlen(table[i].suffix); - break; - } - - if (i >= ELEMENTSOF(table)) { - multiplier = default_unit; - p = e; - } + p = extract_multiplier(e, &multiplier); something = true; -- cgit v1.2.3-54-g00ecf