refactor onError relabeling of DestFile as '.FAILED'

This helps ensure three things:
- each error is reported via ReportMirrorFailure
- if DestFile doesn't exist, do not attempt rename
- renames happen for every error

The last one wasn't the case for Size mismatches, which isn't nice, but
not a exploitable problem per-se as the file isn't picked up and remains
in partial/ where the following download-try will at most take it for a
partial request which fails the hashsum verification later on

Git-Dch: Ignore

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 222b78671128df229d065eac082f5d843991a37f..fcc7c7404c69620d038b0c69ffe797046ecaf6fb 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -143,6 +143,32 @@ void pkgAcquire::Item::Rename(string From,string To)
    }   
 }
                                                                        /*}}}*/
    }   
 }
                                                                        /*}}}*/

+bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const error)/*{{{*/
+{
+   if(FileExists(DestFile))
+      Rename(DestFile, DestFile + ".FAILED");
+
+   switch (error)
+   {
+      case HashSumMismatch:
+        ErrorText = _("Hash Sum mismatch");
+        Status = StatAuthError;
+        ReportMirrorFailure("HashChecksumFailure");
+        break;
+      case SizeMismatch:
+        ErrorText = _("Size mismatch");
+        Status = StatAuthError;
+        ReportMirrorFailure("SizeFailure");
+        break;
+      case InvalidFormat:
+        ErrorText = _("Invalid file format");
+        Status = StatError;
+        // do not report as usually its not the mirrors fault, but Portal/Proxy
+        break;
+   }
+   return false;
+}
+                                                                       /*}}}*/

 // Acquire::Item::ReportMirrorFailure                                  /*{{{*/
 // ---------------------------------------------------------------------
 void pkgAcquire::Item::ReportMirrorFailure(string FailCode)
 // Acquire::Item::ReportMirrorFailure                                  /*{{{*/
 // ---------------------------------------------------------------------
 void pkgAcquire::Item::ReportMirrorFailure(string FailCode)


@@ -595,9 +621,7 @@ void pkgAcqIndexDiffs::Finish(bool allDone)
 
       if(!ExpectedHash.empty() && !ExpectedHash.VerifyFile(DestFile))
       {
 
       if(!ExpectedHash.empty() && !ExpectedHash.VerifyFile(DestFile))
       {

-        Status = StatAuthError;
-        ErrorText = _("MD5Sum mismatch");
-        Rename(DestFile,DestFile + ".FAILED");
+        RenameOnError(HashSumMismatch);

         Dequeue();
         return;
       }
         Dequeue();
         return;
       }


@@ -866,10 +890,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash,
 
       if (!ExpectedHash.empty() && ExpectedHash.toStr() != Hash)
       {
 
       if (!ExpectedHash.empty() && ExpectedHash.toStr() != Hash)
       {

-         Status = StatAuthError;
-         ErrorText = _("Hash Sum mismatch");
-         Rename(DestFile,DestFile + ".FAILED");
-        ReportMirrorFailure("HashChecksumFailure");
+        RenameOnError(HashSumMismatch);

          return;
       }
 
          return;
       }
 


@@ -878,22 +899,18 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash,
       if (Verify == true)
       {
         FileFd fd(DestFile, FileFd::ReadOnly);
       if (Verify == true)
       {
         FileFd fd(DestFile, FileFd::ReadOnly);

-        pkgTagSection sec;
-        pkgTagFile tag(&fd);
-
-         // Only test for correctness if the file is not empty (empty is ok)
-         if (fd.Size() > 0) {
-            if (_error->PendingError() || !tag.Step(sec)) {
-               Status = StatError;
-               _error->DumpErrors();
-               Rename(DestFile,DestFile + ".FAILED");
-               return;
-            } else if (!sec.Exists("Package")) {
-               Status = StatError;
-               ErrorText = ("Encountered a section with no Package: header");
-               Rename(DestFile,DestFile + ".FAILED");
-               return;
-            }
+        // Only test for correctness if the file is not empty (empty is ok)
+        if (fd.FileSize() > 0)
+        {
+           pkgTagSection sec;
+           pkgTagFile tag(&fd);
+
+           // all our current indexes have a field 'Package' in each section
+           if (_error->PendingError() == true || tag.Step(sec) == false || sec.Exists("Package") == false)
+           {
+              RenameOnError(InvalidFormat);
+              return;
+           }

          }
       }
        
          }
       }
        


@@ -1907,18 +1924,14 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size,string CalcHash,
    // Check the size
    if (Size != Version->Size)
    {
    // Check the size
    if (Size != Version->Size)
    {

-      Status = StatError;
-      ErrorText = _("Size mismatch");
+      RenameOnError(SizeMismatch);

       return;
    }
    
    // Check the hash
    if(ExpectedHash.toStr() != CalcHash)
    {
       return;
    }
    
    // Check the hash
    if(ExpectedHash.toStr() != CalcHash)
    {

-      Status = StatError;
-      ErrorText = _("Hash Sum mismatch");
-      if(FileExists(DestFile))
-        Rename(DestFile,DestFile + ".FAILED");
+      RenameOnError(HashSumMismatch);

       return;
    }
 
       return;
    }
 


@@ -2058,9 +2071,7 @@ void pkgAcqFile::Done(string Message,unsigned long long Size,string CalcHash,
    // Check the hash
    if(!ExpectedHash.empty() && ExpectedHash.toStr() != CalcHash)
    {
    // Check the hash
    if(!ExpectedHash.empty() && ExpectedHash.toStr() != CalcHash)
    {

-      Status = StatError;
-      ErrorText = _("Hash Sum mismatch");
-      Rename(DestFile,DestFile + ".FAILED");
+      RenameOnError(HashSumMismatch);

       return;
    }
    
       return;
    }
    

diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 10c855e63d98a91caab3726dbaa708ab08107122..6b4f737089fa240c73ed0008a6a71606324c8512 100644 (file)
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -83,7 +83,7 @@ class pkgAcquire::Item : public WeakPointable
     *  overwritten.
     */
    void Rename(std::string From,std::string To);
     *  overwritten.
     */
    void Rename(std::string From,std::string To);

-   
+

    public:
 
    /** \brief The current status of this item. */
    public:
 
    /** \brief The current status of this item. */


@@ -281,6 +281,21 @@ class pkgAcquire::Item : public WeakPointable
     *  pkgAcquire::Remove.
     */
    virtual ~Item();
     *  pkgAcquire::Remove.
     */
    virtual ~Item();

+
+   protected:
+
+   enum RenameOnErrorState {
+      HashSumMismatch,
+      SizeMismatch,
+      InvalidFormat
+   };
+
+   /** \brief Rename failed file and set error
+    *
+    * \param state respresenting the error we encountered
+    * \param errorMsg a message describing the error
+    */
+   bool RenameOnError(RenameOnErrorState const state);

 };
                                                                        /*}}}*/
 /** \brief Information about an index patch (aka diff). */             /*{{{*/
 };
                                                                        /*}}}*/
 /** \brief Information about an index patch (aka diff). */             /*{{{*/


@@ -982,7 +997,7 @@ class pkgAcqArchive : public pkgAcquire::Item
     *
     *  \param Version The package version to download.
     *
     *
     *  \param Version The package version to download.
     *

-    *  \param StoreFilename A location in which the actual filename of
+    *  \param[out] StoreFilename A location in which the actual filename of

     *  the package should be stored.  It will be set to a guessed
     *  basename in the constructor, and filled in with a fully
     *  qualified filename once the download finishes.
     *  the package should be stored.  It will be set to a guessed
     *  basename in the constructor, and filled in with a fully
     *  qualified filename once the download finishes.