From f4284e9cebac67ad7af3bc27b736cf1107b88127 Mon Sep 17 00:00:00 2001 From: dongshengyuan <545258830@qq.com> Date: Tue, 11 Aug 2026 13:53:44 +0800 Subject: [PATCH] path-lookup: reject empty env path components Only trailing empty components have special meaning: they request appending the built-in defaults. Share the validation between normal path lookup and systemd-analyze verify, so verify does not compose values that path lookup later rejects. Propagate generator path parse errors instead of turning them into empty generator paths. Follow-up for: cf7d80a5fe549d4db11800015e02220dccec3096 --- src/analyze/analyze-verify-util.c | 5 +- src/analyze/test-verify.c | 48 +++++++++++++++ src/core/manager.c | 12 ++-- src/libsystemd/sd-path/path-lookup.c | 21 +++++-- src/libsystemd/sd-path/path-lookup.h | 11 ++-- src/libsystemd/sd-path/sd-path.c | 9 +-- src/test/test-path-lookup.c | 89 ++++++++++++++++++++++------ 7 files changed, 156 insertions(+), 39 deletions(-) diff --git a/src/analyze/analyze-verify-util.c b/src/analyze/analyze-verify-util.c index e2adf760a6..ce572ce461 100644 --- a/src/analyze/analyze-verify-util.c +++ b/src/analyze/analyze-verify-util.c @@ -157,8 +157,11 @@ int verify_set_unit_path(char **filenames) { * otherwise use the defaults. Any duplicates will be filtered out in path-lookup.c. * Treat explicit empty path to mean that nothing should be appended. */ old = getenv("SYSTEMD_UNIT_PATH"); + if (!path_is_valid_search_path(old)) + return -EINVAL; + if (!streq_ptr(old, "") && - !strextend_with_separator(&joined, ":", strempty(old))) + !strextend_with_separator(&joined, ":", streq_ptr(old, ":") ? "" : strempty(old))) return -ENOMEM; assert_se(setenv_unit_path(joined) >= 0); diff --git a/src/analyze/test-verify.c b/src/analyze/test-verify.c index 048d866dbe..04a24ba928 100644 --- a/src/analyze/test-verify.c +++ b/src/analyze/test-verify.c @@ -1,9 +1,16 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include + #include "analyze.h" #include "analyze-verify-util.h" #include "execute.h" +#include "fileio.h" +#include "path-util.h" +#include "rm-rf.h" +#include "string-util.h" #include "tests.h" +#include "tmpfile-util.h" const char *arg_instance = "test_instance"; @@ -17,4 +24,45 @@ TEST(verify_nonexistent) { assert_se(verify_executable(NULL, &(ExecCommand) {.flags = EXEC_COMMAND_IGNORE_FAILURE, .path = (char*) "/bin/echo"}, NULL) == 0); } +static void test_verify_set_unit_path_one(const char *old, const char *expected) { + _cleanup_(rm_rf_physical_and_freep) char *tmp = NULL; + _cleanup_free_ char *unit = NULL; + char *unit_paths[2]; + + ASSERT_OK(mkdtemp_malloc("/tmp/test-verify.XXXXXX", &tmp)); + unit = ASSERT_NOT_NULL(path_join(tmp, "test.service")); + ASSERT_OK(write_string_file(unit, "[Unit]\nDescription=test\n", WRITE_STRING_FILE_CREATE)); + + if (old) + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", old, 1)); + else + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); + + unit_paths[0] = unit; + unit_paths[1] = NULL; + + if (expected) { + ASSERT_OK(verify_set_unit_path(unit_paths)); + ASSERT_STREQ(getenv("SYSTEMD_UNIT_PATH"), strjoina(tmp, expected)); + } else + ASSERT_ERROR(verify_set_unit_path(unit_paths), EINVAL); +} + +TEST(verify_set_unit_path) { + _cleanup_free_ char *old_saved = getenv("SYSTEMD_UNIT_PATH") ? strdup(getenv("SYSTEMD_UNIT_PATH")) : NULL; + ASSERT_TRUE(old_saved || !getenv("SYSTEMD_UNIT_PATH")); + + test_verify_set_unit_path_one(NULL, ":"); + test_verify_set_unit_path_one("", ""); + test_verify_set_unit_path_one(":", ":"); + test_verify_set_unit_path_one("/foo:", ":/foo:"); + test_verify_set_unit_path_one(":foo", NULL); + test_verify_set_unit_path_one("/foo::/bar", NULL); + + if (old_saved) + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", old_saved, 1)); + else + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); +} + DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/core/manager.c b/src/core/manager.c index 8f7c5bbed6..bf4d2333d9 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -4297,9 +4297,9 @@ static int manager_run_environment_generators(Manager *m) { if (MANAGER_IS_TEST_RUN(m) && !(m->test_run_flags & MANAGER_TEST_RUN_ENV_GENERATORS)) return 0; - paths = env_generator_binary_paths(m->runtime_scope); - if (!paths) - return log_oom(); + r = env_generator_binary_paths(m->runtime_scope, &paths); + if (r < 0) + return log_error_errno(r, "Failed to initialize environment generator search paths: %m"); if (!generator_path_any(paths)) return 0; @@ -4450,9 +4450,9 @@ static int manager_run_generators(Manager *m) { if (MANAGER_IS_TEST_RUN(m) && !(m->test_run_flags & MANAGER_TEST_RUN_GENERATORS)) return 0; - paths = generator_binary_paths(m->runtime_scope); - if (!paths) - return log_oom(); + r = generator_binary_paths(m->runtime_scope, &paths); + if (r < 0) + return log_error_errno(r, "Failed to initialize generator search paths: %m"); if (!generator_path_any(paths)) return 0; diff --git a/src/libsystemd/sd-path/path-lookup.c b/src/libsystemd/sd-path/path-lookup.c index ee97164bdc..1a49a6b46b 100644 --- a/src/libsystemd/sd-path/path-lookup.c +++ b/src/libsystemd/sd-path/path-lookup.c @@ -397,6 +397,12 @@ static int patch_root_prefix_strv(char **l, const char *root_dir) { return 0; } +bool path_is_valid_search_path(const char *path) { + return isempty(path) || + (isempty(startswith(path, ":")) && + !strstr(path, "::")); +} + static int get_paths_from_environ(const char *var, char ***ret) { const char *e; int r; @@ -410,11 +416,12 @@ static int get_paths_from_environ(const char *var, char ***ret) { return 0; } + if (!path_is_valid_search_path(e)) + return -EINVAL; + bool append = endswith(e, ":"); /* Whether to append the normal search paths after what's obtained from envvar */ - /* FIXME: empty components in other places should be rejected. */ - r = path_split_and_make_absolute(e, ret); if (r < 0) return r; @@ -796,7 +803,7 @@ static const char* const user_env_generator_paths[] = { NULL, }; -char** generator_binary_paths_internal(RuntimeScope scope, bool env_generator) { +int generator_binary_paths_internal(RuntimeScope scope, bool env_generator, char ***ret) { static const struct { const char *env_name; const char * const *paths[_RUNTIME_SCOPE_MAX]; @@ -818,6 +825,7 @@ char** generator_binary_paths_internal(RuntimeScope scope, bool env_generator) { int r; assert(IN_SET(scope, RUNTIME_SCOPE_SYSTEM, RUNTIME_SCOPE_USER)); + assert(ret); const char *env_name = ASSERT_PTR((env_generator ? environment_generator : unit_generator).env_name); const char * const *generator_paths = ASSERT_PTR((env_generator ? environment_generator : unit_generator).paths[scope]); @@ -825,13 +833,14 @@ char** generator_binary_paths_internal(RuntimeScope scope, bool env_generator) { /* First priority is whatever has been passed to us via env vars */ r = get_paths_from_environ(env_name, &paths); if (r < 0) - return NULL; + return r; if (!paths || r > 0) { r = strv_extend_strv(&paths, (char* const*) generator_paths, /* filter_duplicates= */ true); if (r < 0) - return NULL; + return r; } - return TAKE_PTR(paths); + *ret = TAKE_PTR(paths); + return 0; } diff --git a/src/libsystemd/sd-path/path-lookup.h b/src/libsystemd/sd-path/path-lookup.h index f2caa45170..4590f06ff8 100644 --- a/src/libsystemd/sd-path/path-lookup.h +++ b/src/libsystemd/sd-path/path-lookup.h @@ -83,13 +83,14 @@ static inline int xdg_user_state_dir(const char *suffix, char **ret) { bool path_is_user_data_dir(const char *path); bool path_is_user_config_dir(const char *path); +bool path_is_valid_search_path(const char *path) _pure_; -char** generator_binary_paths_internal(RuntimeScope scope, bool env_generator); -static inline char** generator_binary_paths(RuntimeScope runtime_scope) { - return generator_binary_paths_internal(runtime_scope, false); +int generator_binary_paths_internal(RuntimeScope scope, bool env_generator, char ***ret); +static inline int generator_binary_paths(RuntimeScope runtime_scope, char ***ret) { + return generator_binary_paths_internal(runtime_scope, false, ret); } -static inline char** env_generator_binary_paths(RuntimeScope runtime_scope) { - return generator_binary_paths_internal(runtime_scope, true); +static inline int env_generator_binary_paths(RuntimeScope runtime_scope, char ***ret) { + return generator_binary_paths_internal(runtime_scope, true, ret); } static inline int credential_store_path(RuntimeScope runtime_scope, char ***ret) { diff --git a/src/libsystemd/sd-path/sd-path.c b/src/libsystemd/sd-path/sd-path.c index eebcd20b6f..ef1a4ae7ff 100644 --- a/src/libsystemd/sd-path/sd-path.c +++ b/src/libsystemd/sd-path/sd-path.c @@ -631,11 +631,12 @@ static int get_search(uint64_t type, char ***ret) { bool env_generator = IN_SET(type, SD_PATH_SYSTEMD_SEARCH_SYSTEM_ENVIRONMENT_GENERATOR, SD_PATH_SYSTEMD_SEARCH_USER_ENVIRONMENT_GENERATOR); - char **t = generator_binary_paths_internal(scope, env_generator); - if (!t) - return -ENOMEM; + _cleanup_strv_free_ char **t = NULL; + r = generator_binary_paths_internal(scope, env_generator, &t); + if (r < 0) + return r; - *ret = t; + *ret = TAKE_PTR(t); return 0; } diff --git a/src/test/test-path-lookup.c b/src/test/test-path-lookup.c index 67f3317be2..149212292a 100644 --- a/src/test/test-path-lookup.c +++ b/src/test/test-path-lookup.c @@ -18,13 +18,13 @@ static void test_paths_one(RuntimeScope scope) { assert_se(mkdtemp_malloc("/tmp/test-path-lookup.XXXXXXX", &tmp) >= 0); - assert_se(unsetenv("SYSTEMD_UNIT_PATH") == 0); + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); assert_se(lookup_paths_init(&lp_without_env, scope, 0, NULL) >= 0); assert_se(!strv_isempty(lp_without_env.search_path)); lookup_paths_log(&lp_without_env); systemd_unit_path = strjoina(tmp, "/systemd-unit-path"); - assert_se(setenv("SYSTEMD_UNIT_PATH", systemd_unit_path, 1) == 0); + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", systemd_unit_path, 1)); assert_se(lookup_paths_init(&lp_with_env, scope, 0, NULL) == 0); assert_se(strv_length(lp_with_env.search_path) == 1); ASSERT_STREQ(lp_with_env.search_path[0], systemd_unit_path); @@ -38,14 +38,59 @@ TEST(paths) { test_paths_one(RUNTIME_SCOPE_GLOBAL); } +static void test_paths_empty_components_one(RuntimeScope scope) { + _cleanup_(rm_rf_physical_and_freep) char *tmp = NULL; + _cleanup_(lookup_paths_done) LookupPaths lp_without_env = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_empty = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_separator = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_leading_empty = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_middle_empty = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_trailing_empty = {}; + char *systemd_unit_path; + + assert_se(mkdtemp_malloc("/tmp/test-path-lookup.XXXXXXX", &tmp) >= 0); + + systemd_unit_path = strjoina(tmp, "/systemd-unit-path"); + + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); + ASSERT_OK(lookup_paths_init(&lp_without_env, scope, 0, NULL)); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", "", 1)); + ASSERT_OK(lookup_paths_init(&lp_with_empty, scope, 0, NULL)); + ASSERT_TRUE(strv_isempty(lp_with_empty.search_path)); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", ":", 1)); + ASSERT_OK(lookup_paths_init(&lp_with_separator, scope, 0, NULL)); + ASSERT_TRUE(strv_equal(lp_with_separator.search_path, lp_without_env.search_path)); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", strjoina(":", systemd_unit_path), 1)); + ASSERT_ERROR(lookup_paths_init(&lp_with_leading_empty, scope, 0, NULL), EINVAL); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", strjoina(systemd_unit_path, "::/run"), 1)); + ASSERT_ERROR(lookup_paths_init(&lp_with_middle_empty, scope, 0, NULL), EINVAL); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_UNIT_PATH", strjoina(systemd_unit_path, ":"), 1)); + ASSERT_OK(lookup_paths_init(&lp_with_trailing_empty, scope, 0, NULL)); + ASSERT_STREQ(lp_with_trailing_empty.search_path[0], systemd_unit_path); + ASSERT_GT(strv_length(lp_with_trailing_empty.search_path), 1u); + + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); +} + +TEST(paths_empty_components) { + test_paths_empty_components_one(RUNTIME_SCOPE_SYSTEM); + test_paths_empty_components_one(RUNTIME_SCOPE_USER); + test_paths_empty_components_one(RUNTIME_SCOPE_GLOBAL); +} + TEST(user_and_global_paths) { _cleanup_(lookup_paths_done) LookupPaths lp_global = {}, lp_user = {}; char **u, **g; unsigned k = 0; - assert_se(unsetenv("SYSTEMD_UNIT_PATH") == 0); - assert_se(unsetenv("XDG_DATA_DIRS") == 0); - assert_se(unsetenv("XDG_CONFIG_DIRS") == 0); + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_UNIT_PATH")); + ASSERT_OK_ERRNO(unsetenv("XDG_DATA_DIRS")); + ASSERT_OK_ERRNO(unsetenv("XDG_CONFIG_DIRS")); assert_se(lookup_paths_init(&lp_global, RUNTIME_SCOPE_GLOBAL, 0, NULL) == 0); assert_se(lookup_paths_init(&lp_user, RUNTIME_SCOPE_USER, 0, NULL) == 0); @@ -75,16 +120,18 @@ static void test_generator_binary_paths_one(RuntimeScope scope) { _cleanup_strv_free_ char **env_gp_without_env = NULL; _cleanup_strv_free_ char **gp_with_env = NULL; _cleanup_strv_free_ char **env_gp_with_env = NULL; + _cleanup_strv_free_ char **gp_with_bad_env = NULL; + _cleanup_strv_free_ char **env_gp_with_bad_env = NULL; char *systemd_generator_path = NULL; char *systemd_env_generator_path = NULL; assert_se(mkdtemp_malloc("/tmp/test-path-lookup.XXXXXXX", &tmp) >= 0); - assert_se(unsetenv("SYSTEMD_GENERATOR_PATH") == 0); - assert_se(unsetenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH") == 0); + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_GENERATOR_PATH")); + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH")); - gp_without_env = generator_binary_paths(scope); - env_gp_without_env = env_generator_binary_paths(scope); + ASSERT_OK(generator_binary_paths(scope, &gp_without_env)); + ASSERT_OK(env_generator_binary_paths(scope, &env_gp_without_env)); log_info("Generators dirs (%s):", runtime_scope_to_string(scope)); STRV_FOREACH(dir, gp_without_env) @@ -94,16 +141,16 @@ static void test_generator_binary_paths_one(RuntimeScope scope) { STRV_FOREACH(dir, env_gp_without_env) log_info(" %s", *dir); - assert_se(!strv_isempty(gp_without_env)); - assert_se(!strv_isempty(env_gp_without_env)); + ASSERT_FALSE(strv_isempty(gp_without_env)); + ASSERT_FALSE(strv_isempty(env_gp_without_env)); systemd_generator_path = strjoina(tmp, "/systemd-generator-path"); systemd_env_generator_path = strjoina(tmp, "/systemd-environment-generator-path"); - assert_se(setenv("SYSTEMD_GENERATOR_PATH", systemd_generator_path, 1) == 0); - assert_se(setenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH", systemd_env_generator_path, 1) == 0); + ASSERT_OK_ERRNO(setenv("SYSTEMD_GENERATOR_PATH", systemd_generator_path, 1)); + ASSERT_OK_ERRNO(setenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH", systemd_env_generator_path, 1)); - gp_with_env = generator_binary_paths(scope); - env_gp_with_env = env_generator_binary_paths(scope); + ASSERT_OK(generator_binary_paths(scope, &gp_with_env)); + ASSERT_OK(env_generator_binary_paths(scope, &env_gp_with_env)); log_info("Generators dirs (%s):", runtime_scope_to_string(scope)); STRV_FOREACH(dir, gp_with_env) @@ -113,8 +160,16 @@ static void test_generator_binary_paths_one(RuntimeScope scope) { STRV_FOREACH(dir, env_gp_with_env) log_info(" %s", *dir); - assert_se(strv_equal(gp_with_env, STRV_MAKE(systemd_generator_path))); - assert_se(strv_equal(env_gp_with_env, STRV_MAKE(systemd_env_generator_path))); + ASSERT_TRUE(strv_equal(gp_with_env, STRV_MAKE(systemd_generator_path))); + ASSERT_TRUE(strv_equal(env_gp_with_env, STRV_MAKE(systemd_env_generator_path))); + + ASSERT_OK_ERRNO(setenv("SYSTEMD_GENERATOR_PATH", strjoina(":", systemd_generator_path), 1)); + ASSERT_OK_ERRNO(setenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH", strjoina(":", systemd_env_generator_path), 1)); + ASSERT_ERROR(generator_binary_paths(scope, &gp_with_bad_env), EINVAL); + ASSERT_ERROR(env_generator_binary_paths(scope, &env_gp_with_bad_env), EINVAL); + + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_GENERATOR_PATH")); + ASSERT_OK_ERRNO(unsetenv("SYSTEMD_ENVIRONMENT_GENERATOR_PATH")); } TEST(generator_binary_paths) {