From 62c03398ba6012ff693fbce24c142309cdbc1cd0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Sep 2020 17:49:35 +0200 Subject: [PATCH 1/2] sysusers: modernize file backup logic a bit Let's use _cleanup_ magic to clean up files, let's fully operate by fds whenever we can. --- src/sysusers/sysusers.c | 45 +++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index cdfceb2533..7349e9fcb9 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -201,14 +201,16 @@ static int load_group_database(void) { } static int make_backup(const char *target, const char *x) { - _cleanup_close_ int src = -1; + _cleanup_(unlink_and_freep) char *dst_tmp = NULL; _cleanup_fclose_ FILE *dst = NULL; - _cleanup_free_ char *dst_tmp = NULL; - char *backup; - struct timespec ts[2]; + _cleanup_close_ int src = -1; + const char *backup; struct stat st; int r; + assert(target); + assert(x); + src = open(x, O_RDONLY|O_CLOEXEC|O_NOCTTY); if (src < 0) { if (errno == ENOENT) /* No backup necessary... */ @@ -220,43 +222,38 @@ static int make_backup(const char *target, const char *x) { if (fstat(src, &st) < 0) return -errno; - r = fopen_temporary_label(target, x, &dst, &dst_tmp); + r = fopen_temporary_label( + target, /* The path for which to the lookup the label */ + x, /* Where we want the file actually to end up */ + &dst, + &dst_tmp /* The temporary file we write to */); if (r < 0) return r; r = copy_bytes(src, fileno(dst), (uint64_t) -1, COPY_REFLINK); if (r < 0) - goto fail; - - /* Don't fail on chmod() or chown(). If it stays owned by us - * and/or unreadable by others, then it isn't too bad... */ + return r; backup = strjoina(x, "-"); - /* Copy over the access mask */ - r = chmod_and_chown_unsafe(dst_tmp, st.st_mode & 07777, st.st_uid, st.st_gid); + /* Copy over the access mask. Don't fail on chmod() or chown(). If it stays owned by us and/or + * unreadable by others, then it isn't too bad... */ + r = fchmod_and_chown(fileno(dst), st.st_mode & 07777, st.st_uid, st.st_gid); if (r < 0) log_warning_errno(r, "Failed to change access mode or ownership of %s: %m", backup); - ts[0] = st.st_atim; - ts[1] = st.st_mtim; - if (futimens(fileno(dst), ts) < 0) + if (futimens(fileno(dst), (const struct timespec[2]) { st.st_atim, st.st_mtim }) < 0) log_warning_errno(errno, "Failed to fix access and modification time of %s: %m", backup); - r = fflush_sync_and_check(dst); + r = fsync_full(fileno(dst)); if (r < 0) - goto fail; + return r; - if (rename(dst_tmp, backup) < 0) { - r = -errno; - goto fail; - } + if (rename(dst_tmp, backup) < 0) + return errno; + dst_tmp = mfree(dst_tmp); /* disable the unlink_and_freep() hook now that the file has been renamed*/ return 0; - -fail: - (void) unlink(dst_tmp); - return r; } static int putgrent_with_members(const struct group *gr, FILE *group) { From 1e5bfa2ac8bffc4e53b596b77f0dbdec0bba281a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Sep 2020 17:51:08 +0200 Subject: [PATCH 2/2] fs-util: drop chmod_and_chown_unsafe() which is unused now --- src/basic/fs-util.c | 46 ----------------------------------------- src/basic/fs-util.h | 1 - src/test/test-fs-util.c | 45 ---------------------------------------- 3 files changed, 92 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 7f8b8b22b3..8a4943b8dd 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -280,52 +280,6 @@ int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid) { return do_chown || do_chmod; } -int chmod_and_chown_unsafe(const char *path, mode_t mode, uid_t uid, gid_t gid) { - bool do_chown, do_chmod; - struct stat st; - - assert(path); - - /* Change ownership and access mode of the specified path, see description of fchmod_and_chown(). - * Should only be used on trusted paths. */ - - if (lstat(path, &st) < 0) - return -errno; - - do_chown = - (uid != UID_INVALID && st.st_uid != uid) || - (gid != GID_INVALID && st.st_gid != gid); - - do_chmod = - !S_ISLNK(st.st_mode) && /* chmod is not defined on symlinks */ - ((mode != MODE_INVALID && ((st.st_mode ^ mode) & 07777) != 0) || - do_chown); /* If we change ownership, make sure we reset the mode afterwards, since chown() - * modifies the access mode too */ - - if (mode == MODE_INVALID) - mode = st.st_mode; /* If we only shall do a chown(), save original mode, since chown() might break it. */ - else if ((mode & S_IFMT) != 0 && ((mode ^ st.st_mode) & S_IFMT) != 0) - return -EINVAL; /* insist on the right file type if it was specified */ - - if (do_chown && do_chmod) { - mode_t minimal = st.st_mode & mode; /* the subset of the old and the new mask */ - - if (((minimal ^ st.st_mode) & 07777) != 0) - if (chmod(path, minimal & 07777) < 0) - return -errno; - } - - if (do_chown) - if (lchown(path, uid, gid) < 0) - return -errno; - - if (do_chmod) - if (chmod(path, mode & 07777) < 0) - return -errno; - - return do_chown || do_chmod; -} - int fchmod_umask(int fd, mode_t m) { mode_t u; int r; diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index b184570f9f..eb6e1eee4f 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -34,7 +34,6 @@ int readlink_and_make_absolute(const char *p, char **r); int chmod_and_chown(const char *path, mode_t mode, uid_t uid, gid_t gid); int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid); -int chmod_and_chown_unsafe(const char *path, mode_t mode, uid_t uid, gid_t gid); int fchmod_umask(int fd, mode_t mode); int fchmod_opath(int fd, mode_t m); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index f2df2e35e6..f63b1f5d5f 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -802,50 +802,6 @@ static void test_chmod_and_chown(void) { assert_se(S_ISLNK(st.st_mode)); } -static void test_chmod_and_chown_unsafe(void) { - _cleanup_(rm_rf_physical_and_freep) char *d = NULL; - _unused_ _cleanup_umask_ mode_t u = umask(0000); - struct stat st; - const char *p; - - if (geteuid() != 0) - return; - - log_info("/* %s */", __func__); - - assert_se(mkdtemp_malloc(NULL, &d) >= 0); - - p = strjoina(d, "/reg"); - assert_se(mknod(p, S_IFREG | 0123, 0) >= 0); - - assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0321, 1, 2) >= 0); - assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0555, 3, 4) == -EINVAL); - - assert_se(lstat(p, &st) >= 0); - assert_se(S_ISREG(st.st_mode)); - assert_se((st.st_mode & 07777) == 0321); - - p = strjoina(d, "/dir"); - assert_se(mkdir(p, 0123) >= 0); - - assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0321, 1, 2) >= 0); - assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0555, 3, 4) == -EINVAL); - - assert_se(lstat(p, &st) >= 0); - assert_se(S_ISDIR(st.st_mode)); - assert_se((st.st_mode & 07777) == 0321); - - p = strjoina(d, "/lnk"); - assert_se(symlink("idontexist", p) >= 0); - - assert_se(chmod_and_chown_unsafe(p, S_IFLNK | 0321, 1, 2) >= 0); - assert_se(chmod_and_chown_unsafe(p, S_IFREG | 0555, 3, 4) == -EINVAL); - assert_se(chmod_and_chown_unsafe(p, S_IFDIR | 0555, 3, 4) == -EINVAL); - - assert_se(lstat(p, &st) >= 0); - assert_se(S_ISLNK(st.st_mode)); -} - static void test_path_is_encrypted_one(const char *p, int expect) { int r; @@ -895,7 +851,6 @@ int main(int argc, char *argv[]) { test_fsync_directory_of_file(); test_rename_noreplace(); test_chmod_and_chown(); - test_chmod_and_chown_unsafe(); test_path_is_encrypted(); return 0;