Skip to content

Commit

Permalink
treefile: change remove-from-packages implementation
Browse files Browse the repository at this point in the history 
Fix coreos#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.
  • Loading branch information
@kelvinfan001
kelvinfan001 committed Jul 30, 2020
1 parent 990b05f commit 337278e
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 86 deletions.
19 changes: 19 additions & 0 deletions rust/src/treefile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = 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::*;
69 changes: 55 additions & 14 deletions src/libpriv/rpmostree-core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}

Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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));
Expand Down
68 changes: 0 additions & 68 deletions src/libpriv/rpmostree-postprocess.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down
17 changes: 13 additions & 4 deletions tests/compose/test-misc-tweaks.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 337278e

Please sign in to comment.