From 6178ced491462cbd9869d3eeef6b4af530746d61 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 1 Jun 2023 20:02:20 +0200 Subject: [PATCH 1/4] test: make the multiple-file test more thorough Let's check if we keep the old records after multiple systemd-pstore invocations (i.e. simulate a scenario where we get multiple crashes and multiple machine reboots). --- test/units/testsuite-74.pstore.sh | 118 ++++++++++++++++++------------ 1 file changed, 72 insertions(+), 46 deletions(-) diff --git a/test/units/testsuite-74.pstore.sh b/test/units/testsuite-74.pstore.sh index b8bf5534a1..1b475a3f2a 100755 --- a/test/units/testsuite-74.pstore.sh +++ b/test/units/testsuite-74.pstore.sh @@ -10,8 +10,8 @@ if systemd-detect-virt -cq; then exit 0 fi -DUMMY_DMESG_1="$(mktemp)" -cat >"$DUMMY_DMESG_1" <<\EOF +DUMMY_DMESG_0="$(mktemp)" +cat >"$DUMMY_DMESG_0" <<\EOF 6,17159,5340096332127,-;usb 1-4: USB disconnect, device number 124 6,17160,5340109662397,-;input: WH-1000XM3 (AVRCP) as /devices/virtual/input/input293 6,17161,5343126458360,-;loop0: detected capacity change from 0 to 3145728 @@ -28,8 +28,8 @@ cat >"$DUMMY_DMESG_1" <<\EOF 6,17172,5343158701120,-; loop0: p1 p2 EOF -DUMMY_DMESG_2="$(mktemp)" -cat >"$DUMMY_DMESG_2" <<\EOF +DUMMY_DMESG_1="$(mktemp)" +cat >"$DUMMY_DMESG_1" <<\EOF Nechť již hříšné saxofony ďáblů rozezvučí síň úděsnými tóny waltzu, tanga a quickstepu. Příliš žluťoučký kůň úpěl ďábelské ódy. Zvlášť zákeřný učeň s ďolíčky běží podél zóny úlů. @@ -108,7 +108,7 @@ prepare_pstore_config() { rm -fr /sys/fs/pstore/* /var/lib/systemd/pstore/* mkdir -p /run/systemd/pstore.conf.d - cat >"/run/systemd/pstore.conf.d/99-test.conf" </run/systemd/pstore.conf.d/99-test.conf </run/systemd/system/systemd-pstore.service.d/99-StartLimitInterval.conf </sys/fs/pstore/foo.bar - [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" - start_pstore - [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] - [[ "$(file_count /var/lib/systemd/pstore/)" -ne 0 ]] - filename_1="$(printf "/var/lib/systemd/pstore/%s/%0.3d/dmesg.txt" "$timestamp_1" "$(file_size "$DUMMY_DMESG_1")")" - diff "$DUMMY_DMESG_1" "$filename_1" - filename_2="$(printf "/var/lib/systemd/pstore/%s/%0.3d/dmesg.txt" "$timestamp_2" "$(file_size "$DUMMY_DMESG_2")")" - diff "$DUMMY_DMESG_2" "$filename_2" - grep "hello world" "/var/lib/systemd/pstore/foo.bar" + for i in {0..6}; do + timestamp="$((timestamp + (i * 10)))" + timestamps+=("$timestamp") + # Create a name reference to one of the $DUMMY_DMESG_X variables + dmesg="DUMMY_DMESG_$((i % 2))" + prepare_efi_logs "${!dmesg}" "$timestamp" + # Add one "random" (non-dmesg) file as well + echo "hello world" >/sys/fs/pstore/foo.bar + [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" + start_pstore + [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] + [[ "$(file_count /var/lib/systemd/pstore/)" -ne 0 ]] + filename="$(printf "/var/lib/systemd/pstore/%s/%0.3d/dmesg.txt" "$timestamp" "$(file_size "${!dmesg}")")" + diff "${!dmesg}" "$filename" + grep "hello world" "/var/lib/systemd/pstore/foo.bar" + done + # Check that we kept all previous records as well + for timestamp in "${timestamps[@]}"; do + [[ -d "/var/lib/systemd/pstore/$timestamp" ]] + [[ "$(file_count "/var/lib/systemd/pstore/$timestamp/")" -gt 0 ]] + done : "Backend: EFI; Storage: journal; Unlink: $unlink" timestamp="$(random_efi_timestamp)" prepare_pstore_config "journal" "$unlink" - prepare_efi_logs "$DUMMY_DMESG_1" "$timestamp" + prepare_efi_logs "$DUMMY_DMESG_0" "$timestamp" [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" start_pstore [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] [[ "$(file_count /var/lib/systemd/pstore/)" -eq 0 ]] - diff "$DUMMY_DMESG_1" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") + diff "$DUMMY_DMESG_0" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") : "Backend: ERST; Storage: external; Unlink: $unlink" prepare_pstore_config "external" "$unlink" - last_id="$(prepare_erst_logs "$DUMMY_DMESG_1" 0)" + last_id="$(prepare_erst_logs "$DUMMY_DMESG_0" 0)" [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" start_pstore [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] [[ "$(file_count /var/lib/systemd/pstore/)" -ne 0 ]] # We always log to journal - diff "$DUMMY_DMESG_1" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") + diff "$DUMMY_DMESG_0" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") filename="$(printf "/var/lib/systemd/pstore/%0.16d/dmesg.txt" "$last_id")" - diff "$DUMMY_DMESG_1" "$filename" + diff "$DUMMY_DMESG_0" "$filename" : "Backend: ERST; Storage: external; Unlink: $unlink; multiple dmesg files" + last_ids=() prepare_pstore_config "external" "$unlink" - last_id_1="$(prepare_erst_logs "$DUMMY_DMESG_1" 0)" - last_id_2="$(prepare_erst_logs "$DUMMY_DMESG_2" "$((last_id_1 + 10))")" - # Add one "random" (non-dmesg) file as well - echo "hello world" >/sys/fs/pstore/foo.bar - [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" - start_pstore - [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] - [[ "$(file_count /var/lib/systemd/pstore/)" -ne 0 ]] - filename_1="$(printf "/var/lib/systemd/pstore/%0.16d/dmesg.txt" "$last_id_1")" - diff "$DUMMY_DMESG_1" "$filename_1" - filename_2="$(printf "/var/lib/systemd/pstore/%0.16d/dmesg.txt" "$last_id_2")" - diff "$DUMMY_DMESG_2" "$filename_2" - grep "hello world" "/var/lib/systemd/pstore/foo.bar" + for i in {0..9}; do + # Create a name reference to one of the $DUMMY_DMESG_X variables + dmesg="DUMMY_DMESG_$((i % 2))" + last_id="$(prepare_erst_logs "${!dmesg}" "$((i * 100))")" + last_ids+=("$last_id") + # Add one "random" (non-dmesg) file as well + echo "hello world" >/sys/fs/pstore/foo.bar + [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" + start_pstore + [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] + [[ "$(file_count /var/lib/systemd/pstore/)" -ne 0 ]] + filename="$(printf "/var/lib/systemd/pstore/%0.16d/dmesg.txt" "$last_id")" + diff "${!dmesg}" "$filename" + grep "hello world" "/var/lib/systemd/pstore/foo.bar" + done + # Check that we kept all previous records as well + for last_id in "${last_ids[@]}"; do + directory="$(printf "/var/lib/systemd/pstore/%0.16d" "$last_id")" + [[ -d "$directory" ]] + [[ "$(file_count "$directory")" -gt 0 ]] + done : "Backend: ERST; Storage: journal; Unlink: $unlink" prepare_pstore_config "journal" "$unlink" - last_id="$(prepare_erst_logs "$DUMMY_DMESG_1" 0)" + last_id="$(prepare_erst_logs "$DUMMY_DMESG_0" 0)" [[ "$unlink" == yes ]] && exp_count=0 || exp_count="$(file_count /sys/fs/pstore/)" start_pstore [[ "$(file_count /sys/fs/pstore)" -ge "$exp_count" ]] [[ "$(file_count /var/lib/systemd/pstore/)" -eq 0 ]] - diff "$DUMMY_DMESG_1" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") + diff "$DUMMY_DMESG_0" <(journalctl -o cat --output-fields=FILE --cursor-file=/tmp/journal.cursor | sed "/^$/d") done From b86ed7f710b89b7a7e36da158c1bb8c69ffed144 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 1 Jun 2023 20:22:20 +0200 Subject: [PATCH 2/4] dbus-util: let's take it down a notch when converting file mode to string I'm definitely a fan of precision, but in this case it's a bit too much: $ systemd-run --unit=test --socket-property=ListenFIFO=/tmp/foo --socket-property=SocketMode=0644 true $ systemctl cat test.socket # /run/systemd/transient/test.socket # This is a transient unit file, created programmatically via the systemd API. Do not edit. [Unit] Description=/usr/bin/true [Socket] ListenFIFO=/tmp/foo SocketMode=0000000000000000000000000000000000000644 --- src/core/dbus-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dbus-util.c b/src/core/dbus-util.c index 461f6aafb2..e891cef1ef 100644 --- a/src/core/dbus-util.c +++ b/src/core/dbus-util.c @@ -30,7 +30,7 @@ int bus_property_get_triggered_unit( return sd_bus_message_append(reply, "s", trigger ? trigger->id : NULL); } -BUS_DEFINE_SET_TRANSIENT(mode_t, "u", uint32_t, mode_t, "%040o"); +BUS_DEFINE_SET_TRANSIENT(mode_t, "u", uint32_t, mode_t, "%04o"); BUS_DEFINE_SET_TRANSIENT(unsigned, "u", uint32_t, unsigned, "%" PRIu32); static inline bool valid_user_group_name_or_id_relaxed(const char *u) { From f8b21a08aa0a8db34212bccca13bfc58dbdc5667 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 1 Jun 2023 22:11:01 +0200 Subject: [PATCH 3/4] socket: avoid memory leak on incomplete SocketPort object ==1==ERROR: LeakSanitizer: detected memory leaks Direct leak of 17 byte(s) in 1 object(s) allocated from: #0 0x7fc096c7243b in strdup (/lib64/libasan.so.8+0x7243b) #1 0x7fc095db3899 in bus_socket_set_transient_property ../src/core/dbus-socket.c:386 #2 0x7fc095db5140 in bus_socket_set_property ../src/core/dbus-socket.c:460 #3 0x7fc095dd20f1 in bus_unit_set_properties ../src/core/dbus-unit.c:2473 #4 0x7fc095d87d53 in transient_unit_from_message ../src/core/dbus-manager.c:1025 #5 0x7fc095d8872f in method_start_transient_unit ../src/core/dbus-manager.c:1112 #6 0x7fc0944ddf4f in method_callbacks_run ../src/libsystemd/sd-bus/bus-objects.c:406 #7 0x7fc0944e7854 in object_find_and_run ../src/libsystemd/sd-bus/bus-objects.c:1319 #8 0x7fc0944e8f03 in bus_process_object ../src/libsystemd/sd-bus/bus-objects.c:1439 #9 0x7fc09454ad78 in process_message ../src/libsystemd/sd-bus/sd-bus.c:3011 #10 0x7fc09454b302 in process_running ../src/libsystemd/sd-bus/sd-bus.c:3053 #11 0x7fc09454e158 in bus_process_internal ../src/libsystemd/sd-bus/sd-bus.c:3273 #12 0x7fc09454e2f2 in sd_bus_process ../src/libsystemd/sd-bus/sd-bus.c:3300 #13 0x7fc094551a59 in io_callback ../src/libsystemd/sd-bus/sd-bus.c:3642 #14 0x7fc094727830 in source_dispatch ../src/libsystemd/sd-event/sd-event.c:4187 #15 0x7fc094731009 in sd_event_dispatch ../src/libsystemd/sd-event/sd-event.c:4808 #16 0x7fc094732124 in sd_event_run ../src/libsystemd/sd-event/sd-event.c:4869 #17 0x7fc095f7af9f in manager_loop ../src/core/manager.c:3242 #18 0x41cc7c in invoke_main_loop ../src/core/main.c:1937 #19 0x4252e0 in main ../src/core/main.c:3072 #20 0x7fc092a4a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f) SUMMARY: AddressSanitizer: 17 byte(s) leaked in 1 allocation(s). --- src/core/dbus-socket.c | 2 +- src/core/socket.c | 21 ++++++++++++++------- src/core/socket.h | 3 +++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c index 48e6419130..09a3a9502b 100644 --- a/src/core/dbus-socket.c +++ b/src/core/dbus-socket.c @@ -364,7 +364,7 @@ static int bus_socket_set_transient_property( return r; while ((r = sd_bus_message_read(message, "(ss)", &t, &a)) > 0) { - _cleanup_free_ SocketPort *p = NULL; + _cleanup_(socket_port_freep) SocketPort *p = NULL; p = new(SocketPort, 1); if (!p) diff --git a/src/core/socket.c b/src/core/socket.c index 7c596182c2..f8fe62c919 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -120,6 +120,19 @@ static void socket_cleanup_fd_list(SocketPort *p) { p->n_auxiliary_fds = 0; } +SocketPort *socket_port_free(SocketPort *p) { + if (!p) + return NULL; + + sd_event_source_unref(p->event_source); + + socket_cleanup_fd_list(p); + safe_close(p->fd); + free(p->path); + + return mfree(p); +} + void socket_free_ports(Socket *s) { SocketPort *p; @@ -127,13 +140,7 @@ void socket_free_ports(Socket *s) { while ((p = s->ports)) { LIST_REMOVE(port, s->ports, p); - - sd_event_source_unref(p->event_source); - - socket_cleanup_fd_list(p); - safe_close(p->fd); - free(p->path); - free(p); + socket_port_free(p); } } diff --git a/src/core/socket.h b/src/core/socket.h index 308a072756..191d27f46d 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -172,6 +172,9 @@ int socket_collect_fds(Socket *s, int **fds); /* Called from the service code when a per-connection service ended */ void socket_connection_unref(Socket *s); +SocketPort *socket_port_free(SocketPort *p); +DEFINE_TRIVIAL_CLEANUP_FUNC(SocketPort*, socket_port_free); + void socket_free_ports(Socket *s); int socket_load_service_unit(Socket *s, int cfd, Unit **ret); From dee4a9923b2724362739e956632310de48e24741 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 1 Jun 2023 08:47:51 +0200 Subject: [PATCH 4/4] test: add a couple of tests for systemd-run --- test/test-functions | 2 +- test/units/testsuite-74.run.sh | 200 +++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 1 deletion(-) create mode 100755 test/units/testsuite-74.run.sh diff --git a/test/test-functions b/test/test-functions index 073777a0eb..2502177908 100644 --- a/test/test-functions +++ b/test/test-functions @@ -2668,7 +2668,7 @@ inst_binary() { # nsswitch.conf uses [SUCCESS=merge] (like on Arch Linux) # delv, dig - pull in nss_resolve if `resolve` is in nsswitch.conf # tar - called by machinectl in TEST-25 - bin_rx='/(chown|delv|dig|getent|login|ls|mkfs\.[a-z0-9]+|mksquashfs|mkswap|setfacl|setpriv|stat|su|tar|useradd|userdel)$' + bin_rx='/(chown|delv|dig|getent|id|login|ls|mkfs\.[a-z0-9]+|mksquashfs|mkswap|setfacl|setpriv|stat|su|tar|useradd|userdel)$' if get_bool "$IS_BUILT_WITH_ASAN" && [[ "$bin" =~ $bin_rx ]]; then wrap_binary=1 fi diff --git a/test/units/testsuite-74.run.sh b/test/units/testsuite-74.run.sh new file mode 100755 index 0000000000..33a8e1ebd2 --- /dev/null +++ b/test/units/testsuite-74.run.sh @@ -0,0 +1,200 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +# shellcheck disable=SC2016 +set -eux +set -o pipefail + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +# FIXME(?): +# - empty ExecStart= is always inserted +# - weird results with systemd-run --property=ExecStart=false true +# - group settings for each section? +# - i.e systemd-run --remain-after-exit --on-clock-change +# --timer-property=After=systemd-journald.service true +# generates a unit that has two [Unit] and two [Timer] sections + +systemd-run --help --no-pager +systemd-run --version +systemd-run --no-ask-password true + +export PARENT_FOO=bar +touch /tmp/public-marker + +: "Transient service (system daemon)" +systemd-run --wait --pipe \ + bash -xec '[[ "$(