From f331434d13488425ccd8485327085d15f8f92aea Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Jun 2023 17:20:49 +0200 Subject: [PATCH] env-file: when resolving env vars in command lines, collect list of unset/invalid ones When resolving environment variables we currently silently resolve unset and invalid environment variables to empty strings. Let's do this slightly less silently: log about unset and invalid env vars, but still resolve them to an empty string. Fixes: #27036 --- src/basic/env-file.c | 13 +- src/basic/env-util.c | 248 ++++++++++++++++++++++++++++++--------- src/basic/env-util.h | 8 +- src/core/execute.c | 18 ++- src/run/run.c | 23 +++- src/test/test-env-util.c | 67 +++++++++-- 6 files changed, 290 insertions(+), 87 deletions(-) diff --git a/src/basic/env-file.c b/src/basic/env-file.c index eb5e6404949..5fe045018db 100644 --- a/src/basic/env-file.c +++ b/src/basic/env-file.c @@ -525,6 +525,7 @@ static int merge_env_file_push( char ***env = ASSERT_PTR(userdata); char *expanded_value; + int r; assert(key); @@ -539,12 +540,12 @@ static int merge_env_file_push( return 0; } - expanded_value = replace_env(value, *env, - REPLACE_ENV_USE_ENVIRONMENT| - REPLACE_ENV_ALLOW_BRACELESS| - REPLACE_ENV_ALLOW_EXTENDED); - if (!expanded_value) - return -ENOMEM; + r = replace_env(value, + *env, + REPLACE_ENV_USE_ENVIRONMENT|REPLACE_ENV_ALLOW_BRACELESS|REPLACE_ENV_ALLOW_EXTENDED, + &expanded_value); + if (r < 0) + return log_error_errno(r, "%s:%u: Failed to expand variable '%s': %m", strna(filename), line, value); free_and_replace(value, expanded_value); diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 701c9f9dd79..44772f778de 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -574,7 +574,61 @@ char **strv_env_clean_with_callback(char **e, void (*invalid_callback)(const cha return e; } -char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags flags) { +static int strv_extend_with_length(char ***l, const char *s, size_t n) { + char *c; + + c = strndup(s, n); + if (!c) + return -ENOMEM; + + return strv_consume(l, c); +} + +static int strv_env_get_n_validated( + char **env, + const char *name, + size_t l, + ReplaceEnvFlags flags, + char **ret, /* points into the env block! do not free! */ + char ***unset_variables, /* updated in place */ + char ***bad_variables) { /* ditto */ + + char *e; + int r; + + assert(l == 0 || name); + assert(ret); + + if (env_name_is_valid_n(name, l)) { + e = strv_env_get_n(env, name, l, flags); + if (!e && unset_variables) { + r = strv_extend_with_length(unset_variables, name, l); + if (r < 0) + return r; + } + } else { + e = NULL; /* Resolve invalid variable names the same way as unset ones */ + + if (bad_variables) { + r = strv_extend_with_length(bad_variables, name, l); + if (r < 0) + return r; + } + } + + *ret = e; + return !!e; +} + +int replace_env_full( + const char *format, + size_t n, + char **env, + ReplaceEnvFlags flags, + char **ret, + char ***ret_unset_variables, + char ***ret_bad_variables) { + enum { WORD, CURLY, @@ -585,14 +639,21 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl ALTERNATE_VALUE, } state = WORD; + _cleanup_strv_free_ char **unset_variables = NULL, **bad_variables = NULL; const char *e, *word = format, *test_value = NULL; /* test_value is initialized to appease gcc */ - char *k; _cleanup_free_ char *s = NULL; + char ***pu, ***pb, *k; size_t i, len = 0; /* len is initialized to appease gcc */ - int nest = 0; + int nest = 0, r; assert(format); + if (n == SIZE_MAX) + n = strlen(format); + + pu = ret_unset_variables ? &unset_variables : NULL; + pb = ret_bad_variables ? &bad_variables : NULL; + for (e = format, i = 0; *e && i < n; e ++, i ++) switch (state) { @@ -605,27 +666,28 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl if (*e == '{') { k = strnappend(s, word, e-word-1); if (!k) - return NULL; + return -ENOMEM; free_and_replace(s, k); word = e-1; state = VARIABLE; nest++; + } else if (*e == '$') { k = strnappend(s, word, e-word); if (!k) - return NULL; + return -ENOMEM; free_and_replace(s, k); word = e+1; state = WORD; - } else if (flags & REPLACE_ENV_ALLOW_BRACELESS && strchr(VALID_BASH_ENV_NAME_CHARS, *e)) { + } else if (FLAGS_SET(flags, REPLACE_ENV_ALLOW_BRACELESS) && strchr(VALID_BASH_ENV_NAME_CHARS, *e)) { k = strnappend(s, word, e-word-1); if (!k) - return NULL; + return -ENOMEM; free_and_replace(s, k); @@ -638,12 +700,14 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl case VARIABLE: if (*e == '}') { - const char *t; + char *t; - t = strv_env_get_n(env, word+2, e-word-2, flags); + r = strv_env_get_n_validated(env, word+2, e-word-2, flags, &t, pu, pb); + if (r < 0) + return r; if (!strextend(&s, t)) - return NULL; + return -ENOMEM; word = e+1; state = WORD; @@ -685,18 +749,37 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl nest--; if (nest == 0) { - const char *t; + _cleanup_strv_free_ char **u = NULL, **b = NULL; _cleanup_free_ char *v = NULL; + char *t = NULL; + + r = strv_env_get_n_validated(env, word+2, len, flags, &t, pu, pb); + if (r < 0) + return r; - t = strv_env_get_n(env, word+2, len, flags); + if (t && state == ALTERNATE_VALUE) { + r = replace_env_full(test_value, e-test_value, env, flags, &v, pu ? &u : NULL, pb ? &b : NULL); + if (r < 0) + return r; - if (t && state == ALTERNATE_VALUE) - t = v = replace_env_n(test_value, e-test_value, env, flags); - else if (!t && state == DEFAULT_VALUE) - t = v = replace_env_n(test_value, e-test_value, env, flags); + t = v; + } else if (!t && state == DEFAULT_VALUE) { + r = replace_env_full(test_value, e-test_value, env, flags, &v, pu ? &u : NULL, pb ? &b : NULL); + if (r < 0) + return r; + + t = v; + } + + r = strv_extend_strv(&unset_variables, u, /* filter_duplicates= */ true); + if (r < 0) + return r; + r = strv_extend_strv(&bad_variables, b, /* filter_duplicates= */ true); + if (r < 0) + return r; if (!strextend(&s, t)) - return NULL; + return -ENOMEM; word = e+1; state = WORD; @@ -707,12 +790,14 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl assert(flags & REPLACE_ENV_ALLOW_BRACELESS); if (!strchr(VALID_BASH_ENV_NAME_CHARS, *e)) { - const char *t; + char *t = NULL; - t = strv_env_get_n(env, word+1, e-word-1, flags); + r = strv_env_get_n_validated(env, word+1, e-word-1, flags, &t, &unset_variables, &bad_variables); + if (r < 0) + return r; if (!strextend(&s, t)) - return NULL; + return -ENOMEM; word = e--; i--; @@ -722,58 +807,83 @@ char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags fl } if (state == VARIABLE_RAW) { - const char *t; + char *t; assert(flags & REPLACE_ENV_ALLOW_BRACELESS); - t = strv_env_get_n(env, word+1, e-word-1, flags); - return strjoin(s, t); - } else - return strnappend(s, word, e-word); + r = strv_env_get_n_validated(env, word+1, e-word-1, flags, &t, &unset_variables, &bad_variables); + if (r < 0) + return r; + + if (!strextend(&s, t)) + return -ENOMEM; + + } else if (!strextendn(&s, word, e-word)) + return -ENOMEM; + + if (ret_unset_variables) + *ret_unset_variables = TAKE_PTR(unset_variables); + if (ret_bad_variables) + *ret_bad_variables = TAKE_PTR(bad_variables); + + if (ret) + *ret = TAKE_PTR(s); + + return 0; } -char **replace_env_argv(char **argv, char **env) { - _cleanup_strv_free_ char **ret = NULL; +int replace_env_argv( + char **argv, + char **env, + char ***ret, + char ***ret_unset_variables, + char ***ret_bad_variables) { + + _cleanup_strv_free_ char **n = NULL, **unset_variables = NULL, **bad_variables = NULL; size_t k = 0, l = 0; + int r; l = strv_length(argv); - ret = new(char*, l+1); - if (!ret) - return NULL; + n = new(char*, l+1); + if (!n) + return -ENOMEM; STRV_FOREACH(i, argv) { + const char *word = *i; /* If $FOO appears as single word, replace it by the split up variable */ - if ((*i)[0] == '$' && !IN_SET((*i)[1], '{', '$')) { - char *e; - char **w; + if (word[0] == '$' && !IN_SET(word[1], '{', '$')) { _cleanup_strv_free_ char **m = NULL; + const char *name = word + 1; + char *e, **w; size_t q; - e = strv_env_get(env, *i+1); - if (e) { - int r; - - r = strv_split_full(&m, e, WHITESPACE, EXTRACT_RELAX|EXTRACT_UNQUOTE); - if (r < 0) { - ret[k] = NULL; - return NULL; - } - } + if (env_name_is_valid(name)) { + e = strv_env_get(env, name); + if (e) + r = strv_split_full(&m, e, WHITESPACE, EXTRACT_RELAX|EXTRACT_UNQUOTE); + else if (ret_unset_variables) + r = strv_extend(&unset_variables, name); + else + r = 0; + } else if (ret_bad_variables) + r = strv_extend(&bad_variables, name); + else + r = 0; + if (r < 0) + return r; q = strv_length(m); l = l + q - 1; - w = reallocarray(ret, l + 1, sizeof(char *)); - if (!w) { - ret[k] = NULL; - return NULL; - } + w = reallocarray(n, l + 1, sizeof(char*)); + if (!w) + return -ENOMEM; - ret = w; + n = w; if (m) { - memcpy(ret + k, m, q * sizeof(char*)); + memcpy(n + k, m, (q + 1) * sizeof(char*)); m = mfree(m); } @@ -781,15 +891,41 @@ char **replace_env_argv(char **argv, char **env) { continue; } + _cleanup_strv_free_ char **u = NULL, **b = NULL; + /* If ${FOO} appears as part of a word, replace it by the variable as-is */ - ret[k] = replace_env(*i, env, 0); - if (!ret[k]) - return NULL; - k++; + r = replace_env_full( + word, + /* length= */ SIZE_MAX, + env, + /* flags= */ 0, + n + k, + ret_unset_variables ? &u : NULL, + ret_bad_variables ? &b : NULL); + if (r < 0) + return r; + n[++k] = NULL; + + r = strv_extend_strv(&unset_variables, u, /* filter_duplicates= */ true); + if (r < 0) + return r; + + r = strv_extend_strv(&bad_variables, b, /*filter_duplicates= */ true); + if (r < 0) + return r; } - ret[k] = NULL; - return TAKE_PTR(ret); + if (ret_unset_variables) { + strv_uniq(strv_sort(unset_variables)); + *ret_unset_variables = TAKE_PTR(unset_variables); + } + if (ret_bad_variables) { + strv_uniq(strv_sort(bad_variables)); + *ret_bad_variables = TAKE_PTR(bad_variables); + } + + *ret = TAKE_PTR(n); + return 0; } int getenv_bool(const char *p) { diff --git a/src/basic/env-util.h b/src/basic/env-util.h index c3fc244798e..ec3ac199efe 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -25,12 +25,12 @@ typedef enum ReplaceEnvFlags { REPLACE_ENV_ALLOW_EXTENDED = 1 << 2, } ReplaceEnvFlags; -char *replace_env_n(const char *format, size_t n, char **env, ReplaceEnvFlags flags); -static inline char *replace_env(const char *format, char **env, ReplaceEnvFlags flags) { - return replace_env_n(format, strlen(format), env, flags); +int replace_env_full(const char *format, size_t n, char **env, ReplaceEnvFlags flags, char **ret, char ***ret_unset_variables, char ***ret_bad_variables); +static inline int replace_env(const char *format, char **env, ReplaceEnvFlags flags, char **ret) { + return replace_env_full(format, SIZE_MAX, env, flags, ret, NULL, NULL); } -char **replace_env_argv(char **argv, char **env); +int replace_env_argv(char **argv, char **env, char ***ret, char ***ret_unset_variables, char ***ret_bad_variables); bool strv_env_is_valid(char **e); #define strv_env_clean(l) strv_env_clean_with_callback(l, NULL, NULL) diff --git a/src/core/execute.c b/src/core/execute.c index 9c0a2f4e8f7..90b19b9e9b3 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -5869,12 +5869,24 @@ static int exec_child( } if (!FLAGS_SET(command->flags, EXEC_COMMAND_NO_ENV_EXPAND)) { - replaced_argv = replace_env_argv(command->argv, accum_env); - if (!replaced_argv) { + _cleanup_strv_free_ char **unset_variables = NULL, **bad_variables = NULL; + + r = replace_env_argv(command->argv, accum_env, &replaced_argv, &unset_variables, &bad_variables); + if (r < 0) { *exit_status = EXIT_MEMORY; - return log_oom(); + return log_unit_error_errno(unit, r, "Failed to replace environment variables: %m"); } final_argv = replaced_argv; + + if (!strv_isempty(unset_variables)) { + _cleanup_free_ char *ju = strv_join(unset_variables, ", "); + log_unit_warning(unit, "Referenced but unset environment variable evaluates to an empty string: %s", strna(ju)); + } + + if (!strv_isempty(bad_variables)) { + _cleanup_free_ char *jb = strv_join(bad_variables, ", "); + log_unit_warning(unit, "Invalid environment variable name evaluates to an empty string: %s", strna(jb));; + } } else final_argv = command->argv; diff --git a/src/run/run.c b/src/run/run.c index 7dcd37cedd2..7880a0ecc77 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1472,7 +1472,7 @@ static int start_transient_scope(sd_bus *bus) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; - _cleanup_strv_free_ char **env = NULL, **user_env = NULL, **expanded_cmdline = NULL; + _cleanup_strv_free_ char **env = NULL, **user_env = NULL; _cleanup_free_ char *scope = NULL; const char *object = NULL; sd_id128_t invocation_id; @@ -1615,10 +1615,23 @@ static int start_transient_scope(sd_bus *bus) { log_info("Running scope as unit: %s", scope); if (arg_expand_environment) { - expanded_cmdline = replace_env_argv(arg_cmdline, env); - if (!expanded_cmdline) - return log_oom(); - arg_cmdline = expanded_cmdline; + _cleanup_strv_free_ char **expanded_cmdline = NULL, **unset_variables = NULL, **bad_variables = NULL; + + r = replace_env_argv(arg_cmdline, env, &expanded_cmdline, &unset_variables, &bad_variables); + if (r < 0) + return log_error_errno(r, "Failed to expand environment variables: %m"); + + free_and_replace(arg_cmdline, expanded_cmdline); + + if (!strv_isempty(unset_variables)) { + _cleanup_free_ char *ju = strv_join(unset_variables, ", "); + log_warning("Referenced but unset environment variable evaluates to an empty string: %s", strna(ju)); + } + + if (!strv_isempty(bad_variables)) { + _cleanup_free_ char *jb = strv_join(bad_variables, ", "); + log_warning("Invalid environment variable name evaluates to an empty string: %s", strna(jb)); + } } execvpe(arg_cmdline[0], arg_cmdline, env); diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 0f58d2fed04..a6b5f2f347e 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -200,19 +200,19 @@ static void test_replace_env1(bool braceless) { _cleanup_free_ char *t = NULL, *s = NULL, *q = NULL, *r = NULL, *p = NULL; unsigned flags = REPLACE_ENV_ALLOW_BRACELESS*braceless; - t = replace_env("FOO=$FOO=${FOO}", (char**) env, flags); + assert_se(replace_env("FOO=$FOO=${FOO}", (char**) env, flags, &t) >= 0); assert_se(streq(t, braceless ? "FOO=BAR BAR=BAR BAR" : "FOO=$FOO=BAR BAR")); - s = replace_env("BAR=$BAR=${BAR}", (char**) env, flags); + assert_se(replace_env("BAR=$BAR=${BAR}", (char**) env, flags, &s) >= 0); assert_se(streq(s, braceless ? "BAR=waldo=waldo" : "BAR=$BAR=waldo")); - q = replace_env("BARBAR=$BARBAR=${BARBAR}", (char**) env, flags); + assert_se(replace_env("BARBAR=$BARBAR=${BARBAR}", (char**) env, flags, &q) >= 0); assert_se(streq(q, braceless ? "BARBAR==" : "BARBAR=$BARBAR=")); - r = replace_env("BAR=$BAR$BAR${BAR}${BAR}", (char**) env, flags); + assert_se(replace_env("BAR=$BAR$BAR${BAR}${BAR}", (char**) env, flags, &r) >= 0); assert_se(streq(r, braceless ? "BAR=waldowaldowaldowaldo" : "BAR=$BAR$BARwaldowaldo")); - p = replace_env("${BAR}$BAR$BAR", (char**) env, flags); + assert_se(replace_env("${BAR}$BAR$BAR", (char**) env, flags, &p) >= 0); assert_se(streq(p, braceless ? "waldowaldowaldo" : "waldo$BAR$BAR")); } @@ -227,25 +227,25 @@ static void test_replace_env2(bool extended) { _cleanup_free_ char *t = NULL, *s = NULL, *q = NULL, *r = NULL, *p = NULL, *x = NULL, *y = NULL; unsigned flags = REPLACE_ENV_ALLOW_EXTENDED*extended; - t = replace_env("FOO=${FOO:-${BAR}}", (char**) env, flags); + assert_se(replace_env("FOO=${FOO:-${BAR}}", (char**) env, flags, &t) >= 0); assert_se(streq(t, extended ? "FOO=foo" : "FOO=${FOO:-bar}")); - s = replace_env("BAR=${XXX:-${BAR}}", (char**) env, flags); + assert_se(replace_env("BAR=${XXX:-${BAR}}", (char**) env, flags, &s) >= 0); assert_se(streq(s, extended ? "BAR=bar" : "BAR=${XXX:-bar}")); - q = replace_env("XXX=${XXX:+${BAR}}", (char**) env, flags); + assert_se(replace_env("XXX=${XXX:+${BAR}}", (char**) env, flags, &q) >= 0); assert_se(streq(q, extended ? "XXX=" : "XXX=${XXX:+bar}")); - r = replace_env("FOO=${FOO:+${BAR}}", (char**) env, flags); + assert_se(replace_env("FOO=${FOO:+${BAR}}", (char**) env, flags, &r) >= 0); assert_se(streq(r, extended ? "FOO=bar" : "FOO=${FOO:+bar}")); - p = replace_env("FOO=${FOO:-${BAR}post}", (char**) env, flags); + assert_se(replace_env("FOO=${FOO:-${BAR}post}", (char**) env, flags, &p) >= 0); assert_se(streq(p, extended ? "FOO=foo" : "FOO=${FOO:-barpost}")); - x = replace_env("XXX=${XXX:+${BAR}post}", (char**) env, flags); + assert_se(replace_env("XXX=${XXX:+${BAR}post}", (char**) env, flags, &x) >= 0); assert_se(streq(x, extended ? "XXX=" : "XXX=${XXX:+barpost}")); - y = replace_env("FOO=${FOO}between${BAR:-baz}", (char**) env, flags); + assert_se(replace_env("FOO=${FOO}between${BAR:-baz}", (char**) env, flags, &y) >= 0); assert_se(streq(y, extended ? "FOO=foobetweenbar" : "FOO=foobetween${BAR:-baz}")); } @@ -284,7 +284,7 @@ TEST(replace_env_argv) { }; _cleanup_strv_free_ char **r = NULL; - r = replace_env_argv((char**) line, (char**) env); + assert_se(replace_env_argv((char**) line, (char**) env, &r, NULL, NULL) >= 0); assert_se(r); assert_se(streq(r[0], "FOO$FOO")); assert_se(streq(r[1], "FOO$FOOFOO")); @@ -306,6 +306,47 @@ TEST(replace_env_argv) { assert_se(strv_length(r) == 17); } +TEST(replace_env_argv_bad) { + + const char *env[] = { + "FOO=BAR BAR", + "BAR=waldo", + NULL + }; + + const char *line[] = { + "$FOO", + "A${FOO}B", + "a${~}${%}b", + "x${}y", + "$UNSET2", + "z${UNSET3}z${UNSET1}z", + "piff${UNSET2}piff", + NULL + }; + + _cleanup_strv_free_ char **bad = NULL, **unset = NULL, **replaced = NULL; + + assert_se(replace_env_argv((char**) line, (char**) env, &replaced, &unset, &bad) >= 0); + + assert_se(strv_equal(replaced, STRV_MAKE( + "BAR", + "BAR", + "ABAR BARB", + "ab", + "xy", + "zzz", + "piffpiff"))); + + assert_se(strv_equal(unset, STRV_MAKE( + "UNSET1", + "UNSET2", + "UNSET3"))); + assert_se(strv_equal(bad, STRV_MAKE("", + "%", + "~"))); +} + TEST(env_clean) { _cleanup_strv_free_ char **e = strv_new("FOOBAR=WALDO", "FOOBAR=WALDO",