From 2de7157775551185beb3d7cb56e9f9f353e57ab4 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 21 May 2013 00:05:14 +0200 Subject: rewrite pkgOrderList::DepRemove to stop incorrect immediate setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some squeeze → wheezy upgrades indicate that DepRemove runs amok in complicated setups as it wasn't correctly working with or-groups. Completely rewritten the check is now moving from or-group to or-group instead. The behavior should be the same as the code before, but (hopefully) with less bugs and more comments. Closes: 645713 --- apt-pkg/orderlist.cc | 219 ++++++++++++++++++++++++--------------------------- 1 file changed, 103 insertions(+), 116 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/orderlist.cc b/apt-pkg/orderlist.cc index 80d8fd490..86d2a9478 100644 --- a/apt-pkg/orderlist.cc +++ b/apt-pkg/orderlist.cc @@ -893,149 +893,136 @@ bool pkgOrderList::DepConfigure(DepIterator D) /*}}}*/ // OrderList::DepRemove - Removal ordering /*{{{*/ // --------------------------------------------------------------------- -/* Removal visits all reverse depends. It considers if the dependency - of the Now state version to see if it is okay with removing this - package. This check should always fail, but is provided for symetery - with the other critical handlers. - - Loops are preprocessed and logged. Removal loops can also be - detected in the critical handler. They are characterized by an - old version of A depending on B but the new version of A conflicting - with B, thus either A or B must break to install. */ -bool pkgOrderList::DepRemove(DepIterator D) +/* Checks all given dependencies if they are broken by the remove of a + package and if so fix it by visiting another provider or or-group + member to ensure that the dependee keeps working which is especially + important for Immediate packages like e.g. those depending on an + awk implementation. If the dependency can't be fixed with another + package this means an upgrade of the package will solve the problem. */ +bool pkgOrderList::DepRemove(DepIterator Broken) { - if (D.Reverse() == false) + if (Broken.Reverse() == false) return true; - for (; D.end() == false; ++D) - if (D->Type == pkgCache::Dep::Depends || D->Type == pkgCache::Dep::PreDepends) + + for (; Broken.end() == false; ++Broken) + { + if (Broken->Type != pkgCache::Dep::Depends && + Broken->Type != pkgCache::Dep::PreDepends) + continue; + + PkgIterator BrokenPkg = Broken.ParentPkg(); + // uninstalled packages can't break via a remove + if (BrokenPkg->CurrentVer == 0) + continue; + + // if its already added, we can't do anything useful + if (IsFlag(BrokenPkg, AddPending) == true || IsFlag(BrokenPkg, Added) == true) + continue; + + // if the dependee is going to be removed, visit it now + if (Cache[BrokenPkg].Delete() == true) + return VisitNode(BrokenPkg, "Remove-Dependee"); + + // The package stays around, so find out how this is possible + for (DepIterator D = BrokenPkg.CurrentVer().DependsList(); D.end() == false;) { - // Duplication elimination, consider the current version only - if (D.ParentPkg().CurrentVer() != D.ParentVer()) + // only important or-groups need fixing + if (D->Type != pkgCache::Dep::Depends && + D->Type != pkgCache::Dep::PreDepends) + { + ++D; continue; + } - /* We wish to see if the dep on the parent package is okay - in the removed (install) state of the target pkg. */ - bool tryFixDeps = false; - if (CheckDep(D) == true) + // Start is the beginning of the or-group, D is the first one after or + DepIterator Start = D; + bool foundBroken = false; + for (bool LastOR = true; D.end() == false && LastOR == true; ++D) { - // We want to catch loops with the code below. - if (IsFlag(D.ParentPkg(),AddPending) == false) - continue; + LastOR = (D->CompareOp & pkgCache::Dep::Or) == pkgCache::Dep::Or; + if (D == Broken) + foundBroken = true; } - else - tryFixDeps = true; - // This is the loop detection - if (IsFlag(D.ParentPkg(),Added) == true || - IsFlag(D.ParentPkg(),AddPending) == true) - { - if (IsFlag(D.ParentPkg(),AddPending) == true) - AddLoop(D); + // this or-group isn't the broken one: keep searching + if (foundBroken == false) continue; + + // iterate over all members of the or-group searching for a ready replacement + bool readyReplacement = false; + for (DepIterator OrMember = Start; OrMember != D && readyReplacement == false; ++OrMember) + { + Version ** Replacements = OrMember.AllTargets(); + for (Version **R = Replacements; *R != 0; ++R) + { + VerIterator Ver(Cache,*R); + // only currently installed packages can be a replacement + PkgIterator RPkg = Ver.ParentPkg(); + if (RPkg.CurrentVer() != Ver) + continue; + + // packages going to be removed can't be a replacement + if (Cache[RPkg].Delete() == true) + continue; + + readyReplacement = true; + break; + } + delete[] Replacements; } - if (tryFixDeps == true) + // something else is ready to take over, do nothing + if (readyReplacement == true) + continue; + + // see if we can visit a replacement + bool visitReplacement = false; + for (DepIterator OrMember = Start; OrMember != D && visitReplacement == false; ++OrMember) { - for (pkgCache::DepIterator F = D.ParentPkg().CurrentVer().DependsList(); - F.end() == false; ++F) + Version ** Replacements = OrMember.AllTargets(); + for (Version **R = Replacements; *R != 0; ++R) { - if (F->Type != pkgCache::Dep::Depends && F->Type != pkgCache::Dep::PreDepends) + VerIterator Ver(Cache,*R); + // consider only versions we plan to install + PkgIterator RPkg = Ver.ParentPkg(); + if (Cache[RPkg].Install() == false || Cache[RPkg].InstallVer != Ver) continue; - // Check the Providers - if (F.TargetPkg()->ProvidesList != 0) - { - pkgCache::PrvIterator Prov = F.TargetPkg().ProvidesList(); - for (; Prov.end() == false; ++Prov) - { - pkgCache::PkgIterator const P = Prov.OwnerPkg(); - if (IsFlag(P, InList) == true && - IsFlag(P, AddPending) == true && - IsFlag(P, Added) == false && - Cache[P].InstallVer == 0) - break; - } - if (Prov.end() == false) - for (pkgCache::PrvIterator Prv = F.TargetPkg().ProvidesList(); - Prv.end() == false; ++Prv) - { - pkgCache::PkgIterator const P = Prv.OwnerPkg(); - if (IsFlag(P, InList) == true && - IsFlag(P, AddPending) == false && - Cache[P].InstallVer != 0 && - VisitNode(P, "Remove-P") == true) - { - Flag(P, Immediate); - tryFixDeps = false; - break; - } - } - if (tryFixDeps == false) - break; - } - // Check for Or groups - if ((F->CompareOp & pkgCache::Dep::Or) != pkgCache::Dep::Or) + // loops are not going to help us, so don't create them + if (IsFlag(RPkg, AddPending) == true) continue; - // Lets see if the package is part of the Or group - pkgCache::DepIterator S = F; - for (; S.end() == false; ++S) + + if (IsMissing(RPkg) == true) + continue; + + visitReplacement = true; + if (IsFlag(BrokenPkg, Immediate) == false) { - if (S.TargetPkg() == D.TargetPkg()) + if (VisitNode(RPkg, "Remove-Rep") == true) break; - if ((S->CompareOp & pkgCache::Dep::Or) != pkgCache::Dep::Or || - CheckDep(S)) // Or group is satisfied by another package - for (;S.end() == false; ++S); } - if (S.end() == true) - continue; - // skip to the end of the or group - for (;S.end() == false && (S->CompareOp & pkgCache::Dep::Or) == pkgCache::Dep::Or; ++S); - ++S; - // The soon to be removed is part of the Or group - // start again in the or group and find something which will serve as replacement - for (; F.end() == false && F != S; ++F) + else { - if (IsFlag(F.TargetPkg(), InList) == true && - IsFlag(F.TargetPkg(), AddPending) == false && - Cache[F.TargetPkg()].InstallVer != 0 && - VisitNode(F.TargetPkg(), "Remove-Target") == true) - { - Flag(F.TargetPkg(), Immediate); - tryFixDeps = false; + Flag(RPkg, Immediate); + if (VisitNode(RPkg, "Remove-ImmRep") == true) break; - } - else if (F.TargetPkg()->ProvidesList != 0) - { - pkgCache::PrvIterator Prv = F.TargetPkg().ProvidesList(); - for (; Prv.end() == false; ++Prv) - { - if (IsFlag(Prv.OwnerPkg(), InList) == true && - IsFlag(Prv.OwnerPkg(), AddPending) == false && - Cache[Prv.OwnerPkg()].InstallVer != 0 && - VisitNode(Prv.OwnerPkg(), "Remove-Owner") == true) - { - Flag(Prv.OwnerPkg(), Immediate); - tryFixDeps = false; - break; - } - } - if (Prv.end() == false) - break; - } } - if (tryFixDeps == false) - break; + visitReplacement = false; } + delete[] Replacements; } - - // Skip over missing files - if (IsMissing(D.ParentPkg()) == true) + if (visitReplacement == true) continue; - - if (VisitNode(D.ParentPkg(), "Remove-Parent") == false) + + // the broken package in current version can't be fixed, so install new version + if (IsMissing(BrokenPkg) == true) + break; + + if (VisitNode(BrokenPkg, "Remove-Upgrade") == false) return false; } - + } + return true; } /*}}}*/ -- cgit v1.2.3-70-g09d2 From 978844db25deb7cd88b053bc2f4685caf2c61a75 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 21 May 2013 17:10:21 +0200 Subject: prefer Essentials over Removals in ordering score Doing Removes early is good to have them out of the way, so they don't break 'Inst' or 'Conf' chains, but scoring them above Essentials means that we end up upgrading (many) less important packages before we handle big stuff like libc6 or debconf which not only fails if those less important packages have unannounced (strict) dependencies, but also leads to having these packages unconfigured for a long time triggering bugs in maintainer scripts for no good reason (#708831). So this commits sets the default value for remove scores to 100, which is below the one for essentials (200) and a lot lower than the previous default value (500). --- apt-pkg/orderlist.cc | 5 ++--- debian/changelog | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/orderlist.cc b/apt-pkg/orderlist.cc index 86d2a9478..984ae1d10 100644 --- a/apt-pkg/orderlist.cc +++ b/apt-pkg/orderlist.cc @@ -301,9 +301,8 @@ bool pkgOrderList::OrderConfigure() /* Higher scores order earlier */ int pkgOrderList::Score(PkgIterator Pkg) { - static int const ScoreDelete = _config->FindI("OrderList::Score::Delete", 500); - - // Removal is always done first + // Removals should be done after we dealt with essentials + static int const ScoreDelete = _config->FindI("OrderList::Score::Delete", 100); if (Cache[Pkg].Delete() == true) return ScoreDelete; diff --git a/debian/changelog b/debian/changelog index d7c3494ce..f57a8334f 100644 --- a/debian/changelog +++ b/debian/changelog @@ -5,6 +5,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * remove -ldl from cdrom and -lutil from apt-get linkage * rewrite pkgOrderList::DepRemove to stop incorrect immediate setting (Closes: 645713) + * prefer Essentials over Removals in ordering score -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From 69335858269845904635c592268cf9519e75c1a9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 21 May 2013 18:06:17 +0200 Subject: fix priority sorting by prefering higher in MarkInstall Used to work until a certain (here unnamed) person came along and used the wrong operator causing low-priority packages to be sorted above high-priority packages while choosing a provider in commit 2b5c35c7bb915dbd46fefd7c79f05364ba22f93b from Nov 2011 --- apt-pkg/depcache.cc | 2 +- debian/changelog | 1 + .../test-prefer-higher-priority-providers | 36 ++++++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100755 test/integration/test-prefer-higher-priority-providers (limited to 'apt-pkg') diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc index 6a3e9bfc4..5ca0c2ea5 100644 --- a/apt-pkg/depcache.cc +++ b/apt-pkg/depcache.cc @@ -1004,7 +1004,7 @@ struct CompareProviders { } // higher priority seems like a good idea if (AV->Priority != BV->Priority) - return AV->Priority < BV->Priority; + return AV->Priority > BV->Priority; // prefer native architecture if (strcmp(A.Arch(), B.Arch()) != 0) { diff --git a/debian/changelog b/debian/changelog index f57a8334f..af606453d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -6,6 +6,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * rewrite pkgOrderList::DepRemove to stop incorrect immediate setting (Closes: 645713) * prefer Essentials over Removals in ordering score + * fix priority sorting by prefering higher in MarkInstall -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/test/integration/test-prefer-higher-priority-providers b/test/integration/test-prefer-higher-priority-providers new file mode 100755 index 000000000..66458bee0 --- /dev/null +++ b/test/integration/test-prefer-higher-priority-providers @@ -0,0 +1,36 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework +setupenvironment +configarchitecture 'native' + +insertpackage 'unstable' 'foo' 'all' '1' 'Provides: stuff' 'important' +insertpackage 'unstable' 'bar' 'all' '1' 'Provides: stuff' 'optional' +insertpackage 'unstable' 'baz' 'all' '1' 'Provides: stuff' 'extra' +insertpackage 'unstable' 'awesome' 'all' '1' 'Depends: stuff' + +setupaptarchive + +testequal 'Reading package lists... +Building dependency tree... +The following extra packages will be installed: + foo +The following NEW packages will be installed: + awesome foo +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst foo (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf foo (1 unstable [all]) +Conf awesome (1 unstable [all])' aptget install awesome -s + +testequal 'Reading package lists... +Building dependency tree... +The following NEW packages will be installed: + awesome foo +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst foo (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf foo (1 unstable [all]) +Conf awesome (1 unstable [all])' aptget install awesome foo -s -- cgit v1.2.3-70-g09d2 From 66706285737a895d0baf64c2387c58d5211be4f9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 21 May 2013 21:50:30 +0200 Subject: try all providers in order if uninstallable in MarkInstall --- apt-pkg/depcache.cc | 18 ++++-- debian/changelog | 1 + .../test-prefer-higher-priority-providers | 70 ++++++++++++++++++++++ 3 files changed, 84 insertions(+), 5 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc index 5ca0c2ea5..5bed10d95 100644 --- a/apt-pkg/depcache.cc +++ b/apt-pkg/depcache.cc @@ -1200,16 +1200,23 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst, verlist.insert(Cand); } CompareProviders comp(Start); - APT::VersionList::iterator InstVer = std::max_element(verlist.begin(), verlist.end(), comp); - if (InstVer != verlist.end()) - { + do { + APT::VersionList::iterator InstVer = std::max_element(verlist.begin(), verlist.end(), comp); + + if (InstVer == verlist.end()) + break; + pkgCache::PkgIterator InstPkg = InstVer.ParentPkg(); if(DebugAutoInstall == true) std::clog << OutputInDepth(Depth) << "Installing " << InstPkg.Name() << " as " << Start.DepType() << " of " << Pkg.Name() << std::endl; - MarkInstall(InstPkg, true, Depth + 1, false, ForceImportantDeps); + if (MarkInstall(InstPkg, true, Depth + 1, false, ForceImportantDeps) == false) + { + verlist.erase(InstVer); + continue; + } // now check if we should consider it a automatic dependency or not if(InstPkg->CurrentVer == 0 && Pkg->Section != 0 && ConfigValueInSubTree("APT::Never-MarkAuto-Sections", Pkg.Section())) { @@ -1218,7 +1225,8 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst, << Start.DepType() << " of pkg in APT::Never-MarkAuto-Sections)" << std::endl; MarkAuto(InstPkg, false); } - } + break; + } while(true); continue; } /* Negative dependencies have no or-group diff --git a/debian/changelog b/debian/changelog index af606453d..b64a57bef 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,6 +7,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low (Closes: 645713) * prefer Essentials over Removals in ordering score * fix priority sorting by prefering higher in MarkInstall + * try all providers in order if uninstallable in MarkInstall -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/test/integration/test-prefer-higher-priority-providers b/test/integration/test-prefer-higher-priority-providers index 66458bee0..64b901dd0 100755 --- a/test/integration/test-prefer-higher-priority-providers +++ b/test/integration/test-prefer-higher-priority-providers @@ -34,3 +34,73 @@ Inst foo (1 unstable [all]) Inst awesome (1 unstable [all]) Conf foo (1 unstable [all]) Conf awesome (1 unstable [all])' aptget install awesome foo -s + +testequal "Reading package lists... +Building dependency tree... +Package 'bar' is not installed, so not removed +Package 'baz' is not installed, so not removed +The following extra packages will be installed: + foo +The following NEW packages will be installed: + awesome foo +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst foo (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf foo (1 unstable [all]) +Conf awesome (1 unstable [all])" aptget install awesome bar- baz- -s + +testequal "Reading package lists... +Building dependency tree... +Package 'foo' is not installed, so not removed +The following extra packages will be installed: + bar +The following NEW packages will be installed: + awesome bar +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst bar (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf bar (1 unstable [all]) +Conf awesome (1 unstable [all])" aptget install awesome foo- -s + +testequal "Reading package lists... +Building dependency tree... +Package 'foo' is not installed, so not removed +Package 'baz' is not installed, so not removed +The following extra packages will be installed: + bar +The following NEW packages will be installed: + awesome bar +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst bar (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf bar (1 unstable [all]) +Conf awesome (1 unstable [all])" aptget install awesome foo- baz- -s + +testequal "Reading package lists... +Building dependency tree... +Package 'foo' is not installed, so not removed +Package 'bar' is not installed, so not removed +The following extra packages will be installed: + baz +The following NEW packages will be installed: + awesome baz +0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. +Inst baz (1 unstable [all]) +Inst awesome (1 unstable [all]) +Conf baz (1 unstable [all]) +Conf awesome (1 unstable [all])" aptget install awesome foo- bar- -s + +testequal "Reading package lists... +Building dependency tree... +Package 'foo' is not installed, so not removed +Package 'bar' is not installed, so not removed +Package 'baz' is not installed, so not removed +Some packages could not be installed. This may mean that you have +requested an impossible situation or if you are using the unstable +distribution that some required packages have not yet been created +or been moved out of Incoming. +The following information may help to resolve the situation: + +The following packages have unmet dependencies: + awesome : Depends: stuff +E: Unable to correct problems, you have held broken packages." aptget install awesome foo- bar- baz- -s -- cgit v1.2.3-70-g09d2 From 42d51f333e8ef522fed02cdfc48663488d56c3a3 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 23 May 2013 12:14:56 +0200 Subject: do unpacks before configures in SmartConfigure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splits the big loop over dependencies in SmartConfigure which unpacks and configures dependencies into two loops and reverse their order, so that all dependencies which need to be unpacked are handled first and only after that configures are issued for dependencies. This is needed as otherwise the unpack of a (new) dependency will be issued in between a configure call for two (or more) packages which form a loop, which means the configure calls aren't part of the same dpkg call and therefore dpkg bails out. Such tight loops should really be avoided as they are usually wrong – and in reality the dependencies in libreoffice were greatly simplified thanks to Rene Engelhard so the problem is gone for the benefit of all. Closes: 707578 --- apt-pkg/packagemanager.cc | 113 +++++++++++++-------- debian/changelog | 1 + ...ight-loop-configure-with-unpacking-new-packages | 46 +++++++++ 3 files changed, 115 insertions(+), 45 deletions(-) create mode 100755 test/integration/test-very-tight-loop-configure-with-unpacking-new-packages (limited to 'apt-pkg') diff --git a/apt-pkg/packagemanager.cc b/apt-pkg/packagemanager.cc index e2d7dbf2a..b8932753d 100644 --- a/apt-pkg/packagemanager.cc +++ b/apt-pkg/packagemanager.cc @@ -340,6 +340,7 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) bool Bad = false, Changed = false; const unsigned int max_loops = _config->FindI("APT::pkgPackageManager::MaxLoopCount", 500); unsigned int i=0; + std::list needConfigure; do { Changed = false; @@ -353,7 +354,7 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) continue; Bad = true; - // Search for dependencies which are unpacked but aren't configured yet (maybe loops) + // Check for dependencies that have not been unpacked, probably due to loops. for (DepIterator Cur = Start; true; ++Cur) { SPtrArray VList = Cur.AllTargets(); @@ -373,51 +374,63 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) } // Check if the version that is going to be installed will satisfy the dependency - if (Cache[DepPkg].InstallVer != *I) + if (Cache[DepPkg].InstallVer != *I || List->IsNow(DepPkg) == false) continue; - if (List->IsFlag(DepPkg,pkgOrderList::UnPacked)) + if (PkgLoop == true) { - if (List->IsFlag(DepPkg,pkgOrderList::Loop) && PkgLoop) - { - // This dependency has already been dealt with by another SmartConfigure on Pkg - Bad = false; - break; - } - /* Check for a loop to prevent one forming - If A depends on B and B depends on A, SmartConfigure will - just hop between them if this is not checked. Dont remove the - loop flag after finishing however as loop is already set. - This means that there is another SmartConfigure call for this - package and it will remove the loop flag */ + if (Debug) + std::clog << OutputInDepth(Depth) << "Package " << Pkg << " loops in SmartConfigure" << std::endl; + Bad = false; + break; + } + else + { + if (Debug) + clog << OutputInDepth(Depth) << "Unpacking " << DepPkg.FullName() << " to avoid loop " << Cur << endl; if (PkgLoop == false) List->Flag(Pkg,pkgOrderList::Loop); - if (SmartConfigure(DepPkg, Depth + 1) == true) + if (SmartUnPack(DepPkg, true, Depth + 1) == true) { Bad = false; if (List->IsFlag(DepPkg,pkgOrderList::Loop) == false) - Changed = true; + Changed = true; } if (PkgLoop == false) - List->RmFlag(Pkg,pkgOrderList::Loop); - // If SmartConfigure was succesfull, Bad is false, so break + List->RmFlag(Pkg,pkgOrderList::Loop); if (Bad == false) break; } - else if (List->IsFlag(DepPkg,pkgOrderList::Configured)) - { - Bad = false; - break; - } } - if (Cur == End) + + if (Cur == End || Bad == false) break; - } + } if (Bad == false) continue; - // Check for dependencies that have not been unpacked, probably due to loops. + needConfigure.push_back(Start); + } + if (i++ > max_loops) + return _error->Error("Internal error: MaxLoopCount reached in SmartUnPack (1) for %s, aborting", Pkg.FullName().c_str()); + } while (Changed == true); + + Bad = false, Changed = false, i = 0; + do + { + Changed = false; + for (std::list::iterator D = needConfigure.begin(); D != needConfigure.end(); ++D) + { + // Compute a single dependency element (glob or) + pkgCache::DepIterator Start, End; + D->GlobOr(Start,End); + + if (End->Type != pkgCache::Dep::Depends) + continue; + Bad = true; + + // Search for dependencies which are unpacked but aren't configured yet (maybe loops) for (DepIterator Cur = Start; true; ++Cur) { SPtrArray VList = Cur.AllTargets(); @@ -428,44 +441,54 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) PkgIterator DepPkg = Ver.ParentPkg(); // Check if the version that is going to be installed will satisfy the dependency - if (Cache[DepPkg].InstallVer != *I || List->IsNow(DepPkg) == false) + if (Cache[DepPkg].InstallVer != *I) continue; - if (PkgLoop == true) - { - if (Debug) - std::clog << OutputInDepth(Depth) << "Package " << Pkg << " loops in SmartConfigure" << std::endl; - Bad = false; - break; - } - else + if (List->IsFlag(DepPkg,pkgOrderList::UnPacked)) { - if (Debug) - clog << OutputInDepth(Depth) << "Unpacking " << DepPkg.FullName() << " to avoid loop " << Cur << endl; + if (List->IsFlag(DepPkg,pkgOrderList::Loop) && PkgLoop) + { + // This dependency has already been dealt with by another SmartConfigure on Pkg + Bad = false; + break; + } + /* Check for a loop to prevent one forming + If A depends on B and B depends on A, SmartConfigure will + just hop between them if this is not checked. Dont remove the + loop flag after finishing however as loop is already set. + This means that there is another SmartConfigure call for this + package and it will remove the loop flag */ if (PkgLoop == false) List->Flag(Pkg,pkgOrderList::Loop); - if (SmartUnPack(DepPkg, true, Depth + 1) == true) + if (SmartConfigure(DepPkg, Depth + 1) == true) { Bad = false; if (List->IsFlag(DepPkg,pkgOrderList::Loop) == false) - Changed = true; + Changed = true; } if (PkgLoop == false) - List->RmFlag(Pkg,pkgOrderList::Loop); + List->RmFlag(Pkg,pkgOrderList::Loop); + // If SmartConfigure was succesfull, Bad is false, so break if (Bad == false) break; } + else if (List->IsFlag(DepPkg,pkgOrderList::Configured)) + { + Bad = false; + break; + } } - - if (Cur == End) + if (Cur == End || Bad == false) break; - } + } + + if (Bad == true && Changed == false && Debug == true) std::clog << OutputInDepth(Depth) << "Could not satisfy " << Start << std::endl; } if (i++ > max_loops) - return _error->Error("Internal error: MaxLoopCount reached in SmartUnPack for %s, aborting", Pkg.FullName().c_str()); + return _error->Error("Internal error: MaxLoopCount reached in SmartUnPack (2) for %s, aborting", Pkg.FullName().c_str()); } while (Changed == true); if (Bad) { diff --git a/debian/changelog b/debian/changelog index b64a57bef..8a1194b1b 100644 --- a/debian/changelog +++ b/debian/changelog @@ -8,6 +8,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * prefer Essentials over Removals in ordering score * fix priority sorting by prefering higher in MarkInstall * try all providers in order if uninstallable in MarkInstall + * do unpacks before configures in SmartConfigure (Closes: #707578) -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/test/integration/test-very-tight-loop-configure-with-unpacking-new-packages b/test/integration/test-very-tight-loop-configure-with-unpacking-new-packages new file mode 100755 index 000000000..7f3b05e59 --- /dev/null +++ b/test/integration/test-very-tight-loop-configure-with-unpacking-new-packages @@ -0,0 +1,46 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework +setupenvironment +configarchitecture 'amd64' + +# the difference between version 3 and 4 is the new package 'ure' which +# we have to unpack before we start configuring parts of the loop +insertinstalledpackage 'libreoffice' 'amd64' '3' 'Depends: libreoffice-core (= 3)' +insertinstalledpackage 'libreoffice-core' 'amd64' '3' 'Depends: libreoffice-common (>= 3)' +insertinstalledpackage 'libreoffice-common' 'all' '3' 'Depends: libreoffice-style +Breaks: libreoffice-core (>= 3+), libreoffice-core (<= 3~), libreoffice-style-galaxy (>= 3+), libreoffice-style-galaxy (<= 3~)' +insertinstalledpackage 'libreoffice-style-galaxy' 'amd64' '3' 'Depends: libreoffice-core +Provides: libreoffice-style' + +buildsimplenativepackage 'libreoffice' 'amd64' '4' 'sid' 'Depends: libreoffice-core (= 4)' +buildsimplenativepackage 'libreoffice-core' 'amd64' '4' 'sid' 'Depends: libreoffice-common (>= 4) +Breaks: libreoffice-common (<< 4), libreoffice-style-galaxy (<< 4)' +buildsimplenativepackage 'libreoffice-common' 'all' '4' 'sid' 'Depends: libreoffice-style, ure +Breaks: libreoffice-core (>= 4+), libreoffice-core (<= 4~), libreoffice-style-galaxy (>= 4+), libreoffice-style-galaxy (<= 4~)' +buildsimplenativepackage 'libreoffice-style-galaxy' 'amd64' '4' 'sid' 'Depends: libreoffice-core +Provides: libreoffice-style' + +buildsimplenativepackage 'ure' 'amd64' '4' 'sid' + +setupaptarchive + +testequal 'Reading package lists... +Building dependency tree... +The following NEW packages will be installed: + ure +The following packages will be upgraded: + libreoffice libreoffice-common libreoffice-core libreoffice-style-galaxy +4 upgraded, 1 newly installed, 0 to remove and 0 not upgraded. +Inst libreoffice [3] (4 sid [amd64]) [] +Inst libreoffice-style-galaxy [3] (4 sid [amd64]) [libreoffice-common:amd64 on libreoffice-style-galaxy:amd64] [libreoffice-common:amd64 ] +Inst libreoffice-core [3] (4 sid [amd64]) [libreoffice-core:amd64 on libreoffice-common:amd64] [libreoffice-common:amd64 on libreoffice-core:amd64] [libreoffice-common:amd64 on libreoffice-style-galaxy:amd64] [libreoffice-common:amd64 ] +Inst libreoffice-common [3] (4 sid [all]) [] +Inst ure (4 sid [amd64]) +Conf ure (4 sid [amd64]) +Conf libreoffice-style-galaxy (4 sid [amd64]) +Conf libreoffice-common (4 sid [all]) +Conf libreoffice-core (4 sid [amd64]) +Conf libreoffice (4 sid [amd64])' aptget dist-upgrade -s -- cgit v1.2.3-70-g09d2 From f3c736f9b6fdef1d8045846c465d675858eb1471 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 24 May 2013 10:20:38 +0200 Subject: deprecate InstallProtect as a cpu-eating no-op In the past packages were flagged "Protected" so that install/ remove markings where issued before the ProblemResolver. Nowadays, the marking methods check if they are allowed to modify the marking of a package instead, so that markings set by FromUser calls are not overwritten anymore by automatic calls which elimates the need for InstallProtect which just eats CPU now. The method itself is left untouched for now in case frontend needs it still for some wierd usecase, but they should be eliminated. --- apt-pkg/algorithms.cc | 6 ++++-- apt-pkg/algorithms.h | 7 ++++--- cmdline/apt-get.cc | 4 +--- 3 files changed, 9 insertions(+), 8 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/algorithms.cc b/apt-pkg/algorithms.cc index 6cde4d6cc..d19783983 100644 --- a/apt-pkg/algorithms.cc +++ b/apt-pkg/algorithms.cc @@ -1440,9 +1440,11 @@ bool pkgProblemResolver::ResolveByKeepInternal() return true; } /*}}}*/ -// ProblemResolver::InstallProtect - Install all protected packages /*{{{*/ +// ProblemResolver::InstallProtect - deprecated cpu-eating no-op /*{{{*/ // --------------------------------------------------------------------- -/* This is used to make sure protected packages are installed */ +/* Actions issued with FromUser bit set are protected from further + modification (expect by other calls with FromUser set) nowadays , so we + don't need to reissue actions here, they are already set in stone. */ void pkgProblemResolver::InstallProtect() { pkgDepCache::ActionGroup group(Cache); diff --git a/apt-pkg/algorithms.h b/apt-pkg/algorithms.h index aff8a68f2..7f58c8eed 100644 --- a/apt-pkg/algorithms.h +++ b/apt-pkg/algorithms.h @@ -36,6 +36,8 @@ #include +#include + #ifndef APT_8_CLEANER_HEADERS #include using std::ostream; @@ -132,9 +134,8 @@ class pkgProblemResolver /*{{{*/ // Try to resolve problems only by using keep bool ResolveByKeep(); - // Install all protected packages - void InstallProtect(); - + __deprecated void InstallProtect(); + pkgProblemResolver(pkgDepCache *Cache); ~pkgProblemResolver(); }; diff --git a/cmdline/apt-get.cc b/cmdline/apt-get.cc index 999f2a6a7..85ed80a95 100644 --- a/cmdline/apt-get.cc +++ b/cmdline/apt-get.cc @@ -1962,7 +1962,6 @@ bool DoInstall(CommandLine &CmdL) if (Fix != NULL) { // Call the scored problem resolver - Fix->InstallProtect(); Fix->Resolve(true); delete Fix; } @@ -3123,8 +3122,7 @@ bool DoBuildDep(CommandLine &CmdL) } } } - - Fix.InstallProtect(); + if (Fix.Resolve(true) == false) _error->Discard(); -- cgit v1.2.3-70-g09d2 From c7873955a3b9419c5de49ee4665a6006b3bf6563 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 25 May 2013 20:18:15 +0200 Subject: make the vprintf like _error->Insert public Git-Dch: Ignore --- apt-pkg/contrib/error.h | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/error.h b/apt-pkg/contrib/error.h index 21c51c1be..7d09b2d4a 100644 --- a/apt-pkg/contrib/error.h +++ b/apt-pkg/contrib/error.h @@ -123,6 +123,25 @@ public: /*{{{*/ bool InsertErrno(MsgType const &type, const char* Function, const char* Description,...) __like_printf(4) __cold; + /** \brief adds an errno message with the given type + * + * args needs to be initialized with va_start and terminated + * with va_end by the caller. msgSize is also an out-parameter + * in case the msgSize was not enough to store the complete message. + * + * \param type of the error message + * \param Function which failed + * \param Description is the format string for args + * \param args list from a printf-like function + * \param errsv is the errno the error is for + * \param msgSize is the size of the char[] used to store message + * \return true if the message was added, false if not - the caller + * should call this method again in that case + */ + bool InsertErrno(MsgType type, const char* Function, + const char* Description, va_list &args, + int const errsv, size_t &msgSize); + /** \brief add an fatal error message to the list * * Most of the stuff we consider as "error" is also "fatal" for @@ -185,6 +204,22 @@ public: /*{{{*/ */ bool Insert(MsgType const &type, const char* Description,...) __like_printf(3) __cold; + /** \brief adds an error message with the given type + * + * args needs to be initialized with va_start and terminated + * with va_end by the caller. msgSize is also an out-parameter + * in case the msgSize was not enough to store the complete message. + * + * \param type of the error message + * \param Description is the format string for args + * \param args list from a printf-like function + * \param msgSize is the size of the char[] used to store message + * \return true if the message was added, false if not - the caller + * should call this method again in that case + */ + bool Insert(MsgType type, const char* Description, + va_list &args, size_t &msgSize) __cold; + /** \brief is an error in the list? * * \return \b true if an error is included in the list, \b false otherwise @@ -305,12 +340,6 @@ private: /*{{{*/ }; std::list Stacks; - - bool InsertErrno(MsgType type, const char* Function, - const char* Description, va_list &args, - int const errsv, size_t &msgSize); - bool Insert(MsgType type, const char* Description, - va_list &args, size_t &msgSize); /*}}}*/ }; /*}}}*/ -- cgit v1.2.3-70-g09d2 From ae635e3cf7559f3455b88a2499e7521d2094c416 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 25 May 2013 20:22:06 +0200 Subject: set Fail flag in FileFd on all errors consistently Previously some errors would set the Fail flag while some didn't without a clear reason as all errors leave a bad FileFd behind, so we use a helper now to ensure that all errors set the flag. --- apt-pkg/contrib/fileutl.cc | 167 +++++++++++++++++++-------------------------- apt-pkg/contrib/fileutl.h | 4 ++ debian/changelog | 1 + 3 files changed, 76 insertions(+), 96 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index 46661887a..4a7299e99 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -861,7 +861,7 @@ bool FileFd::Open(string FileName,unsigned int const Mode,CompressMode Compress, return Open(FileName, ReadOnly, Gzip, Perms); if (Compress == Auto && (Mode & WriteOnly) == WriteOnly) - return _error->Error("Autodetection on %s only works in ReadOnly openmode!", FileName.c_str()); + return FileFdError("Autodetection on %s only works in ReadOnly openmode!", FileName.c_str()); std::vector const compressors = APT::Configuration::getCompressors(); std::vector::const_iterator compressor = compressors.begin(); @@ -914,17 +914,17 @@ bool FileFd::Open(string FileName,unsigned int const Mode,CompressMode Compress, case Auto: case Extension: // Unreachable - return _error->Error("Opening File %s in None, Auto or Extension should be already handled?!?", FileName.c_str()); + return FileFdError("Opening File %s in None, Auto or Extension should be already handled?!?", FileName.c_str()); } for (; compressor != compressors.end(); ++compressor) if (compressor->Name == name) break; if (compressor == compressors.end()) - return _error->Error("Can't find a configured compressor %s for file %s", name.c_str(), FileName.c_str()); + return FileFdError("Can't find a configured compressor %s for file %s", name.c_str(), FileName.c_str()); } if (compressor == compressors.end()) - return _error->Error("Can't find a match for specified compressor mode for file %s", FileName.c_str()); + return FileFdError("Can't find a match for specified compressor mode for file %s", FileName.c_str()); return Open(FileName, Mode, *compressor, Perms); } bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Compressor const &compressor, unsigned long const Perms) @@ -933,9 +933,9 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co Flags = AutoClose; if ((Mode & WriteOnly) != WriteOnly && (Mode & (Atomic | Create | Empty | Exclusive)) != 0) - return _error->Error("ReadOnly mode for %s doesn't accept additional flags!", FileName.c_str()); + return FileFdError("ReadOnly mode for %s doesn't accept additional flags!", FileName.c_str()); if ((Mode & ReadWrite) == 0) - return _error->Error("No openmode provided in FileFd::Open for %s", FileName.c_str()); + return FileFdError("No openmode provided in FileFd::Open for %s", FileName.c_str()); if ((Mode & Atomic) == Atomic) { @@ -981,7 +981,7 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co close (iFd); iFd = -1; } - return _error->Errno("open",_("Could not open file %s"), FileName.c_str()); + return FileFdErrno("open",_("Could not open file %s"), FileName.c_str()); } SetCloseExec(iFd,true); @@ -1010,13 +1010,13 @@ bool FileFd::OpenDescriptor(int Fd, unsigned int const Mode, CompressMode Compre case Xz: name = "xz"; break; case Auto: case Extension: - return _error->Error("Opening Fd %d in Auto or Extension compression mode is not supported", Fd); + return FileFdError("Opening Fd %d in Auto or Extension compression mode is not supported", Fd); } for (; compressor != compressors.end(); ++compressor) if (compressor->Name == name) break; if (compressor == compressors.end()) - return _error->Error("Can't find a configured compressor %s for file %s", name.c_str(), FileName.c_str()); + return FileFdError("Can't find a configured compressor %s for file %s", name.c_str(), FileName.c_str()); return OpenDescriptor(Fd, Mode, *compressor, AutoClose); } @@ -1043,7 +1043,7 @@ bool FileFd::OpenDescriptor(int Fd, unsigned int const Mode, APT::Configuration: { if (AutoClose) close (iFd); - return _error->Errno("gzdopen",_("Could not open file descriptor %d"), Fd); + return FileFdErrno("gzdopen",_("Could not open file descriptor %d"), Fd); } return true; } @@ -1105,10 +1105,7 @@ bool FileFd::OpenInternDescriptor(unsigned int const Mode, APT::Configuration::C ExecWait(d->compressor_pid, "FileFdCompressor", true); if ((Mode & ReadWrite) == ReadWrite) - { - Flags |= Fail; - return _error->Error("ReadWrite mode is not supported for file %s", FileName.c_str()); - } + return FileFdError("ReadWrite mode is not supported for file %s", FileName.c_str()); bool const Comp = (Mode & WriteOnly) == WriteOnly; if (Comp == false) @@ -1131,10 +1128,7 @@ bool FileFd::OpenInternDescriptor(unsigned int const Mode, APT::Configuration::C // Create a data pipe int Pipe[2] = {-1,-1}; if (pipe(Pipe) != 0) - { - Flags |= Fail; - return _error->Errno("pipe",_("Failed to create subprocess IPC")); - } + return FileFdErrno("pipe",_("Failed to create subprocess IPC")); for (int J = 0; J != 2; J++) SetCloseExec(Pipe[J],true); @@ -1244,14 +1238,13 @@ bool FileFd::Read(void *To,unsigned long long Size,unsigned long long *Actual) { if (errno == EINTR) continue; - Flags |= Fail; #ifdef HAVE_ZLIB if (d != NULL && d->gz != NULL) { int err; char const * const errmsg = gzerror(d->gz, &err); if (err != Z_ERRNO) - return _error->Error("gzread: %s (%d: %s)", _("Read error"), err, errmsg); + return FileFdError("gzread: %s (%d: %s)", _("Read error"), err, errmsg); } #endif #ifdef HAVE_BZ2 @@ -1260,10 +1253,10 @@ bool FileFd::Read(void *To,unsigned long long Size,unsigned long long *Actual) int err; char const * const errmsg = BZ2_bzerror(d->bz2, &err); if (err != BZ_IO_ERROR) - return _error->Error("BZ2_bzread: %s (%d: %s)", _("Read error"), err, errmsg); + return FileFdError("BZ2_bzread: %s (%d: %s)", _("Read error"), err, errmsg); } #endif - return _error->Errno("read",_("Read error")); + return FileFdErrno("read",_("Read error")); } To = (char *)To + Res; @@ -1284,9 +1277,8 @@ bool FileFd::Read(void *To,unsigned long long Size,unsigned long long *Actual) Flags |= HitEof; return true; } - - Flags |= Fail; - return _error->Error(_("read, still have %llu to read but none left"), Size); + + return FileFdError(_("read, still have %llu to read but none left"), Size); } /*}}}*/ // FileFd::ReadLine - Read a complete line from the file /*{{{*/ @@ -1342,14 +1334,13 @@ bool FileFd::Write(const void *From,unsigned long long Size) continue; if (Res < 0) { - Flags |= Fail; #ifdef HAVE_ZLIB if (d != NULL && d->gz != NULL) { int err; char const * const errmsg = gzerror(d->gz, &err); if (err != Z_ERRNO) - return _error->Error("gzwrite: %s (%d: %s)", _("Write error"), err, errmsg); + return FileFdError("gzwrite: %s (%d: %s)", _("Write error"), err, errmsg); } #endif #ifdef HAVE_BZ2 @@ -1358,10 +1349,10 @@ bool FileFd::Write(const void *From,unsigned long long Size) int err; char const * const errmsg = BZ2_bzerror(d->bz2, &err); if (err != BZ_IO_ERROR) - return _error->Error("BZ2_bzwrite: %s (%d: %s)", _("Write error"), err, errmsg); + return FileFdError("BZ2_bzwrite: %s (%d: %s)", _("Write error"), err, errmsg); } #endif - return _error->Errno("write",_("Write error")); + return FileFdErrno("write",_("Write error")); } From = (char *)From + Res; @@ -1373,9 +1364,8 @@ bool FileFd::Write(const void *From,unsigned long long Size) if (Size == 0) return true; - - Flags |= Fail; - return _error->Error(_("write, still have %llu to write but couldn't"), Size); + + return FileFdError(_("write, still have %llu to write but couldn't"), Size); } bool FileFd::Write(int Fd, const void *From, unsigned long long Size) { @@ -1419,10 +1409,7 @@ bool FileFd::Seek(unsigned long long To) return Skip(To - seekpos); if ((d->openmode & ReadOnly) != ReadOnly) - { - Flags |= Fail; - return _error->Error("Reopen is only implemented for read-only files!"); - } + return FileFdError("Reopen is only implemented for read-only files!"); #ifdef HAVE_BZ2 if (d->bz2 != NULL) { @@ -1443,17 +1430,11 @@ bool FileFd::Seek(unsigned long long To) if (lseek(d->compressed_fd, 0, SEEK_SET) != 0) iFd = d->compressed_fd; if (iFd < 0) - { - Flags |= Fail; - return _error->Error("Reopen is not implemented for pipes opened with FileFd::OpenDescriptor()!"); - } + return FileFdError("Reopen is not implemented for pipes opened with FileFd::OpenDescriptor()!"); } if (OpenInternDescriptor(d->openmode, d->compressor) == false) - { - Flags |= Fail; - return _error->Error("Seek on file %s because it couldn't be reopened", FileName.c_str()); - } + return FileFdError("Seek on file %s because it couldn't be reopened", FileName.c_str()); if (To != 0) return Skip(To); @@ -1469,10 +1450,7 @@ bool FileFd::Seek(unsigned long long To) #endif res = lseek(iFd,To,SEEK_SET); if (res != (signed)To) - { - Flags |= Fail; - return _error->Error("Unable to seek to %llu", To); - } + return FileFdError("Unable to seek to %llu", To); if (d != NULL) d->seekpos = To; @@ -1496,10 +1474,7 @@ bool FileFd::Skip(unsigned long long Over) { unsigned long long toread = std::min((unsigned long long) sizeof(buffer), Over); if (Read(buffer, toread) == false) - { - Flags |= Fail; - return _error->Error("Unable to seek ahead %llu",Over); - } + return FileFdError("Unable to seek ahead %llu",Over); Over -= toread; } return true; @@ -1513,10 +1488,7 @@ bool FileFd::Skip(unsigned long long Over) #endif res = lseek(iFd,Over,SEEK_CUR); if (res < 0) - { - Flags |= Fail; - return _error->Error("Unable to seek ahead %llu",Over); - } + return FileFdError("Unable to seek ahead %llu",Over); if (d != NULL) d->seekpos = res; @@ -1530,17 +1502,11 @@ bool FileFd::Truncate(unsigned long long To) { #if defined HAVE_ZLIB || defined HAVE_BZ2 if (d != NULL && (d->gz != NULL || d->bz2 != NULL)) - { - Flags |= Fail; - return _error->Error("Truncating compressed files is not implemented (%s)", FileName.c_str()); - } + return FileFdError("Truncating compressed files is not implemented (%s)", FileName.c_str()); #endif if (ftruncate(iFd,To) != 0) - { - Flags |= Fail; - return _error->Error("Unable to truncate to %llu",To); - } - + return FileFdError("Unable to truncate to %llu",To); + return true; } /*}}}*/ @@ -1568,10 +1534,7 @@ unsigned long long FileFd::Tell() #endif Res = lseek(iFd,0,SEEK_CUR); if (Res == (off_t)-1) - { - Flags |= Fail; - _error->Errno("lseek","Failed to determine the current file position"); - } + FileFdErrno("lseek","Failed to determine the current file position"); if (d != NULL) d->seekpos = Res; return Res; @@ -1584,10 +1547,7 @@ unsigned long long FileFd::FileSize() { struct stat Buf; if ((d == NULL || d->pipe == false) && fstat(iFd,&Buf) != 0) - { - Flags |= Fail; - return _error->Errno("fstat","Unable to determine the file size"); - } + return FileFdErrno("fstat","Unable to determine the file size"); // for compressor pipes st_size is undefined and at 'best' zero if ((d != NULL && d->pipe == true) || S_ISFIFO(Buf.st_mode)) @@ -1597,10 +1557,7 @@ unsigned long long FileFd::FileSize() if (d != NULL) d->pipe = true; if (stat(FileName.c_str(), &Buf) != 0) - { - Flags |= Fail; - return _error->Errno("stat","Unable to determine the file size"); - } + return FileFdErrno("stat","Unable to determine the file size"); } return Buf.st_size; @@ -1642,16 +1599,10 @@ unsigned long long FileFd::Size() * bits of the file */ // FIXME: Size for gz-files is limited by 32bit… no largefile support if (lseek(iFd, -4, SEEK_END) < 0) - { - Flags |= Fail; - return _error->Errno("lseek","Unable to seek to end of gzipped file"); - } + return FileFdErrno("lseek","Unable to seek to end of gzipped file"); size = 0L; if (read(iFd, &size, 4) != 4) - { - Flags |= Fail; - return _error->Errno("read","Unable to read original size of gzipped file"); - } + return FileFdErrno("read","Unable to read original size of gzipped file"); #ifdef WORDS_BIGENDIAN uint32_t tmp_size = size; @@ -1661,10 +1612,7 @@ unsigned long long FileFd::Size() #endif if (lseek(iFd, oldPos, SEEK_SET) < 0) - { - Flags |= Fail; - return _error->Errno("lseek","Unable to seek in gzipped file"); - } + return FileFdErrno("lseek","Unable to seek in gzipped file"); return size; } @@ -1681,8 +1629,7 @@ time_t FileFd::ModificationTime() struct stat Buf; if ((d == NULL || d->pipe == false) && fstat(iFd,&Buf) != 0) { - Flags |= Fail; - _error->Errno("fstat","Unable to determine the modification time of file %s", FileName.c_str()); + FileFdErrno("fstat","Unable to determine the modification time of file %s", FileName.c_str()); return 0; } @@ -1695,8 +1642,7 @@ time_t FileFd::ModificationTime() d->pipe = true; if (stat(FileName.c_str(), &Buf) != 0) { - Flags |= Fail; - _error->Errno("fstat","Unable to determine the modification time of file %s", FileName.c_str()); + FileFdErrno("fstat","Unable to determine the modification time of file %s", FileName.c_str()); return 0; } } @@ -1752,11 +1698,40 @@ bool FileFd::Close() bool FileFd::Sync() { if (fsync(iFd) != 0) + return FileFdErrno("sync",_("Problem syncing the file")); + return true; +} + /*}}}*/ +// FileFd::FileFdErrno - set Fail and call _error->Errno *{{{*/ +bool FileFd::FileFdErrno(const char *Function, const char *Description,...) +{ + Flags |= Fail; + va_list args; + size_t msgSize = 400; + int const errsv = errno; + while (true) { - Flags |= Fail; - return _error->Errno("sync",_("Problem syncing the file")); + va_start(args,Description); + if (_error->InsertErrno(GlobalError::ERROR, Function, Description, args, errsv, msgSize) == false) + break; + va_end(args); } - return true; + return false; +} + /*}}}*/ +// FileFd::FileFdError - set Fail and call _error->Error *{{{*/ +bool FileFd::FileFdError(const char *Description,...) { + Flags |= Fail; + va_list args; + size_t msgSize = 400; + while (true) + { + va_start(args,Description); + if (_error->Insert(GlobalError::ERROR, Description, args, msgSize) == false) + break; + va_end(args); + } + return false; } /*}}}*/ diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h index 426664d3a..3ec01dd9a 100644 --- a/apt-pkg/contrib/fileutl.h +++ b/apt-pkg/contrib/fileutl.h @@ -149,6 +149,10 @@ class FileFd private: FileFdPrivate* d; bool OpenInternDescriptor(unsigned int const Mode, APT::Configuration::Compressor const &compressor); + + // private helpers to set Fail flag and call _error->Error + bool FileFdErrno(const char* Function, const char* Description,...) __like_printf(3) __cold; + bool FileFdError(const char* Description,...) __like_printf(2) __cold; }; bool RunScripts(const char *Cnf); diff --git a/debian/changelog b/debian/changelog index 9f35441f9..056025509 100644 --- a/debian/changelog +++ b/debian/changelog @@ -10,6 +10,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * try all providers in order if uninstallable in MarkInstall * do unpacks before configures in SmartConfigure (Closes: #707578) * fix support for multiple patterns in apt-cache search (Closes: #691453) + * set Fail flag in FileFd on all errors consistently -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From f97bb5237489134cb971ce38b93c5d6220341ea8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 25 May 2013 20:33:15 +0200 Subject: OpenDescriptor should autoclose fd always on error OpenInternDescriptor failures would cause additional errors to be generated by double-closing an fd. Other errors (although these are generated if the method is used incorrectly, so unlikely) didn't close the fd aswell. Closes: 704608 --- apt-pkg/contrib/fileutl.cc | 23 +++++++++++++++++++---- debian/changelog | 1 + 2 files changed, 20 insertions(+), 4 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index 4a7299e99..7c3a302e2 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -1010,14 +1010,19 @@ bool FileFd::OpenDescriptor(int Fd, unsigned int const Mode, CompressMode Compre case Xz: name = "xz"; break; case Auto: case Extension: + if (AutoClose == true && Fd != -1) + close(Fd); return FileFdError("Opening Fd %d in Auto or Extension compression mode is not supported", Fd); } for (; compressor != compressors.end(); ++compressor) if (compressor->Name == name) break; if (compressor == compressors.end()) + { + if (AutoClose == true && Fd != -1) + close(Fd); return FileFdError("Can't find a configured compressor %s for file %s", name.c_str(), FileName.c_str()); - + } return OpenDescriptor(Fd, Mode, *compressor, AutoClose); } bool FileFd::OpenDescriptor(int Fd, unsigned int const Mode, APT::Configuration::Compressor const &compressor, bool AutoClose) @@ -1039,11 +1044,21 @@ bool FileFd::OpenDescriptor(int Fd, unsigned int const Mode, APT::Configuration: else iFd = Fd; this->FileName = ""; - if (OpenInternDescriptor(Mode, compressor) == false) + if (Fd == -1 || OpenInternDescriptor(Mode, compressor) == false) { - if (AutoClose) + if (iFd != -1 && ( +#ifdef HAVE_ZLIB + compressor.Name == "gzip" || +#endif +#ifdef HAVE_BZ2 + compressor.Name == "bzip2" || +#endif + AutoClose == true)) + { close (iFd); - return FileFdErrno("gzdopen",_("Could not open file descriptor %d"), Fd); + iFd = -1; + } + return FileFdError(_("Could not open file descriptor %d"), Fd); } return true; } diff --git a/debian/changelog b/debian/changelog index a80bac3d7..42a8e540e 100644 --- a/debian/changelog +++ b/debian/changelog @@ -12,6 +12,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * fix support for multiple patterns in apt-cache search (Closes: #691453) * set Fail flag in FileFd on all errors consistently * don't explicitly init ExtractTar InFd with invalid fd + * OpenDescriptor should autoclose fd always on error (Closes: #704608) -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From 2128d3fce44da34ecb1f0d784b703807f66b20f9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 9 Jun 2013 18:33:48 +0200 Subject: fail in CopyFile if the FileFds have error flag set Testing for global PendingErrors in users of CopyFile is incorrect in so far as unrelated errors will prevent us from copying perfectly fine files and checking for the validity of the files is just better in CopyFiles as it already checks if files are at least opened. Add also a higher-level error message to the error stack if it fails. --- apt-pkg/contrib/fileutl.cc | 3 ++- apt-pkg/indexcopy.cc | 6 ++---- debian/changelog | 1 + 3 files changed, 5 insertions(+), 5 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index 7c3a302e2..0f88923cf 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -184,7 +184,8 @@ bool RunScripts(const char *Cnf) /* The caller is expected to set things so that failure causes erasure */ bool CopyFile(FileFd &From,FileFd &To) { - if (From.IsOpen() == false || To.IsOpen() == false) + if (From.IsOpen() == false || To.IsOpen() == false || + From.Failed() == true || To.Failed() == true) return false; // Buffered copy between fds diff --git a/apt-pkg/indexcopy.cc b/apt-pkg/indexcopy.cc index 0e36b3ded..1d61b974d 100644 --- a/apt-pkg/indexcopy.cc +++ b/apt-pkg/indexcopy.cc @@ -544,11 +544,9 @@ bool SigVerify::CopyMetaIndex(string CDROM, string CDName, /*{{{*/ FileFd Rel; Target.Open(TargetF,FileFd::WriteAtomic); Rel.Open(prefix + file,FileFd::ReadOnly); - if (_error->PendingError() == true) - return false; if (CopyFile(Rel,Target) == false) - return false; - + return _error->Error("Copying of '%s' for '%s' from '%s' failed", file.c_str(), CDName.c_str(), prefix.c_str()); + return true; } /*}}}*/ diff --git a/debian/changelog b/debian/changelog index 42a8e540e..cde0aba2f 100644 --- a/debian/changelog +++ b/debian/changelog @@ -13,6 +13,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * set Fail flag in FileFd on all errors consistently * don't explicitly init ExtractTar InFd with invalid fd * OpenDescriptor should autoclose fd always on error (Closes: #704608) + * fail in CopyFile if the FileFds have error flag set -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From b2ea1a47531266377abe4f12c6f21417ea96eea0 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 9 Jun 2013 18:58:34 +0200 Subject: ensure state-dir exists before coyping cdrom files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do the same in the acquire system which handles the 'normal' downloads, so do it here as well even though its unlikely anyone will ever notice (beside testcases of course …) --- apt-pkg/cdrom.cc | 8 ++++++++ debian/changelog | 1 + test/integration/framework | 8 +++++--- test/integration/test-apt-cdrom | 2 +- 4 files changed, 15 insertions(+), 4 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/cdrom.cc b/apt-pkg/cdrom.cc index 9a9a854bf..a5668a50a 100644 --- a/apt-pkg/cdrom.cc +++ b/apt-pkg/cdrom.cc @@ -829,6 +829,14 @@ bool pkgCdrom::Add(pkgCdromStatus *log) /*{{{*/ log->Update(msg.str()); log->Update(_("Copying package lists..."), STEP_COPY); } + + // check for existence and possibly create state directory for copying + string const listDir = _config->FindDir("Dir::State::lists"); + string const partialListDir = listDir + "partial/"; + if (CreateAPTDirectoryIfNeeded(_config->FindDir("Dir::State"), partialListDir) == false && + CreateAPTDirectoryIfNeeded(listDir, partialListDir) == false) + return _error->Errno("cdrom", _("List directory %spartial is missing."), listDir.c_str()); + // take care of the signatures and copy them if they are ok // (we do this before PackageCopy as it modifies "List" and "SourceList") SigVerify SignVerify; diff --git a/debian/changelog b/debian/changelog index cde0aba2f..7582b4f35 100644 --- a/debian/changelog +++ b/debian/changelog @@ -14,6 +14,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * don't explicitly init ExtractTar InFd with invalid fd * OpenDescriptor should autoclose fd always on error (Closes: #704608) * fail in CopyFile if the FileFds have error flag set + * ensure state-dir exists before coyping cdrom files -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/test/integration/framework b/test/integration/framework index 7c2aed592..5c50498a2 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -692,9 +692,11 @@ setupaptarchive() { setupflataptarchive fi signreleasefiles - msgninfo "\tSync APT's cache with the archive… " - aptget update -qq - msgdone "info" + if [ "$1" != '--no-update' ]; then + msgninfo "\tSync APT's cache with the archive… " + aptget update -qq + msgdone "info" + fi } signreleasefiles() { diff --git a/test/integration/test-apt-cdrom b/test/integration/test-apt-cdrom index 3394aa505..6e3533152 100755 --- a/test/integration/test-apt-cdrom +++ b/test/integration/test-apt-cdrom @@ -7,7 +7,7 @@ setupenvironment configarchitecture 'amd64' 'i386' buildsimplenativepackage 'testing' 'amd64,i386' '0.8.15' 'stable' -setupaptarchive +setupaptarchive --no-update changetocdrom 'Debian APT Testdisk 0.8.15' -- cgit v1.2.3-70-g09d2 From 99359751efb1ad84e877219639030feb47fb28f7 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 16 Jun 2013 15:42:31 +0200 Subject: handle missing "Description" in apt-cache show do not blindly assume that all packages stanzas have a "Description:" field in 'apt-cache show' as well as in the cache creation itself. We instead assume now that if the stanza has a Description, it will not be the first field as we look out for "\nDescription" to take care of MD5sum as well as (maybe ignored) translated Descriptions embedded in the package stanza. Closes: #712435 --- apt-pkg/deb/deblistparser.cc | 6 +- apt-pkg/pkgcachegen.cc | 4 +- cmdline/apt-cache.cc | 47 ++++++++---- debian/changelog | 1 + .../test-bug-712435-missing-descriptions | 89 ++++++++++++++++++++++ 5 files changed, 130 insertions(+), 17 deletions(-) create mode 100755 test/integration/test-bug-712435-missing-descriptions (limited to 'apt-pkg') diff --git a/apt-pkg/deb/deblistparser.cc b/apt-pkg/deb/deblistparser.cc index db86bd698..28857176b 100644 --- a/apt-pkg/deb/deblistparser.cc +++ b/apt-pkg/deb/deblistparser.cc @@ -219,8 +219,12 @@ MD5SumValue debListParser::Description_md5() string const value = Section.FindS("Description-md5"); if (value.empty() == true) { + std::string const desc = Description() + "\n"; + if (desc == "\n") + return MD5SumValue(); + MD5Summation md5; - md5.Add((Description() + "\n").c_str()); + md5.Add(desc.c_str()); return md5.Result(); } else if (likely(value.size() == 32)) diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 3f10b6c40..7ce7aba7b 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -296,6 +296,8 @@ bool pkgCacheGenerator::MergeListPackage(ListParser &List, pkgCache::PkgIterator // Find the right version to write the description MD5SumValue CurMd5 = List.Description_md5(); + if (CurMd5.Value().empty() == true || List.Description().empty() == true) + return true; std::string CurLang = List.DescriptionLanguage(); for (Ver = Pkg.VersionList(); Ver.end() == false; ++Ver) @@ -480,7 +482,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator /* Record the Description (it is not translated) */ MD5SumValue CurMd5 = List.Description_md5(); - if (CurMd5.Value().empty() == true) + if (CurMd5.Value().empty() == true || List.Description().empty() == true) return true; std::string CurLang = List.DescriptionLanguage(); diff --git a/cmdline/apt-cache.cc b/cmdline/apt-cache.cc index bda09a5a1..fb4467c2c 100644 --- a/cmdline/apt-cache.cc +++ b/cmdline/apt-cache.cc @@ -1161,7 +1161,11 @@ bool DisplayRecord(pkgCacheFile &CacheFile, pkgCache::VerIterator V) } // Get a pointer to start of Description field - const unsigned char *DescP = (unsigned char*)strstr((char*)Buffer, "Description:"); + const unsigned char *DescP = (unsigned char*)strstr((char*)Buffer, "\nDescription"); + if (DescP != NULL) + ++DescP; + else + DescP = Buffer + V.FileList()->Size; // Write all but Description if (fwrite(Buffer,1,DescP - Buffer,stdout) < (size_t)(DescP - Buffer)) @@ -1173,25 +1177,38 @@ bool DisplayRecord(pkgCacheFile &CacheFile, pkgCache::VerIterator V) // Show the right description pkgRecords Recs(*Cache); pkgCache::DescIterator Desc = V.TranslatedDescription(); - pkgRecords::Parser &P = Recs.Lookup(Desc.FileList()); - cout << "Description" << ( (strcmp(Desc.LanguageCode(),"") != 0) ? "-" : "" ) << Desc.LanguageCode() << ": " << P.LongDesc(); - - // Find the first field after the description (if there is any) - for(DescP++;DescP != &Buffer[V.FileList()->Size];DescP++) + if (Desc.end() == false) { - if(*DescP == '\n' && *(DescP+1) != ' ') + pkgRecords::Parser &P = Recs.Lookup(Desc.FileList()); + cout << "Description" << ( (strcmp(Desc.LanguageCode(),"") != 0) ? "-" : "" ) << Desc.LanguageCode() << ": " << P.LongDesc(); + cout << std::endl << "Description-md5: " << Desc.md5() << std::endl; + + // Find the first field after the description (if there is any) + while ((DescP = (unsigned char*)strchr((char*)DescP, '\n')) != NULL) { - // write the rest of the buffer - const unsigned char *end=&Buffer[V.FileList()->Size]; - if (fwrite(DescP,1,end-DescP,stdout) < (size_t)(end-DescP)) - { - delete [] Buffer; - return false; - } + if (DescP[1] == ' ') + DescP += 2; + else if (strncmp((char*)DescP, "\nDescription", strlen("\nDescription")) == 0) + DescP += strlen("\nDescription"); + else + break; + } + if (DescP != NULL) + ++DescP; + } + // if we have no translation, we found a lonely Description-md5, so don't skip it - break; + if (DescP != NULL) + { + // write the rest of the buffer + const unsigned char *end=&Buffer[V.FileList()->Size]; + if (fwrite(DescP,1,end-DescP,stdout) < (size_t)(end-DescP)) + { + delete [] Buffer; + return false; } } + // write a final newline (after the description) cout< Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/test/integration/test-bug-712435-missing-descriptions b/test/integration/test-bug-712435-missing-descriptions new file mode 100755 index 000000000..9b3c2ee50 --- /dev/null +++ b/test/integration/test-bug-712435-missing-descriptions @@ -0,0 +1,89 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework +setupenvironment +configarchitecture 'amd64' + +PACKAGESTANZA='Version: 0.9.7.8 +Installed-Size: 3270 +Maintainer: APT Development Team +Architecture: amd64 +Filename: pool/main/a/apt/apt_0.9.7.8_amd64.deb +MD5sum: 3a622acda41620df50aa22a9fac6f32e' + +DESCRIPTION='Description: commandline package manager + This APT has Super Cow Powers.' + +TRANSDESCRIPTION='Description-en: commandline package manager + This APT has translated Super Cow Powers.' + +echo "Package: apt-normal +$PACKAGESTANZA +$DESCRIPTION +Description-md5: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + +Package: apt-both-below +$PACKAGESTANZA +$DESCRIPTION +$TRANSDESCRIPTION +Description-md5: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + +Package: apt-both-middle +$PACKAGESTANZA +$DESCRIPTION +Description-md5: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +$TRANSDESCRIPTION + +Package: apt-both-top +$PACKAGESTANZA +Description-md5: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +$DESCRIPTION +$TRANSDESCRIPTION + +Package: apt-trans +$PACKAGESTANZA +$TRANSDESCRIPTION +Description-md5: cccccccccccccccccccccccccccccccc + +Package: apt-md5 +$PACKAGESTANZA +Description-md5: dddddddddddddddddddddddddddddddd + +Package: apt-none +$PACKAGESTANZA" > aptarchive/Packages + +setupaptarchive + +testequal "Package: apt-normal +$PACKAGESTANZA +$DESCRIPTION +Description-md5: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +" aptcache show apt-normal + +# displaying the translated Description would be equally valid, +# but we assume only one description is in a Packages file and +# so we prefer "Description" over "Description-*" currently. +for variant in 'below' 'middle' 'top'; do + testequal "Package: apt-both-$variant +$PACKAGESTANZA +$DESCRIPTION +Description-md5: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +" aptcache show apt-both-$variant +done + +testequal "Package: apt-trans +$PACKAGESTANZA +$TRANSDESCRIPTION +Description-md5: cccccccccccccccccccccccccccccccc +" aptcache show apt-trans + +testequal "Package: apt-md5 +$PACKAGESTANZA +Description-md5: dddddddddddddddddddddddddddddddd +" aptcache show apt-md5 + +testequal "Package: apt-none +$PACKAGESTANZA +" aptcache show apt-none -- cgit v1.2.3-70-g09d2 From 709038652854b71895e06caed2b028b389acefd6 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 15 Jun 2013 10:35:04 +0200 Subject: support \n and \r\n line endings in ReadMessages --- apt-pkg/contrib/strutl.cc | 5 +++-- debian/changelog | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index f4dd3407d..d0e74d8c5 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -758,7 +758,8 @@ bool ReadMessages(int Fd, vector &List) // Look for the end of the message for (char *I = Buffer; I + 1 < End; I++) { - if (I[0] != '\n' || I[1] != '\n') + if (I[1] != '\n' || + (I[0] != '\n' && strncmp(I, "\r\n\r\n", 4) != 0)) continue; // Pull the message out @@ -766,7 +767,7 @@ bool ReadMessages(int Fd, vector &List) PartialMessage += Message; // Fix up the buffer - for (; I < End && *I == '\n'; I++); + for (; I < End && (*I == '\n' || *I == '\r'); ++I); End -= I-Buffer; memmove(Buffer,I,End-Buffer); I = Buffer; diff --git a/debian/changelog b/debian/changelog index 8fef47d14..dfff872a7 100644 --- a/debian/changelog +++ b/debian/changelog @@ -18,6 +18,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * fix file location for configure-index.gz in apt.conf(5) (Closes: #711921) * handle missing "Description" in apt-cache show (Closes: #712435) * try defaults if auto-detection failed in apt-cdrom (Closes: #712433) + * support \n and \r\n line endings in ReadMessages -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From 0aec7d5c2f6fd55301bd1e1ed35ab7a23fd2357e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 15 Jun 2013 23:47:07 +0200 Subject: do not redownload unchanged InRelease files Before we download the 'new' InRelease file the old file will be moved out of the way with the name 'foobar_InRelease.reverify', so if no partial file for the 'new' file exists take the modification time from this reverify file, so that if we get an IMS hit for the InRelease file we can move back the reverify file as new file rather than downloading the 'new' file even though we already have it. We do the same for Release files and this happened to work until the reverify renaming was corrected for InRelease files. --- apt-pkg/acquire-item.cc | 13 ++++++++++++- debian/changelog | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 7b800e42f..c48443eff 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1289,7 +1289,14 @@ void pkgAcqMetaIndex::RetrievalDone(string Message) /*{{{*/ string FinalFile = _config->FindDir("Dir::State::lists"); FinalFile += URItoFileName(RealURI); if (SigFile == DestFile) + { SigFile = FinalFile; + // constructor of pkgAcqMetaClearSig moved it out of the way, + // now move it back in on IMS hit for the 'old' file + string const OldClearSig = DestFile + ".reverify"; + if (RealFileExists(OldClearSig) == true) + Rename(OldClearSig, FinalFile); + } DestFile = FinalFile; } Complete = true; @@ -1595,7 +1602,11 @@ string pkgAcqMetaClearSig::Custom600Headers() struct stat Buf; if (stat(Final.c_str(),&Buf) != 0) - return "\nIndex-File: true\nFail-Ignore: true\n"; + { + Final = DestFile + ".reverify"; + if (stat(Final.c_str(),&Buf) != 0) + return "\nIndex-File: true\nFail-Ignore: true\n"; + } return "\nIndex-File: true\nFail-Ignore: true\nLast-Modified: " + TimeRFC1123(Buf.st_mtime); } diff --git a/debian/changelog b/debian/changelog index dfff872a7..91d4ae536 100644 --- a/debian/changelog +++ b/debian/changelog @@ -19,6 +19,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * handle missing "Description" in apt-cache show (Closes: #712435) * try defaults if auto-detection failed in apt-cdrom (Closes: #712433) * support \n and \r\n line endings in ReadMessages + * do not redownload unchanged InRelease files -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 -- cgit v1.2.3-70-g09d2 From ae99ce2e3cadb07c80b89ab2afc804875b1026c5 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 17 Jun 2013 11:23:13 +0200 Subject: trigger NODATA error for invalid InRelease files With the selfgrown splitting we got the problem of not recovering from networks which just reply with invalid data like those sending us login pages to authenticate with the network (e.g. hotels) back. The good thing about the InRelease file is that we know that it must be clearsigned (a Release file might or might not have a detached sig) so if we get a file but are unable to split it something is seriously wrong, so there is not much point in trying further. The Acquire system already looks out for a NODATA error from gpgv, so this adds a new error message sent to the acquire system in case the splitting we do now ourselves failed including this magic word. Closes: #712486 --- apt-pkg/contrib/gpgv.cc | 2 +- apt-pkg/contrib/gpgv.h | 15 ++++- debian/changelog | 1 + methods/gpgv.cc | 16 +++--- test/integration/framework | 3 + .../test-ubuntu-bug-346386-apt-get-update-paywall | 64 ++++++++++++++++++++++ test/interactive-helper/aptwebserver.cc | 26 +++++++++ 7 files changed, 114 insertions(+), 13 deletions(-) create mode 100755 test/integration/test-ubuntu-bug-346386-apt-get-update-paywall (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 31db7d5fe..f47e7ea48 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -154,7 +154,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (sigFd != -1) unlink(data); ioprintf(std::cerr, "Splitting up %s into data and signature failed", File.c_str()); - exit(EINTERNAL); + exit(112); } Args.push_back(sig); Args.push_back(data); diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index 08b10a97a..45f069058 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -23,9 +23,18 @@ /** \brief generates and run the command to verify a file with gpgv * * If File and FileSig specify the same file it is assumed that we - * deal with a clear-signed message. In that case the file will be - * rewritten to be in a good-known format without uneeded whitespaces - * and additional messages (unsigned or signed). + * deal with a clear-signed message. Note that the method will accept + * and validate files which include additional (unsigned) messages + * without complaining. Do NOT open files accepted by this method + * for reading. Use #OpenMaybeClearSignedFile to access the message + * instead to ensure you are only reading signed data. + * + * The method does not return, but has some noteable exit-codes: + * 111 signals an internal error like the inability to execute gpgv, + * 112 indicates a clear-signed file which doesn't include a message, + * which can happen if APT is run while on a network requiring + * authentication before usage (e.g. in hotels) + * All other exit-codes are passed-through from gpgv. * * @param File is the message (unsigned or clear-signed) * @param FileSig is the signature (detached or clear-signed) diff --git a/debian/changelog b/debian/changelog index 91d4ae536..bd25df1e2 100644 --- a/debian/changelog +++ b/debian/changelog @@ -20,6 +20,7 @@ apt (0.9.8.3) UNRELEASED; urgency=low * try defaults if auto-detection failed in apt-cdrom (Closes: #712433) * support \n and \r\n line endings in ReadMessages * do not redownload unchanged InRelease files + * trigger NODATA error for invalid InRelease files (Closes: #712486) -- David Kalnischkies Sun, 09 Jun 2013 15:06:24 +0200 diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 3f814b9f0..fe8bac6c9 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -55,9 +55,6 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, vector &NoPubKeySigners) { bool const Debug = _config->FindB("Debug::Acquire::gpgv", false); - // setup a (empty) stringstream for formating the return value - std::stringstream ret; - ret.str(""); if (Debug == true) std::clog << "inside VerifyGetSigners" << std::endl; @@ -170,18 +167,19 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, return ""; } else if (WEXITSTATUS(status) == 1) - { return _("At least one invalid signature was encountered."); - } else if (WEXITSTATUS(status) == 111) + return _("Could not execute 'gpgv' to verify signature (is gpgv installed?)"); + else if (WEXITSTATUS(status) == 112) { - ioprintf(ret, _("Could not execute 'gpgv' to verify signature (is gpgv installed?)")); - return ret.str(); + // acquire system checks for "NODATA" to generate GPG errors (the others are only warnings) + std::string errmsg; + //TRANSLATORS: %s is a single techy word like 'NODATA' + strprintf(errmsg, _("Clearsigned file isn't valid, got '%s' (does the network require authentication?)"), "NODATA"); + return errmsg; } else - { return _("Unknown error executing gpgv"); - } } bool GPGVMethod::Fetch(FetchItem *Itm) diff --git a/test/integration/framework b/test/integration/framework index 3f11ac23b..3a02cfb76 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -118,6 +118,9 @@ gdb() { echo "gdb: run »$*«" APT_CONFIG=aptconfig.conf LD_LIBRARY_PATH=${BUILDDIRECTORY} $(which gdb) ${BUILDDIRECTORY}/$1 } +http() { + LD_LIBRARY_PATH=${BUILDDIRECTORY} ${BUILDDIRECTORY}/methods/http +} exitwithstatus() { # error if we about to overflow, but ... diff --git a/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall new file mode 100755 index 000000000..1576c396c --- /dev/null +++ b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall @@ -0,0 +1,64 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture 'native' + +insertpackage 'unstable' 'unrelated' 'all' '1.0' 'stable' +insertsource 'unstable' 'unrelated' 'all' '1.0' 'stable' + +echo 'ni ni ni' > aptarchive/knights + +setupaptarchive +changetowebserver -o 'aptwebserver::overwrite::.*::filename=/knights' + +msgtest 'Acquire test file from the webserver to check' 'overwrite' +echo '601 Configuration +Config-Item: Acquire::http::DependOnSTDIN=0 + +600 Acquire URI +URI: http://localhost:8080/holygrail +Filename: knights-talking +' | http >/dev/null 2>&1 && msgpass || msgfail +testfileequal knights-talking 'ni ni ni' + +ensure_n_canary_strings_in_dir() { + local DIR="$1" + local CANARY_STRING="$2" + local EXPECTED_N="$3" + + msgtest "Testing in $DIR for $EXPECTED_N canary" "$CANARY_STRING" + local N=$(grep "$CANARY_STRING" $DIR/* 2>/dev/null |wc -l ) + test "$N" = "$EXPECTED_N" && msgpass || msgfail "Expected $EXPECTED_N canaries, got $N" +} + +LISTS='rootdir/var/lib/apt/lists' +rm -rf rootdir/var/lib/apt/lists +msgtest 'Got expected NODATA failure in' 'apt-get update' +aptget update -qq 2>&1 | grep -q 'E: GPG error.*NODATA' && msgpass || msgfail + +ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0 +testequal 'partial' ls $LISTS + +# and again with pre-existing files with "valid data" which should remain +for f in Release Release.gpg main_binary-amd64_Packages main_source_Sources; do + echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_${f} +done + +msgtest 'Got expected NODATA failure in' 'apt-get update' +aptget update -qq 2>&1 | grep -q 'E: GPG error.*NODATA' && msgpass || msgfail + +ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 4 +ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0 + +# and now with a pre-existing InRelease file +echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_InRelease +rm -f $LISTS/localhost:8080_dists_stable_Release $LISTS/localhost:8080_dists_stable_Release.gpg +msgtest 'excpected failure of' 'apt-get update' +aptget update -qq 2>&1 | grep -q 'E: GPG error.*NODATA' && msgpass || msgfail + +ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 3 +ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0 diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index de235fa05..05b875673 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -435,6 +435,32 @@ int main(int const argc, const char * argv[]) } } + ::Configuration::Item const *Overwrite = _config->Tree("aptwebserver::overwrite"); + if (Overwrite != NULL) + { + for (::Configuration::Item *I = Overwrite->Child; I != NULL; I = I->Next) + { + regex_t *pattern = new regex_t; + int const res = regcomp(pattern, I->Tag.c_str(), REG_EXTENDED | REG_ICASE | REG_NOSUB); + if (res != 0) + { + char error[300]; + regerror(res, pattern, error, sizeof(error)); + sendError(client, 500, *m, sendContent, error); + continue; + } + if (regexec(pattern, filename.c_str(), 0, 0, 0) == 0) + { + filename = _config->Find("aptwebserver::overwrite::" + I->Tag + "::filename", filename); + if (filename[0] == '/') + filename.erase(0,1); + regfree(pattern); + break; + } + regfree(pattern); + } + } + // deal with the request if (RealFileExists(filename) == true) { -- cgit v1.2.3-70-g09d2 From 6420c00e30cd9512e045a5370bf391321691ca78 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 24 Jun 2013 12:29:48 +0200 Subject: do not modify DepIterator as we might check again fixup for 42d51f333e8ef522fed02cdfc48663488d56c3a3 The for-loop iterating over the DepIterators which need configuration can (and will be in 'complicated' situations) run multiple times, so we can't just GlobOr on the DepIterator as it modifies it, so that the next iteration over the list ends up checking another dependency leading us into a 'Internal error, packages left unconfigured. foopkg' maybe or we are 'lucky' and calculate a solution which might break down the line Git-Dch: Ignore --- apt-pkg/packagemanager.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/packagemanager.cc b/apt-pkg/packagemanager.cc index b8932753d..310934c42 100644 --- a/apt-pkg/packagemanager.cc +++ b/apt-pkg/packagemanager.cc @@ -420,11 +420,14 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) do { Changed = false; - for (std::list::iterator D = needConfigure.begin(); D != needConfigure.end(); ++D) + for (std::list::const_iterator D = needConfigure.begin(); D != needConfigure.end(); ++D) { - // Compute a single dependency element (glob or) + // Compute a single dependency element (glob or) without modifying D pkgCache::DepIterator Start, End; - D->GlobOr(Start,End); + { + pkgCache::DepIterator Discard = *D; + Discard.GlobOr(Start,End); + } if (End->Type != pkgCache::Dep::Depends) continue; @@ -483,9 +486,8 @@ bool pkgPackageManager::SmartConfigure(PkgIterator Pkg, int const Depth) } - if (Bad == true && Changed == false && Debug == true) - std::clog << OutputInDepth(Depth) << "Could not satisfy " << Start << std::endl; + std::clog << OutputInDepth(Depth) << "Could not satisfy " << *D << std::endl; } if (i++ > max_loops) return _error->Error("Internal error: MaxLoopCount reached in SmartUnPack (2) for %s, aborting", Pkg.FullName().c_str()); -- cgit v1.2.3-70-g09d2 From f58a9890e345faa04b5fcb2a01cae39f986a42db Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 24 Jun 2013 12:51:29 +0200 Subject: fix SHA2* cleanups to zero-out the complete context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GCC 4.8 is now clever enough to warn about: contrib/sha2_internal.cc: In function ‘char* SHA256_End(SHA256_CTX*, char*)’: contrib/sha2_internal.cc:656:31: warning: argument to ‘sizeof’ in ‘void* memset(void*, int, size_t)’ call is the same expression as the destination; did you mean to dereference it? [-Wsizeof-pointer-memaccess] MEMSET_BZERO(context, sizeof(context)); So fix it as suggested. Its interesting though that the SHA2* calculation as far as we need it works even without zeroing out. Git-Dch: Ignore --- apt-pkg/contrib/sha2_internal.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/sha2_internal.cc b/apt-pkg/contrib/sha2_internal.cc index 83b5a98d3..f84fb761c 100644 --- a/apt-pkg/contrib/sha2_internal.cc +++ b/apt-pkg/contrib/sha2_internal.cc @@ -632,7 +632,7 @@ void SHA256_Final(sha2_byte digest[], SHA256_CTX* context) { } /* Clean up state data: */ - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); usedspace = 0; } @@ -653,7 +653,7 @@ char *SHA256_End(SHA256_CTX* context, char buffer[]) { } *buffer = (char)0; } else { - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); } MEMSET_BZERO(digest, SHA256_DIGEST_LENGTH); return buffer; @@ -969,7 +969,7 @@ void SHA512_Final(sha2_byte digest[], SHA512_CTX* context) { } /* Zero out state data */ - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); } char *SHA512_End(SHA512_CTX* context, char buffer[]) { @@ -989,7 +989,7 @@ char *SHA512_End(SHA512_CTX* context, char buffer[]) { } *buffer = (char)0; } else { - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); } MEMSET_BZERO(digest, SHA512_DIGEST_LENGTH); return buffer; @@ -1044,7 +1044,7 @@ void SHA384_Final(sha2_byte digest[], SHA384_CTX* context) { } /* Zero out state data */ - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); } char *SHA384_End(SHA384_CTX* context, char buffer[]) { @@ -1064,7 +1064,7 @@ char *SHA384_End(SHA384_CTX* context, char buffer[]) { } *buffer = (char)0; } else { - MEMSET_BZERO(context, sizeof(context)); + MEMSET_BZERO(context, sizeof(*context)); } MEMSET_BZERO(digest, SHA384_DIGEST_LENGTH); return buffer; -- cgit v1.2.3-70-g09d2