From db983479afdb0daddcb1cafcd609f4bce23fc993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:04:53 +0200 Subject: [PATCH 1/8] shared/sleep-config: fix memleak of strv, add test CID #1390921, #1390951. --- src/shared/sleep-config.c | 11 ++++------- src/test/test-sleep.c | 8 ++++++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index fba58ef367..efc547b6c0 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -34,7 +34,7 @@ int parse_sleep_config(const char *verb, char ***_modes, char ***_states, usec_t **suspend_mode = NULL, **suspend_state = NULL, **hibernate_mode = NULL, **hibernate_state = NULL, **hybrid_mode = NULL, **hybrid_state = NULL; - char **modes, **states; + _cleanup_strv_free_ char **modes, **states; /* always initialized below */ usec_t delay = 180 * USEC_PER_MINUTE; const ConfigTableItem items[] = { @@ -90,16 +90,13 @@ int parse_sleep_config(const char *verb, char ***_modes, char ***_states, usec_t assert_not_reached("what verb"); if ((!modes && STR_IN_SET(verb, "hibernate", "hybrid-sleep")) || - (!states && !streq(verb, "suspend-then-hibernate"))) { - strv_free(modes); - strv_free(states); + (!states && !streq(verb, "suspend-then-hibernate"))) return log_oom(); - } if (_modes) - *_modes = modes; + *_modes = TAKE_PTR(modes); if (_states) - *_states = states; + *_states = TAKE_PTR(states); if (_delay) *_delay = delay; diff --git a/src/test/test-sleep.c b/src/test/test-sleep.c index c2cb4ef949..1c5b56f4a0 100644 --- a/src/test/test-sleep.c +++ b/src/test/test-sleep.c @@ -14,6 +14,13 @@ #include "strv.h" #include "util.h" +static void test_parse_sleep_config(void) { + const char *verb; + + FOREACH_STRING(verb, "suspend", "hibernate", "hybrid-sleep", "suspend-then-hibernate") + assert_se(parse_sleep_config(verb, NULL, NULL, NULL) == 0); +} + static int test_fiemap(const char *path) { _cleanup_free_ struct fiemap *fiemap = NULL; _cleanup_close_ int fd = -1; @@ -84,6 +91,7 @@ int main(int argc, char* argv[]) { if (getuid() != 0) log_warning("This program is unlikely to work for unprivileged users"); + test_parse_sleep_config(); test_sleep(); if (argc <= 1) From f201daec89336833452a4d9fe3252c7755fc4e79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:16:03 +0200 Subject: [PATCH 2/8] Introduce _cleanup_(strbuf_cleanupp) and use it to fix null deref on error catalog_update() would call strbuf_cleanup(NULL) on allocation error. CID #1390928. --- src/basic/strbuf.h | 3 +++ src/journal/catalog.c | 50 +++++++++++++------------------------- src/journal/test-catalog.c | 6 ++--- src/test/test-strbuf.c | 4 +-- 4 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/basic/strbuf.h b/src/basic/strbuf.h index 9104bc7e93..84c2e4bfa5 100644 --- a/src/basic/strbuf.h +++ b/src/basic/strbuf.h @@ -11,6 +11,8 @@ #include #include +#include "macro.h" + struct strbuf { char *buf; size_t len; @@ -40,3 +42,4 @@ struct strbuf *strbuf_new(void); ssize_t strbuf_add_string(struct strbuf *str, const char *s, size_t len); void strbuf_complete(struct strbuf *str); void strbuf_cleanup(struct strbuf *str); +DEFINE_TRIVIAL_CLEANUP_FUNC(struct strbuf*, strbuf_cleanup); diff --git a/src/journal/catalog.c b/src/journal/catalog.c index 9c13d7b46d..24bf427103 100644 --- a/src/journal/catalog.c +++ b/src/journal/catalog.c @@ -448,7 +448,7 @@ error: int catalog_update(const char* database, const char* root, const char* const* dirs) { _cleanup_strv_free_ char **files = NULL; char **f; - struct strbuf *sb = NULL; + _cleanup_(strbuf_cleanupp) struct strbuf *sb = NULL; _cleanup_hashmap_free_free_free_ Hashmap *h = NULL; _cleanup_free_ CatalogItem *items = NULL; ssize_t offset; @@ -461,38 +461,29 @@ int catalog_update(const char* database, const char* root, const char* const* di h = hashmap_new(&catalog_hash_ops); sb = strbuf_new(); - - if (!h || !sb) { - r = log_oom(); - goto finish; - } + if (!h || !sb) + return log_oom(); r = conf_files_list_strv(&files, ".catalog", root, 0, dirs); - if (r < 0) { - log_error_errno(r, "Failed to get catalog files: %m"); - goto finish; - } + if (r < 0) + return log_error_errno(r, "Failed to get catalog files: %m"); STRV_FOREACH(f, files) { log_debug("Reading file '%s'", *f); r = catalog_import_file(h, *f); - if (r < 0) { - log_error_errno(r, "Failed to import file '%s': %m", *f); - goto finish; - } + if (r < 0) + return log_error_errno(r, "Failed to import file '%s': %m", *f); } if (hashmap_size(h) <= 0) { log_info("No items in catalog."); - goto finish; + return 0; } else log_debug("Found %u items in catalog.", hashmap_size(h)); items = new(CatalogItem, hashmap_size(h)); - if (!items) { - r = log_oom(); - goto finish; - } + if (!items) + return log_oom(); n = 0; HASHMAP_FOREACH_KEY(payload, i, h, j) { @@ -501,10 +492,9 @@ int catalog_update(const char* database, const char* root, const char* const* di isempty(i->language) ? "C" : i->language); offset = strbuf_add_string(sb, payload, strlen(payload)); - if (offset < 0) { - r = log_oom(); - goto finish; - } + if (offset < 0) + return log_oom(); + i->offset = htole64((uint64_t) offset); items[n++] = *i; } @@ -516,17 +506,11 @@ int catalog_update(const char* database, const char* root, const char* const* di sz = write_catalog(database, sb, items, n); if (sz < 0) - r = log_error_errno(sz, "Failed to write %s: %m", database); - else { - r = 0; - log_debug("%s: wrote %u items, with %zu bytes of strings, %"PRIi64" total size.", - database, n, sb->len, sz); - } + return log_error_errno(sz, "Failed to write %s: %m", database); -finish: - strbuf_cleanup(sb); - - return r; + log_debug("%s: wrote %u items, with %zu bytes of strings, %"PRIi64" total size.", + database, n, sb->len, sz); + return 0; } static int open_mmap(const char *database, int *_fd, struct stat *_st, void **_p) { diff --git a/src/journal/test-catalog.c b/src/journal/test-catalog.c index 9e3476d405..d5640c549c 100644 --- a/src/journal/test-catalog.c +++ b/src/journal/test-catalog.c @@ -177,16 +177,16 @@ static void test_catalog_update(void) { /* Test what happens if there are no files. */ r = catalog_update(database, NULL, NULL); - assert_se(r >= 0); + assert_se(r == 0); /* Test what happens if there are no files in the directory. */ r = catalog_update(database, NULL, no_catalog_dirs); - assert_se(r >= 0); + assert_se(r == 0); /* Make sure that we at least have some files loaded or the catalog_list below will fail. */ r = catalog_update(database, NULL, catalog_dirs); - assert_se(r >= 0); + assert_se(r == 0); } static void test_catalog_file_lang(void) { diff --git a/src/test/test-strbuf.c b/src/test/test-strbuf.c index 7fe9d66223..8e5b8b1d77 100644 --- a/src/test/test-strbuf.c +++ b/src/test/test-strbuf.c @@ -18,7 +18,7 @@ static ssize_t add_string(struct strbuf *sb, const char *s) { } static void test_strbuf(void) { - struct strbuf *sb; + _cleanup_(strbuf_cleanupp) struct strbuf *sb; _cleanup_strv_free_ char **l; ssize_t a, b, c, d, e, f, g, h; @@ -72,8 +72,6 @@ static void test_strbuf(void) { strbuf_complete(sb); assert_se(sb->root == NULL); - - strbuf_cleanup(sb); } int main(int argc, const char *argv[]) { From f20f4a775ed8d2bc486c0ddea8ee0713c672c217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:22:29 +0200 Subject: [PATCH 3/8] basic/format-table: add missing va_end() CID #1390930, #1390940. --- src/basic/format-table.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/basic/format-table.c b/src/basic/format-table.c index ae2c6238c4..582c5b86a4 100644 --- a/src/basic/format-table.c +++ b/src/basic/format-table.c @@ -648,6 +648,7 @@ int table_set_display(Table *t, size_t first_column, ...) { break; } + va_end(ap); return 0; } @@ -676,6 +677,7 @@ int table_set_sort(Table *t, size_t first_column, ...) { if (column == (size_t) -1) break; } + va_end(ap); return 0; } From 36591e108093a16892d8521babc18ad246fb594b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:34:33 +0200 Subject: [PATCH 4/8] logind: fix borked r check CID #1390947, #1390952. --- src/login/logind.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/login/logind.c b/src/login/logind.c index 385c977399..96f48cbd17 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -389,14 +389,18 @@ static int parse_fdname(const char *fdname, char **session_id, dev_t *dev) { if (!streq(parts[0], "session")) return -EINVAL; + id = strdup(parts[1]); if (!id) return -ENOMEM; if (!streq(parts[2], "device")) return -EINVAL; - r = safe_atou(parts[3], &major) || - safe_atou(parts[4], &minor); + + r = safe_atou(parts[3], &major); + if (r < 0) + return r; + r = safe_atou(parts[4], &minor); if (r < 0) return r; From 6a6e9c039ff1576b53e92aed47f8527d4880116d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:45:20 +0200 Subject: [PATCH 5/8] localed: fix memleak in error path CID #1390929. --- src/locale/keymap-util.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index 09bd449925..1aca627658 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -530,9 +530,10 @@ int find_converted_keymap(const char *x11_layout, const char *x11_variant, char return 0; } -int find_legacy_keymap(Context *c, char **new_keymap) { +int find_legacy_keymap(Context *c, char **ret) { const char *map; _cleanup_fclose_ FILE *f = NULL; + _cleanup_free_ char *new_keymap = NULL; unsigned n = 0; unsigned best_matching = 0; int r; @@ -597,7 +598,7 @@ int find_legacy_keymap(Context *c, char **new_keymap) { if (matching > best_matching) { best_matching = matching; - r = free_and_strdup(new_keymap, a[0]); + r = free_and_strdup(&new_keymap, a[0]); if (r < 0) return r; } @@ -617,13 +618,12 @@ int find_legacy_keymap(Context *c, char **new_keymap) { r = find_converted_keymap(l, v, &converted); if (r < 0) return r; - if (r > 0) { - free(*new_keymap); - *new_keymap = converted; - } + if (r > 0) + free_and_replace(new_keymap, converted); } - return (bool) *new_keymap; + *ret = TAKE_PTR(new_keymap); + return (bool) *ret; } int find_language_fallback(const char *lang, char **language) { From 03d3a9d5be0879ccbf2686a528c4e686ef0c0af2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:49:01 +0200 Subject: [PATCH 6/8] udevadm: fix null dererefence on allocation error CID #1390936. --- src/udev/udevadm-hwdb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/udev/udevadm-hwdb.c b/src/udev/udevadm-hwdb.c index c946131fb7..646c8507f7 100644 --- a/src/udev/udevadm-hwdb.c +++ b/src/udev/udevadm-hwdb.c @@ -703,7 +703,8 @@ out: if (trie) { if (trie->root) trie_node_cleanup(trie->root); - strbuf_cleanup(trie->strings); + if (trie->strings) + strbuf_cleanup(trie->strings); free(trie); } return rc; From 027cc9c92eb92ff89aacc22c9a131e7988b72e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 01:55:05 +0200 Subject: [PATCH 7/8] basic/fs-util: remove logically dead code We can jump to chase_one from two places. In the first 'todo' is set to 'buffer', which comes from path_make_absolute_cwd() and is nonnull In the second 'todo' is set to 'joined' which is checked to be nonull a few lines above the jump. So let's kill the code that deals with null todo there. CID #1390941. --- src/basic/fs-util.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 13dccfef54..232a21c193 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -919,25 +919,12 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, return exists; chased_one: - if (ret) { char *c; - if (done) { - if (todo) { - c = strjoin(done, todo); - if (!c) - return -ENOMEM; - } else - c = TAKE_PTR(done); - } else { - if (todo) - c = strdup(todo); - else - c = strdup("/"); - if (!c) - return -ENOMEM; - } + c = strjoin(strempty(done), todo); + if (!c) + return -ENOMEM; *ret = c; } From f1470e424b2b5337e3c383d68dc5a26af1ff4ce6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 May 2018 02:03:23 +0200 Subject: [PATCH 8/8] core/mount-setup: remove part of check which is always true k was set to join_controllers at this point and only incremented, so it cannot be null at this point. CID #1390949. --- src/core/mount-setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c index ac2412bf53..79300332a8 100644 --- a/src/core/mount-setup.c +++ b/src/core/mount-setup.c @@ -279,7 +279,7 @@ int mount_cgroup_controllers(char ***join_controllers) { if (strv_find(*k, controller)) break; - if (k && *k) { + if (*k) { char **i, **j; for (i = *k, j = *k; *i; i++) {