core: fix mtime calculation of dropin files

Nominally, the bug was in unit_load_dropin(), which just took the last mtime
instead of calculating the maximum. But instead of adding code to wrap the
loop, this patch goes in the other direction.

All (correct) callers of config_parse() followed a very similar pattern to
calculate the maximum mtime. So let's simplify things by making config_parse()
assume that mtime is initialized and update it to the maximum. This makes all
the callers that care about mtime simpler and also fixes the issue in
unit_load_dropin().

config_parse_many_nulstr() and config_parse_many() are different, because it
makes sense to call them just once, and current ret_mtime behaviour make sense.

Fixes #17730, https://bugzilla.redhat.com/show_bug.cgi?id=1933137.

src/basic/fs-util.c

@@ -1413,33 +1413,45 @@ int unlinkat_deallocate(int fd, const char *name, UnlinkDeallocateFlags flags) {
 int fsync_directory_of_file(int fd) {
         _cleanup_free_ char *path = NULL;
         _cleanup_close_ int dfd = -1;
+        struct stat st;
         int r;
 
-        r = fd_verify_regular(fd);
+        assert(fd >= 0);
-        if (r < 0)
-                return r;
 
-        r = fd_get_path(fd, &path);
+        /* We only reasonably can do this for regular files and directories, hence check for that */
-        if (r < 0) {
+        if (fstat(fd, &st) < 0)
-                log_debug_errno(r, "Failed to query /proc/self/fd/%d%s: %m",
+                return -errno;
-                                fd,
-                                r == -ENOSYS ? ", ignoring" : "");
 
-                if (r == -ENOSYS)
+        if (S_ISREG(st.st_mode)) {
-                        /* If /proc is not available, we're most likely running in some
-                         * chroot environment, and syncing the directory is not very
-                         * important in that case. Let's just silently do nothing. */
-                        return 0;
 
-                return r;
+                r = fd_get_path(fd, &path);
-        }
+                if (r < 0) {
+                        log_debug_errno(r, "Failed to query /proc/self/fd/%d%s: %m",
+                                        fd,
+                                        r == -ENOSYS ? ", ignoring" : "");
 
-        if (!path_is_absolute(path))
+                        if (r == -ENOSYS)
-                return -EINVAL;
+                                /* If /proc is not available, we're most likely running in some
+                                 * chroot environment, and syncing the directory is not very
+                                 * important in that case. Let's just silently do nothing. */
+                                return 0;
 
-        dfd = open_parent(path, O_CLOEXEC, 0);
+                        return r;
-        if (dfd < 0)
+                }
-                return dfd;
+
+                if (!path_is_absolute(path))
+                        return -EINVAL;
+
+                dfd = open_parent(path, O_CLOEXEC|O_NOFOLLOW, 0);
+                if (dfd < 0)
+                        return dfd;
+
+        } else if (S_ISDIR(st.st_mode)) {
+                dfd = openat(fd, "..", O_RDONLY|O_DIRECTORY|O_CLOEXEC, 0);
+                if (dfd < 0)
+                        return -errno;
+        } else
+                return -ENOTTY;
 
         if (fsync(dfd) < 0)
                 return -errno;
@@ -1453,9 +1465,14 @@ int fsync_full(int fd) {
         /* Sync both the file and the directory */
 
         r = fsync(fd) < 0 ? -errno : 0;
-        q = fsync_directory_of_file(fd);
 
-        return r < 0 ? r : q;
+        q = fsync_directory_of_file(fd);
+        if (r < 0) /* Return earlier error */
+                return r;
+        if (q == -ENOTTY) /* Ignore if the 'fd' refers to a block device or so which doesn't really have a
+                           * parent dir */
+                return 0;
+        return q;
 }
 
 int fsync_path_at(int at_fd, const char *path) {
@@ -1472,8 +1489,7 @@ int fsync_path_at(int at_fd, const char *path) {
                 } else
                         fd = at_fd;
         } else {
-
+                opened_fd = openat(at_fd, path, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
-                opened_fd = openat(at_fd, path, O_RDONLY|O_CLOEXEC);
                 if (opened_fd < 0)
                         return -errno;
 

src/basic/rm-rf.c

@@ -23,23 +23,14 @@ static bool is_physical_fs(const struct statfs *sfs) {
         return !is_temporary_fs(sfs) && !is_cgroup_fs(sfs);
 }
 
-static int unlinkat_harder(
+static int patch_dirfd_mode(
                 int dfd,
-                const char *filename,
+                mode_t *ret_old_mode) {
-                int unlink_flags,
-                RemoveFlags remove_flags) {
 
         struct stat st;
-        int r;
 
-        /* Like unlinkat(), but tries harder: if we get EACCESS we'll try to set the r/w/x bits on the
+        assert(dfd >= 0);
-         * directory. This is useful if we run unprivileged and have some files where the w bit is
+        assert(ret_old_mode);
-         * missing. */
-
-        if (unlinkat(dfd, filename, unlink_flags) >= 0)
-                return 0;
-        if (errno != EACCES || !FLAGS_SET(remove_flags, REMOVE_CHMOD))
-                return -errno;
 
         if (fstat(dfd, &st) < 0)
                 return -errno;
@@ -53,10 +44,68 @@ static int unlinkat_harder(
         if (fchmod(dfd, (st.st_mode | 0700) & 07777) < 0)
                 return -errno;
 
+        *ret_old_mode = st.st_mode;
+        return 0;
+}
+
+static int unlinkat_harder(
+                int dfd,
+                const char *filename,
+                int unlink_flags,
+                RemoveFlags remove_flags) {
+
+        mode_t old_mode;
+        int r;
+
+        /* Like unlinkat(), but tries harder: if we get EACCESS we'll try to set the r/w/x bits on the
+         * directory. This is useful if we run unprivileged and have some files where the w bit is
+         * missing. */
+
+        if (unlinkat(dfd, filename, unlink_flags) >= 0)
+                return 0;
+        if (errno != EACCES || !FLAGS_SET(remove_flags, REMOVE_CHMOD))
+                return -errno;
+
+        r = patch_dirfd_mode(dfd, &old_mode);
+        if (r < 0)
+                return r;
+
         if (unlinkat(dfd, filename, unlink_flags) < 0) {
                 r = -errno;
                 /* Try to restore the original access mode if this didn't work */
-                (void) fchmod(dfd, st.st_mode & 07777);
+                (void) fchmod(dfd, old_mode);
+                return r;
+        }
+
+        /* If this worked, we won't reset the old mode, since we'll need it for other entries too, and we
+         * should destroy the whole thing */
+        return 0;
+}
+
+static int fstatat_harder(
+                int dfd,
+                const char *filename,
+                struct stat *ret,
+                int fstatat_flags,
+                RemoveFlags remove_flags) {
+
+        mode_t old_mode;
+        int r;
+
+        /* Like unlink_harder() but does the same for fstatat() */
+
+        if (fstatat(dfd, filename, ret, fstatat_flags) >= 0)
+                return 0;
+        if (errno != EACCES || !FLAGS_SET(remove_flags, REMOVE_CHMOD))
+                return -errno;
+
+        r = patch_dirfd_mode(dfd, &old_mode);
+        if (r < 0)
+                return r;
+
+        if (fstatat(dfd, filename, ret, fstatat_flags) < 0) {
+                r = -errno;
+                (void) fchmod(dfd, old_mode);
                 return r;
         }
 
@@ -112,9 +161,10 @@ int rm_rf_children(int fd, RemoveFlags flags, struct stat *root_dev) {
 
                 if (de->d_type == DT_UNKNOWN ||
                     (de->d_type == DT_DIR && (root_dev || (flags & REMOVE_SUBVOLUME)))) {
-                        if (fstatat(fd, de->d_name, &st, AT_SYMLINK_NOFOLLOW) < 0) {
+                        r = fstatat_harder(fd, de->d_name, &st, AT_SYMLINK_NOFOLLOW, flags);
-                                if (ret == 0 && errno != ENOENT)
+                        if (r < 0) {
-                                        ret = -errno;
+                                if (ret == 0 && r != -ENOENT)
+                                        ret = r;
                                 continue;
                         }
 

src/core/load-dropin.c

@@ -112,6 +112,7 @@ int unit_load_dropin(Unit *u) {
                         return log_oom();
         }
 
+        u->dropin_mtime = 0;
         STRV_FOREACH(f, u->dropin_paths)
                 (void) config_parse(
                                 u->id, *f, NULL,

src/coredump/coredumpctl.c

@@ -563,6 +563,7 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) {
                 RETRIEVE(d, l, "COREDUMP_EXE", exe);
                 RETRIEVE(d, l, "COREDUMP_COMM", comm);
                 RETRIEVE(d, l, "COREDUMP_CMDLINE", cmdline);
+                RETRIEVE(d, l, "COREDUMP_HOSTNAME", hostname);
                 RETRIEVE(d, l, "COREDUMP_UNIT", unit);
                 RETRIEVE(d, l, "COREDUMP_USER_UNIT", user_unit);
                 RETRIEVE(d, l, "COREDUMP_SESSION", session);
@@ -575,7 +576,6 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) {
                 RETRIEVE(d, l, "COREDUMP", coredump);
                 RETRIEVE(d, l, "_BOOT_ID", boot_id);
                 RETRIEVE(d, l, "_MACHINE_ID", machine_id);
-                RETRIEVE(d, l, "_HOSTNAME", hostname);
                 RETRIEVE(d, l, "MESSAGE", message);
         }
 

src/partition/repart.c

@@ -960,7 +960,6 @@ static int config_parse_label(
                 void *data,
                 void *userdata) {
 
-        _cleanup_free_ char16_t *recoded = NULL;
         _cleanup_free_ char *resolved = NULL;
         char **label = data;
         int r;
@@ -981,11 +980,14 @@ static int config_parse_label(
                 return 0;
         }
 
-        recoded = utf8_to_utf16(resolved, strlen(resolved));
+        r = gpt_partition_label_valid(resolved);
-        if (!recoded)
+        if (r < 0) {
-                return log_oom();
+                log_syntax(unit, LOG_WARNING, filename, line, r,
-
+                           "Failed to check if string is valid as GPT partition label, ignoring: \"%s\" (from \"%s\")",
-        if (char16_strlen(recoded) > 36) {
+                           resolved, rvalue);
+                return 0;
+        }
+        if (!r) {
                 log_syntax(unit, LOG_WARNING, filename, line, 0,
                            "Partition label too long for GPT table, ignoring: \"%s\" (from \"%s\")",
                            resolved, rvalue);

src/shared/conf-parser.c

@@ -263,7 +263,7 @@ int config_parse(
                 const void *table,
                 ConfigParseFlags flags,
                 void *userdata,
-                usec_t *ret_mtime) {
+                usec_t *latest_mtime) {
 
         _cleanup_free_ char *section = NULL, *continuation = NULL;
         _cleanup_fclose_ FILE *ours = NULL;
@@ -275,6 +275,9 @@ int config_parse(
         assert(filename);
         assert(lookup);
 
+        /* latest_mtime is an input-output parameter: it will be updated if the mtime of the file we're
+         * looking at is later than the current *latest_mtime value. */
+
         if (!f) {
                 f = ours = fopen(filename, "re");
                 if (!f) {
@@ -417,8 +420,8 @@ int config_parse(
                 }
         }
 
-        if (ret_mtime)
+        if (latest_mtime)
-                *ret_mtime = mtime;
+                *latest_mtime = MAX(*latest_mtime, mtime);
 
         return 1;
 }
@@ -448,12 +451,9 @@ static int config_parse_many_files(
 
         /* Then read all the drop-ins. */
         STRV_FOREACH(fn, files) {
-                usec_t t;
+                r = config_parse(NULL, *fn, NULL, sections, lookup, table, flags, userdata, &mtime);
-
-                r = config_parse(NULL, *fn, NULL, sections, lookup, table, flags, userdata, &t);
                 if (r < 0)
                         return r;
-                mtime = MAX(mtime, t); /* Find the newest */
         }
 
         if (ret_mtime)

src/shared/conf-parser.h

@@ -89,7 +89,7 @@ int config_parse(
                 const void *table,
                 ConfigParseFlags flags,
                 void *userdata,
-                usec_t *ret_mtime);         /* possibly NULL */
+                usec_t *latest_mtime);      /* input/output, possibly NULL */
 
 int config_parse_many_nulstr(
                 const char *conf_file,      /* possibly NULL */

src/shared/gpt.c

@@ -2,6 +2,7 @@
 
 #include "gpt.h"
 #include "string-util.h"
+#include "utf8.h"
 
 const GptPartitionType gpt_partition_type_table[] = {
         { GPT_ROOT_X86,              "root-x86"              },
@@ -95,3 +96,13 @@ int gpt_partition_type_uuid_from_string(const char *s, sd_id128_t *ret) {
 
         return sd_id128_from_string(s, ret);
 }
+
+int gpt_partition_label_valid(const char *s) {
+        _cleanup_free_ char16_t *recoded = NULL;
+
+        recoded = utf8_to_utf16(s, strlen(s));
+        if (!recoded)
+                return -ENOMEM;
+
+        return char16_strlen(recoded) <= 36;
+}

src/shared/gpt.h

@@ -126,3 +126,5 @@ typedef struct GptPartitionType {
 } GptPartitionType;
 
 extern const GptPartitionType gpt_partition_type_table[];
+
+int gpt_partition_label_valid(const char *s);