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

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index ad8cb7f245795298c4c5a54d361b86012881e534..f13d2f6aefa1befe49a117d575b973d22a7d4ca3 100644 (file)
--- 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<std::string> 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", "<none>") << 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 ac4994738a5c1730d4decc489e3682f32ac1c6dc..e6e5ea12b2fd896368bae04379449830126d7eb2 100644 (file)
--- 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 39cc55bdf1fb997b56a246b841837df7e8d1640e..1ee78d07031a7dbe226c550e6a251d3e45a78b8f 100644 (file)
--- 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);
 

diff --git a/methods/aptmethod.h b/methods/aptmethod.h
index d3c9486365f22be07115332c6842c591dbfcbc85..0963ea21e9b9003d191d754ae5305d570bb9fafc 100644 (file)
--- a/methods/aptmethod.h
+++ b/methods/aptmethod.h
@@ -17,7 +17,8 @@
 
 class aptMethod : public pkgAcqMethod
 {
-   char const * const Binary;
+protected:
+   std::string Binary;
 
 public:
    virtual bool Configuration(std::string Message) APT_OVERRIDE

diff --git a/methods/http.cc b/methods/http.cc
index c61ca1c3fc0f0f614c501766b514a1f4926eedbd..cf5eae06d3b9a62dee672cd74826a0382a24eebe 100644 (file)
--- a/methods/http.cc
+++ b/methods/http.cc
@@ -465,6 +465,12 @@ bool HttpServerState::RunData(FileFd * const File)
    return Owner->Flush() && !_error->PendingError();
 }
                                                                        /*}}}*/
+bool HttpServerState::RunDataToDevNull()                               /*{{{*/
+{
+   FileFd DevNull("/dev/null", FileFd::WriteOnly);
+   return RunData(&DevNull);
+}
+                                                                       /*}}}*/
 bool HttpServerState::ReadHeaderLines(std::string &Data)               /*{{{*/
 {
    return In.WriteTillEl(Data);

diff --git a/methods/http.h b/methods/http.h
index ac5f52314c46d7fb3c824c6d109534d5e4503087..2ac3b8c9b366d0b2e313b05d3e8147c38ddd3a07 100644 (file)
--- a/methods/http.h
+++ b/methods/http.h
@@ -106,6 +106,7 @@ struct HttpServerState: public ServerState
    virtual void Reset() APT_OVERRIDE { ServerState::Reset(); ServerFd = -1; };
 
    virtual bool RunData(FileFd * const File) APT_OVERRIDE;
+   virtual bool RunDataToDevNull() APT_OVERRIDE;
 
    virtual bool Open() APT_OVERRIDE;
    virtual bool IsOpen() APT_OVERRIDE;

diff --git a/methods/https.h b/methods/https.h
index 8592570c63133f7d58f82f76692bd9287c823ef8..85cc7824e16802457d5a6b064ff0637e0b4e0d30 100644 (file)
--- a/methods/https.h
+++ b/methods/https.h
@@ -39,6 +39,7 @@ class HttpsServerState : public ServerState
 
    /** \brief Transfer the data from the socket */
    virtual bool RunData(FileFd * const /*File*/) APT_OVERRIDE { return false; }
+   virtual bool RunDataToDevNull() APT_OVERRIDE { return false; }
 
    virtual bool Open() APT_OVERRIDE { return false; }
    virtual bool IsOpen() APT_OVERRIDE { return false; }

diff --git a/methods/server.cc b/methods/server.cc
index 6d147fe129ccc61ed0e51fa8841de5dd62eca15a..672a3d16dd72a55aa796a7b91c1f984380cef74e 100644 (file)
--- a/methods/server.cc
+++ b/methods/server.cc
@@ -297,12 +297,33 @@ ServerMethod::DealWithHeaders(FetchResult &Res)
         else
            NextURI.clear();
         NextURI.append(DeQuoteString(Server->Location));
+        if (Queue->Uri == NextURI)
+        {
+           SetFailReason("RedirectionLoop");
+           _error->Error("Redirection loop encountered");
+           if (Server->HaveContent == true)
+              return ERROR_WITH_CONTENT_PAGE;
+           return ERROR_UNRECOVERABLE;
+        }
         return TRY_AGAIN_OR_REDIRECT;
       }
       else
       {
         NextURI = DeQuoteString(Server->Location);
         URI tmpURI = NextURI;
+        if (tmpURI.Access == "http" && Binary == "https+http")
+        {
+           tmpURI.Access = "https+http";
+           NextURI = tmpURI;
+        }
+        if (Queue->Uri == NextURI)
+        {
+           SetFailReason("RedirectionLoop");
+           _error->Error("Redirection loop encountered");
+           if (Server->HaveContent == true)
+              return ERROR_WITH_CONTENT_PAGE;
+           return ERROR_UNRECOVERABLE;
+        }
         URI Uri = Queue->Uri;
         // same protocol redirects are okay
         if (tmpURI.Access == Uri.Access)
@@ -486,10 +507,6 @@ bool ServerMethod::Fetch(FetchItem *)
 // ServerMethod::Loop - Main loop                                      /*{{{*/
 int ServerMethod::Loop()
 {
-   typedef vector<string> StringVector;
-   typedef vector<string>::iterator StringVectorIterator;
-   map<string, StringVector> Redirected;
-
    signal(SIGTERM,SigTerm);
    signal(SIGINT,SigTerm);
    
@@ -720,46 +737,16 @@ int ServerMethod::Loop()
            File = 0;
            break;
         }
-        
-         // Try again with a new URL
-         case TRY_AGAIN_OR_REDIRECT:
-         {
-            // Clear rest of response if there is content
-            if (Server->HaveContent)
-            {
-               File = new FileFd("/dev/null",FileFd::WriteExists);
-               Server->RunData(File);
-               delete File;
-               File = 0;
-            }
-
-            /* Detect redirect loops.  No more redirects are allowed
-               after the same URI is seen twice in a queue item. */
-            StringVector &R = Redirected[Queue->DestFile];
-            bool StopRedirects = false;
-            if (R.empty() == true)
-               R.push_back(Queue->Uri);
-            else if (R[0] == "STOP" || R.size() > 10)
-               StopRedirects = true;
-            else
-            {
-               for (StringVectorIterator I = R.begin(); I != R.end(); ++I)
-                  if (Queue->Uri == *I)
-                  {
-                     R[0] = "STOP";
-                     break;
-                  }
- 
-               R.push_back(Queue->Uri);
-            }
- 
-            if (StopRedirects == false)
-               Redirect(NextURI);
-            else
-               Fail();
- 
-            break;
-         }
+
+        // Try again with a new URL
+        case TRY_AGAIN_OR_REDIRECT:
+        {
+           // Clear rest of response if there is content
+           if (Server->HaveContent)
+              Server->RunDataToDevNull();
+           Redirect(NextURI);
+           break;
+        }
 
         default:
         Fail(_("Internal error"));

diff --git a/methods/server.h b/methods/server.h
index af0914b9c6ea9ce25847893d3a6db4f7c1913a2b..b23b0e50ae737969a250afce0ffaa74535eff09d 100644 (file)
--- a/methods/server.h
+++ b/methods/server.h
@@ -91,6 +91,7 @@ struct ServerState
 
    /** \brief Transfer the data from the socket */
    virtual bool RunData(FileFd * const File) = 0;
+   virtual bool RunDataToDevNull() = 0;
 
    virtual bool Open() = 0;
    virtual bool IsOpen() = 0;

diff --git a/test/integration/test-apt-redirect-loop b/test/integration/test-apt-redirect-loop
new file mode 100755 (executable)
index 0000000..b54f742
--- /dev/null
+++ b/test/integration/test-apt-redirect-loop
@@ -0,0 +1,23 @@
+#!/bin/sh
+set -e
+
+TESTDIR="$(readlink -f "$(dirname "$0")")"
+. "$TESTDIR/framework"
+
+setupenvironment
+configarchitecture "i386"
+
+insertpackage 'stable' 'apt' 'all' '1'
+setupaptarchive --no-update
+
+echo 'alright' > aptarchive/working
+changetohttpswebserver
+webserverconfig 'aptwebserver::redirect::replace::/redirectme3/' '/redirectme/'
+webserverconfig 'aptwebserver::redirect::replace::/redirectme2/' '/redirectme3/'
+webserverconfig 'aptwebserver::redirect::replace::/redirectme/' '/redirectme2/'
+
+testfailure apthelper download-file "http://localhost:${APTHTTPPORT}/redirectme/working" httpfile
+testsuccess grep 'Redirection loop encountered' rootdir/tmp/testfailure.output
+
+testfailure apthelper download-file "https://localhost:${APTHTTPSPORT}/redirectme/working" httpsfile
+testsuccess grep 'Redirection loop encountered' rootdir/tmp/testfailure.output