Skip to content

Commit

Permalink
basic/env-util: (mostly) follow POSIX for what variable names are all…
Browse files Browse the repository at this point in the history 
…owed

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 <underscore> ( '_' ) 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 systemd#14878,
https://bugzilla.redhat.com/show_bug.cgi?id=1754395,
https://bugzilla.redhat.com/show_bug.cgi?id=1879216.
  • Loading branch information
@keszybz
keszybz committed Oct 12, 2020
1 parent 0b34564 commit b45c068
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 27 deletions.
24 changes: 14 additions & 10 deletions src/basic/env-util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
28 changes: 19 additions & 9 deletions src/test/test-env-util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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;<mock-chroot>\x07<mock-chroot>\""));
Expand Down
16 changes: 8 additions & 8 deletions src/test/test-load-fragment.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit b45c068

Please sign in to comment.