Merge pull request #17429 from keszybz/revert-17188

Revert the change to allow arbitrary environment variable names

man/systemctl.xml

@@ -1060,6 +1060,14 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err
     <refsect2>
       <title>Environment Commands</title>
 
+      <para><command>systemd</command> 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 <constant>_SC_ARG_MAX</constant> value defined by
+      <citerefentry project='man-pages'><refentrytitle>sysconf</refentrytitle><manvolnum>3</manvolnum></citerefentry>.
+      </para>
+
       <variablelist>
         <varlistentry>
           <term><command>show-environment</command></term>
@@ -1091,8 +1099,9 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err
           <term><command>set-environment <replaceable>VARIABLE=VALUE</replaceable>…</command></term>
 
           <listitem>
-            <para>Set one or more systemd manager environment variables,
-            as specified on the command line.</para>
+            <para>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.</para>
           </listitem>
         </varlistentry>
         <varlistentry>
@@ -1113,13 +1122,11 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err
           </term>
 
           <listitem>
-            <para>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.</para>
+            <para>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.</para>
           </listitem>
         </varlistentry>
       </variablelist>

man/systemd.exec.xml

@@ -2186,13 +2186,18 @@ SystemCallErrorNumber=EPERM</programlisting>
       <varlistentry>
         <term><varname>Environment=</varname></term>
 
-        <listitem><para>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.</para>
+        <listitem><para>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 <literal>$</literal> 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.</para>
+
+        <para>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.</para>
 
         <para>Example:
         <programlisting>Environment="VAR1=word1 word2" VAR2=word3 "VAR3=$word 5 6"</programlisting>

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,18 +41,15 @@ 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;
 }
 
 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) {

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>\""));

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) {