From 57401c48fadc0c78733a67294f9cc20a57e527c9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 2 Aug 2016 22:44:50 +0200 Subject: detect redirection loops in acquire instead of workers Having the detection handled in specific (http) workers means that a redirection loop over different hostnames isn't detected. Its also not a good idea have this implement in each method independently even if it would work --- apt-pkg/acquire-item.cc | 69 ++++++++++++++++++++++++++++++++++------------- apt-pkg/acquire-item.h | 5 +++- apt-pkg/acquire-worker.cc | 10 +++++++ 3 files changed, 64 insertions(+), 20 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index ad8cb7f24..f13d2f6ae 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -685,10 +685,15 @@ class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem /*{{{*/ /*}}}*/ // Acquire::Item::Item - Constructor /*{{{*/ +class pkgAcquire::Item::Private +{ +public: + std::vector PastRedirections; +}; APT_IGNORE_DEPRECATED_PUSH pkgAcquire::Item::Item(pkgAcquire * const owner) : FileSize(0), PartialSize(0), Mode(0), ID(0), Complete(false), Local(false), - QueueCounter(0), ExpectedAdditionalItems(0), Owner(owner), d(NULL) + QueueCounter(0), ExpectedAdditionalItems(0), Owner(owner), d(new Private()) { Owner->Add(this); Status = StatIdle; @@ -699,6 +704,7 @@ APT_IGNORE_DEPRECATED_POP pkgAcquire::Item::~Item() { Owner->Remove(this); + delete d; } /*}}}*/ std::string pkgAcquire::Item::Custom600Headers() const /*{{{*/ @@ -766,32 +772,40 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con } string const FailReason = LookupTag(Message, "FailReason"); - enum { MAXIMUM_SIZE_EXCEEDED, HASHSUM_MISMATCH, WEAK_HASHSUMS, OTHER } failreason = OTHER; + enum { MAXIMUM_SIZE_EXCEEDED, HASHSUM_MISMATCH, WEAK_HASHSUMS, REDIRECTION_LOOP, OTHER } failreason = OTHER; if ( FailReason == "MaximumSizeExceeded") failreason = MAXIMUM_SIZE_EXCEEDED; else if ( FailReason == "WeakHashSums") failreason = WEAK_HASHSUMS; + else if (FailReason == "RedirectionLoop") + failreason = REDIRECTION_LOOP; else if (Status == StatAuthError) failreason = HASHSUM_MISMATCH; if(ErrorText.empty()) { + std::ostringstream out; + switch (failreason) + { + case HASHSUM_MISMATCH: + out << _("Hash Sum mismatch") << std::endl; + break; + case WEAK_HASHSUMS: + out << _("Insufficient information available to perform this download securely") << std::endl; + break; + case REDIRECTION_LOOP: + out << "Redirection loop encountered" << std::endl; + break; + case MAXIMUM_SIZE_EXCEEDED: + out << LookupTag(Message, "Message") << std::endl; + break; + case OTHER: + out << LookupTag(Message, "Message"); + break; + } + if (Status == StatAuthError) { - std::ostringstream out; - switch (failreason) - { - case HASHSUM_MISMATCH: - out << _("Hash Sum mismatch") << std::endl; - break; - case WEAK_HASHSUMS: - out << _("Insufficient information available to perform this download securely") << std::endl; - break; - case MAXIMUM_SIZE_EXCEEDED: - case OTHER: - out << LookupTag(Message, "Message") << std::endl; - break; - } auto const ExpectedHashes = GetExpectedHashes(); if (ExpectedHashes.empty() == false) { @@ -822,10 +836,8 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con } out << "Last modification reported: " << LookupTag(Message, "Last-Modified", "") << std::endl; } - ErrorText = out.str(); } - else - ErrorText = LookupTag(Message,"Message"); + ErrorText = out.str(); } switch (failreason) @@ -833,6 +845,7 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con case MAXIMUM_SIZE_EXCEEDED: RenameOnError(MaximumSizeExceeded); break; case HASHSUM_MISMATCH: RenameOnError(HashSumMismatch); break; case WEAK_HASHSUMS: break; + case REDIRECTION_LOOP: break; case OTHER: break; } @@ -976,6 +989,24 @@ std::string pkgAcquire::Item::HashSum() const /*{{{*/ return hs != NULL ? hs->toStr() : ""; } /*}}}*/ +bool pkgAcquire::Item::IsRedirectionLoop(std::string const &NewURI) /*{{{*/ +{ + if (d->PastRedirections.empty()) + { + d->PastRedirections.push_back(NewURI); + return false; + } + auto const LastURI = std::prev(d->PastRedirections.end()); + // redirections to the same file are a way of restarting/resheduling, + // individual methods will have to make sure that they aren't looping this way + if (*LastURI == NewURI) + return false; + if (std::find(d->PastRedirections.begin(), LastURI, NewURI) != LastURI) + return true; + d->PastRedirections.push_back(NewURI); + return false; +} + /*}}}*/ pkgAcqTransactionItem::pkgAcqTransactionItem(pkgAcquire * const Owner, /*{{{*/ pkgAcqMetaClearSig * const transactionManager, IndexTarget const &target) : diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index ac4994738..e6e5ea12b 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -304,6 +304,8 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ */ virtual ~Item(); + bool APT_HIDDEN IsRedirectionLoop(std::string const &NewURI); + protected: /** \brief The acquire object with which this item is associated. */ pkgAcquire * const Owner; @@ -357,7 +359,8 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ virtual std::string GetFinalFilename() const; private: - void * const d; + class Private; + Private * const d; friend class pkgAcqMetaBase; friend class pkgAcqMetaClearSig; diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index 39cc55bdf..1ee78d070 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -269,6 +269,16 @@ bool pkgAcquire::Worker::RunMessages() for (auto const &Owner: ItmOwners) { pkgAcquire::ItemDesc &desc = Owner->GetItemDesc(); + if (Owner->IsRedirectionLoop(NewURI)) + { + std::string msg = Message; + msg.append("\nFailReason: RedirectionLoop"); + Owner->Failed(msg, Config); + if (Log != nullptr) + Log->Fail(Owner->GetItemDesc()); + continue; + } + if (Log != nullptr) Log->Done(desc); -- cgit v1.2.3