From 86df23b67c73f6a9dafadea065412d11eadea240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 20 Apr 2021 16:10:25 +0200 Subject: [PATCH 01/13] TEST-58: execute the right test --- test/TEST-58-REPART/test.sh | 2 +- test/units/testsuite-58.service | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/TEST-58-REPART/test.sh b/test/TEST-58-REPART/test.sh index d94a9cbfdc..089a15b719 100755 --- a/test/TEST-58-REPART/test.sh +++ b/test/TEST-58-REPART/test.sh @@ -3,4 +3,4 @@ set -e TEST_DESCRIPTION="test systemd-repart" . $TEST_BASE_DIR/test-functions -do_test "$@" 56 +do_test "$@" 58 diff --git a/test/units/testsuite-58.service b/test/units/testsuite-58.service index d8ad589ca0..b962eb0f0f 100644 --- a/test/units/testsuite-58.service +++ b/test/units/testsuite-58.service @@ -1,5 +1,5 @@ [Unit] -Description=TEST-56-EXIT-TYPE +Description=TEST-58-REPART [Service] ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh From 30f56248f56f1738512f2f14e273678bcff8da4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 20 Apr 2021 17:07:41 +0200 Subject: [PATCH 02/13] TEST-58: adjust whitespace and enable pipefail --- test/units/testsuite-58.sh | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/test/units/testsuite-58.sh b/test/units/testsuite-58.sh index 772f1b78d4..1fcc882242 100755 --- a/test/units/testsuite-58.sh +++ b/test/units/testsuite-58.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash set -eux +set -o pipefail export SYSTEMD_LOG_LEVEL=debug export PAGER=cat @@ -8,14 +9,14 @@ mkdir -p /tmp/testsuite-58-defs/ # First part: create a disk image and verify its in order -cat > /tmp/testsuite-58-defs/esp.conf </tmp/testsuite-58-defs/esp.conf < /tmp/testsuite-58-defs/usr.conf </tmp/testsuite-58-defs/usr.conf < /tmp/testsuite-58-defs/root.conf </tmp/testsuite-58-defs/root.conf < /tmp/testsuite-58.dump +sfdisk --dump /var/tmp/testsuite-58.img >/tmp/testsuite-58.dump grep -qxF '/var/tmp/testsuite-58.img1 : start= 2048, size= 20480, type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B, uuid=39107B09-615D-48FB-BA37-C663885FCE67, name="esp"' /tmp/testsuite-58.dump grep -qxF '/var/tmp/testsuite-58.img2 : start= 22528, size= 20480, type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709, uuid=60F33797-1D71-4DCB-AA6F-20564F036CD0, name="root-x86-64"' /tmp/testsuite-58.dump @@ -41,26 +46,31 @@ grep -qxF '/var/tmp/testsuite-58.img3 : start= 43008, size= 20480, t # Second part, duplicate it with CopyBlocks=auto -cat > /tmp/testsuite-58-defs/esp.conf </tmp/testsuite-58-defs/esp.conf < /tmp/testsuite-58-defs/usr.conf </tmp/testsuite-58-defs/usr.conf < /tmp/testsuite-58-defs/root.conf </tmp/testsuite-58-defs/root.conf < Date: Wed, 21 Apr 2021 09:58:26 +0200 Subject: [PATCH 03/13] tests: install mkfs.ext4, mkfs.vfat and modules into the test image This allows TEST-58-REPART to at least start. It fails later with with loopback device errors. --- test/test-functions | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/test/test-functions b/test/test-functions index 8f2ffb1323..6f11b2f1b2 100644 --- a/test/test-functions +++ b/test/test-functions @@ -632,7 +632,8 @@ setup_basic_environment() { install_keymaps install_terminfo install_execs - install_fsck + install_fs_tools + install_modules install_plymouth install_debug_tools install_ld_so_conf @@ -833,13 +834,28 @@ EOF chmod 0755 "$strace_wrapper" } -install_fsck() { +install_fs_tools() { dinfo "Install fsck" dracut_install /sbin/fsck* dracut_install -o /bin/fsck* # fskc.reiserfs calls reiserfsck. so, install it dracut_install -o reiserfsck + + # we use mkfs in system-repart tests + dracut_install /sbin/mkfs.ext4 + dracut_install /sbin/mkfs.vfat +} + +install_modules() { + dinfo "Install modules" + + instmods loop + instmods vfat + + if [[ "$LOOKS_LIKE_SUSE" ]]; then + instmods ext4 + fi } install_dmevent() { @@ -2220,7 +2236,6 @@ setup_suse() { ln -fs ../usr/bin/systemctl "${initdir:?}/bin/" ln -fs ../usr/lib/systemd "$initdir/lib/" inst_simple "/usr/lib/systemd/system/haveged.service" - instmods ext4 } _umount_dir() { From b0f04bafe08549069fc217c69647e929f1c431da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 20 Apr 2021 17:53:55 +0200 Subject: [PATCH 04/13] TEST-58: remove stale artifacts to not fail on repeated invocations We would remove stuff only if successful, so repeated invocations would trivially fail. Also drop "-f", so that if we expect to remove something, it must be there. --- test/units/testsuite-58.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/units/testsuite-58.sh b/test/units/testsuite-58.sh index 1fcc882242..21ad7d5da2 100755 --- a/test/units/testsuite-58.sh +++ b/test/units/testsuite-58.sh @@ -5,6 +5,7 @@ set -o pipefail export SYSTEMD_LOG_LEVEL=debug export PAGER=cat +rm -f /var/tmp/testsuite-58.img /var/tmp/testsuite-58.2.img /tmp/testsuite-58.dump mkdir -p /tmp/testsuite-58-defs/ # First part: create a disk image and verify its in order @@ -74,8 +75,8 @@ systemd-repart --definitions=/tmp/testsuite-58-defs/ \ cmp /var/tmp/testsuite-58.img /var/tmp/testsuite-58.2.img -rm -f /var/tmp/testsuite-58.img /var/tmp/testsuite-58.2.img /tmp/testsuite-58.dump -rm -rf /tmp/testsuite-58-defs/ +rm /var/tmp/testsuite-58.img /var/tmp/testsuite-58.2.img /tmp/testsuite-58.dump +rm -r /tmp/testsuite-58-defs/ echo OK >/testok From 3d3aafa453d21093809b48fc7dc020e3973bc470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Apr 2021 09:37:18 +0200 Subject: [PATCH 05/13] TODO: add some items for repart --- TODO | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/TODO b/TODO index 682f296d60..b8cbd3b793 100644 --- a/TODO +++ b/TODO @@ -89,6 +89,14 @@ Features: or so. (this is useful to factory reset an image, then putting it into another machine, ensuring that luks key is generated on new machine, not old) +* systemd-repart: support setting up dm-integrity with HMAC + +* systemd-repart: maybe remove half-initialized image on failure. It fails + if the output file exists, so a repeated invocation will usually fail if + something goes wrong on the way. + +* systemd-repart: drop pager mode on normal operation? + * move logind udev rules to top-level rule.d/ directory * move multiseat vid/pid matches from logind udev rule to hwdb @@ -242,8 +250,6 @@ Features: * when main nspawn supervisor process gets suspended due to SIGSTOP/SIGTTOU or so, freeze the payload too. -* repart: support setting up dm-integrity with HMAC - * add /etc/integritytab, to support dm-integrity setups. In particular those with HMAC as hash function, so that we can have a protected /home without encryption (leaving encryption to the individual dirs/homed). From 409607c111851b1c24c3c67132bfb241a158fe9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 20 Apr 2021 18:28:19 +0200 Subject: [PATCH 06/13] core: fix typos in comment --- src/core/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 54ef7d182a..2f36d752bc 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1198,8 +1198,8 @@ static void bump_file_max_and_nr_open(void) { #endif #if BUMP_PROC_SYS_FS_FILE_MAX - /* The maximum the kernel allows for this since 5.2 is LONG_MAX, use that. (Previously thing where - * different but the operation would fail silently.) */ + /* The maximum the kernel allows for this since 5.2 is LONG_MAX, use that. (Previously things were + * different, but the operation would fail silently.) */ r = sysctl_writef("fs/file-max", "%li\n", LONG_MAX); if (r < 0) log_full_errno(IN_SET(r, -EROFS, -EPERM, -EACCES) ? LOG_DEBUG : LOG_WARNING, r, "Failed to bump fs.file-max, ignoring: %m"); From 7b87fe4c30c3a5492ba409ebb13b1a5505c00a86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Apr 2021 09:07:30 +0200 Subject: [PATCH 07/13] various: print the image path when setting up of the loopback device fails --- src/dissect/dissect.c | 2 +- src/portable/portable.c | 2 +- src/shared/dissect-image.c | 2 +- src/sysext/sysext.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dissect/dissect.c b/src/dissect/dissect.c index f1288b41a7..745366837f 100644 --- a/src/dissect/dissect.c +++ b/src/dissect/dissect.c @@ -774,7 +774,7 @@ static int run(int argc, char *argv[]) { FLAGS_SET(arg_flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, &d); if (r < 0) - return log_error_errno(r, "Failed to set up loopback device: %m"); + return log_error_errno(r, "Failed to set up loopback device for %s: %m", arg_image); r = dissect_image_and_warn( d->fd, diff --git a/src/portable/portable.c b/src/portable/portable.c index 0799bff53d..53c4d8e25b 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -376,7 +376,7 @@ static int portable_extract_by_path( return r; } else if (r < 0) - return log_debug_errno(r, "Failed to set up loopback device: %m"); + return log_debug_errno(r, "Failed to set up loopback device for %s: %m", path); else { _cleanup_(dissected_image_unrefp) DissectedImage *m = NULL; _cleanup_(rmdir_and_freep) char *tmpdir = NULL; diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index b425014457..05301ef16d 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -2741,7 +2741,7 @@ int mount_image_privately_interactively( FLAGS_SET(flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, &d); if (r < 0) - return log_error_errno(r, "Failed to set up loopback device: %m"); + return log_error_errno(r, "Failed to set up loopback device for %s: %m", image); r = dissect_image_and_warn(d->fd, image, &verity, NULL, d->uevent_seqnum_not_before, d->timestamp_not_before, flags, &dissected_image); if (r < 0) diff --git a/src/sysext/sysext.c b/src/sysext/sysext.c index c5fdf99aa7..76a21afea6 100644 --- a/src/sysext/sysext.c +++ b/src/sysext/sysext.c @@ -525,7 +525,7 @@ static int merge_subprocess(Hashmap *images, const char *workspace) { r = loop_device_make_by_path(img->path, O_RDONLY, 0, &d); if (r < 0) - return log_error_errno(r, "Failed to set up loopback device: %m"); + return log_error_errno(r, "Failed to set up loopback device for %s: %m", img->path); r = dissect_image_and_warn( d->fd, From 7bf20e48bd7d641a39a14a7feb749b7e8b0fc0f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Apr 2021 10:55:49 +0200 Subject: [PATCH 08/13] test: move the logic to support /skipped into shared logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logic to query test state was rather complex. I don't quite grok the point of ret=$((ret+1))… But afaics, the precise result was always ignored by the caller anyway. --- test/TEST-02-UNITTESTS/test.sh | 4 +- test/TEST-24-CRYPTSETUP/test.sh | 2 +- test/TEST-55-OOMD/test.sh | 48 --------------------- test/test-functions | 74 ++++++++++++++++++++++----------- 4 files changed, 52 insertions(+), 76 deletions(-) diff --git a/test/TEST-02-UNITTESTS/test.sh b/test/TEST-02-UNITTESTS/test.sh index 2bfe41a42b..f7545b7620 100755 --- a/test/TEST-02-UNITTESTS/test.sh +++ b/test/TEST-02-UNITTESTS/test.sh @@ -39,7 +39,7 @@ check_result_nspawn() { save_journal "$workspace/var/log/journal" _umount_dir "${initdir:?}" - [[ -n "${TIMED_OUT:=}" ]] && ret=$((ret + 1)) + [[ -n "${TIMED_OUT:=}" ]] && ret=1 return $ret } @@ -67,7 +67,7 @@ check_result_qemu() { save_journal "$initdir/var/log/journal" _umount_dir "$initdir" - [[ -n "${TIMED_OUT:=}" ]] && ret=$((ret + 1)) + [[ -n "${TIMED_OUT:=}" ]] && ret=1 return $ret } diff --git a/test/TEST-24-CRYPTSETUP/test.sh b/test/TEST-24-CRYPTSETUP/test.sh index e4d99d10b9..109d403568 100755 --- a/test/TEST-24-CRYPTSETUP/test.sh +++ b/test/TEST-24-CRYPTSETUP/test.sh @@ -26,7 +26,7 @@ check_result_qemu() { [[ -f "$TESTDIR/failed" ]] && cat "$TESTDIR/failed" echo "${JOURNAL_LIST:-No journals were saved}" - test -s "$TESTDIR/failed" && ret=$((ret + 1)) + test -s "$TESTDIR/failed" && ret=1 return $ret } diff --git a/test/TEST-55-OOMD/test.sh b/test/TEST-55-OOMD/test.sh index 9f7a11aea4..6f7e776c3c 100755 --- a/test/TEST-55-OOMD/test.sh +++ b/test/TEST-55-OOMD/test.sh @@ -19,52 +19,4 @@ EOF ) } -check_result_nspawn() { - local workspace="${1:?}" - local ret=1 - local journald_report="" - local pids="" - - [[ -e "$workspace/testok" ]] && ret=0 - if [[ -e "$workspace/skipped" ]]; then - echo "TEST-56-OOMD was skipped:" - cat "$workspace/skipped" - ret=0 - fi - - [[ -f "$workspace/failed" ]] && cp -a "$workspace/failed" "${TESTDIR:?}" - save_journal "$workspace/var/log/journal" - [[ -f "$TESTDIR/failed" ]] && cat "$TESTDIR/failed" - echo "${JOURNAL_LIST:-No journals were saved}" - - test -s "$TESTDIR/failed" && ret=$((ret + 1)) - [ -n "${TIMED_OUT:=}" ] && ret=$((ret + 1)) - check_asan_reports "$workspace" || ret=$((ret + 1)) - _umount_dir "${initdir:?}" - return $ret -} - -check_result_qemu() { - local ret=1 - - mount_initdir - [[ -e "${initdir:?}/testok" ]] && ret=0 - if [[ -e "$initdir/skipped" ]]; then - echo "TEST-56-OOMD was skipped:" - cat "$initdir/skipped" - ret=0 - fi - - [[ -f "$initdir/failed" ]] && cp -a "$initdir/failed" "${TESTDIR:?}" - save_journal "$initdir/var/log/journal" - check_asan_reports "$initdir" || ret=$((ret + 1)) - _umount_dir "$initdir" - [[ -f "$TESTDIR/failed" ]] && cat "$TESTDIR/failed" - echo "${JOURNAL_LIST:-No journals were saved}" - - test -s "$TESTDIR/failed" && ret=$((ret + 1)) - [ -n "${TIMED_OUT:=}" ] && ret=$((ret + 1)) - return $ret -} - do_test "$@" 55 diff --git a/test/test-functions b/test/test-functions index 6f11b2f1b2..649d03b5af 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1118,42 +1118,66 @@ save_journal() { JOURNAL_LIST="$(ls -l "$dest"*)" } -check_result_nspawn() { +check_result_common() { local workspace="${1:?}" - local ret=1 - local journald_report="" - local pids="" - [[ -e "$workspace/testok" ]] && ret=0 - [[ -f "$workspace/failed" ]] && cp -a "$workspace/failed" "${TESTDIR:?}" + local ret + + if [ -s "$workspace/failed" ]; then + # …/failed only counts if non-empty + ls -l "$workspace/failed" + cp -a "$workspace/failed" "${TESTDIR:?}/" + ret=1 + elif [ -e "$workspace/testok" ]; then + # …/testok always counts (but with lower priority than …/failed) + ret=0 + elif [ -e "$workspace/skipped" ]; then + # …/skipped always counts (a message is expected) + echo "${TESTNAME:?} was skipped:" + cat "$workspace/skipped" + ret=0 + elif [ -n "$TIMED_OUT" ]; then + echo "${TESTNAME:?} timed out!" + ret=2 + else + echo "${TESTNAME:?} did not report a result!" + fi + save_journal "$workspace/var/log/journal" - [[ -f "$TESTDIR/failed" ]] && cat "$TESTDIR/failed" - echo "${JOURNAL_LIST:-"No journals were saved"}" - test -s "$TESTDIR/failed" && ret=$((ret+1)) - [ -n "$TIMED_OUT" ] && ret=$((ret+1)) - check_asan_reports "$workspace" || ret=$((ret+1)) + + check_asan_reports "$workspace" || ret=3 + if [ -d "${ARTIFACT_DIRECTORY}" ] && [ -f "$workspace/strace.out" ]; then cp "$workspace/strace.out" "${ARTIFACT_DIRECTORY}/" fi + + [ -f "$TESTDIR/failed" ] && cat "$TESTDIR/failed" + echo "${JOURNAL_LIST:-"No journals were saved"}" + + return $ret +} + +check_result_nspawn() { + local workspace="${1:?}" + local ret + + check_result_common "${workspace}" + ret=$? + _umount_dir "${initdir:?}" + return $ret } # can be overridden in specific test check_result_qemu() { - local ret=1 + local ret mount_initdir - [[ -e "${initdir:?}/testok" ]] && ret=0 - [[ -f "$initdir/failed" ]] && cp -a "$initdir/failed" "${TESTDIR:?}" - save_journal "$initdir/var/log/journal" - check_asan_reports "$initdir" || ret=$((ret+1)) - if [ -d "${ARTIFACT_DIRECTORY}" ] && [ -f "$initdir/strace.out" ]; then - cp "$initdir/strace.out" "${ARTIFACT_DIRECTORY}/" - fi - _umount_dir "$initdir" - [[ -f "$TESTDIR/failed" ]] && cat "$TESTDIR/failed" - echo "${JOURNAL_LIST:-"No journals were saved"}" - test -s "$TESTDIR/failed" && ret=$((ret+1)) - [ -n "$TIMED_OUT" ] && ret=$((ret+1)) + + check_result_common "${initdir:?}" + ret=$? + + _umount_dir "${initdir:?}" + return $ret } @@ -2208,7 +2232,7 @@ instmods() { ((ret+=$?)) ;; esac - return $ret + return "$ret" } local mod mpargs From dd1fa6c89abdb93cdfdc1cc7710ecedc0f7cfd25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Apr 2021 16:26:18 +0200 Subject: [PATCH 09/13] TEST-58: only run under qemu In a container, /dev/loop* will most likely be inaccessible. --- test/TEST-58-REPART/test.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/TEST-58-REPART/test.sh b/test/TEST-58-REPART/test.sh index 089a15b719..bb08c4fc20 100755 --- a/test/TEST-58-REPART/test.sh +++ b/test/TEST-58-REPART/test.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash set -e TEST_DESCRIPTION="test systemd-repart" +TEST_NO_NSPAWN=1 . $TEST_BASE_DIR/test-functions do_test "$@" 58 From f89a20f1d4a6feb74ac67bef9cc2078852b7aeab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Apr 2021 23:37:57 +0200 Subject: [PATCH 10/13] TEST-58: exit immediately if systemd-repart is not available Debian disables systemd-repart at config time. --- test/units/testsuite-58.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/units/testsuite-58.sh b/test/units/testsuite-58.sh index 21ad7d5da2..13f02f01e3 100755 --- a/test/units/testsuite-58.sh +++ b/test/units/testsuite-58.sh @@ -2,6 +2,11 @@ set -eux set -o pipefail +if ! command -v systemd-repart &>/dev/null; then + echo "no systemd-repart" >/skipped + exit 0 +fi + export SYSTEMD_LOG_LEVEL=debug export PAGER=cat From 1a128a468ddd1070651478c36eae76e31f580b67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Apr 2021 12:39:03 +0200 Subject: [PATCH 11/13] core/service: fix deserialization of non-absolute commands We'd fail with: Apr 23 10:58:26 systemd[1]: Deserializing state... Apr 23 10:58:26 systemd[1]: testsuite-01.service: Failed to parse serialized command "ExecStart 0 sh "sh" "-e" "-x" "-c" "systemctl --state=failed --no-legend --no-pager >/failed ; systemctl daemon-reload ; echo OK >/testok"": Invalid argument Apr 23 10:58:26 systemd[1]: testsuite-01.service: Reinstalled deserialized job testsuite-01.service/start as 209 This was missed in 5008da1ec1, and apparently nobody noticed until now :( --- src/core/service.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index d5f79d274e..f346e38b66 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2783,9 +2783,6 @@ static int service_deserialize_exec_command( case STATE_EXEC_COMMAND_PATH: path = TAKE_PTR(arg); state = STATE_EXEC_COMMAND_ARGS; - - if (!path_is_absolute(path)) - return -EINVAL; break; case STATE_EXEC_COMMAND_ARGS: r = strv_extend(&argv, arg); From 90204792461030dbc8645d8511e7ac8d1b4f1ca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Apr 2021 12:40:07 +0200 Subject: [PATCH 12/13] core/service: also reject deserialized commands with no argv[0] I'm pretty sure that bad things would happen later on. --- src/core/service.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index f346e38b66..d448d558db 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2790,13 +2790,14 @@ static int service_deserialize_exec_command( return -ENOMEM; break; default: - assert_not_reached("Unknown error at deserialization of exec command"); - break; + assert_not_reached("Logic error in exec command deserialization"); } } if (state != STATE_EXEC_COMMAND_ARGS) return -EINVAL; + if (strv_isempty(argv)) + return -EINVAL; /* At least argv[0] must be always present. */ /* Let's check whether exec command on given offset matches data that we just deserialized */ for (command = s->exec_command[id], i = 0; command; command = command->command_next, i++) { From 35243b77360c9cc7d1446617fe4fd304bfdecd4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Apr 2021 12:37:09 +0200 Subject: [PATCH 13/13] test-unit-serialize: add a very basic test that command deserialization works We should test both serialization and deserialization works properly. But the serialization/deserialization code is deeply entwined with the manager state, and I think quite a bit of refactoring will be required before this is possible. But let's at least add this simple test for now. --- src/core/service.c | 2 +- src/core/service.h | 3 ++ src/test/meson.build | 11 ++++++ src/test/test-unit-serialize.c | 68 ++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 src/test/test-unit-serialize.c diff --git a/src/core/service.c b/src/core/service.c index d448d558db..d43189f4f2 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2725,7 +2725,7 @@ static int service_serialize(Unit *u, FILE *f, FDSet *fds) { return 0; } -static int service_deserialize_exec_command( +int service_deserialize_exec_command( Unit *u, const char *key, const char *value) { diff --git a/src/core/service.h b/src/core/service.h index 92c1caf794..0d51fc3153 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -255,3 +255,6 @@ ServiceTimeoutFailureMode service_timeout_failure_mode_from_string(const char *s DEFINE_CAST(SERVICE, Service); #define STATUS_TEXT_MAX (16U*1024U) + +/* Only exported for unit tests */ +int service_deserialize_exec_command(Unit *u, const char *key, const char *value); diff --git a/src/test/meson.build b/src/test/meson.build index e077c8e03f..991c0f027d 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -133,6 +133,17 @@ tests += [ [['src/test/test-serialize.c']], + [['src/test/test-unit-serialize.c'], + [libcore, + libshared], + [threads, + librt, + libseccomp, + libselinux, + libmount, + libblkid], + core_includes], + [['src/test/test-utf8.c']], [['src/test/test-dev-setup.c']], diff --git a/src/test/test-unit-serialize.c b/src/test/test-unit-serialize.c new file mode 100644 index 0000000000..58a0d9dfd4 --- /dev/null +++ b/src/test/test-unit-serialize.c @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "rm-rf.h" +#include "service.h" +#include "tests.h" + +#define EXEC_START_ABSOLUTE \ + "ExecStart 0 /bin/sh \"sh\" \"-e\" \"-x\" \"-c\" \"systemctl --state=failed --no-legend --no-pager >/failed ; systemctl daemon-reload ; echo OK >/testok\"" +#define EXEC_START_RELATIVE \ + "ExecStart 0 sh \"sh\" \"-e\" \"-x\" \"-c\" \"systemctl --state=failed --no-legend --no-pager >/failed ; systemctl daemon-reload ; echo OK >/testok\"" + +static void test_deserialize_exec_command_one(Manager *m, const char *key, const char *line, int expected) { + _cleanup_(unit_freep) Unit *u = NULL; + int r; + + assert_se(unit_new_for_name(m, sizeof(Service), "test.service", &u) >= 0); + + r = service_deserialize_exec_command(u, key, line); + log_debug("[%s] → %d (expected: %d)", line, r, expected); + assert(r == expected); + + /* Note that the command doesn't match any command in the empty list of commands in 's', so it is + * always rejected with "Current command vanished from the unit file", and we don't leak anything. */ +} + +static void test_deserialize_exec_command(void) { + _cleanup_(manager_freep) Manager *m = NULL; + int r; + + log_info("/* %s */", __func__); + + r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m); + if (manager_errno_skip_test(r)) { + log_notice_errno(r, "Skipping test: manager_new: %m"); + return; + } + + assert_se(r >= 0); + + test_deserialize_exec_command_one(m, "main-command", EXEC_START_ABSOLUTE, 0); + test_deserialize_exec_command_one(m, "main-command", EXEC_START_RELATIVE, 0); + test_deserialize_exec_command_one(m, "control-command", EXEC_START_ABSOLUTE, 0); + test_deserialize_exec_command_one(m, "control-command", EXEC_START_RELATIVE, 0); + + test_deserialize_exec_command_one(m, "control-command", "ExecStart 0 /bin/sh \"sh\"", 0); + test_deserialize_exec_command_one(m, "control-command", "ExecStart 0 /no/command ", -EINVAL); + test_deserialize_exec_command_one(m, "control-command", "ExecStart 0 /bad/quote \"", -EINVAL); + test_deserialize_exec_command_one(m, "control-command", "ExecStart s /bad/id x y z", -EINVAL); + test_deserialize_exec_command_one(m, "control-command", "ExecStart 11", -EINVAL); + test_deserialize_exec_command_one(m, "control-command", "ExecWhat 11 /a/b c d e", -EINVAL); +} + +int main(int argc, char *argv[]) { + _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; + int r; + + test_setup_logging(LOG_DEBUG); + + r = enter_cgroup_subroot(NULL); + if (r == -ENOMEDIUM) + return log_tests_skipped("cgroupfs not available"); + + assert_se(runtime_dir = setup_fake_runtime_dir()); + + test_deserialize_exec_command(); + + return EXIT_SUCCESS; +}