From 2923c99c800141200e5e9e71fca059acc7818a0e Mon Sep 17 00:00:00 2001 From: Jan Kolarik Date: Mon, 26 Feb 2024 09:58:33 +0000 Subject: [PATCH 1/2] MergedTransaction: Calculate RPM difference between two same versions as no-op Upstream commit: 54823d82a1369c25ba1a68c18ea2a67c41f4fbe7 If a package of a particular version is installed and would still be installed after a list of transactions, it's more user friendly to treat the whole situation as "do nothing". Resolves: https://issues.redhat.com/browse/RHEL-68770 --- libdnf/transaction/MergedTransaction.cpp | 38 ++++++++++++------- libdnf/transaction/MergedTransaction.hpp | 6 +-- .../transaction/MergedTransactionTest.cpp | 7 +--- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/libdnf/transaction/MergedTransaction.cpp b/libdnf/transaction/MergedTransaction.cpp index a8d878cb51..8f26882f71 100644 --- a/libdnf/transaction/MergedTransaction.cpp +++ b/libdnf/transaction/MergedTransaction.cpp @@ -192,7 +192,7 @@ static bool transaction_item_sort_function(const std::shared_ptr (new action) = (merged action) * - * Erase/Obsolete -> Install/Obsoleting = Reinstall/Downgrade/Upgrade + * Erase/Obsolete -> Install/Obsoleting = Downgrade/Upgrade * * Reinstall/Reason change -> (new action) = (new action) * @@ -210,6 +210,9 @@ static bool transaction_item_sort_function(const std::shared_ptr MergedTransaction::getItems() @@ -261,13 +264,16 @@ getItemIdentifier(ItemPtr item) /** * Resolve the difference between RPMs in the first and second transaction item - * and create a ItemPair of Upgrade, Downgrade or reinstall. + * and create a ItemPair of Upgrade, Downgrade or drop the item from the merged + * transaction set in case of both packages are of the same version. * Method is called when original package is being removed and than installed again. + * \param itemPairMap merged transaction set * \param previousItemPair original item pair * \param mTransItem new transaction item */ void -MergedTransaction::resolveRPMDifference(ItemPair &previousItemPair, +MergedTransaction::resolveRPMDifference(ItemPairMap &itemPairMap, + ItemPair &previousItemPair, TransactionItemBasePtr mTransItem) { auto firstItem = previousItemPair.first->getItem(); @@ -277,11 +283,10 @@ MergedTransaction::resolveRPMDifference(ItemPair &previousItemPair, auto secondRPM = std::dynamic_pointer_cast< RPMItem >(secondItem); if (firstRPM->getVersion() == secondRPM->getVersion() && - firstRPM->getEpoch() == secondRPM->getEpoch()) { - // reinstall - mTransItem->setAction(TransactionItemAction::REINSTALL); - previousItemPair.first = mTransItem; - previousItemPair.second = nullptr; + firstRPM->getEpoch() == secondRPM->getEpoch() && + firstRPM->getRelease() == secondRPM->getRelease()) { + // Drop the item from merged transaction + itemPairMap.erase(getItemIdentifier(firstItem)); return; } else if ((*firstRPM) < (*secondRPM)) { // Upgrade to secondRPM @@ -296,7 +301,9 @@ MergedTransaction::resolveRPMDifference(ItemPair &previousItemPair, } void -MergedTransaction::resolveErase(ItemPair &previousItemPair, TransactionItemBasePtr mTransItem) +MergedTransaction::resolveErase(ItemPairMap &itemPairMap, + ItemPair &previousItemPair, + TransactionItemBasePtr mTransItem) { /* * The original item has been removed - it has to be installed now unless the rpmdb @@ -306,7 +313,7 @@ MergedTransaction::resolveErase(ItemPair &previousItemPair, TransactionItemBaseP if (mTransItem->getAction() == TransactionItemAction::INSTALL) { if (mTransItem->getItem()->getItemType() == ItemType::RPM) { // resolve the difference between RPM packages - resolveRPMDifference(previousItemPair, mTransItem); + resolveRPMDifference(itemPairMap, previousItemPair, mTransItem); } else { // difference between comps can't be resolved mTransItem->setAction(TransactionItemAction::REINSTALL); @@ -323,11 +330,14 @@ MergedTransaction::resolveErase(ItemPair &previousItemPair, TransactionItemBaseP * transaction - new package is used to complete the pair. Items are stored in pairs (Upgrade, * Upgrade) or (Downgraded, Downgrade). With complete transaction pair we need to get the new * Upgrade/Downgrade item and compare its version with the original item from the pair. + * \param itemPairMap merged transaction set * \param previousItemPair original item pair * \param mTransItem new transaction item */ void -MergedTransaction::resolveAltered(ItemPair &previousItemPair, TransactionItemBasePtr mTransItem) +MergedTransaction::resolveAltered(ItemPairMap &itemPairMap, + ItemPair &previousItemPair, + TransactionItemBasePtr mTransItem) { auto newState = mTransItem->getAction(); auto firstState = previousItemPair.first->getAction(); @@ -369,7 +379,7 @@ MergedTransaction::resolveAltered(ItemPair &previousItemPair, TransactionItemBas } else { if (mTransItem->getItem()->getItemType() == ItemType::RPM) { // resolve the difference between RPM packages - resolveRPMDifference(previousItemPair, mTransItem); + resolveRPMDifference(itemPairMap, previousItemPair, mTransItem); } else { // difference between comps can't be resolved previousItemPair.second->setAction(TransactionItemAction::REINSTALL); @@ -405,7 +415,7 @@ MergedTransaction::mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr mT switch (firstState) { case TransactionItemAction::REMOVE: case TransactionItemAction::OBSOLETED: - resolveErase(previousItemPair, mTransItem); + resolveErase(itemPairMap, previousItemPair, mTransItem); break; case TransactionItemAction::INSTALL: // the original package has been installed -> it may be either Removed, or altered @@ -432,7 +442,7 @@ MergedTransaction::mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr mT case TransactionItemAction::UPGRADE: case TransactionItemAction::UPGRADED: case TransactionItemAction::OBSOLETE: - resolveAltered(previousItemPair, mTransItem); + resolveAltered(itemPairMap, previousItemPair, mTransItem); break; case TransactionItemAction::REINSTALLED: break; diff --git a/libdnf/transaction/MergedTransaction.hpp b/libdnf/transaction/MergedTransaction.hpp index dbb8af1183..f85b133a87 100644 --- a/libdnf/transaction/MergedTransaction.hpp +++ b/libdnf/transaction/MergedTransaction.hpp @@ -76,9 +76,9 @@ class MergedTransaction { typedef std::map< std::string, ItemPair > ItemPairMap; void mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr transItem); - void resolveRPMDifference(ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); - void resolveErase(ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); - void resolveAltered(ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); + void resolveRPMDifference(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); + void resolveErase(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); + void resolveAltered(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); }; } // namespace libdnf diff --git a/tests/libdnf/transaction/MergedTransactionTest.cpp b/tests/libdnf/transaction/MergedTransactionTest.cpp index 52507700bc..35fb4250e4 100644 --- a/tests/libdnf/transaction/MergedTransactionTest.cpp +++ b/tests/libdnf/transaction/MergedTransactionTest.cpp @@ -822,12 +822,7 @@ MergedTransactionTest::test_downgrade_upgrade_remove() // test merging trans1, trans2 merged.merge(trans2); auto items2 = merged.getItems(); - CPPUNIT_ASSERT_EQUAL(1, (int)items2.size()); - auto item2 = items2.at(0); - CPPUNIT_ASSERT_EQUAL(std::string("tour-4.8-1.noarch"), item2->getItem()->toStr()); - CPPUNIT_ASSERT_EQUAL(std::string("repo1"), item2->getRepoid()); - CPPUNIT_ASSERT_EQUAL(TransactionItemAction::REINSTALL, item2->getAction()); - CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item2->getReason()); + CPPUNIT_ASSERT_EQUAL(0, (int)items2.size()); // test merging trans1, trans2, trans3 merged.merge(trans3); From 08d5d6d3cb57cff1f6ceac702f306b072384924e Mon Sep 17 00:00:00 2001 From: Jan Kolarik Date: Tue, 23 Apr 2024 14:11:19 +0000 Subject: [PATCH 2/2] MergedTransaction: Fix invalid memory access when dropping items Upstream commit: 90d2ffad964a91a7a798b81e15c16eb1e840f257 When an item is dropped from the merged transaction, the `ItemPair` reference becomes invalid and should no longer be used. Resolves: https://issues.redhat.com/browse/RHEL-68770 --- libdnf/transaction/MergedTransaction.cpp | 18 +++++++++++------- libdnf/transaction/MergedTransaction.hpp | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/libdnf/transaction/MergedTransaction.cpp b/libdnf/transaction/MergedTransaction.cpp index 8f26882f71..75d2c1e78c 100644 --- a/libdnf/transaction/MergedTransaction.cpp +++ b/libdnf/transaction/MergedTransaction.cpp @@ -264,14 +264,15 @@ getItemIdentifier(ItemPtr item) /** * Resolve the difference between RPMs in the first and second transaction item - * and create a ItemPair of Upgrade, Downgrade or drop the item from the merged - * transaction set in case of both packages are of the same version. - * Method is called when original package is being removed and than installed again. + * and create a ItemPair of Upgrade, Downgrade or remove the item from the merged + * transaction set in case of both packages are the same. + * Method is called when original package is being removed and then installed again. * \param itemPairMap merged transaction set * \param previousItemPair original item pair * \param mTransItem new transaction item + * \return true if the original and new transaction item differ */ -void +bool MergedTransaction::resolveRPMDifference(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem) @@ -287,7 +288,7 @@ MergedTransaction::resolveRPMDifference(ItemPairMap &itemPairMap, firstRPM->getRelease() == secondRPM->getRelease()) { // Drop the item from merged transaction itemPairMap.erase(getItemIdentifier(firstItem)); - return; + return false; } else if ((*firstRPM) < (*secondRPM)) { // Upgrade to secondRPM previousItemPair.first->setAction(TransactionItemAction::UPGRADED); @@ -298,6 +299,7 @@ MergedTransaction::resolveRPMDifference(ItemPairMap &itemPairMap, mTransItem->setAction(TransactionItemAction::DOWNGRADE); } previousItemPair.second = mTransItem; + return true; } void @@ -308,12 +310,14 @@ MergedTransaction::resolveErase(ItemPairMap &itemPairMap, /* * The original item has been removed - it has to be installed now unless the rpmdb * has changed. Resolve the difference between packages and mark it as Upgrade, - * Reinstall or Downgrade + * Downgrade or remove it from the transaction */ if (mTransItem->getAction() == TransactionItemAction::INSTALL) { if (mTransItem->getItem()->getItemType() == ItemType::RPM) { // resolve the difference between RPM packages - resolveRPMDifference(itemPairMap, previousItemPair, mTransItem); + if (!resolveRPMDifference(itemPairMap, previousItemPair, mTransItem)) { + return; + } } else { // difference between comps can't be resolved mTransItem->setAction(TransactionItemAction::REINSTALL); diff --git a/libdnf/transaction/MergedTransaction.hpp b/libdnf/transaction/MergedTransaction.hpp index f85b133a87..50212159b7 100644 --- a/libdnf/transaction/MergedTransaction.hpp +++ b/libdnf/transaction/MergedTransaction.hpp @@ -76,7 +76,7 @@ class MergedTransaction { typedef std::map< std::string, ItemPair > ItemPairMap; void mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr transItem); - void resolveRPMDifference(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); + bool resolveRPMDifference(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); void resolveErase(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); void resolveAltered(ItemPairMap &itemPairMap, ItemPair &previousItemPair, TransactionItemBasePtr mTransItem); };