From 00819cc15124addfa2c0d8a5d13ad3eebd009d60 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 10 Sep 2017 12:16:44 +0200 Subject: [PATCH 1/6] core: add new UnsetEnvironment= setting for unit files With this setting we can explicitly unset specific variables for processes of a unit, as last step of assembling the environment block for them. This is useful to fix #6407. While we are at it, greatly expand the documentation on how the environment block for forked off processes is assembled. --- man/systemd.exec.xml | 94 ++++++++++++++++-------- src/core/dbus-execute.c | 44 +++++++++++- src/core/execute.c | 19 +++++ src/core/execute.h | 1 + src/core/load-fragment-gperf.gperf.m4 | 1 + src/core/load-fragment.c | 100 +++++++++++++++++++++++--- src/core/load-fragment.h | 1 + src/shared/bus-unit-util.c | 7 +- 8 files changed, 227 insertions(+), 40 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 625063f1c0..da1b4dde69 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -423,17 +423,17 @@ PassEnvironment= - Pass environment variables from the systemd system - manager to executed processes. Takes a space-separated list of variable - names. This option may be specified more than once, in which case all - listed variables will be set. If the empty string is assigned to this - option, the list of environment variables is reset, all prior - assignments have no effect. Variables that are not set in the system - manager will not be passed and will be silently ignored. + Pass environment variables set for the system service manager to executed processes. Takes a + space-separated list of variable names. This option may be specified more than once, in which case all listed + variables will be passed. If the empty string is assigned to this option, the list of environment variables to + pass is reset, all prior assignments have no effect. Variables specified that are not set for the system + manager will not be passed and will be silently ignored. Note that this option is only relevant for the system + service manager, as system services by default do not automatically inherit any environment variables set for + the service manager itself. However, in case of the user service manager all environment variables are passed + to the executed processes anyway, hence this option is without effect for the user service manager. - Variables passed from this setting are overridden by those passed - from Environment= or - EnvironmentFile=. + Variables set for invoked processes due to this setting are subject to being overridden by those + configured with Environment= or EnvironmentFile=. Example: PassEnvironment=VAR1 VAR2 VAR3 @@ -447,6 +447,30 @@ for details about environment variables. + + UnsetEnvironment= + + Explicitly unset environment variable assignments that would normally be passed from the + service manager to invoked processes of this unit. Takes a space-separated list of variable names or variable + assignments. This option may be specified more than once, in which case all listed variables/assignments will + be unset. If the empty string is assigned to this option, the list of environment variables/assignments to + unset is reset. If a variable assignment is specified (that is: a variable name, followed by + =, followed by its value), then any environment variable matching this precise assignment is + removed. If a variable name is specified (that is a variable name without any following = or + value), then any assignment matching the variable name, regardless of its value is removed. Note that the + effect of UnsetEnvironment= is applied as final step when the environment list passed to + executed processes is compiled. That means it may undo assignments from any configuration source, including + assignments made through Environment= or EnvironmentFile=, inherited from + the system manager's global set of environment variables, inherited via PassEnvironment=, + set by the service manager itself (such as $NOTIFY_SOCKET and such), or set by a PAM module + (in case PAMName= is used). + + + See + environ7 + for details about environment variables. + + StandardInput= Controls where file descriptor 0 (STDIN) of @@ -1799,12 +1823,38 @@ CapabilityBoundingSet=~CAP_B CAP_C Environment variables in spawned processes - Processes started by the system are executed in a clean - environment in which select variables listed below are set. System - processes started by systemd do not inherit variables from PID 1, - but processes started by user systemd instances inherit all - environment variables from the user systemd instance. - + Processes started by the service manager are executed with an environment variable block assembled from + multiple sources. Processes started by the system service manager generally do not inherit environment variables + set for the service manager itself (but this may be altered via PassEnvironment=), but processes + started by the user service manager instances generally do inherit all environment variables set for the service + manager itself. + + For each invoked process the list of environment variables set is compiled from the following sources: + + + Variables globally configured for the service manager, using the + DefaultEnvironment= setting in + systemd-system.conf5, the kernel command line option systemd.setenv= (see + systemd1) or via + systemctl set-environment (see systemctl1). + + Variables defined by the service manager itself (see the list below) + + Variables set in the service manager's own environment variable block (subject to PassEnvironment= for the system service manager) + + Variables set via Environment= in the unit file + + Variables read from files specified via EnvironmentFiles= in the unit file + + Variables set by any PAM modules in case PAMName= is in effect, cf. pam_env8 + + + If the same environment variables are set by multiple of these sources, the later source — according to the + order of the list above — wins. Note that as final step all variables listed in + UnsetEnvironment= are removed again from the compiled environment variable list, immediately + before it is passed to the executed process. + + The following select environment variables are set by the service manager itself for each invoked process: @@ -2120,18 +2170,6 @@ CapabilityBoundingSet=~CAP_B CAP_C - - Additional variables may be configured by the following - means: for processes spawned in specific units, use the - Environment=, EnvironmentFile= - and PassEnvironment= options above; to specify - variables globally, use DefaultEnvironment= - (see - systemd-system.conf5) - or the kernel option systemd.setenv= (see - systemd1). - Additional variables may also be set through PAM, - cf. pam_env8. diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index eb0af24450..2bcdd33659 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -758,6 +758,7 @@ const sd_bus_vtable bus_exec_vtable[] = { SD_BUS_PROPERTY("Environment", "as", NULL, offsetof(ExecContext, environment), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("EnvironmentFiles", "a(sb)", property_get_environment_files, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("PassEnvironment", "as", NULL, offsetof(ExecContext, pass_environment), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("UnsetEnvironment", "as", NULL, offsetof(ExecContext, unset_environment), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("UMask", "u", bus_property_get_mode, offsetof(ExecContext, umask), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("LimitCPU", "t", bus_property_get_rlimit, offsetof(ExecContext, rlimit[RLIMIT_CPU]), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("LimitCPUSoft", "t", bus_property_get_rlimit, offsetof(ExecContext, rlimit[RLIMIT_CPU]), SD_BUS_VTABLE_PROPERTY_CONST), @@ -1849,6 +1850,47 @@ int bus_exec_context_set_transient_property( return 1; + } else if (streq(name, "UnsetEnvironment")) { + + _cleanup_strv_free_ char **l = NULL, **q = NULL; + + r = sd_bus_message_read_strv(message, &l); + if (r < 0) + return r; + + r = unit_full_printf_strv(u, l, &q); + if (r < 0) + return r; + + if (!strv_env_name_or_assignment_is_valid(q)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid UnsetEnvironment= list."); + + if (mode != UNIT_CHECK) { + if (strv_length(q) == 0) { + c->unset_environment = strv_free(c->unset_environment); + unit_write_drop_in_private_format(u, mode, name, "UnsetEnvironment="); + } else { + _cleanup_free_ char *joined = NULL; + char **e; + + e = strv_env_merge(2, c->unset_environment, q); + if (!e) + return -ENOMEM; + + strv_free(c->unset_environment); + c->unset_environment = e; + + /* We write just the new settings out to file, with unresolved specifiers */ + joined = strv_join_quoted(l); + if (!joined) + return -ENOMEM; + + unit_write_drop_in_private_format(u, mode, name, "UnsetEnvironment=%s", joined); + } + } + + return 1; + } else if (streq(name, "TimerSlackNSec")) { nsec_t n; @@ -1965,7 +2007,7 @@ int bus_exec_context_set_transient_property( return r; if (!strv_env_name_is_valid(l)) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PassEnvironment block."); + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PassEnvironment= block."); if (mode != UNIT_CHECK) { if (strv_isempty(l)) { diff --git a/src/core/execute.c b/src/core/execute.c index 21c0149e22..9dcc02aaa4 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1673,8 +1673,10 @@ static int build_pass_environment(const ExecContext *c, char ***ret) { x = strjoin(*i, "=", v); if (!x) return -ENOMEM; + if (!GREEDY_REALLOC(pass_env, n_bufsize, n_env + 2)) return -ENOMEM; + pass_env[n_env++] = x; pass_env[n_env] = NULL; x = NULL; @@ -3031,6 +3033,19 @@ static int exec_child( #endif } + if (!strv_isempty(context->unset_environment)) { + char **ee = NULL; + + ee = strv_env_delete(accum_env, 1, context->unset_environment); + if (!ee) { + *exit_status = EXIT_MEMORY; + return -ENOMEM; + } + + strv_free(accum_env); + accum_env = ee; + } + final_argv = replace_env_argv(argv, accum_env); if (!final_argv) { *exit_status = EXIT_MEMORY; @@ -3222,6 +3237,7 @@ void exec_context_done(ExecContext *c) { c->environment = strv_free(c->environment); c->environment_files = strv_free(c->environment_files); c->pass_environment = strv_free(c->pass_environment); + c->unset_environment = strv_free(c->unset_environment); for (l = 0; l < ELEMENTSOF(c->rlimit); l++) c->rlimit[l] = mfree(c->rlimit[l]); @@ -3582,6 +3598,9 @@ void exec_context_dump(ExecContext *c, FILE* f, const char *prefix) { STRV_FOREACH(e, c->pass_environment) fprintf(f, "%sPassEnvironment: %s\n", prefix, *e); + STRV_FOREACH(e, c->unset_environment) + fprintf(f, "%sUnsetEnvironment: %s\n", prefix, *e); + fprintf(f, "%sRuntimeDirectoryPreserve: %s\n", prefix, exec_preserve_mode_to_string(c->runtime_directory_preserve_mode)); for (dt = 0; dt < _EXEC_DIRECTORY_MAX; dt++) { diff --git a/src/core/execute.h b/src/core/execute.h index 8a7ce8449b..f7c20dbcb3 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -133,6 +133,7 @@ struct ExecContext { char **environment; char **environment_files; char **pass_environment; + char **unset_environment; struct rlimit *rlimit[_RLIMIT_MAX]; char *working_directory, *root_directory, *root_image; diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 94f3d657f6..94e29397f3 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -35,6 +35,7 @@ $1.UMask, config_parse_mode, 0, $1.Environment, config_parse_environ, 0, offsetof($1, exec_context.environment) $1.EnvironmentFile, config_parse_unit_env_file, 0, offsetof($1, exec_context.environment_files) $1.PassEnvironment, config_parse_pass_environ, 0, offsetof($1, exec_context.pass_environment) +$1.UnsetEnvironment, config_parse_unset_environ, 0, offsetof($1, exec_context.unset_environment) $1.DynamicUser, config_parse_bool, true, offsetof($1, exec_context.dynamic_user) $1.StandardInput, config_parse_exec_input, 0, offsetof($1, exec_context) $1.StandardOutput, config_parse_exec_output, 0, offsetof($1, exec_context) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 7fa1bafaef..cec9c60365 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2174,16 +2174,17 @@ int config_parse_environ(const char *unit, } } -int config_parse_pass_environ(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) { +int config_parse_pass_environ( + 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) { const char *whole_rvalue = rvalue; char*** passenv = data; @@ -2238,6 +2239,85 @@ int config_parse_pass_environ(const char *unit, return 0; } +int config_parse_unset_environ( + 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_strv_free_ char **n = NULL; + const char *whole_rvalue = rvalue; + size_t nlen = 0, nbufsize = 0; + char*** unsetenv = data; + Unit *u = userdata; + int r; + + assert(filename); + assert(lvalue); + assert(rvalue); + assert(data); + + if (isempty(rvalue)) { + /* Empty assignment resets the list */ + *unsetenv = strv_free(*unsetenv); + return 0; + } + + for (;;) { + _cleanup_free_ char *word = NULL, *k = NULL; + + r = extract_first_word(&rvalue, &word, NULL, EXTRACT_QUOTES); + if (r == 0) + break; + if (r == -ENOMEM) + return log_oom(); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, + "Trailing garbage in %s, ignoring: %s", lvalue, whole_rvalue); + break; + } + + if (u) { + r = unit_full_printf(u, word, &k); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve specifiers, ignoring: %s", word); + continue; + } + } else { + k = word; + word = NULL; + } + + if (!env_assignment_is_valid(k) && !env_name_is_valid(k)) { + log_syntax(unit, LOG_ERR, filename, line, 0, + "Invalid environment name or assignment %s, ignoring: %s", lvalue, k); + continue; + } + + if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) + return log_oom(); + + n[nlen++] = k; + n[nlen] = NULL; + k = NULL; + } + + if (n) { + r = strv_extend_strv(unsetenv, n, true); + if (r < 0) + return r; + } + + return 0; +} + int config_parse_ip_tos(const char *unit, const char *filename, unsigned line, diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index ec338ccb9a..49b3b405df 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -79,6 +79,7 @@ int config_parse_syscall_archs(const char *unit, const char *filename, unsigned int config_parse_syscall_errno(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); int config_parse_environ(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); int config_parse_pass_environ(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); +int config_parse_unset_environ(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); int config_parse_unit_slice(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); int config_parse_cpu_weight(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); int config_parse_cpu_shares(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); diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index c024f64ca9..40f1d74d8a 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -641,7 +641,7 @@ int bus_append_unit_property_assignment(sd_bus_message *m, const char *assignmen r = sd_bus_message_append(m, "v", "i", (int32_t) q); - } else if (STR_IN_SET(field, "Environment", "PassEnvironment")) { + } else if (STR_IN_SET(field, "Environment", "UnsetEnvironment", "PassEnvironment")) { const char *p; r = sd_bus_message_open_container(m, 'v', "as"); @@ -668,6 +668,11 @@ int bus_append_unit_property_assignment(sd_bus_message *m, const char *assignmen log_error("Invalid environment assignment: %s", word); return -EINVAL; } + } else if (streq(field, "UnsetEnvironment")) { + if (!env_assignment_is_valid(word) && !env_name_is_valid(word)) { + log_error("Invalid environment name or assignment: %s", word); + return -EINVAL; + } } else { /* PassEnvironment */ if (!env_name_is_valid(word)) { log_error("Invalid environment variable name: %s", word); From 42cc99d5ec0197ea05d8da05e0457585e5106419 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Sep 2017 19:10:06 +0200 Subject: [PATCH 2/6] test: add test case for UnsetEnvironment= --- src/test/test-execute.c | 5 +++++ test/meson.build | 1 + test/test-execute/exec-unset-environment.service | 8 ++++++++ 3 files changed, 14 insertions(+) create mode 100644 test/test-execute/exec-unset-environment.service diff --git a/src/test/test-execute.c b/src/test/test-execute.c index ac2cdd50ed..9d1dcfc58e 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -453,6 +453,10 @@ static void test_exec_read_only_path_suceed(Manager *m) { test(m, "exec-read-only-path-succeed.service", 0, CLD_EXITED); } +static void test_exec_unset_environment(Manager *m) { + test(m, "exec-unset-environment.service", 0, CLD_EXITED); +} + static int run_tests(UnitFileScope scope, const test_function_t *tests) { const test_function_t *test = NULL; Manager *m = NULL; @@ -508,6 +512,7 @@ int main(int argc, char *argv[]) { test_exec_ioschedulingclass, test_exec_spec_interpolation, test_exec_read_only_path_suceed, + test_exec_unset_environment, NULL, }; static const test_function_t system_tests[] = { diff --git a/test/meson.build b/test/meson.build index c16ca92702..bddeeb62b4 100644 --- a/test/meson.build +++ b/test/meson.build @@ -94,6 +94,7 @@ test_data_files = ''' test-execute/exec-systemcallfilter-not-failing.service test-execute/exec-systemcallfilter-system-user.service test-execute/exec-systemcallfilter-system-user-nfsnobody.service + test-execute/exec-unset-environment.service test-execute/exec-user.service test-execute/exec-user-nfsnobody.service test-execute/exec-workingdirectory.service diff --git a/test/test-execute/exec-unset-environment.service b/test/test-execute/exec-unset-environment.service new file mode 100644 index 0000000000..5b0123b81e --- /dev/null +++ b/test/test-execute/exec-unset-environment.service @@ -0,0 +1,8 @@ +[Unit] +Description=Test for UnsetEnvironment + +[Service] +ExecStart=/bin/sh -x -c 'test "$$FOO" = "bar" && test "$${QUUX-X}" = "X" && test "$$VAR3" = "value3" && test "$${VAR4-X}" = "X" && test "$$VAR5" = "value5" && test "$${X%b-X}" = "X"' +Type=oneshot +Environment=FOO=bar QUUX=waldo VAR3=value3 VAR4=value4 VAR5=value5 X%b=%U +UnsetEnvironment=QUUX=waldo VAR3=somethingelse VAR4 X%b=%U From 82f93439af6ec0e7444fc35260c4a4fc5b30d19c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 10 Sep 2017 12:19:02 +0200 Subject: [PATCH 3/6] units: properly unset the l10n environment variables where we need to Now that we have UnsetEnvironment=, let's make proper use of it for unsetting l10n settings for console gettys. Fixes: #6407 --- units/debug-shell.service.in | 2 +- units/getty@.service.m4 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/units/debug-shell.service.in b/units/debug-shell.service.in index 59f6e494b3..f72e5ef6b6 100644 --- a/units/debug-shell.service.in +++ b/units/debug-shell.service.in @@ -28,7 +28,7 @@ KillSignal=SIGHUP # Unset locale for the console getty since the console has problems # displaying some internationalized messages. -Environment=LANG= LANGUAGE= LC_CTYPE= LC_NUMERIC= LC_TIME= LC_COLLATE= LC_MONETARY= LC_MESSAGES= LC_PAPER= LC_NAME= LC_ADDRESS= LC_TELEPHONE= LC_MEASUREMENT= LC_IDENTIFICATION= +UnsetEnvironment=LANG LANGUAGE LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION [Install] WantedBy=sysinit.target diff --git a/units/getty@.service.m4 b/units/getty@.service.m4 index b62f1b9225..ff1b3c3d87 100644 --- a/units/getty@.service.m4 +++ b/units/getty@.service.m4 @@ -51,7 +51,7 @@ SendSIGHUP=yes # Unset locale for the console getty since the console has problems # displaying some internationalized messages. -Environment=LANG= LANGUAGE= LC_CTYPE= LC_NUMERIC= LC_TIME= LC_COLLATE= LC_MONETARY= LC_MESSAGES= LC_PAPER= LC_NAME= LC_ADDRESS= LC_TELEPHONE= LC_MEASUREMENT= LC_IDENTIFICATION= +UnsetEnvironment=LANG LANGUAGE LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION [Install] WantedBy=getty.target From f7f3f5c35c546837a1fcd988718726e6f8cf517a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 12 Sep 2017 19:47:58 +0200 Subject: [PATCH 4/6] core: print the right string when we fail to replace specifiers in config_parse_environ() --- src/core/load-fragment.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index cec9c60365..14fd11f9e2 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2108,16 +2108,17 @@ int config_parse_unit_env_file(const char *unit, return 0; } -int config_parse_environ(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) { +int config_parse_environ( + 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) { Unit *u = userdata; char ***env = data; @@ -2153,7 +2154,7 @@ int config_parse_environ(const char *unit, r = unit_full_printf(u, word, &k); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, - "Failed to resolve specifiers, ignoring: %s", k); + "Failed to resolve specifiers, ignoring: %s", word); continue; } } else { @@ -2170,6 +2171,7 @@ int config_parse_environ(const char *unit, r = strv_env_replace(env, k); if (r < 0) return log_oom(); + k = NULL; } } From 41de9cc29ecb0357c6236691c47eb0cf15d2ebd8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 12 Sep 2017 19:48:29 +0200 Subject: [PATCH 5/6] core: support specifier expansion in PassEnvironment= I can't come up with any usecase for this, but let's add this here, to match what we support for Environment=. It's kind surprising if we support specifier expansion for some environment related settings, but not for others. --- src/core/dbus-execute.c | 13 +++++++++---- src/core/load-fragment.c | 28 +++++++++++++++++++++------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 2bcdd33659..95152240ea 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2000,13 +2000,17 @@ int bus_exec_context_set_transient_property( } else if (streq(name, "PassEnvironment")) { - _cleanup_strv_free_ char **l = NULL; + _cleanup_strv_free_ char **l = NULL, **q = NULL; r = sd_bus_message_read_strv(message, &l); if (r < 0) return r; - if (!strv_env_name_is_valid(l)) + r = unit_full_printf_strv(u, l, &q); + if (r < 0) + return r; + + if (!strv_env_name_is_valid(q)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PassEnvironment= block."); if (mode != UNIT_CHECK) { @@ -2016,11 +2020,12 @@ int bus_exec_context_set_transient_property( } else { _cleanup_free_ char *joined = NULL; - r = strv_extend_strv(&c->pass_environment, l, true); + r = strv_extend_strv(&c->pass_environment, q, true); if (r < 0) return r; - joined = strv_join_quoted(c->pass_environment); + /* We write just the new settings out to file, with unresolved specifiers. */ + joined = strv_join_quoted(l); if (!joined) return -ENOMEM; diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 14fd11f9e2..696b4179fb 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2189,9 +2189,10 @@ int config_parse_pass_environ( void *userdata) { const char *whole_rvalue = rvalue; - char*** passenv = data; _cleanup_strv_free_ char **n = NULL; size_t nlen = 0, nbufsize = 0; + char*** passenv = data; + Unit *u = userdata; int r; assert(filename); @@ -2206,7 +2207,7 @@ int config_parse_pass_environ( } for (;;) { - _cleanup_free_ char *word = NULL; + _cleanup_free_ char *word = NULL, *k = NULL; r = extract_first_word(&rvalue, &word, NULL, EXTRACT_QUOTES); if (r == 0) @@ -2219,17 +2220,30 @@ int config_parse_pass_environ( break; } - if (!env_name_is_valid(word)) { - log_syntax(unit, LOG_ERR, filename, line, EINVAL, - "Invalid environment name for %s, ignoring: %s", lvalue, word); + if (u) { + r = unit_full_printf(u, word, &k); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve specifiers, ignoring: %s", word); + continue; + } + } else { + k = word; + word = NULL; + } + + if (!env_name_is_valid(k)) { + log_syntax(unit, LOG_ERR, filename, line, 0, + "Invalid environment name for %s, ignoring: %s", lvalue, k); continue; } if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) return log_oom(); - n[nlen++] = word; + + n[nlen++] = k; n[nlen] = NULL; - word = NULL; + k = NULL; } if (n) { From 1c68232ee2584e20eda31e5ddce91c30ef6ab86c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 12 Sep 2017 20:07:30 +0200 Subject: [PATCH 6/6] core: rework how we treat specifiers in Environment= of transient units Let's validate the data passed in after resolving specifiers, but let's write out to the unit snippet the list without specifiers applied. This way the pre-existing comment actually starts matching what is actually implemented. --- src/core/dbus-execute.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 95152240ea..2c0124229b 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1817,13 +1817,13 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; - if (!strv_env_is_valid(l)) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid environment block."); - r = unit_full_printf_strv(u, l, &q); if (r < 0) return r; + if (!strv_env_is_valid(q)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid environment block."); + if (mode != UNIT_CHECK) { if (strv_length(q) == 0) { c->environment = strv_free(c->environment); @@ -1840,7 +1840,7 @@ int bus_exec_context_set_transient_property( c->environment = e; /* We write just the new settings out to file, with unresolved specifiers */ - joined = strv_join_quoted(q); + joined = strv_join_quoted(l); if (!joined) return -ENOMEM;