From 7f5b2615a7ac9ebe0d95e66818d3f9ba7d2a3277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 7 May 2022 22:36:23 +0200 Subject: [PATCH 1/8] dissect: drop unnecessary {} --- src/dissect/dissect.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/dissect/dissect.c b/src/dissect/dissect.c index 51e3c4b837..bd94a755db 100644 --- a/src/dissect/dissect.c +++ b/src/dissect/dissect.c @@ -372,12 +372,11 @@ static int strv_pair_to_json(char **l, JsonVariant **ret) { static void strv_pair_print(char **l, const char *prefix) { assert(prefix); - STRV_FOREACH_PAIR(p, q, l) { + STRV_FOREACH_PAIR(p, q, l) if (p == l) printf("%s %s=%s\n", prefix, *p, *q); else printf("%*s %s=%s\n", (int) strlen(prefix), "", *p, *q); - } } static int get_sysext_scopes(DissectedImage *m, char ***ret_scopes) { From 44468876c37e01efe2d9fe1b170d19d8b6958373 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 09:41:32 +0200 Subject: [PATCH 2/8] fuzz-systemctl-parse-argv: refuse commandlines above 1k entries oss-fuzz reports timeouts which are created by appending to a very long strv. The code is indeed not very efficient, but it's designed for normal command-line use, where we don't expect more than a dozen of entries. The fact that it is slow with ~100k entries is not particularly interesting. In the future we could rework the code to have better algorithmic complexity. But let's at least stop oss-fuzz from wasting more time on such examples. (My first approach was to set max_len in .options, but apparently this doesn't work for hongfuzz and and AFL.) oss-fuzz-34527: https://oss-fuzz.com/issue/5722283944574976 --- src/systemctl/fuzz-systemctl-parse-argv.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/systemctl/fuzz-systemctl-parse-argv.c b/src/systemctl/fuzz-systemctl-parse-argv.c index eed8c67144..588c8b56c5 100644 --- a/src/systemctl/fuzz-systemctl-parse-argv.c +++ b/src/systemctl/fuzz-systemctl-parse-argv.c @@ -31,6 +31,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (!argv[0]) return 0; /* argv[0] should always be present, but may be zero-length. */ + if (strv_length(argv) > 1024) + return 0; /* oss-fuzz reports timeouts which are caused by appending to a very long strv. + * The code is indeed not very efficient, but it's designed for normal command-line + * use, where we don't expect more than a dozen of entries. The fact that it is + * slow with ~100k entries is not particularly interesting. Let's just refuse such + * long command lines. */ if (getenv_bool("SYSTEMD_FUZZ_OUTPUT") <= 0) { orig_stdout_fd = fcntl(fileno(stdout), F_DUPFD_CLOEXEC, 3); From 40893cbfd5db8b2213c1f4e8047bef224d2258cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 11:18:14 +0200 Subject: [PATCH 3/8] fuzz-journal-remote: allow -ENODATA too It seems that we try to create a new file, which fails with -ENOSPC, and we later fail when reading a file with ENODATA. journal_file_open() will return -ENODATA if the file is too short or if journal_file_verify_header() fails. We'll unlink a file we newly created if we fail to initialize it immediately after creation. I'm not sure if the file we fail to open is the one we newly created and e.g. failed to create the arena and such, or if it's the file we were trying to rotate away from. Either way, I think we should be OK with with a non-fully-initialized journal file. Failed to create rotated journal: No space left on device Failed to write entry of 2 bytes: No space left on device sd_journal_open_files(["/tmp/fuzz-journal-remote.vELRpI.journal"]) failed: No data available Assertion 'IN_SET(r, -ENOMEM, -EMFILE, -ENFILE)' failed at src/journal-remote/fuzz-journal-remote.c:70, function int LLVMFuzzerTestOneInput(const uint8_t *, size_t)(). Aborting. oss-fuzz-39238: https://oss-fuzz.com/issue/4609851129462784 --- src/journal-remote/fuzz-journal-remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal-remote/fuzz-journal-remote.c b/src/journal-remote/fuzz-journal-remote.c index 9206b99406..a8e56e21b2 100644 --- a/src/journal-remote/fuzz-journal-remote.c +++ b/src/journal-remote/fuzz-journal-remote.c @@ -66,7 +66,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { r = sd_journal_open_files(&j, (const char**) STRV_MAKE(name), 0); if (r < 0) { log_error_errno(r, "sd_journal_open_files([\"%s\"]) failed: %m", name); - assert_se(IN_SET(r, -ENOMEM, -EMFILE, -ENFILE)); + assert_se(IN_SET(r, -ENOMEM, -EMFILE, -ENFILE, -ENODATA)); return r; } From 2034c8b8838c9f42d1dc16fbda23d9bc3a2c6f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 12:30:51 +0200 Subject: [PATCH 4/8] tree-wide: drop de-constifying casts for strv iteration When the the iterator variable is declared automatically, it "inherits" the const/non-const status from the argument. We don't need to cast a const table to non-const. If we had a programming error and tried to modify the string, the compiler could now catch this. --- src/core/manager.c | 2 +- src/shared/bootspec.c | 2 +- src/shared/conf-parser.c | 2 +- src/test/test-fs-util.c | 4 ++-- src/test/test-sysctl-util.c | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index d09d14911c..98daa764eb 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3655,7 +3655,7 @@ static bool generator_path_any(const char* const* paths) { /* Optimize by skipping the whole process by not creating output directories * if no generators are found. */ - STRV_FOREACH(path, (char**) paths) + STRV_FOREACH(path, paths) if (access(*path, F_OK) == 0) found = true; else if (errno != ENOENT) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 91cb605fb1..f39d4da6d5 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -960,7 +960,7 @@ int boot_config_augment_from_loader( if (!c) return log_oom(); - STRV_FOREACH_PAIR(a, b, (char**) title_table) + STRV_FOREACH_PAIR(a, b, title_table) if (streq(*a, *i)) { t = strdup(*b); if (!t) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index a84c47cd26..6bd3ab38db 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -497,7 +497,7 @@ static int config_parse_many_files( } /* First read the first found main config file. */ - STRV_FOREACH(fn, (char**) conf_files) { + STRV_FOREACH(fn, conf_files) { r = config_parse(NULL, *fn, NULL, sections, lookup, table, flags, userdata, &st); if (r < 0) return r; diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index f24e97b4cc..5d9d99e272 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -712,7 +712,7 @@ TEST(rename_noreplace) { j = strjoina(z, table[4]); (void) symlink("foobar", j); - STRV_FOREACH(a, (char**) table) { + STRV_FOREACH(a, table) { _cleanup_free_ char *x = NULL, *y = NULL; x = strjoin(z, *a); @@ -723,7 +723,7 @@ TEST(rename_noreplace) { continue; } - STRV_FOREACH(b, (char**) table) { + STRV_FOREACH(b, table) { _cleanup_free_ char *w = NULL; w = strjoin(z, *b); diff --git a/src/test/test-sysctl-util.c b/src/test/test-sysctl-util.c index 02180dc4be..81207f5cfd 100644 --- a/src/test/test-sysctl-util.c +++ b/src/test/test-sysctl-util.c @@ -27,7 +27,7 @@ static const char* const cases[] = { }; TEST(sysctl_normalize) { - STRV_FOREACH_PAIR(s, expected, (const char**) cases) { + STRV_FOREACH_PAIR(s, expected, cases) { _cleanup_free_ char *t; assert_se(t = strdup(*s)); From c9c3b81f12100b84b493575f3f93235108185cec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 12:39:15 +0200 Subject: [PATCH 5/8] libsystemd-network: constify casts It is better to not drop the const-ness from the argument. --- src/libsystemd-network/dhcp-option.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd-network/dhcp-option.c b/src/libsystemd-network/dhcp-option.c index efb676e60b..7f49da7c2d 100644 --- a/src/libsystemd-network/dhcp-option.c +++ b/src/libsystemd-network/dhcp-option.c @@ -62,7 +62,7 @@ static int option_append(uint8_t options[], size_t size, size_t *offset, if (strv_isempty((char **) optval)) return -EINVAL; - STRV_FOREACH(s, (char **) optval) { + STRV_FOREACH(s, (const char* const*) optval) { size_t len = strlen(*s); if (len > 255 || len == 0) @@ -78,7 +78,7 @@ static int option_append(uint8_t options[], size_t size, size_t *offset, options[*offset + 1] = total; *offset += 2; - STRV_FOREACH(s, (char **) optval) { + STRV_FOREACH(s, (const char* const*) optval) { size_t len = strlen(*s); options[*offset] = len; From a9d178d28b437121041b95039e40cdf6804f7482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 12:49:19 +0200 Subject: [PATCH 6/8] basic/virt: use STRV_FOREACH instead of iteration with ELEMENTSOF I think it's a bit simpler, we don't have two indexes. --- src/basic/virt.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/basic/virt.c b/src/basic/virt.c index b3c1ebf3d8..c7ae2afc00 100644 --- a/src/basic/virt.c +++ b/src/basic/virt.c @@ -143,12 +143,13 @@ static Virtualization detect_vm_device_tree(void) { #if defined(__i386__) || defined(__x86_64__) || defined(__arm__) || defined(__aarch64__) || defined(__loongarch64) static Virtualization detect_vm_dmi_vendor(void) { - static const char *const dmi_vendors[] = { + static const char* const dmi_vendors[] = { "/sys/class/dmi/id/product_name", /* Test this before sys_vendor to detect KVM over QEMU */ "/sys/class/dmi/id/sys_vendor", "/sys/class/dmi/id/board_vendor", "/sys/class/dmi/id/bios_vendor", - "/sys/class/dmi/id/product_version" /* For Hyper-V VMs test */ + "/sys/class/dmi/id/product_version", /* For Hyper-V VMs test */ + NULL }; static const struct { @@ -172,10 +173,10 @@ static Virtualization detect_vm_dmi_vendor(void) { }; int r; - for (size_t i = 0; i < ELEMENTSOF(dmi_vendors); i++) { + STRV_FOREACH(vendor, dmi_vendors) { _cleanup_free_ char *s = NULL; - r = read_one_line_file(dmi_vendors[i], &s); + r = read_one_line_file(*vendor, &s); if (r < 0) { if (r == -ENOENT) continue; @@ -183,10 +184,10 @@ static Virtualization detect_vm_dmi_vendor(void) { return r; } - for (size_t j = 0; j < ELEMENTSOF(dmi_vendor_table); j++) - if (startswith(s, dmi_vendor_table[j].vendor)) { - log_debug("Virtualization %s found in DMI (%s)", s, dmi_vendors[i]); - return dmi_vendor_table[j].id; + for (size_t i = 0; i < ELEMENTSOF(dmi_vendor_table); i++) + if (startswith(s, dmi_vendor_table[i].vendor)) { + log_debug("Virtualization %s found in DMI (%s)", s, *vendor); + return dmi_vendor_table[i].id; } } log_debug("No virtualization found in DMI vendor table."); From 68ac5118e72244a391887b68f5aeb86d9c8d767b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 16:20:04 +0200 Subject: [PATCH 7/8] shared/dissect-image: apply standard indentation --- src/shared/dissect-image.c | 52 ++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 057fe14125..5b0b12a265 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -1317,7 +1317,9 @@ static int mount_partition( if (!fstype) return -EAFNOSUPPORT; - /* We are looking at an encrypted partition? This either means stacked encryption, or the caller didn't call dissected_image_decrypt() beforehand. Let's return a recognizable error for this case. */ + /* We are looking at an encrypted partition? This either means stacked encryption, or the caller + * didn't call dissected_image_decrypt() beforehand. Let's return a recognizable error for this + * case. */ if (streq(fstype, "crypto_LUKS")) return -EUNATCH; @@ -3031,31 +3033,31 @@ int mount_image_privately_interactively( } static const char *const partition_designator_table[] = { - [PARTITION_ROOT] = "root", - [PARTITION_ROOT_SECONDARY] = "root-secondary", - [PARTITION_ROOT_OTHER] = "root-other", - [PARTITION_USR] = "usr", - [PARTITION_USR_SECONDARY] = "usr-secondary", - [PARTITION_USR_OTHER] = "usr-other", - [PARTITION_HOME] = "home", - [PARTITION_SRV] = "srv", - [PARTITION_ESP] = "esp", - [PARTITION_XBOOTLDR] = "xbootldr", - [PARTITION_SWAP] = "swap", - [PARTITION_ROOT_VERITY] = "root-verity", - [PARTITION_ROOT_SECONDARY_VERITY] = "root-secondary-verity", - [PARTITION_ROOT_OTHER_VERITY] = "root-other-verity", - [PARTITION_USR_VERITY] = "usr-verity", - [PARTITION_USR_SECONDARY_VERITY] = "usr-secondary-verity", - [PARTITION_USR_OTHER_VERITY] = "usr-other-verity", - [PARTITION_ROOT_VERITY_SIG] = "root-verity-sig", + [PARTITION_ROOT] = "root", + [PARTITION_ROOT_SECONDARY] = "root-secondary", + [PARTITION_ROOT_OTHER] = "root-other", + [PARTITION_USR] = "usr", + [PARTITION_USR_SECONDARY] = "usr-secondary", + [PARTITION_USR_OTHER] = "usr-other", + [PARTITION_HOME] = "home", + [PARTITION_SRV] = "srv", + [PARTITION_ESP] = "esp", + [PARTITION_XBOOTLDR] = "xbootldr", + [PARTITION_SWAP] = "swap", + [PARTITION_ROOT_VERITY] = "root-verity", + [PARTITION_ROOT_SECONDARY_VERITY] = "root-secondary-verity", + [PARTITION_ROOT_OTHER_VERITY] = "root-other-verity", + [PARTITION_USR_VERITY] = "usr-verity", + [PARTITION_USR_SECONDARY_VERITY] = "usr-secondary-verity", + [PARTITION_USR_OTHER_VERITY] = "usr-other-verity", + [PARTITION_ROOT_VERITY_SIG] = "root-verity-sig", [PARTITION_ROOT_SECONDARY_VERITY_SIG] = "root-secondary-verity-sig", - [PARTITION_ROOT_OTHER_VERITY_SIG] = "root-other-verity-sig", - [PARTITION_USR_VERITY_SIG] = "usr-verity-sig", - [PARTITION_USR_SECONDARY_VERITY_SIG] = "usr-secondary-verity-sig", - [PARTITION_USR_OTHER_VERITY_SIG] = "usr-other-verity-sig", - [PARTITION_TMP] = "tmp", - [PARTITION_VAR] = "var", + [PARTITION_ROOT_OTHER_VERITY_SIG] = "root-other-verity-sig", + [PARTITION_USR_VERITY_SIG] = "usr-verity-sig", + [PARTITION_USR_SECONDARY_VERITY_SIG] = "usr-secondary-verity-sig", + [PARTITION_USR_OTHER_VERITY_SIG] = "usr-other-verity-sig", + [PARTITION_TMP] = "tmp", + [PARTITION_VAR] = "var", }; int verity_dissect_and_mount( From 3947432eb0c93e5c327310e0b288fa10ddda997a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 8 May 2022 16:21:10 +0200 Subject: [PATCH 8/8] shared/calendarspec: reduce scope of variables --- src/shared/calendarspec.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c index 79fd1359b6..7dfb50b98d 100644 --- a/src/shared/calendarspec.c +++ b/src/shared/calendarspec.c @@ -76,26 +76,21 @@ static int component_compare(CalendarComponent * const *a, CalendarComponent * c } static void normalize_chain(CalendarComponent **c) { - CalendarComponent **b, *i, **j, *next; - size_t n = 0, k; - assert(c); - for (i = *c; i; i = i->next) { + size_t n = 0; + for (CalendarComponent *i = *c; i; i = i->next) { n++; - /* - * While we're counting the chain, also normalize `stop` - * so the length of the range is a multiple of `repeat` - */ + /* While we're counting the chain, also normalize 'stop' + * so the length of the range is a multiple of 'repeat'. */ if (i->stop > i->start && i->repeat > 0) i->stop -= (i->stop - i->start) % i->repeat; - /* If a repeat value is specified, but it cannot even be triggered once, let's suppress - * it. + /* If a repeat value is specified, but it cannot even be triggered once, let's suppress it. * - * Similar, if the stop value is the same as the start value, then let's just make this a - * non-repeating chain element */ + * Similarly, if the stop value is the same as the start value, then let's just make this a + * non-repeating chain element. */ if ((i->stop > i->start && i->repeat > 0 && i->start + i->repeat > i->stop) || i->start == i->stop) { i->repeat = 0; @@ -106,17 +101,18 @@ static void normalize_chain(CalendarComponent **c) { if (n <= 1) return; - j = b = newa(CalendarComponent*, n); - for (i = *c; i; i = i->next) + CalendarComponent **b, **j; + b = j = newa(CalendarComponent*, n); + for (CalendarComponent *i = *c; i; i = i->next) *(j++) = i; typesafe_qsort(b, n, component_compare); b[n-1]->next = NULL; - next = b[n-1]; + CalendarComponent *next = b[n-1]; /* Drop non-unique entries */ - for (k = n-1; k > 0; k--) { + for (size_t k = n-1; k > 0; k--) { if (component_compare(&b[k-1], &next) == 0) { free(b[k-1]); continue; @@ -253,7 +249,7 @@ static void format_weekdays(FILE *f, const CalendarSpec *c) { "Thu", "Fri", "Sat", - "Sun" + "Sun", }; int l, x; @@ -408,7 +404,7 @@ static int parse_weekdays(const char **p, CalendarSpec *c) { { "Saturday", 5 }, { "Sat", 5 }, { "Sunday", 6 }, - { "Sun", 6 } + { "Sun", 6 }, }; int l = -1;