From 5ec751ab9a06dadc62b30dc07e9dd7a41f8da403 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 4 Mar 2022 18:47:31 +0100 Subject: [PATCH] shared/install: reuse the standard symlink verification subroutine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We save a few lines, but the important thing is that we don't have two different implementations with slightly different rules used for enablement and loading. Fixes #22000. Tested with: - the report in #22000, it now says: $ SYSTEMD_LOG_LEVEL=debug systemctl --root=/ enable test.service Suspicious symlink /etc/systemd/system/test.service→/etc/systemd/system/myown.d/test.service, treating as alias. unit_file_resolve_symlink: self-alias: /etc/systemd/system/test.service → test.service, ignoring. running_in_chroot(): Permission denied Suspicious symlink /etc/systemd/system/test.service→/etc/systemd/system/myown.d/test.service, treating as alias. unit_file_resolve_symlink: self-alias: /etc/systemd/system/test.service → test.service, ignoring. Failed to enable unit, refusing to operate on linked unit file test.service - a symlink to /dev/null: ... unit_file_resolve_symlink: linked unit file: /etc/systemd/system/test3.service → /dev/null Failed to enable unit, unit /etc/systemd/system/test3.service is masked. - the same from the host: ... unit_file_resolve_symlink: linked unit file: /var/lib/machines/rawhide/etc/systemd/system/test3.service → /var/lib/machines/rawhide/dev/null Failed to enable unit, unit /var/lib/machines/rawhide/etc/systemd/system/test3.service is masked. - through the manager: $ sudo systemctl enable test.service Failed to enable unit: Refusing to operate on alias name or linked unit file: test.service $ sudo systemctl enable test3.service Failed to enable unit: Unit file /etc/systemd/system/test3.service is masked. As seen in the first example, the warning is repeated. This is because we call the lookup logic twice: first for sysv-compat, and then again for real. I think that since this is only for broken setups, and when sysv-compat is enabled, and in an infrequent manual operation, at debug level, this is OK. (cherry picked from commit 047d37dc3d376d912275c14d217f7a0dda9a5f0e) Related: #2082131 --- src/basic/unit-file.c | 70 +++++++++++++++++++++++++++++++---------- src/basic/unit-file.h | 10 ++++++ src/shared/install.c | 72 +++++++------------------------------------ 3 files changed, 75 insertions(+), 77 deletions(-) diff --git a/src/basic/unit-file.c b/src/basic/unit-file.c index 25abce932a..f7a10b22c6 100644 --- a/src/basic/unit-file.c +++ b/src/basic/unit-file.c @@ -260,27 +260,50 @@ static int directory_name_is_valid(const char *name) { return false; } -static int unit_file_resolve_symlink( +int unit_file_resolve_symlink( const char *root_dir, char **search_path, const char *dir, int dirfd, const char *filename, + bool resolve_destination_target, char **ret_destination) { - _cleanup_free_ char *target = NULL, *simplified = NULL, *dst = NULL; + _cleanup_free_ char *target = NULL, *simplified = NULL, *dst = NULL, *_dir = NULL, *_filename = NULL; int r; - assert(dir); - assert(dirfd >= 0); + /* This can be called with either dir+dirfd valid and filename just a name, + * or !dir && dirfd==AT_FDCWD, and filename being a full path. + * + * If resolve_destination_target is true, an absolute path will be returned. + * If not, an absolute path is returned for linked unit files, and a relative + * path otherwise. */ + assert(filename); assert(ret_destination); + assert(dir || path_is_absolute(filename)); + assert(dirfd >= 0 || dirfd == AT_FDCWD); r = readlinkat_malloc(dirfd, filename, &target); if (r < 0) return log_warning_errno(r, "Failed to read symlink %s%s%s: %m", dir, dir ? "/" : "", filename); + if (!dir) { + r = path_extract_directory(filename, &_dir); + if (r < 0) + return r; + dir = _dir; + + r = path_extract_filename(filename, &_filename); + if (r < 0) + return r; + if (r == O_DIRECTORY) + return log_warning_errno(SYNTHETIC_ERRNO(EISDIR), + "Unexpected path to a directory \"%s\", refusing.", filename); + filename = _filename; + } + bool is_abs = path_is_absolute(target); if (root_dir || !is_abs) { char *target_abs = path_join(is_abs ? root_dir : dir, target); @@ -296,24 +319,36 @@ static int unit_file_resolve_symlink( return log_warning_errno(r, "Failed to resolve symlink %s/%s pointing to %s: %m", dir, filename, target); + assert(path_is_absolute(simplified)); + /* Check if the symlink goes outside of our search path. - * If yes, it's a linked unit file or mask, and we don't care about the target name. - * Let's just store the link source directly. - * If not, let's verify that it's a good symlink. */ + * If yes, it's a linked unit file or mask, and we don't care about the target name + * when loading units, and we return the link *source* (resolve_destination_target == false); + * When this is called for installation purposes, we want the final destination, + * so we return the *target*. + * + * Otherwise, let's verify that it's a good alias. + */ const char *tail = path_startswith_strv(simplified, search_path); if (!tail) { log_debug("Linked unit file: %s/%s → %s", dir, filename, simplified); - dst = path_join(dir, filename); - if (!dst) - return log_oom(); + if (resolve_destination_target) + dst = TAKE_PTR(simplified); + else { + dst = path_join(dir, filename); + if (!dst) + return log_oom(); + } } else { - r = path_extract_filename(simplified, &dst); + _cleanup_free_ char *target_name = NULL; + + r = path_extract_filename(simplified, &target_name); if (r < 0) return r; - bool self_alias = streq(dst, filename); + bool self_alias = streq(target_name, filename); if (is_path(tail)) log_full(self_alias ? LOG_DEBUG : LOG_WARNING, @@ -324,13 +359,15 @@ static int unit_file_resolve_symlink( if (r < 0) return r; - if (self_alias) - /* A self-alias that has no effect */ + if (self_alias && !resolve_destination_target) + /* A self-alias that has no effect when loading, let's just ignore it. */ return log_debug_errno(SYNTHETIC_ERRNO(ELOOP), "Unit file self-alias: %s/%s → %s, ignoring.", - dir, filename, dst); + dir, filename, target_name); + + log_debug("Unit file alias: %s/%s → %s", dir, filename, target_name); - log_debug("Unit file alias: %s/%s → %s", dir, filename, dst); + dst = resolve_destination_target ? TAKE_PTR(simplified) : TAKE_PTR(target_name); } *ret_destination = TAKE_PTR(dst); @@ -475,6 +512,7 @@ int unit_file_build_name_map( r = unit_file_resolve_symlink(lp->root_dir, lp->search_path, *dir, dirfd(d), de->d_name, + /* resolve_destination_target= */ false, &dst); if (r == -ENOMEM) return r; diff --git a/src/basic/unit-file.h b/src/basic/unit-file.h index cc731a9e06..e29e878cfd 100644 --- a/src/basic/unit-file.h +++ b/src/basic/unit-file.h @@ -44,6 +44,16 @@ int unit_symlink_name_compatible(const char *symlink, const char *target, bool i int unit_validate_alias_symlink_and_warn(const char *filename, const char *target); bool lookup_paths_timestamp_hash_same(const LookupPaths *lp, uint64_t timestamp_hash, uint64_t *ret_new); + +int unit_file_resolve_symlink( + const char *root_dir, + char **search_path, + const char *dir, + int dirfd, + const char *filename, + bool resolve_destination_target, + char **ret_destination); + int unit_file_build_name_map( const LookupPaths *lp, uint64_t *cache_timestamp_hash, diff --git a/src/shared/install.c b/src/shared/install.c index 79e5109ce1..e07ca31797 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1338,76 +1338,26 @@ static int unit_file_load_or_readlink( const char *path, const LookupPaths *lp, SearchFlags flags) { - - _cleanup_free_ char *resolved = NULL; int r; r = unit_file_load(c, info, path, lp->root_dir, flags); if (r != -ELOOP || (flags & SEARCH_DROPIN)) return r; - r = chase_symlinks(path, lp->root_dir, CHASE_WARN | CHASE_NONEXISTENT, &resolved, NULL); - if (r >= 0 && - lp->root_dir && - path_equal_ptr(path_startswith(resolved, lp->root_dir), "dev/null")) - /* When looking under root_dir, we can't expect /dev/ to be mounted, - * so let's see if the path is a (possibly dangling) symlink to /dev/null. */ - info->type = UNIT_FILE_TYPE_MASKED; - - else if (r > 0 && null_or_empty_path(resolved) > 0) + /* This is a symlink, let's read and verify it. */ + r = unit_file_resolve_symlink(lp->root_dir, lp->search_path, + NULL, AT_FDCWD, path, + true, &info->symlink_target); + if (r < 0) + return r; + r = null_or_empty_path_with_root(info->symlink_target, lp->root_dir); + if (r < 0 && r != -ENOENT) + return log_debug_errno(r, "Failed to stat %s: %m", info->symlink_target); + if (r > 0) info->type = UNIT_FILE_TYPE_MASKED; - - else { - _cleanup_free_ char *target = NULL; - const char *bn; - UnitType a, b; - - /* This is a symlink, let's read it. We read the link again, because last time - * we followed the link until resolution, and here we need to do one step. */ - - r = readlink_malloc(path, &target); - if (r < 0) - return r; - - bn = basename(target); - - if (unit_name_is_valid(info->name, UNIT_NAME_PLAIN)) { - - if (!unit_name_is_valid(bn, UNIT_NAME_PLAIN)) - return -EINVAL; - - } else if (unit_name_is_valid(info->name, UNIT_NAME_INSTANCE)) { - - if (!unit_name_is_valid(bn, UNIT_NAME_INSTANCE|UNIT_NAME_TEMPLATE)) - return -EINVAL; - - } else if (unit_name_is_valid(info->name, UNIT_NAME_TEMPLATE)) { - - if (!unit_name_is_valid(bn, UNIT_NAME_TEMPLATE)) - return -EINVAL; - } else - return -EINVAL; - - /* Enforce that the symlink destination does not - * change the unit file type. */ - - a = unit_name_to_type(info->name); - b = unit_name_to_type(bn); - if (a < 0 || b < 0 || a != b) - return -EINVAL; - - if (path_is_absolute(target)) - /* This is an absolute path, prefix the root so that we always deal with fully qualified paths */ - info->symlink_target = path_join(lp->root_dir, target); - else - /* This is a relative path, take it relative to the dir the symlink is located in. */ - info->symlink_target = file_in_same_dir(path, target); - if (!info->symlink_target) - return -ENOMEM; - + else info->type = UNIT_FILE_TYPE_SYMLINK; - } return 0; }