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

diff --git a/man/systemd-run.xml b/man/systemd-run.xml
index 73adbfb9276cf66df12ec8acdeb757165599a296..fecedda25cb22f483898f25240419a016544b657 100644 (file)
--- a/man/systemd-run.xml
+++ b/man/systemd-run.xml
@@ -177,15 +177,19 @@
       <varlistentry>
         <term><option>--expand-environment=<replaceable>BOOL</replaceable></option></term>
 
-        <listitem><para>Expand environment variables in command arguments. If enabled (the default),
-        environment variables specified as <literal>${<replaceable>VARIABLE</replaceable>}</literal> will be
-        expanded in the same way as in commands specified via <varname>ExecStart=</varname> in units. With
+        <listitem><para>Expand environment variables in command arguments. If enabled, environment variables
+        specified as <literal>${<replaceable>VARIABLE</replaceable>}</literal> will be expanded in the same
+        way as in commands specified via <varname>ExecStart=</varname> in units. With
         <varname>--scope</varname>, this expansion is performed by <command>systemd-run</command> 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
         <citerefentry project='man-pages'><refentrytitle>bash</refentrytitle><manvolnum>1</manvolnum></citerefentry>
         and other shells.</para>
 
+        <para>The default is to enable this option in all cases, except for <varname>--scope</varname> 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.</para>
+
         <para>See
         <citerefentry><refentrytitle>systemd.service</refentrytitle><manvolnum>5</manvolnum></citerefentry>
         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 7880a0ecc77d08f9eb6a38ba7cd48480c820d8f8..f6a2ea53deb838b71f358e406de643b1410e2129 100644 (file)
--- 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))