From 8167c56bfa97525a7b12e7c5685576657364e3cf Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 19 Jul 2023 22:56:02 +0100 Subject: [PATCH] run: disable --expand-environment by default for --scope The intention was to have this option enabled by default everywhere, but unfortunately at least one case was found where it breaks compatibility of a program using systemd-run --scopes and expecting variables not to be expanded: https://sources.debian.org/src/pbuilder/0.231/pbuilder-checkparams/#L400 Example run: systemd-run --quiet --scope --description=pbuilder_build_xfce4-notes-plugin_1.10.0-1.dsc '--slice=system-pbuilder-build-xfce4\x2dnotes\x2dplugin_1.10.0\x2d1-449932.slice' chroot /var/cache/pbuilder/build/449932 dpkg-query -W '--showformat=${Version}' apt Restore backward compatibility and make the option disabled by default when --scope is used, and enabled by default for other types. In case --expand-environment is not specified and a '$' character is detected, print a warning to nudge users toward specifying the parameter as needed. In the future we can then flip the default. Follow-up for 2ed7a221fafb25eea937c4e86fb88ee501dba51e --- man/systemd-run.xml | 10 +++++++--- src/run/run.c | 32 +++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index 73adbfb927..fecedda25c 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -177,15 +177,19 @@ - Expand environment variables in command arguments. If enabled (the default), - environment variables specified as ${VARIABLE} will be - expanded in the same way as in commands specified via ExecStart= in units. With + Expand environment variables in command arguments. If enabled, environment variables + specified as ${VARIABLE} will be expanded in the same + way as in commands specified via ExecStart= in units. With --scope, this expansion is performed by systemd-run itself, and in other cases by the service manager that spawns the command. Note that this is similar to, but not the same as variable expansion in bash1 and other shells. + The default is to enable this option in all cases, except for --scope where + it is disabled by default, for backward compatibility reasons. Note that this will be changed in a + future release, where it will be switched to enabled by default as well. + See systemd.service5 for a description of variable expansion. Disabling variable expansion is useful if the specified diff --git a/src/run/run.c b/src/run/run.c index 7880a0ecc7..f6a2ea53de 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -45,7 +45,7 @@ static const char *arg_unit = NULL; static const char *arg_description = NULL; static const char *arg_slice = NULL; static bool arg_slice_inherit = false; -static bool arg_expand_environment = true; +static int arg_expand_environment = -1; static bool arg_send_sighup = false; static BusTransport arg_transport = BUS_TRANSPORT_LOCAL; static const char *arg_host = NULL; @@ -291,11 +291,17 @@ static int parse_argv(int argc, char *argv[]) { arg_slice_inherit = true; break; - case ARG_EXPAND_ENVIRONMENT: - r = parse_boolean_argument("--expand-environment=", optarg, &arg_expand_environment); + case ARG_EXPAND_ENVIRONMENT: { + bool b; + + r = parse_boolean_argument("--expand-environment=", optarg, &b); if (r < 0) return r; + + arg_expand_environment = b; + break; + } case ARG_SEND_SIGHUP: arg_send_sighup = true; @@ -732,7 +738,7 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p /* We disable environment expansion on the server side via ExecStartEx=:. * ExecStartEx was added relatively recently (v243), and some bugs were fixed only later. * So use that feature only if required. It will fail with older systemds. */ - bool use_ex_prop = !arg_expand_environment; + bool use_ex_prop = arg_expand_environment == 0; assert(m); @@ -896,7 +902,7 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p if (use_ex_prop) r = sd_bus_message_append_strv( m, - STRV_MAKE(arg_expand_environment ? NULL : "no-env-expand")); + STRV_MAKE(arg_expand_environment > 0 ? NULL : "no-env-expand")); else r = sd_bus_message_append(m, "b", false); if (r < 0) @@ -1197,7 +1203,7 @@ static int bus_call_with_hint( if (r < 0) { log_error_errno(r, "Failed to start transient %s unit: %s", name, bus_error_message(&error, r)); - if (!arg_expand_environment && + if (arg_expand_environment == 0 && sd_bus_error_has_names(&error, SD_BUS_ERROR_UNKNOWN_PROPERTY, SD_BUS_ERROR_PROPERTY_READ_ONLY)) @@ -1614,7 +1620,7 @@ static int start_transient_scope(sd_bus *bus) { if (!arg_quiet) log_info("Running scope as unit: %s", scope); - if (arg_expand_environment) { + if (arg_expand_environment > 0) { _cleanup_strv_free_ char **expanded_cmdline = NULL, **unset_variables = NULL, **bad_variables = NULL; r = replace_env_argv(arg_cmdline, env, &expanded_cmdline, &unset_variables, &bad_variables); @@ -1868,6 +1874,18 @@ static int run(int argc, char* argv[]) { arg_description = description; } + /* For backward compatibility reasons env var expansion is disabled by default for scopes, and + * enabled by default for everything else. Try to detect it and print a warning, so that we can + * change it in the future and harmonize it. */ + if (arg_expand_environment < 0) { + arg_expand_environment = !arg_scope; + + if (!arg_quiet && arg_scope && strchr(arg_description, '$')) + log_warning("Scope command line contains environment variable, which is not expanded" + " by default for now, but will be expanded by default in the future." + " Use --expand-environment=yes/no to explicitly control it as needed."); + } + /* If --wait is used connect via the bus, unconditionally, as ref/unref is not supported via the limited direct * connection */ if (arg_wait || arg_stdio != ARG_STDIO_NONE || (arg_runtime_scope == RUNTIME_SCOPE_USER && arg_transport != BUS_TRANSPORT_LOCAL)) -- 2.25.1