From 23c3c5d4234cb9f157b5c0d6aa6fbfed601a3202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 08:49:17 +0200 Subject: [PATCH 01/12] meson: redo grouping of tests under src/test/ Move the tests that link to libcore into a separate subgroup. They are special and it makes sense to keep them together. While at it, make the list alphabetical. Also, merge the list additions into one. No idea why it was like that. --- src/test/meson.build | 248 +++++++++++++++++++++---------------------- 1 file changed, 123 insertions(+), 125 deletions(-) diff --git a/src/test/meson.build b/src/test/meson.build index d20c911e2b..0310212700 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -216,25 +216,6 @@ tests += [ 'sources' : files('test-boot-timestamps.c'), 'condition' : 'ENABLE_EFI', }, - { - 'sources' : files('test-bpf-devices.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, - { - 'sources' : files('test-bpf-firewall.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, - { - 'sources' : files('test-bpf-foreign-programs.c'), - 'base' : test_core_base, - }, - { - 'sources' : files('test-bpf-lsm.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, { 'sources' : files('test-btrfs.c'), 'type' : 'manual', @@ -250,27 +231,10 @@ tests += [ 'sources' : files('test-capability.c'), 'dependencies' : libcap, }, - { - 'sources' : files('test-cgroup-cpu.c'), - 'base' : test_core_base, - }, - { - 'sources' : files('test-cgroup-mask.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, - { - 'sources' : files('test-cgroup-unit-default.c'), - 'base' : test_core_base, - }, { 'sources' : files('test-chase-manual.c'), 'type' : 'manual', }, - { - 'sources' : files('test-chown-rec.c'), - 'base' : test_core_base, - }, { 'sources' : files('test-compress-benchmark.c'), 'link_with' : [ @@ -296,27 +260,12 @@ tests += [ 'sources' : files('test-dlopen-so.c'), 'dependencies' : libp11kit_cflags }, - { - 'sources' : files('test-emergency-action.c'), - 'base' : test_core_base, - }, - { - 'sources' : files('test-engine.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, { 'sources' : [ files('test-errno-list.c'), generated_gperf_headers, ], }, - { - 'sources' : files('test-execute.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - 'timeout' : 360, - }, { 'sources' : files('test-fd-util.c'), 'dependencies' : libseccomp, @@ -329,11 +278,6 @@ tests += [ ], 'timeout' : 180, }, - { - 'sources' : files('test-install.c'), - 'base' : test_core_base, - 'type' : 'manual', - }, { 'sources' : [ files('test-ip-protocol-list.c'), @@ -344,11 +288,6 @@ tests += [ 'sources' : files('test-ipcrm.c'), 'type' : 'unsafe', }, - { - 'sources' : files('test-job-type.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, { 'sources' : files('test-json.c'), 'dependencies' : libm, @@ -365,25 +304,10 @@ tests += [ threads, ], }, - { - 'sources' : files('test-load-fragment.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, - { - 'sources' : files('test-loop-block.c'), - 'dependencies' : [threads, libblkid], - 'base' : test_core_base, - 'parallel' : false, - }, { 'sources' : files('test-loopback.c'), 'dependencies' : common_test_dependencies, }, - { - 'sources' : files('test-manager.c'), - 'base' : test_core_base, - }, { 'sources' : files('test-math-util.c'), 'dependencies' : libm, @@ -392,26 +316,12 @@ tests += [ 'sources' : files('test-mempress.c'), 'dependencies' : threads, }, - { - 'sources' : files('test-namespace.c'), - 'dependencies' : [ - libblkid, - threads, - ], - 'base' : test_core_base, - }, { 'sources' : files('test-netlink-manual.c'), 'dependencies' : libkmod, 'condition' : 'HAVE_KMOD', 'type' : 'manual', }, - { - 'sources' : files('test-ns.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - 'type' : 'manual', - }, { 'sources' : files('test-nscd-flush.c'), 'condition' : 'ENABLE_NSCD', @@ -438,12 +348,6 @@ tests += [ 'sources' : files('test-parse-util.c'), 'dependencies' : libm, }, - { - 'sources' : files('test-path.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - 'timeout' : 120, - }, { 'sources' : files('test-process-util.c'), 'dependencies' : threads, @@ -462,11 +366,6 @@ tests += [ 'condition' : 'ENABLE_BOOTLOADER', 'c_args' : '-I@0@'.format(efi_config_h_dir), }, - { - 'sources' : files('test-sched-prio.c'), - 'dependencies' : common_test_dependencies, - 'base' : test_core_base, - }, { 'sources' : files('test-seccomp.c'), 'dependencies' : libseccomp, @@ -524,6 +423,128 @@ tests += [ 'includes' : udev_includes, 'type' : 'manual', }, + { + 'sources' : files('test-utmp.c'), + 'condition' : 'ENABLE_UTMP', + }, + { + 'sources' : files('test-varlink.c'), + 'dependencies' : threads, + }, + { + 'sources' : files('test-watchdog.c'), + 'type' : 'unsafe', + }, + + + # Tests that link to libcore, i.e. tests for pid1 code. + { + 'sources' : files('test-bpf-devices.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-bpf-firewall.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-bpf-foreign-programs.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-bpf-lsm.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-cgroup-cpu.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-cgroup-mask.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-cgroup-unit-default.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-chown-rec.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-emergency-action.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-engine.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-execute.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + 'timeout' : 360, + }, + { + 'sources' : files('test-install.c'), + 'base' : test_core_base, + 'type' : 'manual', + }, + { + 'sources' : files('test-job-type.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-load-fragment.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-loop-block.c'), + 'dependencies' : [threads, libblkid], + 'base' : test_core_base, + 'parallel' : false, + }, + { + 'sources' : files('test-manager.c'), + 'base' : test_core_base, + }, + { + 'sources' : files('test-namespace.c'), + 'dependencies' : [ + libblkid, + threads, + ], + 'base' : test_core_base, + }, + { + 'sources' : files('test-ns.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + 'type' : 'manual', + }, + { + 'sources' : files('test-path.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + 'timeout' : 120, + }, + { + 'sources' : files('test-sched-prio.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-socket-bind.c'), + 'dependencies' : libdl, + 'condition' : 'BPF_FRAMEWORK', + 'base' : test_core_base, + }, { 'sources' : files('test-unit-name.c'), 'dependencies' : common_test_dependencies, @@ -534,30 +555,13 @@ tests += [ 'dependencies' : common_test_dependencies, 'base' : test_core_base, }, - { - 'sources' : files('test-utmp.c'), - 'condition' : 'ENABLE_UTMP', - }, - { - 'sources' : files('test-varlink.c'), - 'dependencies' : threads, - }, { 'sources' : files('test-watch-pid.c'), 'dependencies' : common_test_dependencies, 'base' : test_core_base, }, - { - 'sources' : files('test-watchdog.c'), - 'type' : 'unsafe', - }, -] -############################################################ - -# define some tests here, because the link_with deps were not defined earlier - -tests += [ + # Tests from other directories that have link_with deps that were not defined earlier { 'sources' : files('../libsystemd/sd-bus/test-bus-error.c'), 'link_with' : [ @@ -575,10 +579,4 @@ tests += [ 'link_with' : libudev, 'dependencies' : threads, }, - { - 'sources' : files('test-socket-bind.c'), - 'dependencies' : libdl, - 'condition' : 'BPF_FRAMEWORK', - 'base' : test_core_base, - }, ] From 6eccc3cfa9dcfea3c8b508a66d2d592e6b9fcb93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 11:42:41 +0200 Subject: [PATCH 02/12] test-core-unit: add new test file for unit_escape_setting() and friends None of the existing test files fit very well. test-unit-serialize is pretty close, but it does special cgroup setup, which we don't need in this case. I hope we can add more tests in the future for this basic functionality, so I'm adding a brand new file names after the source file it's testing. --- src/test/meson.build | 5 ++ src/test/test-core-unit.c | 100 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 src/test/test-core-unit.c diff --git a/src/test/meson.build b/src/test/meson.build index 0310212700..640f49596f 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -474,6 +474,11 @@ tests += [ 'sources' : files('test-chown-rec.c'), 'base' : test_core_base, }, + { + 'sources' : files('test-core-unit.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, { 'sources' : files('test-emergency-action.c'), 'base' : test_core_base, diff --git a/src/test/test-core-unit.c b/src/test/test-core-unit.c new file mode 100644 index 0000000000..6593f2fb4b --- /dev/null +++ b/src/test/test-core-unit.c @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "escape.h" +#include "tests.h" +#include "unit.h" + +static void test_unit_escape_setting_one( + const char *s, + const char *expected_exec, + const char *expected_c) { + + _cleanup_free_ char *a = NULL, *b, *c, + *s_esc, *a_esc, *b_esc, *c_esc; + const char *t; + + if (!expected_exec) + expected_exec = s; + if (!expected_c) + expected_c = expected_exec; + assert_se(s_esc = cescape(s)); + + assert_se(t = unit_escape_setting(s, 0, &a)); + assert_se(a_esc = cescape(t)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, a_esc); + assert_se(a == NULL); + assert_se(t == s); + + assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_EXEC_SYNTAX, &b)); + assert_se(b_esc = cescape(t)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); + assert_se(b == NULL || streq(b, t)); + assert_se(streq(t, expected_exec)); + + assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_C, &c)); + assert_se(c_esc = cescape(t)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, c_esc); + assert_se(c == NULL || streq(c, t)); + assert_se(streq(t, expected_c)); +} + +TEST(unit_escape_setting) { + test_unit_escape_setting_one("/sbin/sbash", NULL, NULL); + test_unit_escape_setting_one("$", "\\$", "$"); + test_unit_escape_setting_one("$$", "\\$\\$", "$$"); + test_unit_escape_setting_one("'", "\\'", NULL); + test_unit_escape_setting_one("\"", "\\\"", NULL); + test_unit_escape_setting_one("\t", "\\t", NULL); + test_unit_escape_setting_one(" ", NULL, NULL); + test_unit_escape_setting_one("$;'\"\t\n", "\\$\\;\\'\\\"\\t\\n", "$;\\'\\\"\\t\\n"); +} + +static void test_unit_concat_strv_one( + char **s, + const char *expected_none, + const char *expected_exec, + const char *expected_c) { + + _cleanup_free_ char *a, *b, *c, + *s_ser, *s_esc, *a_esc, *b_esc, *c_esc; + + assert_se(s_ser = strv_join(s, "_")); + assert_se(s_esc = cescape(s_ser)); + if (!expected_exec) + expected_exec = expected_none; + if (!expected_c) + expected_c = expected_none; + + assert_se(a = unit_concat_strv(s, 0)); + assert_se(a_esc = cescape(a)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, a_esc); + assert_se(streq(a, expected_none)); + + assert_se(b = unit_concat_strv(s, UNIT_ESCAPE_EXEC_SYNTAX)); + assert_se(b_esc = cescape(b)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); + assert_se(streq(b, expected_exec)); + + assert_se(c = unit_concat_strv(s, UNIT_ESCAPE_C)); + assert_se(c_esc = cescape(c)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, c_esc); + assert_se(streq(c, expected_c)); +} + +TEST(unit_concat_strv) { + test_unit_concat_strv_one(STRV_MAKE("a", "b", "c"), + "\"a\" \"b\" \"c\"", + NULL, + NULL); + test_unit_concat_strv_one(STRV_MAKE("a", " ", "$", "$$", ""), + "\"a\" \" \" \"$\" \"$$\" \"\"", + "\"a\" \" \" \"\\$\" \"\\$\\$\" \"\"", + NULL); + test_unit_concat_strv_one(STRV_MAKE("\n", " ", "\t"), + "\"\n\" \" \" \"\t\"", + "\"\\n\" \" \" \"\\t\"", + "\"\\n\" \" \" \"\\t\""); +} + +DEFINE_TEST_MAIN(LOG_DEBUG); From a12bc99ef0fc04fa48767c891f7a6db6404e51d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 14:32:39 +0200 Subject: [PATCH 03/12] basic/logarithm: add popcount() wrapper __builtin_popcount() is a bit of a mouthful, so let's provide a helper. Using _Generic has the advantage that if a type other then the ones on the list is given, compilation will fail. This is nice, because if by any change we pass a wider type, it is rejected immediately instead of being truncated. log.h is also needed. It is included transitively, but let's include it directly. macro.h is *not* needed. --- src/boot/efi/splash.c | 9 +++++---- src/{basic => fundamental}/logarithm.h | 10 ++++++++-- src/shared/tpm2-util.c | 4 +++- src/test/test-logarithm.c | 21 +++++++++++++++++++++ 4 files changed, 37 insertions(+), 7 deletions(-) rename src/{basic => fundamental}/logarithm.h (76%) diff --git a/src/boot/efi/splash.c b/src/boot/efi/splash.c index f2d9f20e96..8daeb71cb2 100644 --- a/src/boot/efi/splash.c +++ b/src/boot/efi/splash.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "graphics.h" +#include "logarithm.h" #include "proto/graphics-output.h" #include "splash.h" #include "unaligned-fundamental.h" @@ -141,14 +142,14 @@ static void read_channel_maks( channel_shift[R] = __builtin_ctz(dib->channel_mask_r); channel_shift[G] = __builtin_ctz(dib->channel_mask_g); channel_shift[B] = __builtin_ctz(dib->channel_mask_b); - channel_scale[R] = 0xff / ((1 << __builtin_popcount(dib->channel_mask_r)) - 1); - channel_scale[G] = 0xff / ((1 << __builtin_popcount(dib->channel_mask_g)) - 1); - channel_scale[B] = 0xff / ((1 << __builtin_popcount(dib->channel_mask_b)) - 1); + channel_scale[R] = 0xff / ((1 << popcount(dib->channel_mask_r)) - 1); + channel_scale[G] = 0xff / ((1 << popcount(dib->channel_mask_g)) - 1); + channel_scale[B] = 0xff / ((1 << popcount(dib->channel_mask_b)) - 1); if (dib->size >= SIZEOF_BMP_DIB_RGBA && dib->channel_mask_a != 0) { channel_mask[A] = dib->channel_mask_a; channel_shift[A] = __builtin_ctz(dib->channel_mask_a); - channel_scale[A] = 0xff / ((1 << __builtin_popcount(dib->channel_mask_a)) - 1); + channel_scale[A] = 0xff / ((1 << popcount(dib->channel_mask_a)) - 1); } else { channel_mask[A] = 0; channel_shift[A] = 0; diff --git a/src/basic/logarithm.h b/src/fundamental/logarithm.h similarity index 76% rename from src/basic/logarithm.h rename to src/fundamental/logarithm.h index 646f2d3613..0b03bbded1 100644 --- a/src/basic/logarithm.h +++ b/src/fundamental/logarithm.h @@ -3,8 +3,6 @@ #include -#include "macro.h" - /* Note: log2(0) == log2(1) == 0 here and below. */ #define CONST_LOG2ULL(x) ((x) > 1 ? (unsigned) __builtin_clzll(x) ^ 63U : 0) @@ -30,6 +28,14 @@ static inline unsigned u32ctz(uint32_t n) { #endif } +#define popcount(n) \ + _Generic((n), \ + unsigned char: __builtin_popcount(n), \ + unsigned short: __builtin_popcount(n), \ + unsigned: __builtin_popcount(n), \ + unsigned long: __builtin_popcountl(n), \ + unsigned long long: __builtin_popcountll(n)) + #define CONST_LOG2U(x) ((x) > 1 ? __SIZEOF_INT__ * 8 - __builtin_clz(x) - 1 : 0) #define NONCONST_LOG2U(x) ({ \ unsigned _x = (x); \ diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index 4f51682e8d..97a9a3909b 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -14,6 +14,8 @@ #include "hexdecoct.h" #include "hmac.h" #include "lock-util.h" +#include "log.h" +#include "logarithm.h" #include "memory-util.h" #include "openssl-util.h" #include "parse-util.h" @@ -773,7 +775,7 @@ size_t tpm2_tpms_pcr_selection_weight(const TPMS_PCR_SELECTION *s) { uint32_t mask; tpm2_tpms_pcr_selection_to_mask(s, &mask); - return (size_t)__builtin_popcount(mask); + return popcount(mask); } /* Utility functions for TPML_PCR_SELECTION. */ diff --git a/src/test/test-logarithm.c b/src/test/test-logarithm.c index b6818b422c..b35fea9c27 100644 --- a/src/test/test-logarithm.c +++ b/src/test/test-logarithm.c @@ -71,4 +71,25 @@ TEST(log2i) { assert_se(log2i(INT_MAX) == sizeof(int)*8-2); } +TEST(popcount) { + uint16_t u16a = 0x0000; + uint16_t u16b = 0xFFFF; + uint32_t u32a = 0x00000010; + uint32_t u32b = 0xFFFFFFFF; + uint64_t u64a = 0x0000000000000010; + uint64_t u64b = 0x0100000000100010; + + assert_se(popcount(u16a) == 0); + assert_se(popcount(u16b) == 16); + assert_se(popcount(u32a) == 1); + assert_se(popcount(u32b) == 32); + assert_se(popcount(u64a) == 1); + assert_se(popcount(u64b) == 3); + + /* This would fail: + * error: ‘_Generic’ selector of type ‘int’ is not compatible with any association + * assert_se(popcount(0x10) == 1); + */ +} + DEFINE_TEST_MAIN(LOG_INFO); From e7416db183cb205146fd7fbb4e791b72cdae2b70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 12:43:53 +0200 Subject: [PATCH 04/12] core/unit: fix shell-escaping of strings Our escaping of '$' is '$$', not '\$'. We would write unit files that were not valid: $ systemd-run --user bash -c 'echo $$; sleep 1000' Running as unit: run-r1c7c45b5b69f487c86ae205e12100808.service $ systemctl cat --user run-r1c7c45b5b69f487c86ae205e12100808 # /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service ... ExecStart="/usr/bin/bash" "-c" "echo \$\$\; sleep 1000" $ systemd-analyze verify /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service:7: Ignoring unknown escape sequences: "echo \$\$\; sleep 1000" Similarly, ';' cannot be escaped as '\;'. Only a handful of characters listed in "Supported escapes" is allowed. Escaping of "'" can be done, but it's not useful because we use double quotes around the string anyway whenever we do escaping. unit_write_setting() is called all over the place. In a great majority of places we write either fixed strings or something that we generate ourselves, so no escaping or quoting is needed. (And it's not allowed, e.g. 'Type="oneshot"' would not work.) But if we forgot to add escaping or quoting for a free-style string, it would probably allow writing a unit file that would be read completely wrong. I looked over various places where unit_write_setting() is called, and I couldn't find any place where quoting/escaping was forgotten. But trying to figure out the full ramifications of this change is not easy. --- src/core/unit.c | 15 ++++++++++++--- src/test/test-core-unit.c | 10 +++++----- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 846d15b415..2d47d9de8c 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4332,10 +4332,17 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) } /* We either do C-escaping or shell-escaping, to additionally escape characters that we parse for - * ExecStart= and friends, i.e. '$' and ';' and quotes. */ + * ExecStart= and friends, i.e. '$' and quotes. */ if (flags & UNIT_ESCAPE_EXEC_SYNTAX) { - char *t2 = shell_escape(s, "$;'\""); + char *t2; + + t2 = strreplace(s, "$", "$$"); + if (!t2) + return NULL; + free_and_replace(t, t2); + + t2 = shell_escape(t, "\""); if (!t2) return NULL; free_and_replace(t, t2); @@ -4343,7 +4350,9 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) s = t; } else if (flags & UNIT_ESCAPE_C) { - char *t2 = cescape(s); + char *t2; + + t2 = cescape(s); if (!t2) return NULL; free_and_replace(t, t2); diff --git a/src/test/test-core-unit.c b/src/test/test-core-unit.c index 6593f2fb4b..ea514a5c8c 100644 --- a/src/test/test-core-unit.c +++ b/src/test/test-core-unit.c @@ -41,13 +41,13 @@ static void test_unit_escape_setting_one( TEST(unit_escape_setting) { test_unit_escape_setting_one("/sbin/sbash", NULL, NULL); - test_unit_escape_setting_one("$", "\\$", "$"); - test_unit_escape_setting_one("$$", "\\$\\$", "$$"); - test_unit_escape_setting_one("'", "\\'", NULL); + test_unit_escape_setting_one("$", "$$", "$"); + test_unit_escape_setting_one("$$", "$$$$", "$$"); + test_unit_escape_setting_one("'", "'", "\\'"); test_unit_escape_setting_one("\"", "\\\"", NULL); test_unit_escape_setting_one("\t", "\\t", NULL); test_unit_escape_setting_one(" ", NULL, NULL); - test_unit_escape_setting_one("$;'\"\t\n", "\\$\\;\\'\\\"\\t\\n", "$;\\'\\\"\\t\\n"); + test_unit_escape_setting_one("$;'\"\t\n", "$$;'\\\"\\t\\n", "$;\\'\\\"\\t\\n"); } static void test_unit_concat_strv_one( @@ -89,7 +89,7 @@ TEST(unit_concat_strv) { NULL); test_unit_concat_strv_one(STRV_MAKE("a", " ", "$", "$$", ""), "\"a\" \" \" \"$\" \"$$\" \"\"", - "\"a\" \" \" \"\\$\" \"\\$\\$\" \"\"", + "\"a\" \" \" \"$$\" \"$$$$\" \"\"", NULL); test_unit_concat_strv_one(STRV_MAKE("\n", " ", "\t"), "\"\n\" \" \" \"\t\"", From f3af62905004964a5c1aab763a250fe710cb802c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 14:50:12 +0200 Subject: [PATCH 05/12] =?UTF-8?q?core/unit:=20rename=20UNIT=5FESCAPE=5FEXE?= =?UTF-8?q?C=5FSYNTAX=20=E2=86=92=20*=5FENV?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In preparation for future changes. --- src/core/dbus-execute.c | 4 ++-- src/core/unit.c | 4 ++-- src/core/unit.h | 16 ++++++++-------- src/test/test-core-unit.c | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index d5ef796e52..88be591fe8 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1606,7 +1606,7 @@ int bus_set_transient_exec_command( if (!exec_chars) return -ENOMEM; - a = unit_concat_strv(c->argv, UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX); + a = unit_concat_strv(c->argv, UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX_ENV); if (!a) return -ENOMEM; @@ -1617,7 +1617,7 @@ int bus_set_transient_exec_command( const char *p; p = unit_escape_setting(c->path, - UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX, &t); + UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX_ENV, &t); if (!p) return -ENOMEM; diff --git a/src/core/unit.c b/src/core/unit.c index 2d47d9de8c..a6a0328e4d 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4312,7 +4312,7 @@ static const char* unit_drop_in_dir(Unit *u, UnitWriteFlags flags) { const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) { assert(s); - assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX | UNIT_ESCAPE_C)); + assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX_ENV | UNIT_ESCAPE_C)); assert(buf); _cleanup_free_ char *t = NULL; @@ -4334,7 +4334,7 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) /* We either do C-escaping or shell-escaping, to additionally escape characters that we parse for * ExecStart= and friends, i.e. '$' and quotes. */ - if (flags & UNIT_ESCAPE_EXEC_SYNTAX) { + if (flags & UNIT_ESCAPE_EXEC_SYNTAX_ENV) { char *t2; t2 = strreplace(s, "$", "$$"); diff --git a/src/core/unit.h b/src/core/unit.h index 420405b2b7..ff29b785bb 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -528,22 +528,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, + /* Apply specifier escaping */ + UNIT_ESCAPE_SPECIFIERS = 1 << 3, - /* Escape elements of ExecStart= syntax before writing */ - UNIT_ESCAPE_EXEC_SYNTAX = 1 << 4, + /* Escape elements of ExecStart= syntax, incl. prevention of variable expansion */ + UNIT_ESCAPE_EXEC_SYNTAX_ENV = 1 << 4, /* Apply C escaping before writing */ - UNIT_ESCAPE_C = 1 << 5, + UNIT_ESCAPE_C = 1 << 5, } UnitWriteFlags; /* Returns true if neither persistent, nor runtime storage is requested, i.e. this is a check invocation only */ diff --git a/src/test/test-core-unit.c b/src/test/test-core-unit.c index ea514a5c8c..91e6cdd6a3 100644 --- a/src/test/test-core-unit.c +++ b/src/test/test-core-unit.c @@ -26,7 +26,7 @@ static void test_unit_escape_setting_one( assert_se(a == NULL); assert_se(t == s); - assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_EXEC_SYNTAX, &b)); + assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_EXEC_SYNTAX_ENV, &b)); assert_se(b_esc = cescape(t)); log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); assert_se(b == NULL || streq(b, t)); @@ -71,7 +71,7 @@ static void test_unit_concat_strv_one( log_debug("%s: [%s] → [%s]", __func__, s_esc, a_esc); assert_se(streq(a, expected_none)); - assert_se(b = unit_concat_strv(s, UNIT_ESCAPE_EXEC_SYNTAX)); + assert_se(b = unit_concat_strv(s, UNIT_ESCAPE_EXEC_SYNTAX_ENV)); assert_se(b_esc = cescape(b)); log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); assert_se(streq(b, expected_exec)); From 8c41640a71fd03e4a2a45a28e311bbfd08e4c49a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 14:45:46 +0200 Subject: [PATCH 06/12] core/unit: add UNIT_ESCAPE_EXEC_SYNTAX Unfortunately we can't escape $ when ':' is used to prohibit variable expansion: ExecStart=:echo $$ is not the same as ExecStart=:echo $ This just adds the functionality and the unittests, without using it anywhere for real yet. --- src/core/unit.c | 17 +++++++----- src/core/unit.h | 5 +++- src/test/test-core-unit.c | 56 ++++++++++++++++++++++++++------------- 3 files changed, 52 insertions(+), 26 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index a6a0328e4d..6e20e86a63 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -36,6 +36,7 @@ #include "load-dropin.h" #include "load-fragment.h" #include "log.h" +#include "logarithm.h" #include "macro.h" #include "missing_audit.h" #include "mkdir-label.h" @@ -4312,7 +4313,7 @@ static const char* unit_drop_in_dir(Unit *u, UnitWriteFlags flags) { const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) { assert(s); - assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX_ENV | UNIT_ESCAPE_C)); + assert(popcount(flags & (UNIT_ESCAPE_EXEC_SYNTAX_ENV | UNIT_ESCAPE_EXEC_SYNTAX | UNIT_ESCAPE_C)) <= 1); assert(buf); _cleanup_free_ char *t = NULL; @@ -4334,15 +4335,17 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) /* We either do C-escaping or shell-escaping, to additionally escape characters that we parse for * ExecStart= and friends, i.e. '$' and quotes. */ - if (flags & UNIT_ESCAPE_EXEC_SYNTAX_ENV) { + if (flags & (UNIT_ESCAPE_EXEC_SYNTAX_ENV | UNIT_ESCAPE_EXEC_SYNTAX)) { char *t2; - t2 = strreplace(s, "$", "$$"); - if (!t2) - return NULL; - free_and_replace(t, t2); + if (flags & UNIT_ESCAPE_EXEC_SYNTAX_ENV) { + t2 = strreplace(s, "$", "$$"); + if (!t2) + return NULL; + free_and_replace(t, t2); + } - t2 = shell_escape(t, "\""); + t2 = shell_escape(t ?: s, "\""); if (!t2) return NULL; free_and_replace(t, t2); diff --git a/src/core/unit.h b/src/core/unit.h index ff29b785bb..06e5237e2c 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -542,8 +542,11 @@ typedef enum UnitWriteFlags { /* Escape elements of ExecStart= syntax, incl. prevention of variable expansion */ UNIT_ESCAPE_EXEC_SYNTAX_ENV = 1 << 4, + /* Escape elements of ExecStart=: syntax (no variable expansion) */ + UNIT_ESCAPE_EXEC_SYNTAX = 1 << 5, + /* Apply C escaping before writing */ - UNIT_ESCAPE_C = 1 << 5, + UNIT_ESCAPE_C = 1 << 6, } UnitWriteFlags; /* Returns true if neither persistent, nor runtime storage is requested, i.e. this is a check invocation only */ diff --git a/src/test/test-core-unit.c b/src/test/test-core-unit.c index 91e6cdd6a3..1a08507d1d 100644 --- a/src/test/test-core-unit.c +++ b/src/test/test-core-unit.c @@ -7,15 +7,18 @@ static void test_unit_escape_setting_one( const char *s, + const char *expected_exec_env, const char *expected_exec, const char *expected_c) { - _cleanup_free_ char *a = NULL, *b, *c, - *s_esc, *a_esc, *b_esc, *c_esc; + _cleanup_free_ char *a = NULL, *b, *c, *d, + *s_esc, *a_esc, *b_esc, *c_esc, *d_esc; const char *t; + if (!expected_exec_env) + expected_exec_env = s; if (!expected_exec) - expected_exec = s; + expected_exec = expected_exec_env; if (!expected_c) expected_c = expected_exec; assert_se(s_esc = cescape(s)); @@ -30,37 +33,46 @@ static void test_unit_escape_setting_one( assert_se(b_esc = cescape(t)); log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); assert_se(b == NULL || streq(b, t)); - assert_se(streq(t, expected_exec)); + assert_se(streq(t, expected_exec_env)); - assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_C, &c)); + assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_EXEC_SYNTAX, &c)); assert_se(c_esc = cescape(t)); log_debug("%s: [%s] → [%s]", __func__, s_esc, c_esc); assert_se(c == NULL || streq(c, t)); + assert_se(streq(t, expected_exec)); + + assert_se(t = unit_escape_setting(s, UNIT_ESCAPE_C, &d)); + assert_se(d_esc = cescape(t)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, d_esc); + assert_se(d == NULL || streq(d, t)); assert_se(streq(t, expected_c)); } TEST(unit_escape_setting) { - test_unit_escape_setting_one("/sbin/sbash", NULL, NULL); - test_unit_escape_setting_one("$", "$$", "$"); - test_unit_escape_setting_one("$$", "$$$$", "$$"); - test_unit_escape_setting_one("'", "'", "\\'"); - test_unit_escape_setting_one("\"", "\\\"", NULL); - test_unit_escape_setting_one("\t", "\\t", NULL); - test_unit_escape_setting_one(" ", NULL, NULL); - test_unit_escape_setting_one("$;'\"\t\n", "$$;'\\\"\\t\\n", "$;\\'\\\"\\t\\n"); + test_unit_escape_setting_one("/sbin/sbash", NULL, NULL, NULL); + test_unit_escape_setting_one("$", "$$", "$", "$"); + test_unit_escape_setting_one("$$", "$$$$", "$$", "$$"); + test_unit_escape_setting_one("'", "'", NULL, "\\'"); + test_unit_escape_setting_one("\"", "\\\"", NULL, NULL); + test_unit_escape_setting_one("\t", "\\t", NULL, NULL); + test_unit_escape_setting_one(" ", NULL, NULL, NULL); + test_unit_escape_setting_one("$;'\"\t\n", "$$;'\\\"\\t\\n", "$;'\\\"\\t\\n", "$;\\'\\\"\\t\\n"); } static void test_unit_concat_strv_one( char **s, const char *expected_none, + const char *expected_exec_env, const char *expected_exec, const char *expected_c) { - _cleanup_free_ char *a, *b, *c, - *s_ser, *s_esc, *a_esc, *b_esc, *c_esc; + _cleanup_free_ char *a, *b, *c, *d, + *s_ser, *s_esc, *a_esc, *b_esc, *c_esc, *d_esc; assert_se(s_ser = strv_join(s, "_")); assert_se(s_esc = cescape(s_ser)); + if (!expected_exec_env) + expected_exec_env = expected_none; if (!expected_exec) expected_exec = expected_none; if (!expected_c) @@ -74,26 +86,34 @@ static void test_unit_concat_strv_one( assert_se(b = unit_concat_strv(s, UNIT_ESCAPE_EXEC_SYNTAX_ENV)); assert_se(b_esc = cescape(b)); log_debug("%s: [%s] → [%s]", __func__, s_esc, b_esc); - assert_se(streq(b, expected_exec)); + assert_se(streq(b, expected_exec_env)); - assert_se(c = unit_concat_strv(s, UNIT_ESCAPE_C)); + assert_se(c = unit_concat_strv(s, UNIT_ESCAPE_EXEC_SYNTAX)); assert_se(c_esc = cescape(c)); log_debug("%s: [%s] → [%s]", __func__, s_esc, c_esc); - assert_se(streq(c, expected_c)); + assert_se(streq(c, expected_exec)); + + assert_se(d = unit_concat_strv(s, UNIT_ESCAPE_C)); + assert_se(d_esc = cescape(d)); + log_debug("%s: [%s] → [%s]", __func__, s_esc, d_esc); + assert_se(streq(d, expected_c)); } TEST(unit_concat_strv) { test_unit_concat_strv_one(STRV_MAKE("a", "b", "c"), "\"a\" \"b\" \"c\"", NULL, + NULL, NULL); test_unit_concat_strv_one(STRV_MAKE("a", " ", "$", "$$", ""), "\"a\" \" \" \"$\" \"$$\" \"\"", "\"a\" \" \" \"$$\" \"$$$$\" \"\"", + NULL, NULL); test_unit_concat_strv_one(STRV_MAKE("\n", " ", "\t"), "\"\n\" \" \" \"\t\"", "\"\\n\" \" \" \"\\t\"", + "\"\\n\" \" \" \"\\t\"", "\"\\n\" \" \" \"\\t\""); } From 0a27d86a3f3d6b577a094f6024a1db0eac76da85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 13:56:18 +0200 Subject: [PATCH 07/12] core: fix writing of ExecStartEx and friends The property name is called ExecStartEx, but we have to write it as ExecStart= in the unit file. :( Bug introduced in b3d593673c5b8b0b7d781fd26ab2062ca6e7dbdb when ex-properties were initially added. In addition, we cannot escape $ as $$, because when ":" is used, we wouldn't unescape $$ back to $. --- src/core/dbus-execute.c | 18 +++++++++++------- src/core/dbus-service.c | 3 ++- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 88be591fe8..6ad1d6b5fe 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1516,6 +1516,9 @@ int bus_set_transient_exec_command( unsigned n = 0; int r; + /* Drop Ex from the written setting. E.g. ExecStart=, not ExecStartEx=. */ + const char *written_name = is_ex_prop ? strndupa(name, strlen(name) - 2) : name; + r = sd_bus_message_enter_container(message, 'a', is_ex_prop ? "(sasas)" : "(sasb)"); if (r < 0) return r; @@ -1597,31 +1600,32 @@ int bus_set_transient_exec_command( if (!f) return -ENOMEM; - fprintf(f, "%s=\n", name); + fprintf(f, "%s=\n", written_name); LIST_FOREACH(command, c, *exec_command) { _cleanup_free_ char *a = NULL, *exec_chars = NULL; + UnitWriteFlags esc_flags = UNIT_ESCAPE_SPECIFIERS | + (FLAGS_SET(c->flags, EXEC_COMMAND_NO_ENV_EXPAND) ? UNIT_ESCAPE_EXEC_SYNTAX : UNIT_ESCAPE_EXEC_SYNTAX_ENV); exec_chars = exec_command_flags_to_exec_chars(c->flags); if (!exec_chars) return -ENOMEM; - a = unit_concat_strv(c->argv, UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX_ENV); + a = unit_concat_strv(c->argv, esc_flags); if (!a) return -ENOMEM; if (streq_ptr(c->path, c->argv ? c->argv[0] : NULL)) - fprintf(f, "%s=%s%s\n", name, exec_chars, a); + fprintf(f, "%s=%s%s\n", written_name, exec_chars, a); else { _cleanup_free_ char *t = NULL; const char *p; - p = unit_escape_setting(c->path, - UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX_ENV, &t); + p = unit_escape_setting(c->path, esc_flags, &t); if (!p) return -ENOMEM; - fprintf(f, "%s=%s@%s %s\n", name, exec_chars, p, a); + fprintf(f, "%s=%s@%s %s\n", written_name, exec_chars, p, a); } } @@ -1629,7 +1633,7 @@ int bus_set_transient_exec_command( if (r < 0) return r; - unit_write_setting(u, flags, name, buf); + unit_write_setting(u, flags, written_name, buf); } return 1; diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 0f6e315233..ff61495c34 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -635,7 +635,8 @@ static int bus_service_set_transient_property( return bus_set_transient_exit_status(u, name, &s->success_status, message, flags, error); ci = service_exec_command_from_string(name); - ci = (ci >= 0) ? ci : service_exec_ex_command_from_string(name); + if (ci < 0) + ci = service_exec_ex_command_from_string(name); if (ci >= 0) return bus_set_transient_exec_command(u, name, &s->exec_command[ci], message, flags, error); From ac9a75d05e671021add6f9334878b7a8056cfb2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 2 Apr 2023 21:50:19 +0200 Subject: [PATCH 08/12] run: simplify returning of status start_transient_service() would return two ints: one normally and one via *retval. We can just return one int and propagate it directly, because we use DEFINE_MAIN_FUNCTION_WITH_POSITIVE_FAILURE(). --- src/run/run.c | 51 +++++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index 8377c2e8cd..ad8cd82d8f 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1109,10 +1109,7 @@ static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) { return 0; } -static int start_transient_service( - sd_bus *bus, - int *retval) { - +static int start_transient_service(sd_bus *bus) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; @@ -1121,7 +1118,6 @@ static int start_transient_service( int r; assert(bus); - assert(retval); if (arg_stdio == ARG_STDIO_PTY) { @@ -1360,16 +1356,15 @@ static int start_transient_service( /* Try to propagate the service's return value. But if the service defines * e.g. SuccessExitStatus, honour this, and return 0 to mean "success". */ if (streq_ptr(c.result, "success")) - *retval = 0; - else if (streq_ptr(c.result, "exit-code") && c.exit_status > 0) - *retval = c.exit_status; - else if (streq_ptr(c.result, "signal")) - *retval = EXIT_EXCEPTION; - else - *retval = EXIT_FAILURE; + return EXIT_SUCCESS; + if (streq_ptr(c.result, "exit-code") && c.exit_status > 0) + return c.exit_status; + if (streq_ptr(c.result, "signal")) + return EXIT_EXCEPTION; + return EXIT_FAILURE; } - return 0; + return EXIT_SUCCESS; } static int acquire_invocation_id(sd_bus *bus, sd_id128_t *ret) { @@ -1555,10 +1550,7 @@ static int start_transient_scope(sd_bus *bus) { return log_error_errno(errno, "Failed to execute: %m"); } -static int start_transient_trigger( - sd_bus *bus, - const char *suffix) { - +static int start_transient_trigger(sd_bus *bus, const char *suffix) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; @@ -1707,7 +1699,7 @@ static int start_transient_trigger( log_info("Will run service as unit: %s", service); } - return 0; + return EXIT_SUCCESS; } static bool shall_make_executable_absolute(void) { @@ -1726,7 +1718,7 @@ static bool shall_make_executable_absolute(void) { static int run(int argc, char* argv[]) { _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *description = NULL; - int r, retval = EXIT_SUCCESS; + int r; log_show_color(true); log_parse_environment(); @@ -1773,19 +1765,14 @@ static int run(int argc, char* argv[]) { return bus_log_connect_error(r, arg_transport); if (arg_scope) - r = start_transient_scope(bus); - else if (arg_path_property) - r = start_transient_trigger(bus, ".path"); - else if (arg_socket_property) - r = start_transient_trigger(bus, ".socket"); - else if (arg_with_timer) - r = start_transient_trigger(bus, ".timer"); - else - r = start_transient_service(bus, &retval); - if (r < 0) - return r; - - return retval; + return start_transient_scope(bus); + if (arg_path_property) + return start_transient_trigger(bus, ".path"); + if (arg_socket_property) + return start_transient_trigger(bus, ".socket"); + if (arg_with_timer) + return start_transient_trigger(bus, ".timer"); + return start_transient_service(bus); } DEFINE_MAIN_FUNCTION_WITH_POSITIVE_FAILURE(run); From b58026bddce8cc418c10e1c69f96de34b0dffcbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 2 Apr 2023 22:27:58 +0200 Subject: [PATCH 09/12] run: split out creation of unit creation messages Just refactoring, in preparation for future changes. (Though I think it'd be reasonable to do anyway, those functions were awfully long.) 'git diff' displays this badly. The middle part of start_transient_service() is moved to make_transient_service_unit(), and the middle part of start_transient_trigger() is moved to make_transient_trigger_unit(). --- src/run/run.c | 218 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 132 insertions(+), 86 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index ad8cd82d8f..409212cbfa 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1109,6 +1109,54 @@ static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) { return 0; } +static int make_transient_service_unit( + sd_bus *bus, + sd_bus_message **message, + const char *service, + const char *pty_path) { + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + int r; + + assert(bus); + assert(message); + assert(service); + + r = bus_message_new_method_call(bus, &m, bus_systemd_mgr, "StartTransientUnit"); + if (r < 0) + return bus_log_create_error(r); + + r = sd_bus_message_set_allow_interactive_authorization(m, arg_ask_password); + if (r < 0) + return bus_log_create_error(r); + + /* Name and mode */ + r = sd_bus_message_append(m, "ss", service, "fail"); + if (r < 0) + return bus_log_create_error(r); + + /* Properties */ + r = sd_bus_message_open_container(m, 'a', "(sv)"); + if (r < 0) + return bus_log_create_error(r); + + r = transient_service_set_properties(m, pty_path); + if (r < 0) + return r; + + r = sd_bus_message_close_container(m); + if (r < 0) + return bus_log_create_error(r); + + /* Auxiliary units */ + r = sd_bus_message_append(m, "a(sa(sv))", 0); + if (r < 0) + return bus_log_create_error(r); + + *message = TAKE_PTR(m); + return 0; +} + static int start_transient_service(sd_bus *bus) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -1190,37 +1238,10 @@ static int start_transient_service(sd_bus *bus) { return r; } - r = bus_message_new_method_call(bus, &m, bus_systemd_mgr, "StartTransientUnit"); - if (r < 0) - return bus_log_create_error(r); - - r = sd_bus_message_set_allow_interactive_authorization(m, arg_ask_password); - if (r < 0) - return bus_log_create_error(r); - - /* Name and mode */ - r = sd_bus_message_append(m, "ss", service, "fail"); - if (r < 0) - return bus_log_create_error(r); - - /* Properties */ - r = sd_bus_message_open_container(m, 'a', "(sv)"); - if (r < 0) - return bus_log_create_error(r); - - r = transient_service_set_properties(m, pty_path); + r = make_transient_service_unit(bus, &m, service, pty_path); if (r < 0) return r; - r = sd_bus_message_close_container(m); - if (r < 0) - return bus_log_create_error(r); - - /* Auxiliary units */ - r = sd_bus_message_append(m, "a(sa(sv))", 0); - if (r < 0) - return bus_log_create_error(r); - polkit_agent_open_if_enabled(arg_transport, arg_ask_password); r = sd_bus_call(bus, m, 0, &error, &reply); @@ -1550,67 +1571,21 @@ static int start_transient_scope(sd_bus *bus) { return log_error_errno(errno, "Failed to execute: %m"); } -static int start_transient_trigger(sd_bus *bus, const char *suffix) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; - _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; - _cleanup_free_ char *trigger = NULL, *service = NULL; - const char *object = NULL; +static int make_transient_trigger_unit( + sd_bus *bus, + sd_bus_message **message, + const char *suffix, + const char *trigger, + const char *service) { + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; int r; assert(bus); - - r = bus_wait_for_jobs_new(bus, &w); - if (r < 0) - return log_oom(); - - if (arg_unit) { - switch (unit_name_to_type(arg_unit)) { - - case UNIT_SERVICE: - service = strdup(arg_unit); - if (!service) - return log_oom(); - - r = unit_name_change_suffix(service, suffix, &trigger); - if (r < 0) - return log_error_errno(r, "Failed to change unit suffix: %m"); - break; - - case UNIT_TIMER: - trigger = strdup(arg_unit); - if (!trigger) - return log_oom(); - - r = unit_name_change_suffix(trigger, ".service", &service); - if (r < 0) - return log_error_errno(r, "Failed to change unit suffix: %m"); - break; - - default: - r = unit_name_mangle_with_suffix(arg_unit, "as unit", - arg_quiet ? 0 : UNIT_NAME_MANGLE_WARN, - ".service", &service); - if (r < 0) - return log_error_errno(r, "Failed to mangle unit name: %m"); - - r = unit_name_mangle_with_suffix(arg_unit, "as trigger", - arg_quiet ? 0 : UNIT_NAME_MANGLE_WARN, - suffix, &trigger); - if (r < 0) - return log_error_errno(r, "Failed to mangle unit name: %m"); - - break; - } - } else { - r = make_unit_name(bus, UNIT_SERVICE, &service); - if (r < 0) - return r; - - r = unit_name_change_suffix(service, suffix, &trigger); - if (r < 0) - return log_error_errno(r, "Failed to change unit suffix: %m"); - } + assert(message); + assert(suffix); + assert(trigger); + assert(service); r = bus_message_new_method_call(bus, &m, bus_systemd_mgr, "StartTransientUnit"); if (r < 0) @@ -1679,6 +1654,77 @@ static int start_transient_trigger(sd_bus *bus, const char *suffix) { if (r < 0) return bus_log_create_error(r); + *message = TAKE_PTR(m); + return 0; +} + +static int start_transient_trigger(sd_bus *bus, const char *suffix) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; + _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; + _cleanup_free_ char *trigger = NULL, *service = NULL; + const char *object = NULL; + int r; + + assert(bus); + assert(suffix); + + r = bus_wait_for_jobs_new(bus, &w); + if (r < 0) + return log_oom(); + + if (arg_unit) { + switch (unit_name_to_type(arg_unit)) { + + case UNIT_SERVICE: + service = strdup(arg_unit); + if (!service) + return log_oom(); + + r = unit_name_change_suffix(service, suffix, &trigger); + if (r < 0) + return log_error_errno(r, "Failed to change unit suffix: %m"); + break; + + case UNIT_TIMER: + trigger = strdup(arg_unit); + if (!trigger) + return log_oom(); + + r = unit_name_change_suffix(trigger, ".service", &service); + if (r < 0) + return log_error_errno(r, "Failed to change unit suffix: %m"); + break; + + default: + r = unit_name_mangle_with_suffix(arg_unit, "as unit", + arg_quiet ? 0 : UNIT_NAME_MANGLE_WARN, + ".service", &service); + if (r < 0) + return log_error_errno(r, "Failed to mangle unit name: %m"); + + r = unit_name_mangle_with_suffix(arg_unit, "as trigger", + arg_quiet ? 0 : UNIT_NAME_MANGLE_WARN, + suffix, &trigger); + if (r < 0) + return log_error_errno(r, "Failed to mangle unit name: %m"); + + break; + } + } else { + r = make_unit_name(bus, UNIT_SERVICE, &service); + if (r < 0) + return r; + + r = unit_name_change_suffix(service, suffix, &trigger); + if (r < 0) + return log_error_errno(r, "Failed to change unit suffix: %m"); + } + + r = make_transient_trigger_unit(bus, &m, suffix, trigger, service); + if (r < 0) + return r; + polkit_agent_open_if_enabled(arg_transport, arg_ask_password); r = sd_bus_call(bus, m, 0, &error, &reply); From f872ddd182bd33d9ba0569d050374b9b9a9a2ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 2 Apr 2023 23:17:58 +0200 Subject: [PATCH 10/12] run: add --expand-environment=no to disable server-side envvar expansion This uses StartExecEx to get the equivalent of ExecStart=:. StartExecEx was added in b3d593673c5b8b0b7d781fd26ab2062ca6e7dbdb, so this will not work with older systemds. A hint is emitted if we get an error indicating lack of support. PID1 returns SD_BUS_ERROR_PROPERTY_READ_ONLY, but I'm checking for SD_BUS_ERROR_UNKNOWN_PROPERTY too for safety. --- man/systemd-run.xml | 26 +++++++- src/run/run.c | 149 +++++++++++++++++++++++++++++--------------- 2 files changed, 123 insertions(+), 52 deletions(-) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index cd9e50d5b8..2ad68d8884 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -92,6 +92,11 @@ Consider using the service type (i.e. ) to ensure that systemd-run returns successfully only if the specified command line has been successfully started. + + After systemd-run passes the command to the service manager, the manager + performs variable expansion. This means that dollar characters ($) which should not be + expanded need to be escaped as $$. Expansion can also be disabled using + --expand-environment=no. @@ -169,6 +174,24 @@ + + + + Expand environment variables in command arguments. If enabled (the default), the + service manager that spawns the actual command will expand variables specified as + ${VARIABLE} in the same way as in commands specied via + ExecStart= in units. Note that this is similar to, but not the same as variable + expansion in + bash1 + and other shells. + + See + systemd.service5 + for a description of variable expansion. Disabling variable expansion is useful if the specified + command includes or may include a $ sign. + + + @@ -533,7 +556,8 @@ There is a screen on: $ systemd-run --user --wait true $ systemd-run --user --wait -p SuccessExitStatus=11 bash -c 'exit 11' -$ systemd-run --user --wait -p SuccessExitStatus=SIGUSR1 bash -c 'kill -SIGUSR1 $$$$' +$ systemd-run --user --wait -p SuccessExitStatus=SIGUSR1 --expand-environment=no \ + bash -c 'kill -SIGUSR1 $$' Those three invocations will succeed, i.e. terminate with an exit code of 0. diff --git a/src/run/run.c b/src/run/run.c index 409212cbfa..3ad866aaa6 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -45,6 +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 bool arg_send_sighup = false; static BusTransport arg_transport = BUS_TRANSPORT_LOCAL; static const char *arg_host = NULL; @@ -102,6 +103,8 @@ static int help(void) { " --description=TEXT Description for unit\n" " --slice=SLICE Run in the specified slice\n" " --slice-inherit Inherit the slice\n" + " --expand-environment=BOOL Control server-side expansion of environment\n" + " variables\n" " --no-block Do not wait until operation finished\n" " -r --remain-after-exit Leave service around until explicitly stopped\n" " --wait Wait until service stopped again\n" @@ -168,6 +171,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_DESCRIPTION, ARG_SLICE, ARG_SLICE_INHERIT, + ARG_EXPAND_ENVIRONMENT, ARG_SEND_SIGHUP, ARG_SERVICE_TYPE, ARG_EXEC_USER, @@ -192,47 +196,48 @@ static int parse_argv(int argc, char *argv[]) { }; static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, ARG_VERSION }, - { "user", no_argument, NULL, ARG_USER }, - { "system", no_argument, NULL, ARG_SYSTEM }, - { "scope", no_argument, NULL, ARG_SCOPE }, - { "unit", required_argument, NULL, 'u' }, - { "description", required_argument, NULL, ARG_DESCRIPTION }, - { "slice", required_argument, NULL, ARG_SLICE }, - { "slice-inherit", no_argument, NULL, ARG_SLICE_INHERIT }, - { "remain-after-exit", no_argument, NULL, 'r' }, - { "send-sighup", no_argument, NULL, ARG_SEND_SIGHUP }, - { "host", required_argument, NULL, 'H' }, - { "machine", required_argument, NULL, 'M' }, - { "service-type", required_argument, NULL, ARG_SERVICE_TYPE }, - { "wait", no_argument, NULL, ARG_WAIT }, - { "uid", required_argument, NULL, ARG_EXEC_USER }, - { "gid", required_argument, NULL, ARG_EXEC_GROUP }, - { "nice", required_argument, NULL, ARG_NICE }, - { "setenv", required_argument, NULL, 'E' }, - { "property", required_argument, NULL, 'p' }, - { "tty", no_argument, NULL, 't' }, /* deprecated alias */ - { "pty", no_argument, NULL, 't' }, - { "pipe", no_argument, NULL, 'P' }, - { "quiet", no_argument, NULL, 'q' }, - { "on-active", required_argument, NULL, ARG_ON_ACTIVE }, - { "on-boot", required_argument, NULL, ARG_ON_BOOT }, - { "on-startup", required_argument, NULL, ARG_ON_STARTUP }, - { "on-unit-active", required_argument, NULL, ARG_ON_UNIT_ACTIVE }, - { "on-unit-inactive", required_argument, NULL, ARG_ON_UNIT_INACTIVE }, - { "on-calendar", required_argument, NULL, ARG_ON_CALENDAR }, - { "on-timezone-change",no_argument, NULL, ARG_ON_TIMEZONE_CHANGE}, - { "on-clock-change", no_argument, NULL, ARG_ON_CLOCK_CHANGE }, - { "timer-property", required_argument, NULL, ARG_TIMER_PROPERTY }, - { "path-property", required_argument, NULL, ARG_PATH_PROPERTY }, - { "socket-property", required_argument, NULL, ARG_SOCKET_PROPERTY }, - { "no-block", no_argument, NULL, ARG_NO_BLOCK }, - { "no-ask-password", no_argument, NULL, ARG_NO_ASK_PASSWORD }, - { "collect", no_argument, NULL, 'G' }, - { "working-directory", required_argument, NULL, ARG_WORKING_DIRECTORY }, - { "same-dir", no_argument, NULL, 'd' }, - { "shell", no_argument, NULL, 'S' }, + { "help", no_argument, NULL, 'h' }, + { "version", no_argument, NULL, ARG_VERSION }, + { "user", no_argument, NULL, ARG_USER }, + { "system", no_argument, NULL, ARG_SYSTEM }, + { "scope", no_argument, NULL, ARG_SCOPE }, + { "unit", required_argument, NULL, 'u' }, + { "description", required_argument, NULL, ARG_DESCRIPTION }, + { "slice", required_argument, NULL, ARG_SLICE }, + { "slice-inherit", no_argument, NULL, ARG_SLICE_INHERIT }, + { "remain-after-exit", no_argument, NULL, 'r' }, + { "expand-environment", required_argument, NULL, ARG_EXPAND_ENVIRONMENT }, + { "send-sighup", no_argument, NULL, ARG_SEND_SIGHUP }, + { "host", required_argument, NULL, 'H' }, + { "machine", required_argument, NULL, 'M' }, + { "service-type", required_argument, NULL, ARG_SERVICE_TYPE }, + { "wait", no_argument, NULL, ARG_WAIT }, + { "uid", required_argument, NULL, ARG_EXEC_USER }, + { "gid", required_argument, NULL, ARG_EXEC_GROUP }, + { "nice", required_argument, NULL, ARG_NICE }, + { "setenv", required_argument, NULL, 'E' }, + { "property", required_argument, NULL, 'p' }, + { "tty", no_argument, NULL, 't' }, /* deprecated alias */ + { "pty", no_argument, NULL, 't' }, + { "pipe", no_argument, NULL, 'P' }, + { "quiet", no_argument, NULL, 'q' }, + { "on-active", required_argument, NULL, ARG_ON_ACTIVE }, + { "on-boot", required_argument, NULL, ARG_ON_BOOT }, + { "on-startup", required_argument, NULL, ARG_ON_STARTUP }, + { "on-unit-active", required_argument, NULL, ARG_ON_UNIT_ACTIVE }, + { "on-unit-inactive", required_argument, NULL, ARG_ON_UNIT_INACTIVE }, + { "on-calendar", required_argument, NULL, ARG_ON_CALENDAR }, + { "on-timezone-change", no_argument, NULL, ARG_ON_TIMEZONE_CHANGE }, + { "on-clock-change", no_argument, NULL, ARG_ON_CLOCK_CHANGE }, + { "timer-property", required_argument, NULL, ARG_TIMER_PROPERTY }, + { "path-property", required_argument, NULL, ARG_PATH_PROPERTY }, + { "socket-property", required_argument, NULL, ARG_SOCKET_PROPERTY }, + { "no-block", no_argument, NULL, ARG_NO_BLOCK }, + { "no-ask-password", no_argument, NULL, ARG_NO_ASK_PASSWORD }, + { "collect", no_argument, NULL, 'G' }, + { "working-directory", required_argument, NULL, ARG_WORKING_DIRECTORY }, + { "same-dir", no_argument, NULL, 'd' }, + { "shell", no_argument, NULL, 'S' }, {}, }; @@ -284,6 +289,12 @@ 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); + if (r < 0) + return r; + break; + case ARG_SEND_SIGHUP: arg_send_sighup = true; break; @@ -716,6 +727,11 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p bool send_term = false; int r; + /* 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; + assert(m); r = transient_unit_set_properties(m, UNIT_SERVICE, arg_property); @@ -847,19 +863,23 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_append(m, "s", "ExecStart"); + r = sd_bus_message_append(m, "s", + use_ex_prop ? "ExecStartEx" : "ExecStart"); if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_open_container(m, 'v', "a(sasb)"); + r = sd_bus_message_open_container(m, 'v', + use_ex_prop ? "a(sasas)" : "a(sasb)"); if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_open_container(m, 'a', "(sasb)"); + r = sd_bus_message_open_container(m, 'a', + use_ex_prop ? "(sasas)" : "(sasb)"); if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_open_container(m, 'r', "sasb"); + r = sd_bus_message_open_container(m, 'r', + use_ex_prop ? "sasas" : "sasb"); if (r < 0) return bus_log_create_error(r); @@ -871,7 +891,12 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_append(m, "b", false); + if (use_ex_prop) + r = sd_bus_message_append_strv( + m, + STRV_MAKE(arg_expand_environment ? NULL : "no-env-expand")); + else + r = sd_bus_message_append(m, "b", false); if (r < 0) return bus_log_create_error(r); @@ -1157,6 +1182,29 @@ static int make_transient_service_unit( return 0; } +static int bus_call_with_hint( + sd_bus *bus, + sd_bus_message *message, + const char *name, + sd_bus_message **reply) { + + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + int r; + + r = sd_bus_call(bus, message, 0, &error, reply); + if (r < 0) { + log_error_errno(r, "Failed to start transient %s unit: %s", name, bus_error_message(&error, r)); + + if (!arg_expand_environment && + sd_bus_error_has_names(&error, + SD_BUS_ERROR_UNKNOWN_PROPERTY, + SD_BUS_ERROR_PROPERTY_READ_ONLY)) + log_notice_errno(r, "Hint: --expand-environment=no is not supported by old systemd"); + } + + return r; +} + static int start_transient_service(sd_bus *bus) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -1244,9 +1292,9 @@ static int start_transient_service(sd_bus *bus) { polkit_agent_open_if_enabled(arg_transport, arg_ask_password); - r = sd_bus_call(bus, m, 0, &error, &reply); + r = bus_call_with_hint(bus, m, "service", &reply); if (r < 0) - return log_error_errno(r, "Failed to start transient service unit: %s", bus_error_message(&error, r)); + return r; if (w) { const char *object; @@ -1659,7 +1707,6 @@ static int make_transient_trigger_unit( } static int start_transient_trigger(sd_bus *bus, const char *suffix) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; _cleanup_free_ char *trigger = NULL, *service = NULL; @@ -1727,9 +1774,9 @@ static int start_transient_trigger(sd_bus *bus, const char *suffix) { polkit_agent_open_if_enabled(arg_transport, arg_ask_password); - r = sd_bus_call(bus, m, 0, &error, &reply); + r = bus_call_with_hint(bus, m, suffix + 1, &reply); if (r < 0) - return log_error_errno(r, "Failed to start transient %s unit: %s", suffix + 1, bus_error_message(&error, r)); + return r; r = sd_bus_message_read(reply, "o", &object); if (r < 0) From de99fadd3117d2bbe3d5fdf1c6e7b6855fccf465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 3 Apr 2023 08:26:56 +0200 Subject: [PATCH 11/12] man/systemd-run: add examples explaining how variable expansion is performed --- man/systemd-run.xml | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index 2ad68d8884..f33190f4c5 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -551,6 +551,42 @@ There is a screen on: $ loginctl enable-linger + + Variable expansion by the manager + + $ systemd-run -t echo "<${INVOCATION_ID}>" '<${INVOCATION_ID}>' + <> <5d0149bfa2c34b79bccb13074001eb20> + + + The first argument is expanded by the shell (double quotes), but the second one is not expanded + by the shell (single quotes). echo is called with [/usr/bin/echo, + [], [${INVOCATION_ID}]] as the argument array, and then + systemd generates ${INVOCATION_ID} and substitutes it in the + command-line. This substitution could not be done on the client side, because the target ID that will + be set for the service isn't known before the call is made. + + + + Variable expansion and output redirection using a shell + + Variable expansion by systemd can be disabled with + --expand-environment=no. + + Disabling variable expansion can be useful if the command to execute contains dollar characters + and escaping them would be inconvenient. For example, when a shell is used: + + $ systemd-run --expand-environment=no -t bash \ + -c 'echo $SHELL $$ >/dev/stdout' +/bin/bash 12345 + + + The last argument is passed verbatim to the bash shell which is started by the + service unit. The shell expands $SHELL to the path of the shell, and + $$ to its process number, and then those strings are passed to the + echo built-in and printed to standard output (which in this case is connected to the + calling terminal). + + Return value From 2ed7a221fafb25eea937c4e86fb88ee501dba51e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 4 Apr 2023 21:18:33 +0200 Subject: [PATCH 12/12] run: expand variables also with --scope This makes syntax be the same for commands which are started by the manager and those which are spawned directly (when --scope is used). Before: $ systemd-run -q -t echo '$TERM' xterm-256color $ systemd-run -q --scope echo '$TERM' $TERM Now: $ systemd-run -q --scope echo '$TERM' xterm-256color Previous behaviour can be restored via --expand-environment=no: $ systemd-run -q --scope --expand-environment=no echo '$TERM' $TERM Fixes #22948. At some level, this is a compat break. Fortunately --scope is not very widely used, so I think we can get away with this. Having different syntax depending on whether --scope was used or not was bad UX. A NEWS entry will be required. --- man/systemd-run.xml | 11 ++++++----- src/run/run.c | 12 +++++++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index f33190f4c5..73adbfb927 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -177,11 +177,12 @@ - Expand environment variables in command arguments. If enabled (the default), the - service manager that spawns the actual command will expand variables specified as - ${VARIABLE} in the same way as in commands specied via - ExecStart= in units. Note that this is similar to, but not the same as variable - expansion in + 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 + --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. diff --git a/src/run/run.c b/src/run/run.c index 3ad866aaa6..2f5ec9c73b 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -103,8 +103,7 @@ static int help(void) { " --description=TEXT Description for unit\n" " --slice=SLICE Run in the specified slice\n" " --slice-inherit Inherit the slice\n" - " --expand-environment=BOOL Control server-side expansion of environment\n" - " variables\n" + " --expand-environment=BOOL Control expansion of environment variables\n" " --no-block Do not wait until operation finished\n" " -r --remain-after-exit Leave service around until explicitly stopped\n" " --wait Wait until service stopped again\n" @@ -1472,7 +1471,7 @@ static int start_transient_scope(sd_bus *bus) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL; - _cleanup_strv_free_ char **env = NULL, **user_env = NULL; + _cleanup_strv_free_ char **env = NULL, **user_env = NULL, **expanded_cmdline = NULL; _cleanup_free_ char *scope = NULL; const char *object = NULL; sd_id128_t invocation_id; @@ -1614,6 +1613,13 @@ static int start_transient_scope(sd_bus *bus) { if (!arg_quiet) log_info("Running scope as unit: %s", scope); + if (arg_expand_environment) { + expanded_cmdline = replace_env_argv(arg_cmdline, env); + if (!expanded_cmdline) + return log_oom(); + arg_cmdline = expanded_cmdline; + } + execvpe(arg_cmdline[0], arg_cmdline, env); return log_error_errno(errno, "Failed to execute: %m");