From 6c279977e3e065e8d3c14747d19dc2bf80150490 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 10:50:42 +0200 Subject: [PATCH 1/8] basic/unit-file: make sure we don't call streq() on NULL add_names is called in two places, with fragment_basename=NULL in one of them. gcc warns that it can be NULL. Fixes #23646. --- src/basic/unit-file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/basic/unit-file.c b/src/basic/unit-file.c index 83c29bb25f..ac0236d635 100644 --- a/src/basic/unit-file.c +++ b/src/basic/unit-file.c @@ -689,6 +689,7 @@ static int add_names( return log_debug_errno(r, "Cannot find instance fragment %s: %m", inst); if (inst_fragment && + fragment_basename && !streq(basename(inst_fragment), fragment_basename)) { log_debug("Instance %s has fragment %s and is not an alias of %s.", inst, inst_fragment, unit_name); From 9908091b960e1dbafa50ad88113bf24aa0b6ef4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 10:52:19 +0200 Subject: [PATCH 2/8] sd-hwdb: fix NULL arg to %s in error messages gcc warns about $subject, and it is obviously correct. path may or may not be set. --- src/libsystemd/sd-hwdb/sd-hwdb.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/libsystemd/sd-hwdb/sd-hwdb.c b/src/libsystemd/sd-hwdb/sd-hwdb.c index f73d1fc21e..51be54b1e1 100644 --- a/src/libsystemd/sd-hwdb/sd-hwdb.c +++ b/src/libsystemd/sd-hwdb/sd-hwdb.c @@ -283,7 +283,6 @@ static int trie_search_f(sd_hwdb *hwdb, const char *search) { static int hwdb_new(const char *path, sd_hwdb **ret) { _cleanup_(sd_hwdb_unrefp) sd_hwdb *hwdb = NULL; - const char *hwdb_bin_path = NULL; const char sig[] = HWDB_SIG; assert_return(ret, -EINVAL); @@ -294,20 +293,20 @@ static int hwdb_new(const char *path, sd_hwdb **ret) { hwdb->n_ref = 1; - /* find hwdb.bin in hwdb_bin_paths, or from an explicit path if provided */ + /* Find hwdb.bin in the explicit path if provided, or iterate over hwdb_bin_paths otherwise */ if (!isempty(path)) { log_debug("Trying to open \"%s\"...", path); hwdb->f = fopen(path, "re"); if (!hwdb->f) return log_debug_errno(errno, "Failed to open %s: %m", path); } else { - NULSTR_FOREACH(hwdb_bin_path, hwdb_bin_paths) { - log_debug("Trying to open \"%s\"...", hwdb_bin_path); - hwdb->f = fopen(hwdb_bin_path, "re"); + NULSTR_FOREACH(path, hwdb_bin_paths) { + log_debug("Trying to open \"%s\"...", path); + hwdb->f = fopen(path, "re"); if (hwdb->f) break; if (errno != ENOENT) - return log_debug_errno(errno, "Failed to open %s: %m", hwdb_bin_path); + return log_debug_errno(errno, "Failed to open %s: %m", path); } if (!hwdb->f) @@ -316,23 +315,20 @@ static int hwdb_new(const char *path, sd_hwdb **ret) { } if (fstat(fileno(hwdb->f), &hwdb->st) < 0) - return log_debug_errno(errno, "Failed to stat %s: %m", hwdb_bin_path); + return log_debug_errno(errno, "Failed to stat %s: %m", path); if (hwdb->st.st_size < (off_t) offsetof(struct trie_header_f, strings_len) + 8) - return log_debug_errno(SYNTHETIC_ERRNO(EIO), - "File %s is too short: %m", hwdb_bin_path); + return log_debug_errno(SYNTHETIC_ERRNO(EIO), "File %s is too short: %m", path); if (file_offset_beyond_memory_size(hwdb->st.st_size)) - return log_debug_errno(SYNTHETIC_ERRNO(EFBIG), - "File %s is too long: %m", hwdb_bin_path); + return log_debug_errno(SYNTHETIC_ERRNO(EFBIG), "File %s is too long: %m", path); hwdb->map = mmap(0, hwdb->st.st_size, PROT_READ, MAP_SHARED, fileno(hwdb->f), 0); if (hwdb->map == MAP_FAILED) - return log_debug_errno(errno, "Failed to map %s: %m", hwdb_bin_path); + return log_debug_errno(errno, "Failed to map %s: %m", path); if (memcmp(hwdb->map, sig, sizeof(hwdb->head->signature)) != 0 || (size_t) hwdb->st.st_size != le64toh(hwdb->head->file_size)) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "Failed to recognize the format of %s", - hwdb_bin_path); + "Failed to recognize the format of %s", path); log_debug("=== trie on-disk ==="); log_debug("tool version: %"PRIu64, le64toh(hwdb->head->tool_version)); From 85b0ff8abf29cd90d02ef22bac036b370e4d5b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 11:00:35 +0200 Subject: [PATCH 3/8] sd-journal: fix NULL arg to %s in error messages and hashmap lookup The lookup "works", but is not useful. It was introduced in 9c66f528138f4fc856b3e9e137245b7048d5747d. And printf will NULL args is invalid was introduced in 5d1ce25728856956c1fbfe05b491067f83bd2216 when support for fds was initally added :( --- src/libsystemd/sd-journal/sd-journal.c | 48 +++++++++++++++----------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/libsystemd/sd-journal/sd-journal.c b/src/libsystemd/sd-journal/sd-journal.c index 705a35de3f..5a94a51377 100644 --- a/src/libsystemd/sd-journal/sd-journal.c +++ b/src/libsystemd/sd-journal/sd-journal.c @@ -1277,6 +1277,7 @@ static int add_any_file( assert(fd >= 0 || path); if (fd < 0) { + assert(path); /* For gcc. */ if (j->toplevel_fd >= 0) /* If there's a top-level fd defined make the path relative, explicitly, since otherwise * openat() ignores the first argument. */ @@ -1299,53 +1300,58 @@ static int add_any_file( } if (fstat(fd, &st) < 0) { - r = log_debug_errno(errno, "Failed to fstat file '%s': %m", path); + r = log_debug_errno(errno, "Failed to fstat %s: %m", path ?: "fd"); goto finish; } r = stat_verify_regular(&st); if (r < 0) { - log_debug_errno(r, "Refusing to open '%s', as it is not a regular file.", path); + log_debug_errno(r, "Refusing to open %s: %m", path ?: "fd"); goto finish; } - f = ordered_hashmap_get(j->files, path); - if (f) { - if (stat_inode_same(&f->last_stat, &st)) { + if (path) { + f = ordered_hashmap_get(j->files, path); + if (f) { + if (stat_inode_same(&f->last_stat, &st)) { + /* We already track this file, under the same path and with the same + * device/inode numbers, it's hence really the same. Mark this file as seen + * in this generation. This is used to GC old files in process_q_overflow() + * to detect journal files that are still there and discern them from those + * which are gone. */ - /* We already track this file, under the same path and with the same device/inode numbers, it's - * hence really the same. Mark this file as seen in this generation. This is used to GC old - * files in process_q_overflow() to detect journal files that are still there and discern them - * from those which are gone. */ + f->last_seen_generation = j->generation; + r = 0; + goto finish; + } - f->last_seen_generation = j->generation; - r = 0; - goto finish; + /* So we tracked a file under this name, but it has a different inode/device. In that + * case, it got replaced (probably due to rotation?), let's drop it hence from our + * list. */ + remove_file_real(j, f); + f = NULL; } - - /* So we tracked a file under this name, but it has a different inode/device. In that case, it got - * replaced (probably due to rotation?), let's drop it hence from our list. */ - remove_file_real(j, f); - f = NULL; } if (ordered_hashmap_size(j->files) >= JOURNAL_FILES_MAX) { - log_debug("Too many open journal files, not adding %s.", path); + log_debug("Too many open journal files, not adding %s.", path ?: "fd"); r = -ETOOMANYREFS; goto finish; } r = journal_file_open(fd, path, O_RDONLY, 0, 0, 0, NULL, j->mmap, NULL, &f); if (r < 0) { - log_debug_errno(r, "Failed to open journal file %s: %m", path); + log_debug_errno(r, "Failed to open journal file %s: %m", path ?: "from fd"); goto finish; } /* journal_file_dump(f); */ + /* journal_file_open() generates an replacement fname if necessary, so we can use f->path. */ r = ordered_hashmap_put(j->files, f->path, f); if (r < 0) { - f->close_fd = false; /* make sure journal_file_close() doesn't close the caller's fd (or our own). We'll let the caller do that, or ourselves */ + f->close_fd = false; /* Make sure journal_file_close() doesn't close the caller's fd + * (or our own). The caller or we will do that ourselves. */ (void) journal_file_close(f); goto finish; } @@ -1368,7 +1374,7 @@ finish: safe_close(fd); if (r < 0) { - k = journal_put_error(j, r, path); + k = journal_put_error(j, r, path); /* path==NULL is OK. */ if (k < 0) return k; } From 70f50a4731dc6372f9925705faa278f3e9cb5be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 11:12:48 +0200 Subject: [PATCH 4/8] sd-journal: use _cleanup_ --- src/libsystemd/sd-journal/sd-journal.c | 47 ++++++++++---------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/src/libsystemd/sd-journal/sd-journal.c b/src/libsystemd/sd-journal/sd-journal.c index 5a94a51377..96d0430a00 100644 --- a/src/libsystemd/sd-journal/sd-journal.c +++ b/src/libsystemd/sd-journal/sd-journal.c @@ -1268,10 +1268,10 @@ static int add_any_file( int fd, const char *path) { - bool close_fd = false; + _cleanup_close_ int our_fd = -1; JournalFile *f; struct stat st; - int r, k; + int r; assert(j); assert(fd >= 0 || path); @@ -1282,32 +1282,30 @@ static int add_any_file( /* If there's a top-level fd defined make the path relative, explicitly, since otherwise * openat() ignores the first argument. */ - fd = openat(j->toplevel_fd, skip_slash(path), O_RDONLY|O_CLOEXEC|O_NONBLOCK); + fd = our_fd = openat(j->toplevel_fd, skip_slash(path), O_RDONLY|O_CLOEXEC|O_NONBLOCK); else - fd = open(path, O_RDONLY|O_CLOEXEC|O_NONBLOCK); + fd = our_fd = open(path, O_RDONLY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) { r = log_debug_errno(errno, "Failed to open journal file %s: %m", path); - goto finish; + goto error; } - close_fd = true; - r = fd_nonblock(fd, false); if (r < 0) { r = log_debug_errno(errno, "Failed to turn off O_NONBLOCK for %s: %m", path); - goto finish; + goto error; } } if (fstat(fd, &st) < 0) { r = log_debug_errno(errno, "Failed to fstat %s: %m", path ?: "fd"); - goto finish; + goto error; } r = stat_verify_regular(&st); if (r < 0) { log_debug_errno(r, "Refusing to open %s: %m", path ?: "fd"); - goto finish; + goto error; } if (path) { @@ -1321,8 +1319,7 @@ static int add_any_file( * which are gone. */ f->last_seen_generation = j->generation; - r = 0; - goto finish; + return 0; } /* So we tracked a file under this name, but it has a different inode/device. In that @@ -1334,15 +1331,15 @@ static int add_any_file( } if (ordered_hashmap_size(j->files) >= JOURNAL_FILES_MAX) { - log_debug("Too many open journal files, not adding %s.", path ?: "fd"); - r = -ETOOMANYREFS; - goto finish; + r = log_debug_errno(SYNTHETIC_ERRNO(ETOOMANYREFS), + "Too many open journal files, not adding %s.", path ?: "fd"); + goto error; } r = journal_file_open(fd, path, O_RDONLY, 0, 0, 0, NULL, j->mmap, NULL, &f); if (r < 0) { log_debug_errno(r, "Failed to open journal file %s: %m", path ?: "from fd"); - goto finish; + goto error; } /* journal_file_dump(f); */ @@ -1353,10 +1350,10 @@ static int add_any_file( f->close_fd = false; /* Make sure journal_file_close() doesn't close the caller's fd * (or our own). The caller or we will do that ourselves. */ (void) journal_file_close(f); - goto finish; + goto error; } - close_fd = false; /* the fd is now owned by the JournalFile object */ + TAKE_FD(our_fd); /* the fd is now owned by the JournalFile object */ f->last_seen_generation = j->generation; @@ -1367,18 +1364,10 @@ static int add_any_file( log_debug("File %s added.", f->path); - r = 0; - -finish: - if (close_fd) - safe_close(fd); - - if (r < 0) { - k = journal_put_error(j, r, path); /* path==NULL is OK. */ - if (k < 0) - return k; - } + return 0; +error: + (void) journal_put_error(j, r, path); /* path==NULL is OK. */ return r; } From abcdc02cd0a138cdf9cc5fea8b44d9d491bd6cb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 11:19:47 +0200 Subject: [PATCH 5/8] sd-journal: inline variable declarations --- src/libsystemd/sd-journal/sd-journal.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-journal/sd-journal.c b/src/libsystemd/sd-journal/sd-journal.c index 96d0430a00..5138993f05 100644 --- a/src/libsystemd/sd-journal/sd-journal.c +++ b/src/libsystemd/sd-journal/sd-journal.c @@ -147,8 +147,6 @@ static void set_location(sd_journal *j, JournalFile *f, Object *o) { } static int match_is_valid(const void *data, size_t size) { - const char *b, *p; - assert(data); if (size < 2) @@ -157,8 +155,8 @@ static int match_is_valid(const void *data, size_t size) { if (((char*) data)[0] == '_' && ((char*) data)[1] == '_') return false; - b = data; - for (p = b; p < b + size; p++) { + const char *b = data; + for (const char *p = b; p < b + size; p++) { if (*p == '=') return p > b; @@ -180,9 +178,8 @@ static int match_is_valid(const void *data, size_t size) { static bool same_field(const void *_a, size_t s, const void *_b, size_t t) { const uint8_t *a = _a, *b = _b; - size_t j; - for (j = 0; j < s && j < t; j++) { + for (size_t j = 0; j < s && j < t; j++) { if (a[j] != b[j]) return false; @@ -818,7 +815,7 @@ static int next_beyond_location(sd_journal *j, JournalFile *f, direction_t direc static int real_journal_next(sd_journal *j, direction_t direction) { JournalFile *new_file = NULL; - unsigned i, n_files; + unsigned n_files; const void **files; Object *o; int r; @@ -830,7 +827,7 @@ static int real_journal_next(sd_journal *j, direction_t direction) { if (r < 0) return r; - for (i = 0; i < n_files; i++) { + for (unsigned i = 0; i < n_files; i++) { JournalFile *f = (JournalFile *)files[i]; bool found; @@ -2084,7 +2081,6 @@ _public_ int sd_journal_open_directory_fd(sd_journal **ret, int fd, int flags) { _public_ int sd_journal_open_files_fd(sd_journal **ret, int fds[], unsigned n_fds, int flags) { JournalFile *f; _cleanup_(sd_journal_closep) sd_journal *j = NULL; - unsigned i; int r; assert_return(ret, -EINVAL); @@ -2095,7 +2091,7 @@ _public_ int sd_journal_open_files_fd(sd_journal **ret, int fds[], unsigned n_fd if (!j) return -ENOMEM; - for (i = 0; i < n_fds; i++) { + for (unsigned i = 0; i < n_fds; i++) { struct stat st; if (fds[i] < 0) { @@ -2232,8 +2228,6 @@ _public_ int sd_journal_get_monotonic_usec(sd_journal *j, uint64_t *ret, sd_id12 } static bool field_is_valid(const char *field) { - const char *p; - assert(field); if (isempty(field)) @@ -2242,7 +2236,7 @@ static bool field_is_valid(const char *field) { if (startswith(field, "__")) return false; - for (p = field; *p; p++) { + for (const char *p = field; *p; p++) { if (*p == '_') continue; @@ -2261,7 +2255,6 @@ static bool field_is_valid(const char *field) { _public_ int sd_journal_get_data(sd_journal *j, const char *field, const void **data, size_t *size) { JournalFile *f; - uint64_t i, n; size_t field_length; int r; Object *o; @@ -2286,8 +2279,8 @@ _public_ int sd_journal_get_data(sd_journal *j, const char *field, const void ** field_length = strlen(field); - n = journal_file_entry_n_items(o); - for (i = 0; i < n; i++) { + uint64_t n = journal_file_entry_n_items(o); + for (uint64_t i = 0; i < n; i++) { Object *d; uint64_t p, l; size_t t; From 1b3502b072a9930af2345879d3ad42854ba3a287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 11:20:09 +0200 Subject: [PATCH 6/8] test-mountpoint-util: fix NULL arg to %s --- src/test/test-mountpoint-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-mountpoint-util.c b/src/test/test-mountpoint-util.c index 6f9e68f4d9..6daab022aa 100644 --- a/src/test/test-mountpoint-util.c +++ b/src/test/test-mountpoint-util.c @@ -20,7 +20,7 @@ static void test_mount_propagation_flags_one(const char *name, int ret, unsigned long expected) { unsigned long flags; - log_info("/* %s(%s) */", __func__, name); + log_info("/* %s(%s) */", __func__, strnull(name)); assert_se(mount_propagation_flags_from_string(name, &flags) == ret); From 817d19402f620f9cc97a813b9174b7b998a68fd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 12:13:42 +0200 Subject: [PATCH 7/8] test-bus-address: silence gcc warning about NULL arg to strcmp() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code was overly complicated… We know what value we expect, we can test it directly. --- src/libsystemd/sd-bus/test-bus-address.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libsystemd/sd-bus/test-bus-address.c b/src/libsystemd/sd-bus/test-bus-address.c index ec9ee83fac..347ba1a39b 100644 --- a/src/libsystemd/sd-bus/test-bus-address.c +++ b/src/libsystemd/sd-bus/test-bus-address.c @@ -15,11 +15,9 @@ static void test_one_address(sd_bus *b, r = bus_set_address_system_remote(b, host); log_info("\"%s\" → %d, \"%s\"", host, r, strna(r >= 0 ? b->address : NULL)); - if (result < 0 || expected) { - assert_se(r == result); - if (r >= 0) - assert_se(streq(b->address, expected)); - } + assert_se(r == result); + if (r >= 0) + assert_se(streq_ptr(b->address, expected)); } TEST(bus_set_address_system_remote) { @@ -38,8 +36,8 @@ TEST(bus_set_address_system_remote) { 0, "unixexec:path=ssh,argv1=-xT,argv2=-p,argv3=123,argv4=--,argv5=host,argv6=systemd-stdio-bridge"); test_one_address(b, "host:123:123", -EINVAL, NULL); - test_one_address(b, "host:", - -EINVAL, NULL); + test_one_address(b, "host:", + -EINVAL, NULL); test_one_address(b, "user@host", 0, "unixexec:path=ssh,argv1=-xT,argv2=--,argv3=user%40host,argv4=systemd-stdio-bridge"); test_one_address(b, "user@host@host", From e0d537950b6536bd5288c67765e09948449025d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jun 2022 12:22:29 +0200 Subject: [PATCH 8/8] test-load-fragment: don't print NULLs --- src/test/test-load-fragment.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index 9cbf22312f..cc6301e9dc 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -70,10 +70,10 @@ static void check_execcommand(ExecCommand *c, assert_se(c); log_info("expect: \"%s\" [\"%s\" \"%s\" \"%s\"]", - path, argv0 ?: path, argv1, argv2); + path, argv0 ?: path, strnull(argv1), strnull(argv2)); n = strv_length(c->argv); log_info("actual: \"%s\" [\"%s\" \"%s\" \"%s\"]", - c->path, c->argv[0], n > 0 ? c->argv[1] : NULL, n > 1 ? c->argv[2] : NULL); + c->path, c->argv[0], n > 0 ? c->argv[1] : "(null)", n > 1 ? c->argv[2] : "(null)"); assert_se(streq(c->path, path)); assert_se(streq(c->argv[0], argv0 ?: path)); if (n > 0)