From 640f3b143d77b02612dc694a6a2be08f98d6c0e4 Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Thu, 28 May 2020 12:09:32 -0700 Subject: [PATCH 1/2] core: check null_or_empty for masked units instead of /dev/null There's some inconsistency in the what is considered a masked unit: some places (i.e. load-fragment.c) use `null_or_empty()` while others check if the file path is symlinked to "/dev/null". Since the latter doesn't account for things like non-absolute symlinks to "/dev/null", this commit switches the check for "/dev/null" to use `null_or_empty_path()` --- src/core/unit.c | 2 +- src/shared/install.c | 19 ++++++++++--------- src/test/test-install-root.c | 24 ++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 18bf0cd52a..c739c0a561 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5955,7 +5955,7 @@ const char *unit_label_path(const Unit *u) { return NULL; /* If a unit is masked, then don't read the SELinux label of /dev/null, as that really makes no sense */ - if (path_equal(p, "/dev/null")) + if (null_or_empty_path(p) > 0) return NULL; return p; diff --git a/src/shared/install.c b/src/shared/install.c index bb2eff7387..0f2d407a36 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1324,26 +1324,27 @@ static int unit_file_load_or_readlink( const char *path, const char *root_dir, SearchFlags flags) { - - _cleanup_free_ char *target = NULL; + struct stat st; int r; r = unit_file_load(c, info, path, root_dir, flags); if (r != -ELOOP || (flags & SEARCH_DROPIN)) return r; - /* This is a symlink, let's read it. */ - - r = readlink_malloc(path, &target); - if (r < 0) - return r; - - if (path_equal(target, "/dev/null")) + r = chase_symlinks_and_stat(path, root_dir, CHASE_WARN, NULL, &st, NULL); + if (r > 0 && null_or_empty(&st)) 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. */ + + r = readlink_malloc(path, &target); + if (r < 0) + return r; + bn = basename(target); if (unit_name_is_valid(info->name, UNIT_NAME_PLAIN)) { diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index 515f14b8ca..d437686bae 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -4,6 +4,7 @@ #include "alloc-util.h" #include "fileio.h" +#include "fs-util.h" #include "install.h" #include "mkdir.h" #include "rm-rf.h" @@ -23,6 +24,7 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "b.service", NULL) == -ENOENT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "c.service", NULL) == -ENOENT); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "d.service", NULL) == -ENOENT); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "e.service", NULL) == -ENOENT); p = strjoina(root, "/usr/lib/systemd/system/a.service"); assert_se(write_string_file(p, @@ -150,6 +152,22 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "b.service", &state) >= 0 && state == UNIT_FILE_ALIAS); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "c.service", &state) >= 0 && state == UNIT_FILE_ALIAS); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "d.service", &state) >= 0 && state == UNIT_FILE_ALIAS); + + /* Test masking with relative symlinks */ + + p = strjoina(root, "/usr/lib/systemd/system/e.service"); + assert_se(symlink("../../../../../../dev/null", p) >= 0); + + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "e.service", NULL) >= 0); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "e.service", &state) >= 0 && state == UNIT_FILE_MASKED); + + assert_se(unlink(p) == 0); + assert_se(symlink("/usr/../dev/null", p) >= 0); + + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "e.service", NULL) >= 0); + assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "e.service", &state) >= 0 && state == UNIT_FILE_MASKED); + + assert_se(unlink(p) == 0); } static void test_linked_units(const char *root) { @@ -1227,6 +1245,12 @@ int main(int argc, char *argv[]) { p = strjoina(root, "/usr/lib/systemd/system-preset/"); assert_se(mkdir_p(p, 0755) >= 0); + p = strjoina(root, "/dev/"); + assert_se(mkdir_p(p, 0755) >= 0); + + p = strjoina(root, "/dev/null"); + assert_se(touch(p) >= 0); + test_basic_mask_and_enable(root); test_linked_units(root); test_default(root); From 4276749dd3546dea6cc4a5967c698886829f4573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 5 Jul 2020 20:03:48 +0200 Subject: [PATCH 2/2] shared/install: do not require /dev/null to be present in chroots This partially undoes the parent commit. We follow the symlink and if it appears to be a symlink to /dev/null, even if /dev/null is not present, we treat it as such. The addition of creation of /dev/null in the test is reverted. --- src/shared/install.c | 22 +++++++++++++++++----- src/test/test-install-root.c | 6 ------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 0f2d407a36..fb5e166ff0 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1236,8 +1236,7 @@ static int unit_file_load( "%s: unit type %s cannot be templated, ignoring.", path, unit_type_to_string(type)); if (!(flags & SEARCH_LOAD)) { - r = lstat(path, &st); - if (r < 0) + if (lstat(path, &st) < 0) return -errno; if (null_or_empty(&st)) @@ -1324,6 +1323,7 @@ static int unit_file_load_or_readlink( const char *path, const char *root_dir, SearchFlags flags) { + _cleanup_free_ char *resolved = NULL; struct stat st; int r; @@ -1331,15 +1331,27 @@ static int unit_file_load_or_readlink( if (r != -ELOOP || (flags & SEARCH_DROPIN)) return r; - r = chase_symlinks_and_stat(path, root_dir, CHASE_WARN, NULL, &st, NULL); - if (r > 0 && null_or_empty(&st)) + r = chase_symlinks(path, root_dir, CHASE_WARN | CHASE_NONEXISTENT, &resolved, NULL); + if (r >= 0 && + root_dir && + path_equal_ptr(path_startswith(resolved, 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 && + stat(resolved, &st) >= 0 && + null_or_empty(&st)) + + 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. */ + /* 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) diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index d437686bae..f309160889 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -1245,12 +1245,6 @@ int main(int argc, char *argv[]) { p = strjoina(root, "/usr/lib/systemd/system-preset/"); assert_se(mkdir_p(p, 0755) >= 0); - p = strjoina(root, "/dev/"); - assert_se(mkdir_p(p, 0755) >= 0); - - p = strjoina(root, "/dev/null"); - assert_se(touch(p) >= 0); - test_basic_mask_and_enable(root); test_linked_units(root); test_default(root);