From 451ae5a11adfdd7fa37d05543e97c966a5802924 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Oct 2020 13:49:05 +0200 Subject: [PATCH 1/3] basic/env-util: make function shorter --- src/basic/env-util.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 8b26b36cfe..03a2ad83f1 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -52,10 +52,7 @@ static bool env_name_is_valid_n(const char *e, size_t n) { } bool env_name_is_valid(const char *e) { - if (!e) - return false; - - return env_name_is_valid_n(e, strlen(e)); + return env_name_is_valid_n(e, strlen_ptr(e)); } bool env_value_is_valid(const char *e) { From ff461576de63fd509713618289a698875b1a69ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Oct 2020 14:24:32 +0200 Subject: [PATCH 2/3] Revert "basic/env-util: (mostly) follow POSIX for what variable names are allowed" This reverts commit b45c068dd8fac7661a15e99e7cf699ff06010b13. I think the idea was generally sound, but didn't take into account the limitations of show-environment and how it is used. People expect to be able to eval systemctl show-environment output in bash, and no escaping syntax is defined for environment *names* (we only do escaping for *values*). We could skip such problematic variables in 'systemctl show-environment', and only allow them to be inherited directly. But this would be confusing and ugly. The original motivation for this change was that various import operations would fail. a4ccce22d9552dc74b6916cc5ec57f2a0b686b4f changed systemctl to filter invalid variables in import-environment. https://gitlab.gnome.org/GNOME/gnome-session/-/issues/71 does a similar change in GNOME. So those problematic variables should not cause failures, but just be silently ignored. Finally, the environment block is becoming a dumping ground. In my gnome session 'systemctl show-environment --user' includes stuff like PWD, FPATH (from zsh), SHLVL=0 (no idea what that is). This is not directly related to variable names (since all those are allowed under the stricter rules too), but I think we should start pushing people away from running import-environment and towards importing only select variables. https://github.com/systemd/systemd/pull/17188#issuecomment-708676511 --- src/basic/env-util.c | 17 +++++++---------- src/test/test-env-util.c | 25 ++++++++----------------- src/test/test-load-fragment.c | 3 +-- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 03a2ad83f1..802dff2485 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -21,21 +21,18 @@ DIGITS LETTERS \ "_" -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) { + const char *p; + 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 @@ -44,8 +41,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 (const char *p = e; p < e + n; p++) - if (!printable_portable_character(*p) || *p == '=') + for (p = e; p < e + n; p++) + if (!strchr(VALID_BASH_ENV_NAME_CHARS, *p)) return false; return true; diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 4fede158cd..7c418209a9 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -274,12 +274,10 @@ 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], "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); + 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); } static void test_env_name_is_valid(void) { @@ -292,11 +290,8 @@ 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_unexpected_but_valid")); + assert_se(!env_name_is_valid("5_starting_with_a_number_is_wrong")); 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) { @@ -325,13 +320,9 @@ 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=")); - /* 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…). */ + /* 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=")); 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 bf74cbe6e1..96be244b6b 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -765,9 +765,8 @@ static void test_config_parse_pass_environ(void) { "PassEnvironment", 0, "'invalid name' 'normal_name' A=1 'special_name$$' \\", &passenv, NULL); assert_se(r >= 0); - assert_se(strv_length(passenv) == 2); + assert_se(strv_length(passenv) == 1); assert_se(streq(passenv[0], "normal_name")); - assert_se(streq(passenv[1], "special_name$$")); } static void test_unit_dump_config_items(void) { From 0dc9fd56a53ef833fdc5b9b81bc958e7c3dd41a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Oct 2020 15:47:33 +0200 Subject: [PATCH 3/3] man: document what variables are allowed --- man/systemctl.xml | 25 ++++++++++++++++--------- man/systemd.exec.xml | 19 ++++++++++++------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 61f8d9c9fe..e67469ee46 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1060,6 +1060,14 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err Environment Commands + systemd supports an environment block that is passed to processes the manager + spawns. The names of the variables can contain ASCII letters, digits, and the underscore + character. Variable names cannot be empty or start with a digit. In variable values, most characters + are allowed, but non-printable characters are currently rejected. The total length of the environment + block is limited to _SC_ARG_MAX value defined by + sysconf3. + + show-environment @@ -1091,8 +1099,9 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err set-environment VARIABLE=VALUE… - Set one or more systemd manager environment variables, - as specified on the command line. + Set one or more systemd manager environment variables, as specified on the command + line. This command will fail if variable names and values do not conform to the rules listed + above. @@ -1113,13 +1122,11 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err - Import all, one or more environment variables set on - the client into the systemd manager environment block. If - no arguments are passed, the entire environment block is - imported. Otherwise, a list of one or more environment - variable names should be passed, whose client-side values - are then imported into the manager's environment - block. + Import all, one or more environment variables set on the client into the systemd manager + environment block. If no arguments are passed, the entire environment block is imported. + Otherwise, a list of one or more environment variable names should be passed, whose client-side + values are then imported into the manager's environment block. This command will silently ignore + any assignments which do not conform to the rules listed above. diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 9da919c379..454dd66199 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -2186,13 +2186,18 @@ SystemCallErrorNumber=EPERM Environment= - Sets environment variables for executed processes. Takes a space-separated list of variable - assignments. This option may be specified more than once, in which case all listed variables will be set. If - the same variable is set twice, the later setting will override the earlier setting. If the empty string is - assigned to this option, the list of environment variables is reset, all prior assignments have no - effect. Variable expansion is not performed inside the strings, however, specifier expansion is possible. The $ - character has no special meaning. If you need to assign a value containing spaces or the equals sign to a - variable, use double quotes (") for the assignment. + Sets environment variables for executed processes. Takes a space-separated list of + variable assignments. This option may be specified more than once, in which case all listed variables + will be set. If the same variable is set twice, the later setting will override the earlier + setting. If the empty string is assigned to this option, the list of environment variables is reset, + all prior assignments have no effect. Variable expansion is not performed inside the strings, + however, specifier expansion is possible. The $ character has no special + meaning. If you need to assign a value containing spaces or the equals sign to a variable, use double + quotes (") for the assignment. + + The names of the variables can contain ASCII letters, digits, and the underscore + character. Variable names cannot be empty or start with a digit. In variable values, most characters + are allowed, but non-printable characters are currently rejected. Example: Environment="VAR1=word1 word2" VAR2=word3 "VAR3=$word 5 6"