From 4bbcde8498eb59557ebddd7830efb47c0297ff4b Mon Sep 17 00:00:00 2001 From: Yonathan Randolph Date: Sun, 26 Dec 2021 17:12:00 -0800 Subject: [PATCH 1/4] man: clarify Environmentfile format Remove incorrect claim that C escapes (such as \t and \n) are recognized and that control characters are disallowed. Specify the allowed characters and escapes with single quotes, with double quotes, and without quotes. --- man/systemd.exec.xml | 49 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 36a884c9f4..2c00c08aff 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -2513,18 +2513,39 @@ SystemCallErrorNumber=EPERM EnvironmentFile= - Similar to Environment= but reads the environment variables from a text - file. The text file should contain new-line-separated variable assignments. Empty lines, lines without an - = separator, or lines starting with ; or # will be ignored, which may be used for - commenting. A line ending with a backslash will be concatenated with the following one, allowing multiline - variable definitions. The parser strips leading and trailing whitespace from the values of assignments, unless - you use double quotes ("). + Similar to Environment= but reads the environment variables from a text file. + The text file should contain newline-separated variable assignments. Empty lines, lines without an + = separator, or lines starting with ; or # will be + ignored, which may be used for commenting. The file must be UTF-8 encoded. Valid characters are unicode scalar values other than noncharacters, U+0000 NUL, and U+FEFF byte order mark. Control codes other than NUL + are allowed. - C escapes - are supported, but not - most control characters. - \t and \n can be used to insert tabs and newlines within - EnvironmentFile=. + In the file, an unquoted value after the = is parsed with the same backslash-escape + rules as unquoted + text in a POSIX shell, but unlike in a shell, interior whitespace is preserved and quotes after the + first non-whitespace character are preserved. Leading and trailing whitespace (space, tab, carriage return) is + discarded, but interior whitespace within the line is preserved verbatim. A line ending with a backslash will be + continued to the following one, with the newline itself discarded. A backslash + \ followed by any character other than newline will preserve the following character, so that + \\ will become the value \. + + In the file, a '-quoted value after the = can span multiple lines + and contain any character verbatim other than single quote, like single-quoted + text in a POSIX shell. No backslash-escape sequences are recognized. Leading and trailing whitespace + outside of the single quotes is discarded. + + In the file, a "-quoted value after the = can span multiple lines, + and the same escape sequences are recognized as in double-quoted + text of a POSIX shell. Backslash (\) followed by any of "\`$ will + preserve that character. A backslash followed by newline is a line continuation, and the newline itself is + discarded. A backslash followed by any other character is ignored; both the backslash and the following + character are preserved verbatim. Leading and trailing whitespace outside of the double quotes is + discarded. The argument passed should be an absolute filename or wildcard expression, optionally prefixed with -, which indicates that if the file does not exist, it will not be read and no error or @@ -2557,12 +2578,6 @@ SystemCallErrorNumber=EPERM Variables set for invoked processes due to this setting are subject to being overridden by those configured with Environment= or EnvironmentFile=. - C escapes - are supported, but not - most control characters. - \t and \n can be used to insert tabs and newlines within - EnvironmentFile=. - Example: PassEnvironment=VAR1 VAR2 VAR3 passes three variables VAR1, From 3ef86964ed151aa2464701eef46d665adfd70895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Jan 2022 09:39:55 +0100 Subject: [PATCH 2/4] test-load-fragment: add a basic test for config_parse_unit_env_file() --- src/test/test-load-fragment.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index e878979a89..68172be43e 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -769,6 +769,70 @@ TEST(config_parse_pass_environ) { assert_se(streq(passenv[0], "normal_name")); } +TEST(config_parse_unit_env_file) { + /* int config_parse_unit_env_file( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) */ + + _cleanup_(manager_freep) Manager *m = NULL; + Unit *u; + _cleanup_strv_free_ char **files = NULL; + int r; + + r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m); + if (manager_errno_skip_test(r)) { + log_notice_errno(r, "Skipping test: manager_new: %m"); + return; + } + + assert_se(r >= 0); + assert_se(manager_startup(m, NULL, NULL, NULL) >= 0); + + assert_se(u = unit_new(m, sizeof(Service))); + assert_se(unit_add_name(u, "foobar.service") == 0); + + r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1, + "EnvironmentFile", 0, "not-absolute", + &files, u); + assert_se(r == 0); + assert_se(strv_length(files) == 0); + + r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1, + "EnvironmentFile", 0, "/absolute1", + &files, u); + assert_se(r == 0); + assert_se(strv_length(files) == 1); + + r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1, + "EnvironmentFile", 0, "/absolute2", + &files, u); + assert_se(r == 0); + assert_se(strv_length(files) == 2); + assert_se(streq(files[0], "/absolute1")); + assert_se(streq(files[1], "/absolute2")); + + r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1, + "EnvironmentFile", 0, "", + &files, u); + assert_se(r == 0); + assert_se(strv_isempty(files)); + + r = config_parse_unit_env_file(u->id, "fake", 1, "section", 1, + "EnvironmentFile", 0, "/path/%n.conf", + &files, u); + assert_se(r == 0); + assert_se(strv_length(files) == 1); + assert_se(streq(files[0], "/path/foobar.service.conf")); +} + TEST(unit_dump_config_items) { unit_dump_config_items(stdout); } From 398a5009169fdc0c4eb147692c0cd929b9fe4c84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Jan 2022 09:51:19 +0100 Subject: [PATCH 3/4] core/execute: use _cleanup_ in exec_context_load_environment() Also rename variables. --- src/core/execute.c | 51 +++++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 78c8d966df..acc59ef9db 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -5501,20 +5501,18 @@ static int exec_context_named_iofds( return targets == 0 ? 0 : -ENOENT; } -static int exec_context_load_environment(const Unit *unit, const ExecContext *c, char ***l) { - char **i, **r = NULL; +static int exec_context_load_environment(const Unit *unit, const ExecContext *c, char ***ret) { + _cleanup_strv_free_ char **v = NULL; + char **i; + int r; assert(c); - assert(l); + assert(ret); STRV_FOREACH(i, c->environment_files) { - char *fn; - int k; - bool ignore = false; - char **p; _cleanup_globfree_ glob_t pglob = {}; - - fn = *i; + bool ignore = false; + char *fn = *i; if (fn[0] == '-') { ignore = true; @@ -5524,33 +5522,30 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, if (!path_is_absolute(fn)) { if (ignore) continue; - - strv_free(r); return -EINVAL; } /* Filename supports globbing, take all matching files */ - k = safe_glob(fn, 0, &pglob); - if (k < 0) { + r = safe_glob(fn, 0, &pglob); + if (r < 0) { if (ignore) continue; - - strv_free(r); - return k; + return r; } /* When we don't match anything, -ENOENT should be returned */ assert(pglob.gl_pathc > 0); for (unsigned n = 0; n < pglob.gl_pathc; n++) { - k = load_env_file(NULL, pglob.gl_pathv[n], &p); - if (k < 0) { + _cleanup_strv_free_ char **p = NULL; + + r = load_env_file(NULL, pglob.gl_pathv[n], &p); + if (r < 0) { if (ignore) continue; - - strv_free(r); - return k; + return r; } + /* Log invalid environment variables with filename */ if (p) { InvalidEnvInfo info = { @@ -5561,23 +5556,19 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, p = strv_env_clean_with_callback(p, invalid_env, &info); } - if (!r) - r = p; + if (!v) + v = TAKE_PTR(p); else { - char **m; - - m = strv_env_merge(r, p); - strv_free(r); - strv_free(p); + char **m = strv_env_merge(v, p); if (!m) return -ENOMEM; - r = m; + strv_free_and_replace(v, m); } } } - *l = r; + *ret = TAKE_PTR(v); return 0; } From e9e982a29d52834375e5fb553cecb45bc905cf91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Jan 2022 10:15:50 +0100 Subject: [PATCH 4/4] test-env-file: add tests for quoting in env files --- src/test/test-env-file.c | 66 +++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/src/test/test-env-file.c b/src/test/test-env-file.c index 6cc2455c1f..7b132447bf 100644 --- a/src/test/test-env-file.c +++ b/src/test/test-env-file.c @@ -13,11 +13,11 @@ "a=a\n" \ "b=b\\\n" \ "c\n" \ - "d=d\\\n" \ - "e\\\n" \ - "f\n" \ + "d= d\\\n" \ + "e \\\n" \ + "f \n" \ "g=g\\ \n" \ - "h=h\n" \ + "h= ąęół\\ śćńźżµ \n" \ "i=i\\" #define env_file_2 \ @@ -26,22 +26,34 @@ #define env_file_3 \ "#SPAMD_ARGS=\"-d --socketpath=/var/lib/bulwark/spamd \\\n" \ "#--nouser-config \\\n" \ - "normal=line" + "normal=line \\\n" \ + ";normal=ignored \\\n" \ + "normal_ignored \\\n" \ + "normal ignored \\\n" -#define env_file_4 \ - "# Generated\n" \ - "\n" \ - "HWMON_MODULES=\"coretemp f71882fg\"\n" \ - "\n" \ - "# For compatibility reasons\n" \ - "\n" \ - "MODULE_0=coretemp\n" \ - "MODULE_1=f71882fg" +#define env_file_4 \ + "# Generated\n" \ + "\n" \ + "HWMON_MODULES=\"coretemp f71882fg\"\n" \ + "\n" \ + "# For compatibility reasons\n" \ + "\n" \ + "MODULE_0=coretemp\n" \ + "MODULE_1=f71882fg" #define env_file_5 \ - "a=\n" \ + "a=\n" \ "b=" +#define env_file_6 \ + "a=\\ \\n \\t \\x \\y \\' \n" \ + "b= \\$' \n" \ + "c= ' \\n\\t\\$\\`\\\\\n" \ + "' \n" \ + "d= \" \\n\\t\\$\\`\\\\\n" \ + "\" \n" + + TEST(load_env_file_1) { _cleanup_strv_free_ char **data = NULL; int r; @@ -57,9 +69,9 @@ TEST(load_env_file_1) { assert_se(r == 0); assert_se(streq(data[0], "a=a")); assert_se(streq(data[1], "b=bc")); - assert_se(streq(data[2], "d=def")); + assert_se(streq(data[2], "d=de f")); assert_se(streq(data[3], "g=g ")); - assert_se(streq(data[4], "h=h")); + assert_se(streq(data[4], "h=ąęół śćńźżµ")); assert_se(streq(data[5], "i=i")); assert_se(data[6] == NULL); } @@ -133,6 +145,26 @@ TEST(load_env_file_5) { assert_se(data[2] == NULL); } +TEST(load_env_file_6) { + _cleanup_strv_free_ char **data = NULL; + int r; + + _cleanup_(unlink_tempfilep) char name[] = "/tmp/test-load-env-file.XXXXXX"; + _cleanup_close_ int fd; + + fd = mkostemp_safe(name); + assert_se(fd >= 0); + assert_se(write(fd, env_file_6, strlen(env_file_6)) == strlen(env_file_6)); + + r = load_env_file(NULL, name, &data); + assert_se(r == 0); + assert_se(streq(data[0], "a= n t x y '")); + assert_se(streq(data[1], "b=$'")); + assert_se(streq(data[2], "c= \\n\\t\\$\\`\\\\\n")); + assert_se(streq(data[3], "d= \\n\\t$`\\\n")); + assert_se(data[4] == NULL); +} + TEST(write_and_load_env_file) { const char *v;