From 337278ec05bfcb0ed4c307348bba4ffc906f2f8e Mon Sep 17 00:00:00 2001 From: Kelvin Fan Date: Fri, 24 Jul 2020 22:00:50 +0000 Subject: [PATCH] treefile: change `remove-from-packages` implementation Fix https://github.com/coreos/rpm-ostree/issues/2068: `remove-from-packages` deleting files that it shouldn't. Filter out files that user wants removed at `checkout_package_into_root()`, instead of at the `handle_remove_files_from_package()` function that does not check whether files are used by other rpms before removing them. --- rust/src/treefile.rs | 19 ++++++++ src/libpriv/rpmostree-core.c | 69 +++++++++++++++++++++++------ src/libpriv/rpmostree-postprocess.c | 68 ---------------------------- tests/compose/test-misc-tweaks.sh | 17 +++++-- 4 files changed, 87 insertions(+), 86 deletions(-) diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs index 26d2efe5c6..c9bd27ee12 100644 --- a/rust/src/treefile.rs +++ b/rust/src/treefile.rs @@ -1490,5 +1490,24 @@ mod ffi { Box::from_raw(tf); } } + + #[no_mangle] + pub extern "C" fn ror_treefile_get_files_remove_regex( + tf: *mut Treefile, + package: *const libc::c_char, + ) -> *mut *mut libc::c_char { + let package = ffi_view_os_str(package); + let package = package.to_string_lossy().into_owned(); + let mut files_to_remove: Vec = Vec::new(); + let tf = ref_from_raw_ptr(tf); + if let Some(ref packages) = tf.parsed.remove_from_packages { + for pkg in packages.iter() { + if pkg[0] == package { + files_to_remove.extend_from_slice(&pkg[1..pkg.len()]); + } + } + } + files_to_remove.to_glib_full() + } } pub use self::ffi::*; diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c index 4aedcc45f7..a042348a64 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -41,6 +41,7 @@ #include "rpmostree-kernel.h" #include "rpmostree-importer.h" #include "rpmostree-output.h" +#include "rpmostree-rust.h" #define RPMOSTREE_MESSAGE_COMMIT_STATS SD_ID128_MAKE(e6,37,2e,38,41,21,42,a9,bc,13,b6,32,b3,f8,93,44) #define RPMOSTREE_MESSAGE_SELINUX_RELABEL SD_ID128_MAKE(5a,e0,56,34,f2,d7,49,3b,b1,58,79,b7,0c,02,e6,5d) @@ -2860,21 +2861,51 @@ handle_file_dispositions (RpmOstreeContext *self, return TRUE; } +typedef struct { + GHashTable *files_skip; + GStrv files_remove_regex; +} FilterData; + static OstreeRepoCheckoutFilterResult checkout_filter (OstreeRepo *self, const char *path, struct stat *st_buf, gpointer user_data) { - GHashTable *files_skip = user_data; - if (g_hash_table_contains (files_skip, path)) - return OSTREE_REPO_CHECKOUT_FILTER_SKIP; - /* Hack for nsswitch.conf: the glibc.i686 copy is identical to the one in glibc.x86_64, - * but because we modify it at treecompose time, UNION_IDENTICAL wouldn't save us here. A - * better heuristic here might be to skip all /etc files which have a different digest - * than the one registered in the rpmdb */ - if (g_str_equal (path, "/usr/etc/nsswitch.conf")) - return OSTREE_REPO_CHECKOUT_FILTER_SKIP; + GHashTable *files_skip = ((FilterData*)user_data)->files_skip; + GStrv files_remove_regex = ((FilterData*)user_data)->files_remove_regex; + + if (files_skip && g_hash_table_size (files_skip) > 0) + { + if (g_hash_table_contains (files_skip, path)) + return OSTREE_REPO_CHECKOUT_FILTER_SKIP; + + /* Hack for nsswitch.conf: the glibc.i686 copy is identical to the one in glibc.x86_64, + * but because we modify it at treecompose time, UNION_IDENTICAL wouldn't save us here. A + * better heuristic here might be to skip all /etc files which have a different digest + * than the one registered in the rpmdb */ + if (g_str_equal (path, "/usr/etc/nsswitch.conf")) + return OSTREE_REPO_CHECKOUT_FILTER_SKIP; + } + + if (g_strv_length (files_remove_regex) > 0) + { + guint len = g_strv_length(files_remove_regex); + for (guint i = 0; i < len; i++) + { + const char *remove_regex_pattern = files_remove_regex[i]; + GRegex *regex = g_regex_new (remove_regex_pattern, G_REGEX_JAVASCRIPT_COMPAT, 0, NULL); + if (!regex) + continue; + + if (g_regex_match (regex, path, 0, NULL)) + { + g_print ("Skipping file %s from checkout\n", path); + return OSTREE_REPO_CHECKOUT_FILTER_SKIP; + } + } + } + return OSTREE_REPO_CHECKOUT_FILTER_ALLOW; } @@ -2884,7 +2915,7 @@ checkout_package (OstreeRepo *repo, const char *path, OstreeRepoDevInoCache *devino_cache, const char *pkg_commit, - GHashTable *files_skip, + FilterData *filter_data, OstreeRepoCheckoutOverwriteMode ovwmode, gboolean force_copy_zerosized, GCancellable *cancellable, @@ -2904,10 +2935,15 @@ checkout_package (OstreeRepo *repo, /* Used in the no-rofiles-fuse path */ opts.force_copy_zerosized = force_copy_zerosized; - if (files_skip && g_hash_table_size (files_skip) > 0) + /* If called by `checkout_package_into_root()`, there may be files that need to be filtered */ + if (filter_data) { - opts.filter = checkout_filter; - opts.filter_user_data = files_skip; + if ((filter_data->files_skip && g_hash_table_size (filter_data->files_skip) > 0) || + g_strv_length (filter_data->files_remove_regex) > 0) + { + opts.filter = checkout_filter; + opts.filter_user_data = filter_data; + } } return ostree_repo_checkout_at (repo, &opts, dfd, path, @@ -2926,6 +2962,11 @@ checkout_package_into_root (RpmOstreeContext *self, GCancellable *cancellable, GError **error) { + g_auto(GStrv) files_remove_regex = + ror_treefile_get_files_remove_regex (self->treefile_rs, dnf_package_get_name (pkg)); + + FilterData filter_data = { files_skip, files_remove_regex, }; + OstreeRepo *pkgcache_repo = get_pkgcache_repo (self); /* The below is currently TRUE only in the --unified-core path. We probably want to @@ -2943,7 +2984,7 @@ checkout_package_into_root (RpmOstreeContext *self, } if (!checkout_package (pkgcache_repo, dfd, path, - devino_cache, pkg_commit, files_skip, ovwmode, + devino_cache, pkg_commit, &filter_data, ovwmode, !self->enable_rofiles, cancellable, error)) return glnx_prefix_error (error, "Checkout %s", dnf_package_get_nevra (pkg)); diff --git a/src/libpriv/rpmostree-postprocess.c b/src/libpriv/rpmostree-postprocess.c index a3f9cfffca..3dc79342ef 100644 --- a/src/libpriv/rpmostree-postprocess.c +++ b/src/libpriv/rpmostree-postprocess.c @@ -1097,55 +1097,6 @@ rpmostree_postprocess_final (int rootfs_dfd, return TRUE; } -static gboolean -handle_remove_files_from_package (int rootfs_fd, - RpmOstreeRefSack *refsack, - JsonArray *removespec, - GCancellable *cancellable, - GError **error) -{ - const char *pkgname = json_array_get_string_element (removespec, 0); - const guint len = json_array_get_length (removespec); - hy_autoquery HyQuery query = hy_query_create (refsack->sack); - hy_query_filter (query, HY_PKG_NAME, HY_EQ, pkgname); - g_autoptr(GPtrArray) pkglist = hy_query_run (query); - guint npackages = pkglist->len; - if (npackages == 0) - return glnx_throw (error, "Unable to find package '%s' specified in remove-from-packages", pkgname); - - for (guint j = 0; j < npackages; j++) - { - DnfPackage *pkg = pkglist->pdata[j]; - g_auto(GStrv) pkg_files = dnf_package_get_files (pkg); - - for (guint i = 1; i < len; i++) - { - const char *remove_regex_pattern = json_array_get_string_element (removespec, i); - - GRegex *regex = g_regex_new (remove_regex_pattern, G_REGEX_JAVASCRIPT_COMPAT, 0, error); - if (!regex) - return FALSE; - - for (char **strviter = pkg_files; strviter && strviter[0]; strviter++) - { - const char *file = *strviter; - - if (g_regex_match (regex, file, 0, NULL)) - { - if (file[0] == '/') - file++; - - g_print ("Deleting: %s\n", file); - if (!glnx_shutil_rm_rf_at (rootfs_fd, file, cancellable, error)) - return FALSE; - } - } - } - } - - return TRUE; -} - gboolean rpmostree_rootfs_symlink_emptydir_at (int rootfs_fd, const char *dest, @@ -1657,25 +1608,6 @@ rpmostree_treefile_postprocessing (int rootfs_fd, if (!rename_if_exists (rootfs_fd, "usr/etc", rootfs_fd, "etc", error)) return FALSE; - if (json_object_has_member (treefile, "remove-from-packages")) - { - remove = json_object_get_array_member (treefile, "remove-from-packages"); - len = json_array_get_length (remove); - - g_autoptr(RpmOstreeRefSack) refsack = - rpmostree_get_refsack_for_root (rootfs_fd, ".", error); - if (!refsack) - return glnx_prefix_error (error, "Reading package set"); - - for (guint i = 0; i < len; i++) - { - JsonArray *elt = json_array_get_array_element (remove, i); - if (!handle_remove_files_from_package (rootfs_fd, refsack, elt, cancellable, error)) - return FALSE; - } - - } - { const char *base_version = NULL; diff --git a/tests/compose/test-misc-tweaks.sh b/tests/compose/test-misc-tweaks.sh index 8720acb932..d0754f2594 100755 --- a/tests/compose/test-misc-tweaks.sh +++ b/tests/compose/test-misc-tweaks.sh @@ -13,11 +13,18 @@ build_rpm foobar-rec build_rpm quuz build_rpm corge version 1.0 build_rpm corge version 2.0 +# test `remove-from-packages` (files shared with other pkgs should not be removed) +build_rpm barbar \ + files "/etc/sharedfile" \ + install "mkdir -p %{buildroot}/etc && echo network data > %{buildroot}/etc/sharedfile" +build_rpm barbaz \ + files "/etc/sharedfile" \ + install "mkdir -p %{buildroot}/etc && echo network data > %{buildroot}/etc/sharedfile" echo gpgcheck=0 >> yumrepo.repo ln "$PWD/yumrepo.repo" config/yumrepo.repo # the top-level manifest doesn't have any packages, so just set it -treefile_append "packages" $'["\'foobar >= 0.5\' quuz \'corge < 2.0\'"]' +treefile_append "packages" $'["\'foobar >= 0.5\' quuz \'corge < 2.0\' barbar barbaz"]' # With docs and recommends, also test multi includes cat > config/documentation.yaml <<'EOF' @@ -67,7 +74,8 @@ EOF chmod a+x postprocess.sh treefile_set "remove-files" '["etc/hosts"]' -treefile_set "remove-from-packages" '[["setup", "/etc/csh\..*"]]' +treefile_set "remove-from-packages" '[["setup", "/etc/csh\..*"], + ["barbar", "/etc/sharedfile"]]' rnd=$RANDOM echo $rnd > config/foo.txt echo bar > config/bar.txt @@ -149,8 +157,9 @@ assert_not_file_has_content out.txt '/usr/etc/hosts$' echo "ok remove-files" ostree --repo=${repo} ls ${treeref} /usr/etc > out.txt -assert_not_file_has_content out.txt '/usr/etc/csh\.login$' -assert_not_file_has_content out.txt '/usr/etc/csh\.cshrc$' +assert_not_file_has_content out.txt '/etc/csh\.login' +assert_not_file_has_content out.txt '/etc/csh\.cshrc' +assert_file_has_content out.txt '/etc/sharedfile' echo "ok remove-from-packages" # https://github.com/projectatomic/rpm-ostree/issues/669