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

diff --git a/src/basic/env-util.c b/src/basic/env-util.c
index 03a2ad83f1cbccf919890d7b023fb8a8b039e654..802dff2485949c8bd365274633ba72357fd66e18 100644 (file)
--- 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 4fede158cd075eb9a403d4cd743052b0894e5285..7c418209a92072b371b6be469e1ae2e455d4f433 100644 (file)
--- 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;<mock-chroot>\x07<mock-chroot>\""));

diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c
index bf74cbe6e1411029728b0a8f193507ae24db4d5b..96be244b6b2a4338c351d55402d1eb7509ef5b31 100644 (file)
--- 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) {