From ded246bb61b9b0f4ca658be45c1691844e1dc122 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Wed, 5 Aug 2020 11:04:45 +0200
Subject: http: Fix infinite loop on read errors

If there was a transient error and the server fd was closed, the
code would infinitely retry - it never reached FailCounter >= 2
because it falls through to the end of the loop, which sets
FailCounter = 0.

Add a continue just like the DNS rotation code has, so that the
retry actually fails after 2 attempts.

Also rework the error logic to forward the actual error message.
---
 methods/basehttp.cc | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/methods/basehttp.cc b/methods/basehttp.cc
index e659da255..cd319fce1 100644
--- a/methods/basehttp.cc
+++ b/methods/basehttp.cc
@@ -773,16 +773,23 @@ int BaseHttpMethod::Loop()
 	       if (Server->IsOpen() == false)
 	       {
 		  FailCounter++;
-		  _error->Discard();
 		  Server->Close();
+
 		  
 		  if (FailCounter >= 2)
 		  {
-		     Fail(_("Connection failed"),true);
+		     Fail(true);
 		     FailCounter = 0;
 		  }
-		  
+		  else
+		  {
+		     _error->Discard();
+		  }
+
+		  // Reset the pipeline
 		  QueueBack = Queue;
+		  Server->PipelineAnswersReceived = 0;
+		  continue;
 	       }
 	       else
                {
-- 
cgit v1.2.3


From 4b439208203cd584e158fd240a3a4a72d1248099 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Wed, 5 Aug 2020 14:14:19 +0200
Subject: basehttp: Correctly handle non-transient failure from RunData()

When we failed after a retry, we only communicated failure as
transient, but this seems wrong, especially given that the code
now always triggers a retry when Die() is called, as Die() closes
the server fd.

Instead, remove the error handling in that code path, and reuse
the existing fatal-ish error code handling path.
---
 methods/basehttp.cc | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/methods/basehttp.cc b/methods/basehttp.cc
index cd319fce1..5e29e0ce1 100644
--- a/methods/basehttp.cc
+++ b/methods/basehttp.cc
@@ -770,21 +770,11 @@ int BaseHttpMethod::Loop()
 	    }
 	    else
 	    {
-	       if (Server->IsOpen() == false)
+	       if (Server->IsOpen() == false && FailCounter < 1)
 	       {
 		  FailCounter++;
 		  Server->Close();
-
-		  
-		  if (FailCounter >= 2)
-		  {
-		     Fail(true);
-		     FailCounter = 0;
-		  }
-		  else
-		  {
-		     _error->Discard();
-		  }
+		  _error->Discard();
 
 		  // Reset the pipeline
 		  QueueBack = Queue;
@@ -794,6 +784,7 @@ int BaseHttpMethod::Loop()
 	       else
                {
                   Server->Close();
+		  FailCounter = 0;
 		  switch (Result)
 		  {
 		  case ResultState::TRANSIENT_ERROR:
-- 
cgit v1.2.3


From fa375493c5a4ed9c10d4e5257ac82c6e687862d3 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Mon, 10 Aug 2020 11:39:30 +0200
Subject: Do not retry on failure to fetch

While we fixed the infinite retrying earlier, we still have
problems if we retry in the middle of a transfer, we might
end up resuming downloads that are already done and read
more than we should (removing the IsOpen() check so that
it always retries makes test-ubuntu-bug-1098738-apt-get-source-md5sum
fail with wrong file sizes).

I think the retrying was added to fixup pipelining messups,
but we have better solutions now, so let's get rid of it,
until we have implemented this properly.
---
 methods/basehttp.cc | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/methods/basehttp.cc b/methods/basehttp.cc
index 5e29e0ce1..b8ab73155 100644
--- a/methods/basehttp.cc
+++ b/methods/basehttp.cc
@@ -770,31 +770,24 @@ int BaseHttpMethod::Loop()
 	    }
 	    else
 	    {
-	       if (Server->IsOpen() == false && FailCounter < 1)
+	       if (not Server->IsOpen())
 	       {
-		  FailCounter++;
-		  Server->Close();
-		  _error->Discard();
-
 		  // Reset the pipeline
 		  QueueBack = Queue;
 		  Server->PipelineAnswersReceived = 0;
-		  continue;
 	       }
-	       else
-               {
-                  Server->Close();
-		  FailCounter = 0;
-		  switch (Result)
-		  {
-		  case ResultState::TRANSIENT_ERROR:
-		     Fail(true);
-		     break;
-		  case ResultState::FATAL_ERROR:
-		  case ResultState::SUCCESSFUL:
-		     Fail(false);
-		     break;
-		  }
+
+	       Server->Close();
+	       FailCounter = 0;
+	       switch (Result)
+	       {
+	       case ResultState::TRANSIENT_ERROR:
+		  Fail(true);
+		  break;
+	       case ResultState::FATAL_ERROR:
+	       case ResultState::SUCCESSFUL:
+		  Fail(false);
+		  break;
 	       }
 	    }
 	    break;
-- 
cgit v1.2.3