From 24b69167557dc11ce70eccae262f31949faf3708 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 5 Jun 2019 15:05:22 -0400 Subject: [PATCH] core: Strengthen how we enforce lockfiles One problem with how we use lockfiles right now is that we don't enforce them for dependencies. That is, if `foo` requires `bar`, but only `foo` is in the manifest, then while `foo` will be locked, `bar` will never be checked against the lockfile because it was never explicitly requested. Higher-level though, I don't like how indirect the locking here feels. See some comments about that in: https://github.com/projectatomic/rpm-ostree/pull/1745#discussion_r288772527 https://github.com/projectatomic/rpm-ostree/pull/1745#discussion_r289419017 Essentially, the manifest is an input file of patterns, and all we really know from the lockfile output is that the set of packages in there satisfies this input in some way. But: 1. there are multiple ways to satisfy the same input (hence why hints like `SOLVER_FAVOR` exist) 2. the solution is dependent on how the solver is implemented (i.e. different libsolv versions might yield different solutions) 3. the solution is dependent on flags fed to the solver (i.e. different libdnf versions might yield different solutions) So any attempt at cross-checking between the input file and the lockfile is going to be very hard. Using a stricter mode as I suggested in #1745 of only allowing pure pkgnames or NEVRAs would help, but it wouldn't address the dependency issue. (Though I'm still thinking about possibly doing this anyway.) The solution I propose here is instead to take the nuclear approach: we completely exclude from the sack all packages of the same name as packages in our lockfiles, but which do not match the NEVRA. Therefore, any possible solution has to also satisfy our lockfile (or error out). Closes: #1849 Approved by: cgwalters --- src/libpriv/rpmostree-core.c | 75 +++++++++++++++++++++++++--- src/libpriv/rpmostree-rpm-util.c | 26 ---------- src/libpriv/rpmostree-rpm-util.h | 3 -- tests/compose-tests/test-lockfile.sh | 15 +++--- 4 files changed, 74 insertions(+), 45 deletions(-) diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c index 2f13c69ec4..2ab3cf03fa 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -1830,6 +1830,50 @@ setup_rojig_state (RpmOstreeContext *self, return TRUE; } +static gboolean +check_locked_pkgs (RpmOstreeContext *self, + GError **error) +{ + if (!self->vlockmap) + return TRUE; + + HyGoal goal = dnf_context_get_goal (self->dnfctx); + + /* sanity check it's all pure installs */ + { g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, + DNF_PACKAGE_INFO_REMOVE, + DNF_PACKAGE_INFO_OBSOLETE, + DNF_PACKAGE_INFO_REINSTALL, + DNF_PACKAGE_INFO_UPDATE, + DNF_PACKAGE_INFO_DOWNGRADE, + -1); + if (packages->len > 0) + return throw_package_list (error, "Packages not marked for pure install", packages); + } + + g_autoptr(GPtrArray) packages = + dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, -1); + + for (guint i = 0; i < packages->len; i++) + { + DnfPackage *pkg = packages->pdata[i]; + const char *nevra = dnf_package_get_nevra (pkg); + const char *chksum = g_hash_table_lookup (self->vlockmap, nevra); + if (!chksum) + continue; + + g_autofree char *repodata_chksum = NULL; + if (!rpmostree_get_repodata_chksum_repr (pkg, &repodata_chksum, error)) + return FALSE; + + if (chksum && !g_str_equal (chksum, repodata_chksum)) + return glnx_throw (error, "Locked package %s checksum mismatch: expected %s, got %s", + nevra, chksum, repodata_chksum); + } + + return TRUE; +} + /* Check for/download new rpm-md, then depsolve */ gboolean rpmostree_context_prepare (RpmOstreeContext *self, @@ -1936,6 +1980,27 @@ rpmostree_context_prepare (RpmOstreeContext *self, * uninstall. We don't want to mix those two steps, otherwise we might confuse libdnf, * see: https://github.com/rpm-software-management/libdnf/issues/700 */ + /* Before adding any pkgs to the goal; filter out from the sack all pkgs which don't match + * our lockfile. (But of course, leave other pkgs untouched.) */ + if (self->vlockmap != NULL) + { + g_assert_cmpuint (g_strv_length (cached_replace_pkgs), ==, 0); + g_assert_cmpuint (g_strv_length (removed_base_pkgnames), ==, 0); + + GLNX_HASH_TABLE_FOREACH (self->vlockmap, const char*, nevra) + { + g_autofree char *name = NULL; + if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error)) + return FALSE; + hy_autoquery HyQuery query = hy_query_create (sack); + hy_query_filter (query, HY_PKG_NAME, HY_EQ, name); + hy_query_filter (query, HY_PKG_NEVRA, HY_NEQ, nevra); + DnfPackageSet *pset = hy_query_run_set (query); + dnf_sack_add_excludes (sack, pset); + dnf_packageset_free (pset); + } + } + /* First, handle packages to remove */ g_autoptr(GPtrArray) removed_pkgnames = g_ptr_array_new (); for (char **it = removed_base_pkgnames; it && *it; it++) @@ -1958,13 +2023,6 @@ rpmostree_context_prepare (RpmOstreeContext *self, for (char **it = pkgnames; it && *it; it++) { const char *pkgname = *it; - g_autoptr(DnfPackage) pkg = rpmostree_get_locked_package (sack, self->vlockmap, pkgname); - if (pkg) - { - hy_goal_install (goal, pkg); - continue; - } - g_assert (!self->rojig_pure); g_autoptr(GError) local_error = NULL; if (!dnf_context_install (dnfctx, pkgname, &local_error)) @@ -2005,7 +2063,8 @@ rpmostree_context_prepare (RpmOstreeContext *self, /* XXX: consider a --allow-uninstall switch? */ if (!dnf_goal_depsolve (goal, actions, error) || - !check_goal_solution (self, removed_pkgnames, replaced_nevras, error)) + !check_goal_solution (self, removed_pkgnames, replaced_nevras, error) || + !check_locked_pkgs (self, error)) return FALSE; g_clear_pointer (&self->pkgs, (GDestroyNotify)g_ptr_array_unref); self->pkgs = dnf_goal_get_packages (dnf_context_get_goal (dnfctx), diff --git a/src/libpriv/rpmostree-rpm-util.c b/src/libpriv/rpmostree-rpm-util.c index 5cfbe3044f..5eb045c287 100644 --- a/src/libpriv/rpmostree-rpm-util.c +++ b/src/libpriv/rpmostree-rpm-util.c @@ -1285,32 +1285,6 @@ rpmostree_create_pkglist_variant (GPtrArray *pkglist, return TRUE; } -DnfPackage * -rpmostree_get_locked_package (DnfSack *sack, - GHashTable *lockmap, - const char *name) -{ - if (!lockmap || !name) - return NULL; - - /* The manifest might specify not only package names (foo-1.x) but also - * something-that-foo-provides */ - g_autoptr(GPtrArray) alts = rpmostree_get_matching_packages (sack, name); - - for (gsize i = 0; i < alts->len; i++) - { - DnfPackage *pkg = alts->pdata[i]; - g_autofree gchar *repodata_chksum = NULL; - if (!rpmostree_get_repodata_chksum_repr (pkg, &repodata_chksum, NULL)) - continue; - - const char *chksum = g_hash_table_lookup (lockmap, dnf_package_get_nevra (pkg)); - if (chksum && !g_strcmp0 (chksum, repodata_chksum)) - return g_object_ref (pkg); - } - return NULL; -} - /* Simple wrapper around hy_split_nevra() that adds allow-none and GError convention */ gboolean rpmostree_decompose_nevra (const char *nevra, diff --git a/src/libpriv/rpmostree-rpm-util.h b/src/libpriv/rpmostree-rpm-util.h index d01b6e33d3..995b5fbe6e 100644 --- a/src/libpriv/rpmostree-rpm-util.h +++ b/src/libpriv/rpmostree-rpm-util.h @@ -216,9 +216,6 @@ rpmostree_create_pkglist_variant (GPtrArray *pkglist, GCancellable *cancellable, GError **error); -DnfPackage * -rpmostree_get_locked_package (DnfSack *sack, GHashTable *lockmap, const char *name); - char * rpmostree_get_cache_branch_for_n_evr_a (const char *name, const char *evr, const char *arch); char *rpmostree_get_cache_branch_header (Header hdr); char *rpmostree_get_rojig_branch_header (Header hdr); diff --git a/tests/compose-tests/test-lockfile.sh b/tests/compose-tests/test-lockfile.sh index 2c5b8f071e..693df956c8 100755 --- a/tests/compose-tests/test-lockfile.sh +++ b/tests/compose-tests/test-lockfile.sh @@ -7,9 +7,8 @@ dn=$(cd $(dirname $0) && pwd) prepare_compose_test "lockfile" # Add a local rpm-md repo so we can mutate local test packages pyappendjsonmember "repos" '["test-repo"]' -build_rpm test-pkg \ - files "/usr/bin/test-pkg" \ - install "mkdir -p %{buildroot}/usr/bin && echo localpkg data > %{buildroot}/usr/bin/test-pkg" +build_rpm test-pkg-common +build_rpm test-pkg requires test-pkg-common # The test suite writes to pwd, but we need repos in composedata # Also we need to disable gpgcheck echo gpgcheck=0 >> yumrepo.repo @@ -28,15 +27,15 @@ echo "ok compose" assert_has_file "versions.lock" assert_file_has_content $PWD/versions.lock 'packages' assert_file_has_content $PWD/versions.lock 'test-pkg-1.0-1.x86_64' +assert_file_has_content $PWD/versions.lock 'test-pkg-common-1.0-1.x86_64' echo "lockfile created" # Read lockfile back -build_rpm test-pkg \ - version 2.0 \ - files "/usr/bin/test-pkg" \ - install "mkdir -p %{buildroot}/usr/bin && echo localpkg data > %{buildroot}/usr/bin/test-pkg" +build_rpm test-pkg-common version 2.0 +build_rpm test-pkg version 2.0 requires test-pkg-common runcompose --ex-lockfile=$PWD/versions.lock --cachedir $(pwd)/cache echo "ok compose with lockfile" -rpm-ostree --repo=${repobuild} db list ${treeref} test-pkg >test-pkg-list.txt +rpm-ostree --repo=${repobuild} db list ${treeref} > test-pkg-list.txt assert_file_has_content test-pkg-list.txt 'test-pkg-1.0-1.x86_64' +assert_file_has_content test-pkg-list.txt 'test-pkg-common-1.0-1.x86_64' echo "lockfile read"