From 37543971aff79f3a37646ffc2bb5845c9394797b Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 21 Feb 2024 19:31:14 +0000 Subject: [PATCH 1/3] os-util: allow matching versioned image with extension-release file Currently app_1.0.raw is refused if it contains extension-release.d/extension-release.app, which stops one from using versioned images without using the force flag to disable the check. Relax it so that only the actual name, and not the version, is compared, like it already happens in other places. --- src/basic/os-util.c | 55 ++++++++++++++++++++++++++++++++-- src/basic/os-util.h | 1 + src/portable/portable.c | 61 ++++++-------------------------------- test/units/testsuite-29.sh | 13 ++++++++ 4 files changed, 75 insertions(+), 55 deletions(-) diff --git a/src/basic/os-util.c b/src/basic/os-util.c index dbd067fd44..985d89bc7e 100644 --- a/src/basic/os-util.c +++ b/src/basic/os-util.c @@ -61,6 +61,39 @@ bool image_name_is_valid(const char *s) { return true; } +int path_extract_image_name(const char *path, char **ret) { + _cleanup_free_ char *fn = NULL; + int r; + + assert(path); + assert(ret); + + /* Extract last component from path, without any "/" suffixes. */ + r = path_extract_filename(path, &fn); + if (r < 0) + return r; + + if (r != O_DIRECTORY) { + /* Chop off any image suffixes we recognize (unless we already know this must refer to some dir */ + FOREACH_STRING(suffix, ".sysext.raw", ".confext.raw", ".raw") { + char *m = endswith(fn, suffix); + if (m) { + *m = 0; + break; + } + } + } + + /* Truncate the version/counting suffixes */ + fn[strcspn(fn, "_+")] = 0; + + if (!image_name_is_valid(fn)) + return -EINVAL; + + *ret = TAKE_PTR(fn); + return 0; +} + int path_is_extension_tree(ImageClass image_class, const char *path, const char *extension, bool relax_extension_release_check) { int r; @@ -230,9 +263,25 @@ int open_extension_release_at( continue; } - if (!relax_extension_release_check && - extension_release_strict_xattr_value(fd, dir_path, de->d_name) != 0) - continue; + if (!relax_extension_release_check) { + _cleanup_free_ char *base_image_name = NULL, *base_extension = NULL; + + r = path_extract_image_name(image_name, &base_image_name); + if (r < 0) { + log_debug_errno(r, "Failed to extract image name from %s/%s, ignoring: %m", dir_path, de->d_name); + continue; + } + + r = path_extract_image_name(extension, &base_extension); + if (r < 0) { + log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", extension); + continue; + } + + if (!streq(base_image_name, base_extension) && + extension_release_strict_xattr_value(fd, dir_path, image_name) != 0) + continue; + } /* We already found what we were looking for, but there's another candidate? We treat this as * an error, as we want to enforce that there are no ambiguities in case we are in the diff --git a/src/basic/os-util.h b/src/basic/os-util.h index 7cee3dd119..f6a12a3fb1 100644 --- a/src/basic/os-util.h +++ b/src/basic/os-util.h @@ -25,6 +25,7 @@ ImageClass image_class_from_string(const char *s) _pure_; * in accordance with the OS extension specification, rather than for /usr/lib/ or /etc/os-release. */ bool image_name_is_valid(const char *s) _pure_; +int path_extract_image_name(const char *path, char **ret); int path_is_extension_tree(ImageClass image_class, const char *path, const char *extension, bool relax_extension_release_check); static inline int path_is_os_tree(const char *path) { diff --git a/src/portable/portable.c b/src/portable/portable.c index a3bec83f0d..a83f7b68cb 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1666,7 +1666,6 @@ int portable_attach( static bool marker_matches_images(const char *marker, const char *name_or_path, char **extension_image_paths) { _cleanup_strv_free_ char **root_and_extensions = NULL; - const char *a; int r; assert(marker); @@ -1687,7 +1686,7 @@ static bool marker_matches_images(const char *marker, const char *name_or_path, return r; STRV_FOREACH(image_name_or_path, root_and_extensions) { - _cleanup_free_ char *image = NULL; + _cleanup_free_ char *image = NULL, *base_image = NULL, *base_image_name_or_path = NULL; r = extract_first_word(&marker, &image, ":", EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE); if (r < 0) @@ -1695,58 +1694,16 @@ static bool marker_matches_images(const char *marker, const char *name_or_path, if (r == 0) return false; - a = last_path_component(image); + r = path_extract_image_name(image, &base_image); + if (r < 0) + return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", image); - if (image_name_is_valid(*image_name_or_path)) { - const char *e, *underscore; + r = path_extract_image_name(*image_name_or_path, &base_image_name_or_path); + if (r < 0) + return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", *image_name_or_path); - /* We shall match against an image name. In that case let's compare the last component, and optionally - * allow either a suffix of ".raw" or a series of "/". - * But allow matching on a different version of the same image, when a "_" is used as a separator. */ - underscore = strchr(*image_name_or_path, '_'); - if (underscore) { - if (strneq(a, *image_name_or_path, underscore - *image_name_or_path)) - continue; - return false; - } - - e = startswith(a, *image_name_or_path); - if (!e) - return false; - - if(!(e[strspn(e, "/")] == 0 || streq(e, ".raw"))) - return false; - } else { - const char *b, *underscore; - size_t l; - - /* We shall match against a path. Let's ignore any prefix here though, as often there are many ways to - * reach the same file. However, in this mode, let's validate any file suffix. - * But also ensure that we don't fail if both components don't have a '/' at all - * (strcspn returns the full length of the string in that case, which might not - * match as the versions might differ). */ - - l = strcspn(a, "/"); - b = last_path_component(*image_name_or_path); - - if ((a[l] != '/') != !strchr(b, '/')) /* One is a directory, the other is not */ - return false; - - if (a[l] != 0 && strcspn(b, "/") != l) - return false; - - underscore = strchr(b, '_'); - if (underscore) - l = underscore - b; - else { /* Either component could be versioned */ - underscore = strchr(a, '_'); - if (underscore) - l = underscore - a; - } - - if (!strneq(a, b, l)) - return false; - } + if (!streq(base_image, base_image_name_or_path)) + return false; } return true; diff --git a/test/units/testsuite-29.sh b/test/units/testsuite-29.sh index d40cef2551..adafdbbecc 100755 --- a/test/units/testsuite-29.sh +++ b/test/units/testsuite-29.sh @@ -139,6 +139,19 @@ grep -q -F "LogExtraFields=PORTABLE_EXTENSION_NAME_AND_VERSION=app" /run/systemd portablectl detach --now --runtime --extension /usr/share/app0.raw /usr/share/minimal_1.raw app0 +# Ensure versioned images are accepted without needing to use --force to override the extension-release +# matching + +cp /usr/share/app0.raw /tmp/app0_1.0.raw +portablectl "${ARGS[@]}" attach --now --runtime --extension /tmp/app0_1.0.raw /usr/share/minimal_0.raw app0 + +systemctl is-active app0.service +status="$(portablectl is-attached --extension app0_1 minimal_0)" +[[ "${status}" == "running-runtime" ]] + +portablectl detach --now --runtime --extension /tmp/app0_1.0.raw /usr/share/minimal_1.raw app0 +rm -f /tmp/app0_1.0.raw + portablectl "${ARGS[@]}" attach --now --runtime --extension /usr/share/app1.raw /usr/share/minimal_0.raw app1 systemctl is-active app1.service From 1cbb79276319923e6f2ebbbc6b5e127a14c16154 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Thu, 28 Mar 2024 11:11:45 +0000 Subject: [PATCH 2/3] Ensure that a portable is not detached when another portable that shares the same base is detached The matches line includes all images, but the logic returned immediately with a successful match if the first element matches. --- src/portable/portable.c | 34 ++++++++++++++++++---------------- test/units/testsuite-29.sh | 24 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index a83f7b68cb..a544a01342 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1685,26 +1685,28 @@ static bool marker_matches_images(const char *marker, const char *name_or_path, if (r < 0) return r; - STRV_FOREACH(image_name_or_path, root_and_extensions) { - _cleanup_free_ char *image = NULL, *base_image = NULL, *base_image_name_or_path = NULL; + /* Ensure the number of images passed matches the number of images listed in the marker */ + while (!isempty(marker)) + STRV_FOREACH(image_name_or_path, root_and_extensions) { + _cleanup_free_ char *image = NULL, *base_image = NULL, *base_image_name_or_path = NULL; - r = extract_first_word(&marker, &image, ":", EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE); - if (r < 0) - return log_debug_errno(r, "Failed to parse marker: %s", marker); - if (r == 0) - return false; + r = extract_first_word(&marker, &image, ":", EXTRACT_UNQUOTE|EXTRACT_RETAIN_ESCAPE); + if (r < 0) + return log_debug_errno(r, "Failed to parse marker: %s", marker); + if (r == 0) + return false; - r = path_extract_image_name(image, &base_image); - if (r < 0) - return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", image); + r = path_extract_image_name(image, &base_image); + if (r < 0) + return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", image); - r = path_extract_image_name(*image_name_or_path, &base_image_name_or_path); - if (r < 0) - return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", *image_name_or_path); + r = path_extract_image_name(*image_name_or_path, &base_image_name_or_path); + if (r < 0) + return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", *image_name_or_path); - if (!streq(base_image, base_image_name_or_path)) - return false; - } + if (!streq(base_image, base_image_name_or_path)) + return false; + } return true; } diff --git a/test/units/testsuite-29.sh b/test/units/testsuite-29.sh index adafdbbecc..d529286c6c 100755 --- a/test/units/testsuite-29.sh +++ b/test/units/testsuite-29.sh @@ -224,6 +224,30 @@ test -f /run/systemd/system.attached/app0.service test -L /run/systemd/system.attached/app0.service.d/10-profile.conf portablectl detach --runtime --extension /usr/share/app0.raw /usr/share/minimal_0.raw app0 +# Ensure that when two portables share the same base image, removing one doesn't remove the other too + +portablectl "${ARGS[@]}" attach --runtime --extension /usr/share/app0.raw /usr/share/minimal_0.raw app0 +portablectl "${ARGS[@]}" attach --runtime --extension /usr/share/app1.raw /usr/share/minimal_0.raw app1 + +status="$(portablectl is-attached --extension app0 minimal_0)" +[[ "${status}" == "attached-runtime" ]] +status="$(portablectl is-attached --extension app1 minimal_0)" +[[ "${status}" == "attached-runtime" ]] + +(! portablectl detach --runtime /usr/share/minimal_0.raw app) + +status="$(portablectl is-attached --extension app0 minimal_0)" +[[ "${status}" == "attached-runtime" ]] +status="$(portablectl is-attached --extension app1 minimal_0)" +[[ "${status}" == "attached-runtime" ]] + +portablectl detach --runtime --extension /usr/share/app0.raw /usr/share/minimal_0.raw app + +status="$(portablectl is-attached --extension app1 minimal_0)" +[[ "${status}" == "attached-runtime" ]] + +portablectl detach --runtime --extension /usr/share/app1.raw /usr/share/minimal_0.raw app + # portablectl also works with directory paths rather than images mkdir /tmp/rootdir /tmp/app0 /tmp/app1 /tmp/overlay /tmp/os-release-fix /tmp/os-release-fix/etc From 373a1e47b2878a278c130712ab2910139ccc18a2 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Thu, 28 Mar 2024 14:16:39 +0000 Subject: [PATCH 3/3] portable: fix 'portablectl list' to show the actual state for extensions When listing images they are inspected one by one, so in case of a portable with extensions they always resulted as not found. Allow a partial match when listing, so that we can find the appropriate unit that an image belongs to, and list the correct state as attached. --- src/portable/portable.c | 19 +++++++++++++------ test/units/testsuite-29.sh | 6 ++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index a544a01342..60dc98c5ba 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1664,7 +1664,7 @@ int portable_attach( return 0; } -static bool marker_matches_images(const char *marker, const char *name_or_path, char **extension_image_paths) { +static bool marker_matches_images(const char *marker, const char *name_or_path, char **extension_image_paths, bool match_all) { _cleanup_strv_free_ char **root_and_extensions = NULL; int r; @@ -1675,7 +1675,9 @@ static bool marker_matches_images(const char *marker, const char *name_or_path, * list of images/paths. We enforce strict 1:1 matching, so that we are sure * we are detaching exactly what was attached. * For each image, starting with the root, we look for a token in the marker, - * and return a negative answer on any non-matching combination. */ + * and return a negative answer on any non-matching combination. + * If a partial match is allowed, then return immediately once it is found, otherwise + * ensure that everything matches. */ root_and_extensions = strv_new(name_or_path); if (!root_and_extensions) @@ -1704,11 +1706,14 @@ static bool marker_matches_images(const char *marker, const char *name_or_path, if (r < 0) return log_debug_errno(r, "Failed to extract image name from %s, ignoring: %m", *image_name_or_path); - if (!streq(base_image, base_image_name_or_path)) - return false; + if (!streq(base_image, base_image_name_or_path)) { + if (match_all) + return false; + } else if (!match_all) + return true; } - return true; + return match_all; } static int test_chroot_dropin( @@ -1763,7 +1768,9 @@ static int test_chroot_dropin( if (!name_or_path) r = true; else - r = marker_matches_images(marker, name_or_path, extension_image_paths); + /* When detaching we want to match exactly on all images, but when inspecting we only need + * to get the state of one component */ + r = marker_matches_images(marker, name_or_path, extension_image_paths, ret_marker != NULL); if (ret_marker) *ret_marker = TAKE_PTR(marker); diff --git a/test/units/testsuite-29.sh b/test/units/testsuite-29.sh index d529286c6c..7af0ed4578 100755 --- a/test/units/testsuite-29.sh +++ b/test/units/testsuite-29.sh @@ -241,6 +241,12 @@ status="$(portablectl is-attached --extension app0 minimal_0)" status="$(portablectl is-attached --extension app1 minimal_0)" [[ "${status}" == "attached-runtime" ]] +# Ensure 'portablectl list' shows the correct status for both images +portablectl list +portablectl list | grep -F "minimal_0" | grep -q -F "attached-runtime" +portablectl list | grep -F "app0" | grep -q -F "attached-runtime" +portablectl list | grep -F "app1" | grep -q -F "attached-runtime" + portablectl detach --runtime --extension /usr/share/app0.raw /usr/share/minimal_0.raw app status="$(portablectl is-attached --extension app1 minimal_0)"