From 23c3c5d4234cb9f157b5c0d6aa6fbfed601a3202 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 08:49:17 +0200 Subject: 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 | 242 +++++++++++++++++++++++++-------------------------- 1 file changed, 120 insertions(+), 122 deletions(-) (limited to 'src/test') 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, @@ -525,39 +424,144 @@ tests += [ 'type' : 'manual', }, { - 'sources' : files('test-unit-name.c'), + '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-unit-serialize.c'), + 'sources' : files('test-bpf-firewall.c'), 'dependencies' : common_test_dependencies, 'base' : test_core_base, }, { - 'sources' : files('test-utmp.c'), - 'condition' : 'ENABLE_UTMP', + 'sources' : files('test-bpf-foreign-programs.c'), + 'base' : test_core_base, }, { - 'sources' : files('test-varlink.c'), - 'dependencies' : threads, + 'sources' : files('test-bpf-lsm.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, }, { - 'sources' : files('test-watch-pid.c'), + '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-watchdog.c'), - 'type' : 'unsafe', + '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, + 'base' : test_core_base, + }, + { + 'sources' : files('test-unit-serialize.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, + }, + { + 'sources' : files('test-watch-pid.c'), + 'dependencies' : common_test_dependencies, + 'base' : test_core_base, }, -] - -############################################################ - -# 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, - }, ] -- cgit v1.2.3 From 6eccc3cfa9dcfea3c8b508a66d2d592e6b9fcb93 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 11:42:41 +0200 Subject: 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 (limited to 'src/test') 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); -- cgit v1.2.3 From a12bc99ef0fc04fa48767c891f7a6db6404e51d5 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 14:32:39 +0200 Subject: 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/basic/logarithm.h | 53 ---------------------------------------- src/boot/efi/splash.c | 9 ++++--- src/fundamental/logarithm.h | 59 +++++++++++++++++++++++++++++++++++++++++++++ src/shared/tpm2-util.c | 4 ++- src/test/test-logarithm.c | 21 ++++++++++++++++ 5 files changed, 88 insertions(+), 58 deletions(-) delete mode 100644 src/basic/logarithm.h create mode 100644 src/fundamental/logarithm.h (limited to 'src/test') diff --git a/src/basic/logarithm.h b/src/basic/logarithm.h deleted file mode 100644 index 646f2d3613..0000000000 --- a/src/basic/logarithm.h +++ /dev/null @@ -1,53 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -#pragma once - -#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) -#define NONCONST_LOG2ULL(x) ({ \ - unsigned long long _x = (x); \ - _x > 1 ? (unsigned) __builtin_clzll(_x) ^ 63U : 0; \ - }) -#define LOG2ULL(x) __builtin_choose_expr(__builtin_constant_p(x), CONST_LOG2ULL(x), NONCONST_LOG2ULL(x)) - -static inline unsigned log2u64(uint64_t x) { -#if __SIZEOF_LONG_LONG__ == 8 - return LOG2ULL(x); -#else -# error "Wut?" -#endif -} - -static inline unsigned u32ctz(uint32_t n) { -#if __SIZEOF_INT__ == 4 - return n != 0 ? __builtin_ctz(n) : 32; -#else -# error "Wut?" -#endif -} - -#define CONST_LOG2U(x) ((x) > 1 ? __SIZEOF_INT__ * 8 - __builtin_clz(x) - 1 : 0) -#define NONCONST_LOG2U(x) ({ \ - unsigned _x = (x); \ - _x > 1 ? __SIZEOF_INT__ * 8 - __builtin_clz(_x) - 1 : 0; \ - }) -#define LOG2U(x) __builtin_choose_expr(__builtin_constant_p(x), CONST_LOG2U(x), NONCONST_LOG2U(x)) - -static inline unsigned log2i(int x) { - return LOG2U(x); -} - -static inline unsigned log2u(unsigned x) { - return LOG2U(x); -} - -static inline unsigned log2u_round_up(unsigned x) { - if (x <= 1) - return 0; - - return log2u(x - 1) + 1; -} 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/fundamental/logarithm.h b/src/fundamental/logarithm.h new file mode 100644 index 0000000000..0b03bbded1 --- /dev/null +++ b/src/fundamental/logarithm.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include + +/* Note: log2(0) == log2(1) == 0 here and below. */ + +#define CONST_LOG2ULL(x) ((x) > 1 ? (unsigned) __builtin_clzll(x) ^ 63U : 0) +#define NONCONST_LOG2ULL(x) ({ \ + unsigned long long _x = (x); \ + _x > 1 ? (unsigned) __builtin_clzll(_x) ^ 63U : 0; \ + }) +#define LOG2ULL(x) __builtin_choose_expr(__builtin_constant_p(x), CONST_LOG2ULL(x), NONCONST_LOG2ULL(x)) + +static inline unsigned log2u64(uint64_t x) { +#if __SIZEOF_LONG_LONG__ == 8 + return LOG2ULL(x); +#else +# error "Wut?" +#endif +} + +static inline unsigned u32ctz(uint32_t n) { +#if __SIZEOF_INT__ == 4 + return n != 0 ? __builtin_ctz(n) : 32; +#else +# error "Wut?" +#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); \ + _x > 1 ? __SIZEOF_INT__ * 8 - __builtin_clz(_x) - 1 : 0; \ + }) +#define LOG2U(x) __builtin_choose_expr(__builtin_constant_p(x), CONST_LOG2U(x), NONCONST_LOG2U(x)) + +static inline unsigned log2i(int x) { + return LOG2U(x); +} + +static inline unsigned log2u(unsigned x) { + return LOG2U(x); +} + +static inline unsigned log2u_round_up(unsigned x) { + if (x <= 1) + return 0; + + return log2u(x - 1) + 1; +} 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); -- cgit v1.2.3 From e7416db183cb205146fd7fbb4e791b72cdae2b70 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 12:43:53 +0200 Subject: 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(-) (limited to 'src/test') 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\"", -- cgit v1.2.3 From f3af62905004964a5c1aab763a250fe710cb802c Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 14:50:12 +0200 Subject: core/unit: rename UNIT_ESCAPE_EXEC_SYNTAX → *_ENV 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(-) (limited to 'src/test') 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)); -- cgit v1.2.3 From 8c41640a71fd03e4a2a45a28e311bbfd08e4c49a Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 3 Apr 2023 14:45:46 +0200 Subject: 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(-) (limited to 'src/test') 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\""); } -- cgit v1.2.3