Browse Source

do fail on weakhash/loop earlier in acquire

The bugreport shows a segfault caused by the code not doing the correct
magical dance to remove an item from inside a queue in all cases. We
could try hard to fix this, but it is actually better and also easier to
perform these checks (which cause instant failure) earlier so that they
haven't entered queue(s) yet, which in return makes cleanup trivial.

The result is that we actually end up failing "too early" as if we
wouldn't be careful download errors would be logged before that process
was even started. Not a problem for the acquire system, but likely to
confuse users and programs alike if they see the download process
producing errors before apt was technically allowed to do an acquire
(it didn't, so no violation, but it looks like it to the untrained eye).

Closes: 835195
tags/debian/1.3_rc3
David Kalnischkies 4 years ago
parent
commit
2e2865ae53
8 changed files with 70 additions and 50 deletions
  1. +1
    -2
      apt-pkg/acquire-item.cc
  2. +1
    -41
      apt-pkg/acquire-worker.cc
  3. +56
    -3
      apt-pkg/acquire.cc
  4. +1
    -0
      test/integration/framework
  5. +0
    -3
      test/integration/test-apt-redirect-loop
  6. +4
    -0
      test/integration/test-bug-605394-versioned-or-groups
  7. +1
    -1
      test/integration/test-bug-722207-print-uris-even-if-very-quiet
  8. +6
    -0
      test/integration/test-handle-redirect-as-used-mirror-change

+ 1
- 2
apt-pkg/acquire-item.cc View File

@@ -3279,9 +3279,8 @@ bool pkgAcqArchive::QueueNext()

// Create the item
Local = false;
QueueURI(Desc);

++Vf;
QueueURI(Desc);
return true;
}
return false;


+ 1
- 41
apt-pkg/acquire-worker.cc View File

@@ -661,55 +661,15 @@ bool pkgAcquire::Worker::QueueItem(pkgAcquire::Queue::QItem *Item)
if (OutFd == -1)
return false;

HashStringList const hsl = Item->GetExpectedHashes();

if (isDoomedItem(Item->Owner))
return true;

if (hsl.usable() == false && Item->Owner->HashesRequired() &&
_config->Exists("Acquire::ForceHash") == false)
{
std::string const Message = "400 URI Failure"
"\nURI: " + Item->URI +
"\nFilename: " + Item->Owner->DestFile +
"\nFailReason: WeakHashSums";

auto const ItmOwners = Item->Owners;
for (auto &O: ItmOwners)
{
O->Status = pkgAcquire::Item::StatAuthError;
O->Failed(Message, Config);
if (Log != nullptr)
Log->Fail(O->GetItemDesc());
}
// "queued" successfully, the item just instantly failed
return true;
}

if (Item->Owner->IsRedirectionLoop(Item->URI))
{
std::string const Message = "400 URI Failure"
"\nURI: " + Item->URI +
"\nFilename: " + Item->Owner->DestFile +
"\nFailReason: RedirectionLoop";

auto const ItmOwners = Item->Owners;
for (auto &O: ItmOwners)
{
O->Status = pkgAcquire::Item::StatError;
O->Failed(Message, Config);
if (Log != nullptr)
Log->Fail(O->GetItemDesc());
}
// "queued" successfully, the item just instantly failed
return true;
}

string Message = "600 URI Acquire\n";
Message.reserve(300);
Message += "URI: " + Item->URI;
Message += "\nFilename: " + Item->Owner->DestFile;

HashStringList const hsl = Item->GetExpectedHashes();
for (HashStringList::const_iterator hs = hsl.begin(); hs != hsl.end(); ++hs)
Message += "\nExpected-" + hs->HashType() + ": " + hs->HashValue();



+ 56
- 3
apt-pkg/acquire.cc View File

@@ -269,6 +269,42 @@ void pkgAcquire::Remove(Worker *Work)
it is constructed which creates a queue (based on the current queue
mode) and puts the item in that queue. If the system is running then
the queue might be started. */
static bool DoesAcquireResultInInstantFailure(pkgAcquire::Item * const Item,
pkgAcquire::MethodConfig const * const Config, pkgAcquireStatus * const Log)
{
auto SavedDesc = Item->GetItemDesc();
if (Item->IsRedirectionLoop(SavedDesc.URI))
{
std::string const Message = "400 URI Failure"
"\nURI: " + SavedDesc.URI +
"\nFilename: " + Item->DestFile +
"\nFailReason: RedirectionLoop";

Item->Status = pkgAcquire::Item::StatError;
Item->Failed(Message, Config);
if (Log != nullptr)
Log->Fail(SavedDesc);
return true;
}

HashStringList const hsl = Item->GetExpectedHashes();
if (hsl.usable() == false && Item->HashesRequired() &&
_config->Exists("Acquire::ForceHash") == false)
{
std::string const Message = "400 URI Failure"
"\nURI: " + SavedDesc.URI +
"\nFilename: " + Item->DestFile +
"\nFailReason: WeakHashSums";

auto SavedDesc = Item->GetItemDesc();
Item->Status = pkgAcquire::Item::StatAuthError;
Item->Failed(Message, Config);
if (Log != nullptr)
Log->Fail(SavedDesc);
return true;
}
return false;
}
void pkgAcquire::Enqueue(ItemDesc &Item)
{
// Determine which queue to put the item in
@@ -277,6 +313,13 @@ void pkgAcquire::Enqueue(ItemDesc &Item)
if (Name.empty() == true)
return;

/* the check for running avoids that we produce errors
in logging before we actually have started, which would
be easier to implement but would confuse users/implementations
so we check the items skipped here in #Startup */
if (Running && DoesAcquireResultInInstantFailure(Item.Owner, Config, Log))
return;

// Find the queue structure
Queue *I = Queues;
for (; I != 0 && I->Name != Name; I = I->Next);
@@ -912,10 +955,20 @@ bool pkgAcquire::Queue::Startup()
if (Workers == 0)
{
URI U(Name);
pkgAcquire::MethodConfig *Cnf = Owner->GetConfig(U.Access);
if (Cnf == 0)
pkgAcquire::MethodConfig * const Cnf = Owner->GetConfig(U.Access);
if (unlikely(Cnf == nullptr))
return false;

// now-running twin of the pkgAcquire::Enqueue call
for (QItem *I = Items; I != 0; )
{
bool pointless = false;
for (auto &&O: I->Owners)
if (DoesAcquireResultInInstantFailure(O, Cnf, Owner->Log))
pointless = true;
I = pointless ? Items : I->Next;
}

Workers = new Worker(this,Cnf,Owner->Log);
Owner->Add(Workers);
if (Workers->Start() == false)


+ 1
- 0
test/integration/framework View File

@@ -875,6 +875,7 @@ Filename: pool/main/${NAME}/${NAME}_${VERSION}_${arch}.deb"
test -z "$DEPENDENCIES" || echo "$DEPENDENCIES"
echo "Description: $(printf '%s' "$DESCRIPTION" | head -n 1)"
echo "Description-md5: $(printf '%s' "$DESCRIPTION" | md5sum | cut -d' ' -f 1)"
echo "SHA256: 0000000000000000000000000000000000000000000000000000000000000000"
echo
} >> "${PPATH}/Packages"
done


+ 0
- 3
test/integration/test-apt-redirect-loop View File

@@ -7,9 +7,6 @@ TESTDIR="$(readlink -f "$(dirname "$0")")"
setupenvironment
configarchitecture "i386"

insertpackage 'stable' 'apt' 'all' '1'
setupaptarchive --no-update

echo 'alright' > aptarchive/working
changetohttpswebserver
webserverconfig 'aptwebserver::redirect::replace::/redirectme3/' '/redirectme/'


+ 4
- 0
test/integration/test-bug-605394-versioned-or-groups View File

@@ -24,3 +24,7 @@ if aptget dist-upgrade --trivial-only -o Debug::pkgProblemResolver=1 -o Debug::p
else
msgpass
fi

# the Packages file includes only MD5
testfailure aptget dist-upgrade -y
testsuccess grep 'Insufficient information available to perform this download securely' rootdir/tmp/testfailure.output

+ 1
- 1
test/integration/test-bug-722207-print-uris-even-if-very-quiet View File

@@ -20,7 +20,7 @@ APTARCHIVE=$(readlink -f ./aptarchive)
testsuccessequal "'file://${APTARCHIVE}/pool/main/apt/apt_2_all.deb' apt_2_all.deb 0 " aptget upgrade -qq --print-uris
testsuccessequal "'file://${APTARCHIVE}/pool/main/apt/apt_2_all.deb' apt_2_all.deb 0 " aptget dist-upgrade -qq --print-uris
testsuccessequal "'file://${APTARCHIVE}/pool/main/apt/apt_2_all.deb' apt_2_all.deb 0 " aptget install apt -qq --print-uris
testsuccessequal "'file://${APTARCHIVE}/pool/main/apt/apt_2_all.deb' apt_2_all.deb 0 " aptget download apt -qq --print-uris
testsuccessequal "'file://${APTARCHIVE}/pool/main/apt/apt_2_all.deb' apt_2_all.deb 0 SHA256:0000000000000000000000000000000000000000000000000000000000000000" aptget download apt -qq --print-uris
testsuccessequal "'file://${APTARCHIVE}/apt_2.dsc' apt_2.dsc 9 SHA256:7776436a6d741497f1cd958014e1a05b352224231428152aae39da3c17fd2fd4
'file://${APTARCHIVE}/apt_2.tar.gz' apt_2.tar.gz 12 SHA256:f57f565eabe3fde0ec6e6e0bcc8db1d86fe2b4d6344a380a23520ddbb7728e99" aptget source apt -qq --print-uris
testsuccessequal "'http://metadata.ftp-master.debian.org/changelogs/main/a/apt/apt_2_changelog' apt.changelog" aptget changelog apt -qq --print-uris


+ 6
- 0
test/integration/test-handle-redirect-as-used-mirror-change View File

@@ -78,3 +78,9 @@ testsuccessequal "Ign:1 http://0.0.0.0:${APTHTTPPORT}/storage unstable InRelease
404 Not Found
Hit:2 http://0.0.0.0:${APTHTTPPORT} unstable Release
Reading package lists..." aptget update

rm -rf rootdir/var/lib/apt/lists
find aptarchive -name 'Release.gpg' -delete
find aptarchive -name 'Release' -delete
testwarning aptget update
testsuccess grep 'does not have a Release file' rootdir/tmp/testwarning.output

Loading…
Cancel
Save