From 1b1e53d2a83fe5267cb549b57ca6c84475729fe8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 27 May 2022 13:37:27 +0900 Subject: [PATCH 1/4] test: drop redundant log message --- src/test/test-mountpoint-util.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/test-mountpoint-util.c b/src/test/test-mountpoint-util.c index 3c2523006a..6f9e68f4d9 100644 --- a/src/test/test-mountpoint-util.c +++ b/src/test/test-mountpoint-util.c @@ -54,8 +54,6 @@ TEST(mnt_id) { void *k; int r; - log_info("/* %s */", __func__); - assert_se(f = fopen("/proc/self/mountinfo", "re")); assert_se(h = hashmap_new(&trivial_hash_ops)); From 6a49fcdb3c90e00d8da350464816dcb63336353f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 30 May 2022 04:34:29 +0900 Subject: [PATCH 2/4] portable: try to remove unit files even in a spurious state --- src/portable/portable.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index c4f42f9500..254d9f6abe 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1560,7 +1560,6 @@ int portable_detach( FOREACH_DIRENT(de, d, return log_debug_errno(errno, "Failed to enumerate '%s' directory: %m", where)) { _cleanup_free_ char *marker = NULL; - UnitFileState state; if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY)) continue; @@ -1578,12 +1577,6 @@ int portable_detach( if (r == 0) continue; - r = unit_file_lookup_state(LOOKUP_SCOPE_SYSTEM, &paths, de->d_name, &state); - if (r < 0) - return log_debug_errno(r, "Failed to determine unit file state of '%s': %m", de->d_name); - if (!IN_SET(state, UNIT_FILE_STATIC, UNIT_FILE_DISABLED, UNIT_FILE_LINKED, UNIT_FILE_RUNTIME, UNIT_FILE_LINKED_RUNTIME)) - return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, "Unit file '%s' is in state '%s', can't detach.", de->d_name, unit_file_state_to_string(state)); - r = unit_file_is_active(bus, de->d_name, error); if (r < 0) return r; From edea370222eec3d185dd84a7d9e942ec5933af63 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 27 May 2022 14:11:56 +0900 Subject: [PATCH 3/4] portable: remove drop-in configs even if the main unit file does not exist When we run `portablectl detach --enable --runtime`, then it triggers `DisableUnitFilesWithFlags` DBus method and the main unit file is removed, but its drop-ins are not. Hence, portable_detach() failed to list existing portable units. This makes the loop for listing portable units also accept drop-in directories. So, all remaining drop-in directories are correctly removed. Before: ``` testsuite-29.sh[600]: + portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 portablectl[1391]: (Matching unit files with prefixes 'minimal-app0'.) portablectl[1391]: Queued /org/freedesktop/systemd1/job/1812 to call StopUnit on portable service minimal-app0-foo.service. portablectl[1391]: Removed "/run/systemd/system.attached/minimal-app0-foo.service". portablectl[1391]: Queued /org/freedesktop/systemd1/job/1813 to call StopUnit on portable service minimal-app0.service. portablectl[1391]: Removed "/run/systemd/system.attached/minimal-app0.service". portablectl[1391]: Got result done/Success for job minimal-app0-foo.service portablectl[1391]: Got result done/Success for job minimal-app0.service portablectl[1391]: DetachImage failed: No unit files associated with '/tmp/rootdir' found attached to the system. Image not attached? ``` After: ``` testsuite-29.sh[508]: + portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 portablectl[1076]: (Matching unit files with prefixes 'minimal-app0'.) portablectl[1076]: Queued /org/freedesktop/systemd1/job/1946 to call StopUnit on portable service minimal-app0-foo.service. portablectl[1076]: Removed "/run/systemd/system.attached/minimal-app0-foo.service". portablectl[1076]: Queued /org/freedesktop/systemd1/job/1947 to call StopUnit on portable service minimal-app0.service. portablectl[1076]: Removed "/run/systemd/system.attached/minimal-app0.service". portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0.service.d/10-profile.conf. portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0.service.d/20-portable.conf. portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0.service.d. portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0-foo.service.d/10-profile.conf. portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0-foo.service.d/20-portable.conf. portablectl[1076]: Removed /run/systemd/system.attached/minimal-app0-foo.service.d. portablectl[1076]: Removed /run/portables/rootdir. portablectl[1076]: Removed /run/systemd/system.attached. ``` --- src/portable/portable.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index 254d9f6abe..a8536c6692 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1559,33 +1559,47 @@ int portable_detach( } FOREACH_DIRENT(de, d, return log_debug_errno(errno, "Failed to enumerate '%s' directory: %m", where)) { - _cleanup_free_ char *marker = NULL; + _cleanup_free_ char *marker = NULL, *unit_name = NULL; + const char *dot; - if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY)) + /* When a portable service is enabled with "portablectl --runtime attach", and is disabled + * with "portablectl --enable detach", which calls DisableUnitFilesWithFlags DBus method, + * the main unit file is removed, but its drop-ins are not. Hence, here we need to list both + * main unit files and drop-in directories (without the main unit files). */ + + dot = endswith(de->d_name, ".d"); + if (dot) + unit_name = strndup(de->d_name, dot - de->d_name); + else + unit_name = strdup(de->d_name); + if (!unit_name) + return -ENOMEM; + + if (!unit_name_is_valid(unit_name, UNIT_NAME_ANY)) continue; /* Filter out duplicates */ - if (set_contains(unit_files, de->d_name)) + if (set_contains(unit_files, unit_name)) continue; - if (!IN_SET(de->d_type, DT_LNK, DT_REG)) + if (dot ? !IN_SET(de->d_type, DT_LNK, DT_DIR) : !IN_SET(de->d_type, DT_LNK, DT_REG)) continue; - r = test_chroot_dropin(d, where, de->d_name, name_or_path, extension_image_paths, &marker); + r = test_chroot_dropin(d, where, unit_name, name_or_path, extension_image_paths, &marker); if (r < 0) return r; if (r == 0) continue; - r = unit_file_is_active(bus, de->d_name, error); + r = unit_file_is_active(bus, unit_name, error); if (r < 0) return r; if (!FLAGS_SET(flags, PORTABLE_REATTACH) && r > 0) - return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, "Unit file '%s' is active, can't detach.", de->d_name); + return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, "Unit file '%s' is active, can't detach.", unit_name); - r = set_put_strdup(&unit_files, de->d_name); + r = set_ensure_consume(&unit_files, &string_hash_ops_free, TAKE_PTR(unit_name)); if (r < 0) - return log_debug_errno(r, "Failed to add unit name '%s' to set: %m", de->d_name); + return log_oom_debug(); for (const char *p = marker;;) { _cleanup_free_ char *image = NULL; From eec21613bb0d7ee460ed77f0d61e44c9f7d78473 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Wed, 25 May 2022 20:30:34 +0200 Subject: [PATCH 4/4] test: add coverage for #23481 Provide some coverage for systemd/systemd#23481. Without 794da5a: ``` [ 34.730815] testsuite-29.sh[600]: + portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 Stopping minimal-app0-foo.service... [ OK ] Stopped minimal-app0-foo.service. Stopping minimal-app0.service... [ OK ] Stopped minimal-app0.service. [ 34.878050] testsuite-29.sh[1383]: ================================================================= [ 34.878421] testsuite-29.sh[1383]: ==1383==ERROR: LeakSanitizer: detected memory leaks [ 34.878784] testsuite-29.sh[1383]: Direct leak of 48 byte(s) in 2 object(s) allocated from: [ 34.879174] testsuite-29.sh[1383]: #0 0x7fdf9c8b0f8c in reallocarray (/lib64/libasan.so.6+0xaef8c) [ 34.879554] testsuite-29.sh[1383]: #1 0x7fdf9b4270f5 in unit_file_changes_add ../src/shared/install.c:282 [ 34.879926] testsuite-29.sh[1383]: #2 0x7fdf9b2ad9e5 in bus_deserialize_and_dump_unit_file_changes ../src/shared/bus-unit-util.c:2688 [ 34.880267] testsuite-29.sh[1383]: #3 0x40bc27 in maybe_enable_disable ../src/portable/portablectl.c:582 [ 34.880673] testsuite-29.sh[1383]: #4 0x40ef56 in maybe_stop_disable ../src/portable/portablectl.c:810 [ 34.881064] testsuite-29.sh[1383]: #5 0x410bc2 in detach_image ../src/portable/portablectl.c:924 [ 34.881493] testsuite-29.sh[1383]: #6 0x7fdf9b5df424 in dispatch_verb ../src/shared/verbs.c:103 [ 34.881953] testsuite-29.sh[1383]: #7 0x41604a in run ../src/portable/portablectl.c:1427 [ 34.882459] testsuite-29.sh[1383]: #8 0x416106 in main ../src/portable/portablectl.c:1430 [ 34.882947] testsuite-29.sh[1383]: #9 0x7fdf99d5de8f in __libc_start_call_main (/lib64/libc.so.6+0x44e8f) [ 34.883368] testsuite-29.sh[1383]: Indirect leak of 104 byte(s) in 2 object(s) allocated from: [ 34.883732] testsuite-29.sh[1383]: #0 0x7fdf9c85b8f7 in strdup (/lib64/libasan.so.6+0x598f7) [ 34.884089] testsuite-29.sh[1383]: #1 0x7fdf9b4271aa in unit_file_changes_add ../src/shared/install.c:288 [ 34.884508] testsuite-29.sh[1383]: #2 0x7fdf9b2ad9e5 in bus_deserialize_and_dump_unit_file_changes ../src/shared/bus-unit-util.c:2688 [ 34.884926] testsuite-29.sh[1383]: #3 0x40bc27 in maybe_enable_disable ../src/portable/portablectl.c:582 [ 34.885307] testsuite-29.sh[1383]: #4 0x40ef56 in maybe_stop_disable ../src/portable/portablectl.c:810 [ 34.885647] testsuite-29.sh[1383]: #5 0x410bc2 in detach_image ../src/portable/portablectl.c:924 [ 34.885987] testsuite-29.sh[1383]: #6 0x7fdf9b5df424 in dispatch_verb ../src/shared/verbs.c:103 [ 34.886271] testsuite-29.sh[1383]: #7 0x41604a in run ../src/portable/portablectl.c:1427 [ 34.886557] testsuite-29.sh[1383]: #8 0x416106 in main ../src/portable/portablectl.c:1430 [ 34.886892] testsuite-29.sh[1383]: #9 0x7fdf99d5de8f in __libc_start_call_main (/lib64/libc.so.6+0x44e8f) [ 34.887187] testsuite-29.sh[1383]: Indirect leak of 2 byte(s) in 2 object(s) allocated from: [ 34.887520] testsuite-29.sh[1383]: #0 0x7fdf9c85b8f7 in strdup (/lib64/libasan.so.6+0x598f7) [ 34.887797] testsuite-29.sh[1383]: #1 0x7fdf9b427249 in unit_file_changes_add ../src/shared/install.c:296 [ 34.888117] testsuite-29.sh[1383]: #2 0x7fdf9b2ad9e5 in bus_deserialize_and_dump_unit_file_changes ../src/shared/bus-unit-util.c:2688 [ 34.888434] testsuite-29.sh[1383]: #3 0x40bc27 in maybe_enable_disable ../src/portable/portablectl.c:582 [ 34.888693] testsuite-29.sh[1383]: #4 0x40ef56 in maybe_stop_disable ../src/portable/portablectl.c:810 [ 34.888990] testsuite-29.sh[1383]: #5 0x410bc2 in detach_image ../src/portable/portablectl.c:924 [ 34.889254] testsuite-29.sh[1383]: #6 0x7fdf9b5df424 in dispatch_verb ../src/shared/verbs.c:103 [ 34.889580] testsuite-29.sh[1383]: #7 0x41604a in run ../src/portable/portablectl.c:1427 [ 34.889877] testsuite-29.sh[1383]: #8 0x416106 in main ../src/portable/portablectl.c:1430 [ 34.890193] testsuite-29.sh[1383]: #9 0x7fdf99d5de8f in __libc_start_call_main (/lib64/libc.so.6+0x44e8f) [ 34.890482] testsuite-29.sh[1383]: SUMMARY: AddressSanitizer: 154 byte(s) leaked in 6 allocation(s). ``` With 794da5a: ``` [ OK ] Started minimal-app0.service. [ 36.794367] testsuite-29.sh[600]: + portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 Stopping minimal-app0-foo.service... [ OK ] Stopped minimal-app0-foo.service. Stopping minimal-app0.service... [ OK ] Stopped minimal-app0.service. [ 36.851251] testsuite-29.sh[600]: + umount /tmp/rootdir ``` --- test/units/testsuite-29.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/units/testsuite-29.sh b/test/units/testsuite-29.sh index e99e1d2cba..3b1395619a 100755 --- a/test/units/testsuite-29.sh +++ b/test/units/testsuite-29.sh @@ -152,6 +152,14 @@ portablectl inspect --cat --extension app0 --extension app1 rootdir app0 app1 | portablectl detach --now --runtime --extension /tmp/app0 --extension /tmp/app1 /tmp/rootdir app0 app1 +# Attempt to disable the app unit during detaching. Requires --copy=symlink to reproduce. +# Provides coverage for https://github.com/systemd/systemd/issues/23481 +portablectl "${ARGS[@]}" attach --copy=symlink --now --runtime /tmp/rootdir minimal-app0 +portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 +# attach and detach again to check if all drop-in configs are removed even if the main unit files are removed +portablectl "${ARGS[@]}" attach --copy=symlink --now --runtime /tmp/rootdir minimal-app0 +portablectl detach --now --runtime --enable /tmp/rootdir minimal-app0 + umount /tmp/rootdir umount /tmp/app0 umount /tmp/app1