From 6a6707ce8589fb5074a33ccbe6fdbd476e8b7021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 22 Aug 2022 13:37:13 +0200 Subject: [PATCH 1/2] man/run: we accept relative paths for run I think this is a left-over from before we changed ExecStart= to allow non-absolute paths, *and* changed systemd-run itself to resolve paths too. --- man/systemd-run.xml | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index faf88d560a..5910565427 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -72,7 +72,7 @@ processes of the command are managed by the service manager similarly to normal services, and will show up in the output of systemctl list-units. Execution in this case is synchronous, and will return only when the command finishes. This mode is enabled via the switch - (see below). + (see below). If a command is run with path, socket, or timer options such as (see below), a transient path, socket, or timer unit is created alongside the service unit for the specified command. Only the @@ -82,15 +82,16 @@ .path, .socket, or .timer unit that triggers the specified unit. - By default, services created with systemd-run default to the type, - see the description of Type= in + By default, services created with systemd-run default to the + type, see the description of Type= in systemd.service5 for - details. Note that when this type is used the service manager (and thus the systemd-run command) - considers service start-up successful as soon as the fork() for the main service process - succeeded, i.e. before the execve() is invoked, and thus even if the specified command cannot - be started. Consider using the service type (i.e. ) to - ensure that systemd-run returns successfully only if the specified command line has been - successfully started. + details. Note that when this type is used, the service manager (and thus the + systemd-run command) considers service start-up successful as soon as the + fork() for the main service process succeeded, i.e. before the + execve() is invoked, and thus even if the specified command cannot be started. + Consider using the service type (i.e. ) to + ensure that systemd-run returns successfully only if the specified command line has + been successfully started. @@ -411,10 +412,8 @@ - All command line arguments after the first non-option - argument become part of the command line of the launched - process. If a command is run as service unit, the first argument - needs to be an absolute program path. + All command line arguments after the first non-option argument become part of the command line of + the launched process. From 24536bebe0fe4b674fda3ddf960287754e3e3cfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Aug 2022 07:34:49 +0200 Subject: [PATCH 2/2] core: escape ExecStart command-line received over d-bus When some transient unit setting is received over D-Bus, we write it it to a transient unit file. We escape backslashes and specifiers. For most settings this is enough, because most settings only do parsing and interpolation of specifiers. When systemd-run is called (or something equivalent that gives us a command strv), we write ExecStart=, but when reading it, we not only do parsing and interpolation of specifiers, but also split on semicolons and do variable substitution. This results in an ugly situation where the commandline is interpolated twice, once on the caller side, and once in the manager. I think we need to treat this as a bug: current behaviour seems to be an accident of implementation and hard to explain in a reasonable way. If we *were* doing specifier expansion, then it'd be somewhat reasonable so say that "the commandline is handled the same as ExecStart=". But since we explicitly prevent specifier expansion, we best we could say is "the commandline has some subset of features of ExecStart=". I think this is not useful, and unexpected by users. Since most people use use a shell to call systemd-run, one level of variable expansion is already done on the caller side, and having to take into account another level of expansion (with slightly different rules), creates a big mental overhead when the commandline needs to include a dollar character or such. Not doing any expansion is much cleaner and easier to explain or use. Thus I think it's better to change behaviour here, even though in principle some people could be relying on current behaviour. I think it's more likely that nobody noticed, because people generally don't use systemd-run for complicated commandlines. Thus this commit adds an additional mode of escaping that prevents variable explansion and other elements of ExecStart= syntax. I looked over all the places where unit_escape_setting() is called, and I think that only two need to be changed to use the new flag. Fixes #23631. --- src/core/dbus-execute.c | 5 ++-- src/core/unit.c | 53 +++++++++++++++++++++++++---------------- src/core/unit.h | 13 ++++++---- 3 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 8de092ee4e..37d0afb0cb 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1591,7 +1591,7 @@ int bus_set_transient_exec_command( if (!exec_chars) return -ENOMEM; - a = unit_concat_strv(c->argv, UNIT_ESCAPE_C|UNIT_ESCAPE_SPECIFIERS); + a = unit_concat_strv(c->argv, UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX); if (!a) return -ENOMEM; @@ -1601,7 +1601,8 @@ int bus_set_transient_exec_command( _cleanup_free_ char *t = NULL; const char *p; - p = unit_escape_setting(c->path, UNIT_ESCAPE_C|UNIT_ESCAPE_SPECIFIERS, &t); + p = unit_escape_setting(c->path, + UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX, &t); if (!p) return -ENOMEM; diff --git a/src/core/unit.c b/src/core/unit.c index 5f1c6109b0..05cee225ed 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4254,51 +4254,62 @@ static const char* unit_drop_in_dir(Unit *u, UnitWriteFlags flags) { } char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) { - char *ret = NULL; + assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX | UNIT_ESCAPE_C)); + + _cleanup_free_ char *t = NULL; if (!s) return NULL; - /* Escapes the input string as requested. Returns the escaped string. If 'buf' is specified then the allocated - * return buffer pointer is also written to *buf, except if no escaping was necessary, in which case *buf is - * set to NULL, and the input pointer is returned as-is. This means the return value always contains a properly - * escaped version, but *buf when passed only contains a pointer if an allocation was necessary. If *buf is - * not specified, then the return value always needs to be freed. Callers can use this to optimize memory - * allocations. */ + /* Escapes the input string as requested. Returns the escaped string. If 'buf' is specified then the + * allocated return buffer pointer is also written to *buf, except if no escaping was necessary, in + * which case *buf is set to NULL, and the input pointer is returned as-is. This means the return + * value always contains a properly escaped version, but *buf when passed only contains a pointer if + * an allocation was necessary. If *buf is not specified, then the return value always needs to be + * freed. Callers can use this to optimize memory allocations. */ if (flags & UNIT_ESCAPE_SPECIFIERS) { - ret = specifier_escape(s); - if (!ret) + t = specifier_escape(s); + if (!t) return NULL; - s = ret; + s = t; } - if (flags & UNIT_ESCAPE_C) { - char *a; + /* We either do c-escaping or shell-escaping, to additionally escape characters that we parse for + * ExecStart= and friend, i.e. '$' and ';' and quotes. */ - a = cescape(s); - free(ret); - if (!a) + if (flags & UNIT_ESCAPE_EXEC_SYNTAX) { + char *t2 = shell_escape(s, "$;'\""); + if (!t2) return NULL; + free_and_replace(t, t2); - ret = a; + s = t; + + } else if (flags & UNIT_ESCAPE_C) { + char *t2 = cescape(s); + if (!t2) + return NULL; + free_and_replace(t, t2); + + s = t; } if (buf) { - *buf = ret; - return ret ?: (char*) s; + *buf = TAKE_PTR(t); + return (char*) s; } - return ret ?: strdup(s); + return TAKE_PTR(t) ?: strdup(s); } char* unit_concat_strv(char **l, UnitWriteFlags flags) { _cleanup_free_ char *result = NULL; size_t n = 0; - /* Takes a list of strings, escapes them, and concatenates them. This may be used to format command lines in a - * way suitable for ExecStart= stanzas */ + /* Takes a list of strings, escapes them, and concatenates them. This may be used to format command + * lines in a way suitable for ExecStart= stanzas. */ STRV_FOREACH(i, l) { _cleanup_free_ char *buf = NULL; diff --git a/src/core/unit.h b/src/core/unit.h index fc8edaade5..01cef89525 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -531,19 +531,22 @@ typedef struct UnitStatusMessageFormats { /* Flags used when writing drop-in files or transient unit files */ typedef enum UnitWriteFlags { /* Write a runtime unit file or drop-in (i.e. one below /run) */ - UNIT_RUNTIME = 1 << 0, + UNIT_RUNTIME = 1 << 0, /* Write a persistent drop-in (i.e. one below /etc) */ - UNIT_PERSISTENT = 1 << 1, + UNIT_PERSISTENT = 1 << 1, /* Place this item in the per-unit-type private section, instead of [Unit] */ - UNIT_PRIVATE = 1 << 2, + UNIT_PRIVATE = 1 << 2, /* Apply specifier escaping before writing */ - UNIT_ESCAPE_SPECIFIERS = 1 << 3, + UNIT_ESCAPE_SPECIFIERS = 1 << 3, + + /* Escape elements of ExecStart= syntax before writing */ + UNIT_ESCAPE_EXEC_SYNTAX = 1 << 4, /* Apply C escaping before writing */ - UNIT_ESCAPE_C = 1 << 4, + UNIT_ESCAPE_C = 1 << 5, } UnitWriteFlags; /* Returns true if neither persistent, nor runtime storage is requested, i.e. this is a check invocation only */