From 60b17d6fcd988c9995b7d1476d3aba1c4cbbfddd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 09:39:39 +0200 Subject: [PATCH 01/10] shared: fix assert call Fixup for 3572d3df8f8. Coverity CID#1403013. --- src/shared/bus-wait-for-units.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bus-wait-for-units.c b/src/shared/bus-wait-for-units.c index 4ee2a2908d..78b2a60403 100644 --- a/src/shared/bus-wait-for-units.c +++ b/src/shared/bus-wait-for-units.c @@ -190,7 +190,7 @@ static void wait_for_item_check_ready(WaitForItem *item) { BusWaitForUnits *d; assert(item); - assert(d = item->parent); + assert_se(d = item->parent); if (FLAGS_SET(item->flags, BUS_WAIT_FOR_MAINTENANCE_END)) { From ba5d26ccb23f6a312a11ba710fefc2ea92160ac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 09:49:01 +0200 Subject: [PATCH 02/10] shared: voidify call to loop_write() and trim duplicate code Coverity CID#1402375. --- src/basic/terminal-util.c | 82 ++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 48 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 76d6d1a20c..1f39c17306 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -526,71 +526,57 @@ int terminal_vhangup(const char *name) { } int vt_disallocate(const char *name) { - _cleanup_close_ int fd = -1; - const char *e, *n; - unsigned u; + const char *e; int r; /* Deallocate the VT if possible. If not possible * (i.e. because it is the active one), at least clear it - * entirely (including the scrollback buffer) */ + * entirely (including the scrollback buffer). */ e = path_startswith(name, "/dev/"); if (!e) return -EINVAL; - if (!tty_is_vc(name)) { - /* So this is not a VT. I guess we cannot deallocate - * it then. But let's at least clear the screen */ + if (tty_is_vc(name)) { + _cleanup_close_ int fd = -1; + unsigned u; + const char *n; - fd = open_terminal(name, O_RDWR|O_NOCTTY|O_CLOEXEC); + n = startswith(e, "tty"); + if (!n) + return -EINVAL; + + r = safe_atou(n, &u); + if (r < 0) + return r; + + if (u <= 0) + return -EINVAL; + + /* Try to deallocate */ + fd = open_terminal("/dev/tty0", O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) return fd; - loop_write(fd, - "\033[r" /* clear scrolling region */ - "\033[H" /* move home */ - "\033[2J", /* clear screen */ - 10, false); - return 0; + r = ioctl(fd, VT_DISALLOCATE, u); + if (r >= 0) + return 0; + if (errno != EBUSY) + return -errno; } - n = startswith(e, "tty"); - if (!n) - return -EINVAL; + /* So this is not a VT (in which case we cannot deallocate it), + * or we failed to deallocate. Let's at least clear the screen. */ - r = safe_atou(n, &u); - if (r < 0) - return r; + _cleanup_close_ int fd2 = open_terminal(name, O_RDWR|O_NOCTTY|O_CLOEXEC); + if (fd2 < 0) + return fd2; - if (u <= 0) - return -EINVAL; - - /* Try to deallocate */ - fd = open_terminal("/dev/tty0", O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); - if (fd < 0) - return fd; - - r = ioctl(fd, VT_DISALLOCATE, u); - fd = safe_close(fd); - - if (r >= 0) - return 0; - - if (errno != EBUSY) - return -errno; - - /* Couldn't deallocate, so let's clear it fully with - * scrollback */ - fd = open_terminal(name, O_RDWR|O_NOCTTY|O_CLOEXEC); - if (fd < 0) - return fd; - - loop_write(fd, - "\033[r" /* clear scrolling region */ - "\033[H" /* move home */ - "\033[3J", /* clear screen including scrollback, requires Linux 2.6.40 */ - 10, false); + (void) loop_write(fd2, + "\033[r" /* clear scrolling region */ + "\033[H" /* move home */ + "\033[3J", /* clear screen including scrollback, requires Linux 2.6.40 */ + 10, false); return 0; } From eba048bb6e58b4f0b367b87af588af257be18204 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 10:01:24 +0200 Subject: [PATCH 03/10] coredumpctl: use free_and_replace in one more place --- src/coredump/coredumpctl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 0529654788..2b17dc5859 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -305,9 +305,7 @@ static int retrieve(const void *data, if (!v) return log_oom(); - free(*var); - *var = v; - + free_and_replace(*var, v); return 1; } From 2d0a880fea8541289929bb8af146524ceebf6d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 10:02:13 +0200 Subject: [PATCH 04/10] coredumpctl: check return value retrieve() allocates memory, so it may fail. Coverity CID#1402338. --- src/coredump/coredumpctl.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 2b17dc5859..292c1861dc 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -738,9 +738,22 @@ static int save_core(sd_journal *j, FILE *file, char **path, bool *unlink_temp) /* Look for a coredump on disk first. */ r = sd_journal_get_data(j, "COREDUMP_FILENAME", (const void**) &data, &len); - if (r == 0) - retrieve(data, len, "COREDUMP_FILENAME", &filename); - else { + if (r == 0) { + r = retrieve(data, len, "COREDUMP_FILENAME", &filename); + if (r < 0) + return r; + assert(r > 0); + + if (access(filename, R_OK) < 0) + return log_error_errno(errno, "File \"%s\" is not readable: %m", filename); + + if (path && !endswith(filename, ".xz") && !endswith(filename, ".lz4")) { + *path = TAKE_PTR(filename); + + return 0; + } + + } else { if (r != -ENOENT) return log_error_errno(r, "Failed to retrieve COREDUMP_FILENAME field: %m"); /* Check that we can have a COREDUMP field. We still haven't set a high @@ -754,17 +767,6 @@ static int save_core(sd_journal *j, FILE *file, char **path, bool *unlink_temp) return log_error_errno(r, "Failed to retrieve COREDUMP field: %m"); } - if (filename) { - if (access(filename, R_OK) < 0) - return log_error_errno(errno, "File \"%s\" is not readable: %m", filename); - - if (path && !endswith(filename, ".xz") && !endswith(filename, ".lz4")) { - *path = TAKE_PTR(filename); - - return 0; - } - } - if (path) { const char *vt; From 622ecfa869047f9a62aa97895306f880d22f93bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 10:09:18 +0200 Subject: [PATCH 05/10] nspawn: fix memleak in argument parsing Coverity CID#1402297. --- src/nspawn/nspawn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 117a637699..848e100344 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1282,7 +1282,7 @@ static int parse_argv(int argc, char *argv[]) { case ARG_RLIMIT: { const char *eq; - char *name; + _cleanup_free_ char *name = NULL; int rl; if (streq(optarg, "help")) { From fa8b675ae0c341daa141cce8783c106e048045e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 10:14:34 +0200 Subject: [PATCH 06/10] nspawn: fix misplaced parenthesis and merge two error handling paths I don't think we need to provide the two separate error messages, let's shorten the code a bit by merging them. Coverity CID#1402320. --- src/nspawn/nspawn-oci.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/nspawn/nspawn-oci.c b/src/nspawn/nspawn-oci.c index ef67731339..4519c74b95 100644 --- a/src/nspawn/nspawn-oci.c +++ b/src/nspawn/nspawn-oci.c @@ -2092,13 +2092,9 @@ static int oci_hook_timeout(const char *name, JsonVariant *v, JsonDispatchFlags uintmax_t k; k = json_variant_unsigned(v); - if (k == 0) - return json_log(v, flags, SYNTHETIC_ERRNO(EINVAL), - "Hook timeout cannot be zero."); - - if (k > (UINT64_MAX-1/USEC_PER_SEC)) - return json_log(v, flags, SYNTHETIC_ERRNO(EINVAL), - "Hook timeout too large."); + if (k == 0 || k > (UINT64_MAX-1)/USEC_PER_SEC) + return json_log(v, flags, SYNTHETIC_ERRNO(ERANGE), + "Hook timeout value out of range."); *u = k * USEC_PER_SEC; return 0; From 8a07b4033e5d3c86931b3dd2ddbca41118c05c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 11:13:20 +0200 Subject: [PATCH 07/10] =?UTF-8?q?shared/conf-parser,networkd:=20EXTRACT=5F?= =?UTF-8?q?UNQUOTE|EXTRACT=5FRETAIN=5FESCAPE=20=E2=86=92=20EXTRACT=5FUNQUO?= =?UTF-8?q?TE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's hard to even say what exactly this combination means. Escaping is necessary when quoting to have quotes within the string. So the escaping of quote characters is inherently tied to quoting. When unquoting, it seems natural to remove escaping which was done for the quoting purposes. But with both flags we would be expected to re-add this escaping after unqouting? Or maybe keep the escaping which is not necessary for quoting but otherwise present? This all seems too complicated, let's just forbid such usage and always fully unescape when unquoting. --- src/basic/extract-word.c | 2 ++ src/libsystemd-network/network-internal.c | 2 +- src/shared/conf-parser.c | 2 +- src/test/test-extract-word.c | 24 +++++++++++++++++++++++ 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/basic/extract-word.c b/src/basic/extract-word.c index 34cfb36a4a..b7ae2ed1cd 100644 --- a/src/basic/extract-word.c +++ b/src/basic/extract-word.c @@ -28,6 +28,8 @@ int extract_first_word(const char **p, char **ret, const char *separators, Extra assert(p); assert(ret); + /* Those two don't make sense together. */ + assert(!FLAGS_SET(flags, EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE)); /* Bail early if called after last value or with no input */ if (!*p) diff --git a/src/libsystemd-network/network-internal.c b/src/libsystemd-network/network-internal.c index 1f2e5c7e65..f18ec88300 100644 --- a/src/libsystemd-network/network-internal.c +++ b/src/libsystemd-network/network-internal.c @@ -254,7 +254,7 @@ int config_parse_match_strv( for (;;) { _cleanup_free_ char *word = NULL, *k = NULL; - r = extract_first_word(&p, &word, NULL, EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE); + r = extract_first_word(&p, &word, NULL, EXTRACT_UNQUOTE); if (r == 0) return 0; if (r == -ENOMEM) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index c27c499eda..62fc1c97b7 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -758,7 +758,7 @@ int config_parse_strv( for (;;) { char *word = NULL; - r = extract_first_word(&rvalue, &word, NULL, EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE); + r = extract_first_word(&rvalue, &word, NULL, EXTRACT_UNQUOTE); if (r == 0) break; if (r == -ENOMEM) diff --git a/src/test/test-extract-word.c b/src/test/test-extract-word.c index f148b3e6f1..bf47a598a9 100644 --- a/src/test/test-extract-word.c +++ b/src/test/test-extract-word.c @@ -83,6 +83,30 @@ static void test_extract_first_word(void) { free(t); assert_se(isempty(p)); + p = original = "KEY=val \"KEY2=val with space\" \"KEY3=val with \\\"quotation\\\"\""; + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_UNQUOTE) == 1); + assert_se(streq(t, "KEY=val")); + free(t); + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_UNQUOTE) == 1); + assert_se(streq(t, "KEY2=val with space")); + free(t); + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_UNQUOTE) == 1); + assert_se(streq(t, "KEY3=val with \"quotation\"")); + free(t); + assert_se(isempty(p)); + + p = original = "KEY=val \"KEY2=val space\" \"KEY3=val with \\\"quotation\\\"\""; + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_RETAIN_ESCAPE) == 1); + assert_se(streq(t, "KEY=val")); + free(t); + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_RETAIN_ESCAPE) == 1); + assert_se(streq(t, "\"KEY2=val")); + free(t); + assert_se(extract_first_word(&p, &t, NULL, EXTRACT_RETAIN_ESCAPE) == 1); + assert_se(streq(t, "space\"")); + free(t); + assert_se(startswith(p, "\"KEY3=")); + p = original = "\'fooo"; assert_se(extract_first_word(&p, &t, NULL, EXTRACT_UNQUOTE) == -EINVAL); assert_se(p == original + 5); From 4337b0afaef6bc915f8a9ffebe462a2cb9b838bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 11:16:53 +0200 Subject: [PATCH 08/10] test-networkd-conf: add missing assert The test would not pass before, because EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE didn't work (we'd get "KEY3=val with \\quotation\\" as the last string. Now we are only doing EXTRACT_UNQUOTE, so we get the expected "KEY3=val with \"quotation\"". Coverity CID#1402781. --- src/network/test-networkd-conf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/network/test-networkd-conf.c b/src/network/test-networkd-conf.c index 07ca127654..6883cbe1ec 100644 --- a/src/network/test-networkd-conf.c +++ b/src/network/test-networkd-conf.c @@ -233,7 +233,14 @@ static void test_config_parse_match_strv(void) { assert_se(config_parse_match_strv("network", "filename", 1, "section", 1, "Name", 0, "KEY=val \"KEY2=val with space\" \"KEY3=val with \\\"quotation\\\"\"", &names, NULL) == 0); - strv_equal(names, STRV_MAKE("!hoge", "!hogehoge", "!foo", "!baz", "KEY=val", "KEY2=val with space", "KEY3=val with \"quotation\"")); + assert_se(strv_equal(names, + STRV_MAKE("!hoge", + "!hogehoge", + "!foo", + "!baz", + "KEY=val", + "KEY2=val with space", + "KEY3=val with \"quotation\""))); } int main(int argc, char **argv) { From 3d4d5abf2a5af19dcfe42a432794d06f27a99c07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 11:20:36 +0200 Subject: [PATCH 09/10] sd-bus: voidify two calls to hashmap_iterate() Coverity CID#1402304 and CID#1402307. --- src/libsystemd/sd-bus/bus-track.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-track.c b/src/libsystemd/sd-bus/bus-track.c index efbd3ed5f0..f609854925 100644 --- a/src/libsystemd/sd-bus/bus-track.c +++ b/src/libsystemd/sd-bus/bus-track.c @@ -305,7 +305,7 @@ _public_ const char* sd_bus_track_first(sd_bus_track *track) { track->modified = false; track->iterator = ITERATOR_FIRST; - hashmap_iterate(track->names, &track->iterator, NULL, (const void**) &n); + (void) hashmap_iterate(track->names, &track->iterator, NULL, (const void**) &n); return n; } @@ -318,7 +318,7 @@ _public_ const char* sd_bus_track_next(sd_bus_track *track) { if (track->modified) return NULL; - hashmap_iterate(track->names, &track->iterator, NULL, (const void**) &n); + (void) hashmap_iterate(track->names, &track->iterator, NULL, (const void**) &n); return n; } From 7b9103a622683575192f35d1865794c8680f99c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 17 Jul 2019 11:30:04 +0200 Subject: [PATCH 10/10] sd-device: voidify and simplify calls to ordered_hashmap_iterate() Coverity CID#1402356 and CID#1402335. --- src/libsystemd/sd-device/sd-device.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 617d6de667..c4a7f2f3d3 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1540,7 +1540,6 @@ int device_properties_prepare(sd_device *device) { _public_ const char *sd_device_get_property_first(sd_device *device, const char **_value) { const char *key; - const char *value; int r; assert_return(device, NULL); @@ -1552,16 +1551,12 @@ _public_ const char *sd_device_get_property_first(sd_device *device, const char device->properties_iterator_generation = device->properties_generation; device->properties_iterator = ITERATOR_FIRST; - ordered_hashmap_iterate(device->properties, &device->properties_iterator, (void**)&value, (const void**)&key); - - if (_value) - *_value = value; + (void) ordered_hashmap_iterate(device->properties, &device->properties_iterator, (void**)_value, (const void**)&key); return key; } _public_ const char *sd_device_get_property_next(sd_device *device, const char **_value) { const char *key; - const char *value; int r; assert_return(device, NULL); @@ -1573,10 +1568,7 @@ _public_ const char *sd_device_get_property_next(sd_device *device, const char * if (device->properties_iterator_generation != device->properties_generation) return NULL; - ordered_hashmap_iterate(device->properties, &device->properties_iterator, (void**)&value, (const void**)&key); - - if (_value) - *_value = value; + (void) ordered_hashmap_iterate(device->properties, &device->properties_iterator, (void**)_value, (const void**)&key); return key; }