From 0b3456428be9e11d7067e499ede8456717d225af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 28 Sep 2020 14:54:16 +0200 Subject: [PATCH 1/3] test-env-util: print function headers --- src/test/test-env-util.c | 60 +++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index f0ffe89729b..9eed8e8f474 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -10,6 +10,8 @@ #include "util.h" static void test_strv_env_delete(void) { + log_info("/* %s */", __func__); + _cleanup_strv_free_ char **a = NULL, **b = NULL, **c = NULL, **d = NULL; a = strv_new("FOO=BAR", "WALDO=WALDO", "WALDO=", "PIEP", "SCHLUMPF=SMURF"); @@ -30,9 +32,9 @@ static void test_strv_env_delete(void) { } static void test_strv_env_get(void) { - char **l; + log_info("/* %s */", __func__); - l = STRV_MAKE("ONE_OR_TWO=1", "THREE=3", "ONE_OR_TWO=2", "FOUR=4"); + char **l = STRV_MAKE("ONE_OR_TWO=1", "THREE=3", "ONE_OR_TWO=2", "FOUR=4"); assert_se(streq(strv_env_get(l, "ONE_OR_TWO"), "2")); assert_se(streq(strv_env_get(l, "THREE"), "3")); @@ -40,6 +42,8 @@ static void test_strv_env_get(void) { } static void test_strv_env_unset(void) { + log_info("/* %s */", __func__); + _cleanup_strv_free_ char **l = NULL; l = strv_new("PIEP", "SCHLUMPF=SMURFF", "NANANANA=YES"); @@ -53,6 +57,8 @@ static void test_strv_env_unset(void) { } static void test_strv_env_set(void) { + log_info("/* %s */", __func__); + _cleanup_strv_free_ char **l = NULL, **r = NULL; l = strv_new("PIEP", "SCHLUMPF=SMURFF", "NANANANA=YES"); @@ -69,6 +75,8 @@ static void test_strv_env_set(void) { } static void test_strv_env_merge(void) { + log_info("/* %s */", __func__); + _cleanup_strv_free_ char **a = NULL, **b = NULL, **r = NULL; a = strv_new("FOO=BAR", "WALDO=WALDO", "WALDO=", "PIEP", "SCHLUMPF=SMURF"); @@ -97,6 +105,8 @@ static void test_strv_env_merge(void) { } static void test_env_strv_get_n(void) { + log_info("/* %s */", __func__); + const char *_env[] = { "FOO=NO NO NO", "FOO=BAR BAR", @@ -127,6 +137,8 @@ static void test_env_strv_get_n(void) { } static void test_replace_env(bool braceless) { + log_info("/* %s(braceless=%s) */", __func__, yes_no(braceless)); + const char *env[] = { "FOO=BAR BAR", "BAR=waldo", @@ -152,6 +164,8 @@ static void test_replace_env(bool braceless) { } static void test_replace_env2(bool extended) { + log_info("/* %s(extended=%s) */", __func__, yes_no(extended)); + const char *env[] = { "FOO=foo", "BAR=bar", @@ -180,6 +194,8 @@ static void test_replace_env2(bool extended) { } static void test_replace_env_argv(void) { + log_info("/* %s */", __func__); + const char *env[] = { "FOO=BAR BAR", "BAR=waldo", @@ -230,24 +246,24 @@ static void test_replace_env_argv(void) { } static void test_env_clean(void) { - _cleanup_strv_free_ char **e; + log_info("/* %s */", __func__); - e = strv_new("FOOBAR=WALDO", - "FOOBAR=WALDO", - "FOOBAR", - "F", - "X=", - "F=F", - "=", - "=F", - "", - "0000=000", - "äöüß=abcd", - "abcd=äöüß", - "xyz\n=xyz", - "xyz=xyz\n", - "another=one", - "another=final one"); + _cleanup_strv_free_ char **e = strv_new("FOOBAR=WALDO", + "FOOBAR=WALDO", + "FOOBAR", + "F", + "X=", + "F=F", + "=", + "=F", + "", + "0000=000", + "äöüß=abcd", + "abcd=äöüß", + "xyz\n=xyz", + "xyz=xyz\n", + "another=one", + "another=final one"); assert_se(e); assert_se(!strv_env_is_valid(e)); assert_se(strv_env_clean(e) == e); @@ -263,6 +279,8 @@ static void test_env_clean(void) { } static void test_env_name_is_valid(void) { + log_info("/* %s */", __func__); + assert_se(env_name_is_valid("test")); assert_se(!env_name_is_valid(NULL)); @@ -275,6 +293,8 @@ static void test_env_name_is_valid(void) { } static void test_env_value_is_valid(void) { + log_info("/* %s */", __func__); + assert_se(env_value_is_valid("")); assert_se(env_value_is_valid("głąb kapuściany")); assert_se(env_value_is_valid("printf \"\\x1b]0;\\x07\"")); @@ -283,6 +303,8 @@ static void test_env_value_is_valid(void) { } static void test_env_assignment_is_valid(void) { + log_info("/* %s */", __func__); + assert_se(env_assignment_is_valid("a=")); assert_se(env_assignment_is_valid("b=głąb kapuściany")); assert_se(env_assignment_is_valid("c=\\007\\009\\011")); From b45c068dd8fac7661a15e99e7cf699ff06010b13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 28 Sep 2020 16:30:53 +0200 Subject: [PATCH 2/3] basic/env-util: (mostly) follow POSIX for what variable names are allowed There was some confusion about what POSIX says about variable names: names shall not contain the character '='. For values to be portable across systems conforming to POSIX.1-2008, the value shall be composed of characters from the portable character set (except NUL and as indicated below). i.e. it allows almost all ASCII in variable names (without NUL and DEL and '='). OTOH, it says that *utilities* use a smaller set of characters: Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2008 consist solely of uppercase letters, digits, and the ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit. When enforcing variable names in environment blocks, we need to use this first definition, so that we can propagate all valid variables. I think having non-printable characters in variable names is too much, so I took out the whitespace stuff from the first definition. OTOH, when we use *shell syntax*, for example doing variable expansion, it seems enough to support expansion of variables that the shell would allow. Fixes #14878, https://bugzilla.redhat.com/show_bug.cgi?id=1754395, https://bugzilla.redhat.com/show_bug.cgi?id=1879216. --- src/basic/env-util.c | 24 ++++++++++++++---------- src/test/test-env-util.c | 28 +++++++++++++++++++--------- src/test/test-load-fragment.c | 16 ++++++++-------- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 179408c3994..8b26b36cfe5 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -16,22 +16,26 @@ #include "strv.h" #include "utf8.h" -#define VALID_CHARS_ENV_NAME \ +/* We follow bash for the character set. Different shells have different rules. */ +#define VALID_BASH_ENV_NAME_CHARS \ DIGITS LETTERS \ "_" -static bool env_name_is_valid_n(const char *e, size_t n) { - const char *p; +static bool printable_portable_character(char c) { + /* POSIX.1-2008 specifies almost all ASCII characters as "portable". (Only DEL is excluded, and + * additionally NUL and = are not allowed in variable names). We are stricter, and additionally + * reject BEL, BS, HT, CR, LF, VT, FF and SPACE, i.e. all whitespace. */ + return c >= '!' && c <= '~'; +} + +static bool env_name_is_valid_n(const char *e, size_t n) { if (!e) return false; if (n <= 0) return false; - if (e[0] >= '0' && e[0] <= '9') - return false; - /* POSIX says the overall size of the environment block cannot * be > ARG_MAX, an individual assignment hence cannot be * either. Discounting the equal sign and trailing NUL this @@ -40,8 +44,8 @@ static bool env_name_is_valid_n(const char *e, size_t n) { if (n > (size_t) sysconf(_SC_ARG_MAX) - 2) return false; - for (p = e; p < e + n; p++) - if (!strchr(VALID_CHARS_ENV_NAME, *p)) + for (const char *p = e; p < e + n; p++) + if (!printable_portable_character(*p) || *p == '=') return false; return true; @@ -546,7 +550,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { word = e+1; state = WORD; - } else if (flags & REPLACE_ENV_ALLOW_BRACELESS && strchr(VALID_CHARS_ENV_NAME, *e)) { + } else if (flags & REPLACE_ENV_ALLOW_BRACELESS && strchr(VALID_BASH_ENV_NAME_CHARS, *e)) { k = strnappend(r, word, e-word-1); if (!k) return NULL; @@ -636,7 +640,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { case VARIABLE_RAW: assert(flags & REPLACE_ENV_ALLOW_BRACELESS); - if (!strchr(VALID_CHARS_ENV_NAME, *e)) { + if (!strchr(VALID_BASH_ENV_NAME_CHARS, *e)) { const char *t; t = strv_env_get_n(env, word+1, e-word-1, flags); diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 9eed8e8f474..b0af660e3ba 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -263,7 +263,8 @@ static void test_env_clean(void) { "xyz\n=xyz", "xyz=xyz\n", "another=one", - "another=final one"); + "another=final one", + "BASH_FUNC_foo%%=() { echo foo\n}"); assert_se(e); assert_se(!strv_env_is_valid(e)); assert_se(strv_env_clean(e) == e); @@ -272,10 +273,12 @@ static void test_env_clean(void) { assert_se(streq(e[0], "FOOBAR=WALDO")); assert_se(streq(e[1], "X=")); assert_se(streq(e[2], "F=F")); - assert_se(streq(e[3], "abcd=äöüß")); - assert_se(streq(e[4], "xyz=xyz\n")); - assert_se(streq(e[5], "another=final one")); - assert_se(e[6] == NULL); + assert_se(streq(e[3], "0000=000")); + assert_se(streq(e[4], "abcd=äöüß")); + assert_se(streq(e[5], "xyz=xyz\n")); + assert_se(streq(e[6], "another=final one")); + assert_se(streq(e[7], "BASH_FUNC_foo%%=() { echo foo\n}")); + assert_se(e[8] == NULL); } static void test_env_name_is_valid(void) { @@ -288,8 +291,11 @@ static void test_env_name_is_valid(void) { assert_se(!env_name_is_valid("xxx\a")); assert_se(!env_name_is_valid("xxx\007b")); assert_se(!env_name_is_valid("\007\009")); - assert_se(!env_name_is_valid("5_starting_with_a_number_is_wrong")); + assert_se( env_name_is_valid("5_starting_with_a_number_is_unexpected_but_valid")); assert_se(!env_name_is_valid("#¤%&?_only_numbers_letters_and_underscore_allowed")); + assert_se( env_name_is_valid("BASH_FUNC_foo%%")); + assert_se(!env_name_is_valid("with spaces%%")); + assert_se(!env_name_is_valid("with\nnewline%%")); } static void test_env_value_is_valid(void) { @@ -316,9 +322,13 @@ static void test_env_assignment_is_valid(void) { assert_se(!env_assignment_is_valid("a b=")); assert_se(!env_assignment_is_valid("a =")); assert_se(!env_assignment_is_valid(" b=")); - /* no dots or dashes: http://tldp.org/LDP/abs/html/gotchas.html */ - assert_se(!env_assignment_is_valid("a.b=")); - assert_se(!env_assignment_is_valid("a-b=")); + /* Names with dots and dashes makes those variables inaccessible as bash variables (as the syntax + * simply does not allow such variable names, see http://tldp.org/LDP/abs/html/gotchas.html). They + * are still valid variables according to POSIX though. */ + assert_se( env_assignment_is_valid("a.b=")); + assert_se( env_assignment_is_valid("a-b=")); + /* Those are not ASCII, so not valid according to POSIX (though zsh does allow unicode variable + * names…). */ assert_se(!env_assignment_is_valid("\007=głąb kapuściany")); assert_se(!env_assignment_is_valid("c\009=\007\009\011")); assert_se(!env_assignment_is_valid("głąb=printf \"\x1b]0;\x07\"")); diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index 799ac6a51a2..bf74cbe6e14 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -748,26 +748,26 @@ static void test_config_parse_pass_environ(void) { _cleanup_strv_free_ char **passenv = NULL; r = config_parse_pass_environ(NULL, "fake", 1, "section", 1, - "PassEnvironment", 0, "A B", - &passenv, NULL); + "PassEnvironment", 0, "A B", + &passenv, NULL); assert_se(r >= 0); assert_se(strv_length(passenv) == 2); assert_se(streq(passenv[0], "A")); assert_se(streq(passenv[1], "B")); r = config_parse_pass_environ(NULL, "fake", 1, "section", 1, - "PassEnvironment", 0, "", - &passenv, NULL); + "PassEnvironment", 0, "", + &passenv, NULL); assert_se(r >= 0); assert_se(strv_isempty(passenv)); r = config_parse_pass_environ(NULL, "fake", 1, "section", 1, - "PassEnvironment", 0, "'invalid name' 'normal_name' A=1 \\", - &passenv, NULL); + "PassEnvironment", 0, "'invalid name' 'normal_name' A=1 'special_name$$' \\", + &passenv, NULL); assert_se(r >= 0); - assert_se(strv_length(passenv) == 1); + assert_se(strv_length(passenv) == 2); assert_se(streq(passenv[0], "normal_name")); - + assert_se(streq(passenv[1], "special_name$$")); } static void test_unit_dump_config_items(void) { From a4ccce22d9552dc74b6916cc5ec57f2a0b686b4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 28 Sep 2020 17:29:34 +0200 Subject: [PATCH 3/3] systemctl: ignore invalid variables in import-environment When doing import-environment, we shouldn't fail if some assignment is invalid. OTOH, if the invalid assignment is specified as a positional argument, we should keep failing. This would also fix https://bugzilla.redhat.com/show_bug.cgi?id=1754395, by ignoring certain variables which are not important in that scenario. It seems like the right thing to do in general. --- src/systemctl/systemctl-set-environment.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/systemctl/systemctl-set-environment.c b/src/systemctl/systemctl-set-environment.c index 462924f5c9a..ac1ec7d6fea 100644 --- a/src/systemctl/systemctl-set-environment.c +++ b/src/systemctl/systemctl-set-environment.c @@ -61,6 +61,12 @@ int show_environment(int argc, char *argv[], void *userdata) { return 0; } +static void invalid_callback(const char *p, void *userdata) { + _cleanup_free_ char *t = cescape(p); + + log_debug("Ignoring invalid environment assignment \"%s\".", strnull(t)); +} + int set_environment(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; @@ -112,9 +118,18 @@ int import_environment(int argc, char *argv[], void *userdata) { if (r < 0) return bus_log_create_error(r); - if (argc < 2) - r = sd_bus_message_append_strv(m, environ); - else { + if (argc < 2) { + _cleanup_strv_free_ char **copy = NULL; + + copy = strv_copy(environ); + if (!copy) + return log_oom(); + + strv_env_clean_with_callback(copy, invalid_callback, NULL); + + r = sd_bus_message_append_strv(m, copy); + + } else { char **a, **b; r = sd_bus_message_open_container(m, 'a', "s");