chown finished partial files earlier

partial files are chowned by the Item baseclass to let the methods work
with them. Now, this baseclass is also responsible for chowning the
files back to root instead of having various deeper levels do this.

The consequence is that all overloaded Failed() methods now call the
Item::Failed base as their first step. The same is done for Done().

The effect is that even in partial files usually don't belong to
_apt anymore, helping sneakernets and reducing possibilities of a bad
method modifying files not belonging to them.

The change is supported by the framework not only supporting being run
as root, but with proper permission management, too, so that privilege
dropping can be tested with them.

4 files changed, 89 insertions, 65 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 704e285b5..11c522ae5 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -67,11 +67,11 @@ static void printHashSumComparision(std::string const &URI, HashStringList const
 									/*}}}*/
 static void ChangeOwnerAndPermissionOfFile(char const * const requester, char const * const file, char const * const user, char const * const group, mode_t const mode) /*{{{*/
 {
-   // ensure the file is owned by root and has good permissions
-   struct passwd const * const pw = getpwnam(user);
-   struct group const * const gr = getgrnam(group);
-   if (getuid() == 0) // if we aren't root, we can't chown, so don't try it
+   if (getuid() == 0 && strlen(user) != 0 && strlen(group) != 0) // if we aren't root, we can't chown, so don't try it
    {
+      // ensure the file is owned by root and has good permissions
+      struct passwd const * const pw = getpwnam(user);
+      struct group const * const gr = getgrnam(group);
       if (pw != NULL && gr != NULL && chown(file, pw->pw_uid, gr->gr_gid) != 0)
 	 _error->WarningE(requester, "chown to %s:%s of file %s failed", user, group, file);
    }
@@ -155,7 +155,10 @@ pkgAcquire::Item::~Item()
    fetch this object */
 void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 {
-   if(ErrorText == "")
+   if (RealFileExists(DestFile))
+      ChangeOwnerAndPermissionOfFile("Item::Failed", DestFile.c_str(), "root", "root", 0644);
+
+   if(ErrorText.empty())
       ErrorText = LookupTag(Message,"Message");
    UsedMirror =  LookupTag(Message,"UsedMirror");
    if (QueueCounter <= 1)
@@ -179,9 +182,9 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       Status = StatIdle;
 
    // check fail reason
-   string FailReason = LookupTag(Message, "FailReason");
+   string const FailReason = LookupTag(Message, "FailReason");
    if(FailReason == "MaximumSizeExceeded")
-      Rename(DestFile, DestFile+".FAILED");
+      RenameOnError(MaximumSizeExceeded);
 
    // report mirror failure back to LP if we actually use a mirror
    if(FailReason.size() != 0)
@@ -197,6 +200,7 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 void pkgAcquire::Item::Start(string /*Message*/,unsigned long long Size)
 {
    Status = StatFetching;
+   ErrorText.clear();
    if (FileSize == 0 && Complete == false)
       FileSize = Size;
 }
@@ -215,6 +219,8 @@ void pkgAcquire::Item::Done(string Message,unsigned long long Size,HashStringLis
       if (Owner->Log != 0)
 	 Owner->Log->Fetched(Size,atoi(LookupTag(Message,"Resume-Point","0").c_str()));
    }
+   if (RealFileExists(DestFile))
+      ChangeOwnerAndPermissionOfFile("Item::Done", DestFile.c_str(), "root", "root", 0644);
 
    if (FileSize == 0)
       FileSize= Size;
@@ -231,6 +237,7 @@ bool pkgAcquire::Item::Rename(string From,string To)
 {
    if (rename(From.c_str(),To.c_str()) == 0)
       return true;
+   ChangeOwnerAndPermissionOfFile("Item::Failed", To.c_str(), "root", "root", 0644);
 
    std::string S;
    strprintf(S, _("rename failed, %s (%s -> %s)."), strerror(errno),
@@ -258,7 +265,7 @@ void pkgAcquire::Item::Dequeue()					/*{{{*/
 									/*}}}*/
 bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const error)/*{{{*/
 {
-   if(FileExists(DestFile))
+   if (RealFileExists(DestFile))
       Rename(DestFile, DestFile + ".FAILED");
 
    switch (error)
@@ -283,7 +290,11 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const
 	 Status = StatError;
 	 break;
       case NotClearsigned:
-         ErrorText = _("Does not start with a cleartext signature");
+	 ErrorText = _("Does not start with a cleartext signature");
+	 Status = StatError;
+	 break;
+      case MaximumSizeExceeded:
+	 // the method is expected to report a good error for this
 	 Status = StatError;
 	 break;
    }
@@ -702,14 +713,14 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string IndexDiffFile)		/*{{{*/
 									/*}}}*/
 void pkgAcqDiffIndex::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{*/
 {
+   Item::Failed(Message,Cnf);
+   Status = StatDone;
+
    if(Debug)
       std::clog << "pkgAcqDiffIndex failed: " << Desc.URI << " with " << Message << std::endl
 		<< "Falling back to normal index file acquire" << std::endl;
 
    new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser);
-
-   Item::Failed(Message,Cnf);
-   Status = StatDone;
 }
 									/*}}}*/
 void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList const &Hashes,	/*{{{*/
@@ -792,8 +803,11 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *Owner,
    }
 }
 									/*}}}*/
-void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * /*Cnf*/)/*{{{*/
+void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{*/
 {
+   Item::Failed(Message,Cnf);
+   Status = StatDone;
+
    if(Debug)
       std::clog << "pkgAcqIndexDiffs failed: " << Desc.URI << " with " << Message << std::endl
 		<< "Falling back to normal index file acquire" << std::endl;
@@ -1281,11 +1295,14 @@ string pkgAcqIndex::Custom600Headers() const
 // pkgAcqIndex::Failed - getting the indexfile failed			/*{{{*/
 void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 {
+   Item::Failed(Message,Cnf);
+
    size_t const nextExt = CompressionExtensions.find(' ');
    if (nextExt != std::string::npos)
    {
       CompressionExtensions = CompressionExtensions.substr(nextExt+1);
       Init(RealURI, Desc.Description, Desc.ShortDesc);
+      Status = StatIdle;
       return;
    }
 
@@ -1295,8 +1312,6 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       unlink(EraseFileName.c_str());
    }
 
-   Item::Failed(Message,Cnf);
-
    /// cancel the entire transaction
    TransactionManager->AbortTransaction();
 }
@@ -1521,6 +1536,8 @@ string pkgAcqIndexTrans::Custom600Headers() const
 // AcqIndexTrans::Failed - Silence failure messages for missing files	/*{{{*/
 void pkgAcqIndexTrans::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 {
+   Item::Failed(Message,Cnf);
+
    size_t const nextExt = CompressionExtensions.find(' ');
    if (nextExt != std::string::npos)
    {
@@ -1530,8 +1547,6 @@ void pkgAcqIndexTrans::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       return;
    }
 
-   Item::Failed(Message,Cnf);
-
    // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor
    if (Cnf->LocalOnly == true ||
        StringToBool(LookupTag(Message,"Transient-Failure"),false) == false)
@@ -1563,16 +1578,9 @@ void pkgAcqMetaBase::AbortTransaction()
       if ((*I)->Status == pkgAcquire::Item::StatIdle)
          (*I)->Status = pkgAcquire::Item::StatDone;
 
-      // kill failed files in partial
-      if ((*I)->Status == pkgAcquire::Item::StatError)
-      {
-         std::string const PartialFile = GetPartialFileName(flNotDir((*I)->DestFile));
-         if(FileExists(PartialFile))
-            Rename(PartialFile, PartialFile + ".FAILED");
-      }
-      // fix permissions for existing files which were part of a reverify
-      // like InRelease files or files in partial we might work with next time
-      else if (FileExists((*I)->DestFile))
+      // reverify might act on a file outside of partial
+      // (as it itself is good, but needed to verify others, like Release)
+      if ((*I)->DestFile == (*I)->PartialFile && RealFileExists((*I)->DestFile))
 	 ChangeOwnerAndPermissionOfFile("AbortTransaction", (*I)->DestFile.c_str(), "root", "root", 0644);
    }
    Transaction.clear();
@@ -1608,8 +1616,6 @@ void pkgAcqMetaBase::CommitTransaction()
 	       << (*I)->DescURI() << std::endl;
 
 	 Rename((*I)->PartialFile, (*I)->DestFile);
-	 ChangeOwnerAndPermissionOfFile("CommitTransaction", (*I)->DestFile.c_str(), "root", "root", 0644);
-
       } else {
          if(_config->FindB("Debug::Acquire::Transaction", false) == true)
             std::clog << "rm "
@@ -1758,16 +1764,17 @@ void pkgAcqMetaSig::Done(string Message,unsigned long long Size,
 									/*}}}*/
 void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/
 {
-   string Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI);
+   Item::Failed(Message,Cnf);
 
    // check if we need to fail at this point 
    if (AuthPass == true && CheckStopAuthentication(RealURI, Message))
          return;
 
    // FIXME: meh, this is not really elegant
-   string InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12,
+   string const Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI);
+   string const InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12,
                                          "InRelease");
-   string FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI);
+   string const FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI);
 
    if (RealFileExists(Final) || RealFileExists(FinalInRelease))
    {
@@ -1782,7 +1789,7 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/
          _error->Warning(_("This is normally not allowed, but the option "
                            "Acquire::AllowDowngradeToInsecureRepositories was "
                            "given to override it."));
-         
+         Status = StatDone;
       } else {
          _error->Error("%s", downgrade_msg.c_str());
          Rename(MetaIndexFile, MetaIndexFile+".FAILED");
@@ -1810,8 +1817,6 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/
       QueueIndexes(true);
    }
 
-   Item::Failed(Message,Cnf);
-
    // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor
    if (Cnf->LocalOnly == true ||
        StringToBool(LookupTag(Message,"Transient-Failure"),false) == false)
@@ -2226,10 +2231,12 @@ string pkgAcqMetaClearSig::Custom600Headers() const
 									/*}}}*/
 // pkgAcqMetaClearSig::Done - We got a file                     	/*{{{*/
 // ---------------------------------------------------------------------
-void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long /*Size*/,
-                              HashStringList const &/*Hashes*/,
+void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long Size,
+                              HashStringList const &Hashes,
                               pkgAcquire::MethodConfig *Cnf)
 {
+   Item::Done(Message, Size, Hashes, Cnf);
+
    // if we expect a ClearTextSignature (InRelase), ensure that
    // this is what we get and if not fail to queue a 
    // Release/Release.gpg, see #346386
@@ -2567,7 +2574,6 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size, HashStringList
    string FinalFile = _config->FindDir("Dir::Cache::Archives");
    FinalFile += flNotDir(StoreFilename);
    Rename(DestFile,FinalFile);
-   ChangeOwnerAndPermissionOfFile("pkgAcqArchive::Done", FinalFile.c_str(), "root", "root", 0644);
    StoreFilename = DestFile = FinalFile;
    Complete = true;
 }
@@ -2577,8 +2583,8 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size, HashStringList
 /* Here we try other sources */
 void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 {
-   ErrorText = LookupTag(Message,"Message");
-   
+   Item::Failed(Message,Cnf);
+
    /* We don't really want to retry on failed media swaps, this prevents 
       that. An interesting observation is that permanent failures are not
       recorded. */
@@ -2588,10 +2594,10 @@ void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       // Vf = Version.FileList();
       while (Vf.end() == false) ++Vf;
       StoreFilename = string();
-      Item::Failed(Message,Cnf);
       return;
    }
-   
+
+   Status = StatIdle;
    if (QueueNext() == false)
    {
       // This is the retry counter
@@ -2604,9 +2610,9 @@ void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 	 if (QueueNext() == true)
 	    return;
       }
-      
+
       StoreFilename = string();
-      Item::Failed(Message,Cnf);
+      Status = StatError;
    }
 }
 									/*}}}*/
@@ -2726,7 +2732,12 @@ void pkgAcqFile::Done(string Message,unsigned long long Size,HashStringList cons
       // Symlink the file
       if (symlink(FileName.c_str(),DestFile.c_str()) != 0)
       {
-	 ErrorText = "Link to " + DestFile + " failure ";
+	 _error->PushToStack();
+	 _error->Errno("pkgAcqFile::Done", "Symlinking file %s failed", DestFile.c_str());
+	 std::stringstream msg;
+	 _error->DumpErrors(msg);
+	 _error->RevertToStack();
+	 ErrorText = msg.str();
 	 Status = StatError;
 	 Complete = false;
       }      
@@ -2738,19 +2749,19 @@ void pkgAcqFile::Done(string Message,unsigned long long Size,HashStringList cons
 /* Here we try other sources */
 void pkgAcqFile::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
 {
-   ErrorText = LookupTag(Message,"Message");
-   
+   Item::Failed(Message,Cnf);
+
    // This is the retry counter
    if (Retries != 0 &&
        Cnf->LocalOnly == false &&
        StringToBool(LookupTag(Message,"Transient-Failure"),false) == true)
    {
-      Retries--;
+      --Retries;
       QueueURI(Desc);
+      Status = StatIdle;
       return;
    }
-   
-   Item::Failed(Message,Cnf);
+
 }
 									/*}}}*/
 // AcqIndex::Custom600Headers - Insert custom request headers		/*{{{*/
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 68d5a01ce..4b4ef5fca 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -323,6 +323,7 @@ class pkgAcquire::Item : public WeakPointable
       InvalidFormat,
       SignatureError,
       NotClearsigned,
+      MaximumSizeExceeded
    };
 
    /** \brief Rename failed file and set error
diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index 1e20b74be..2c89c2dea 100644
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -54,23 +54,38 @@ pkgAcquire::pkgAcquire() : LockFD(-1), Queues(0), Workers(0), Configs(0), Log(NU
 			   Debug(_config->FindB("Debug::pkgAcquire",false)),
 			   Running(false)
 {
-   string const Mode = _config->Find("Acquire::Queue-Mode","host");
-   if (strcasecmp(Mode.c_str(),"host") == 0)
-      QueueMode = QueueHost;
-   if (strcasecmp(Mode.c_str(),"access") == 0)
-      QueueMode = QueueAccess;
+   Initialize();
 }
 pkgAcquire::pkgAcquire(pkgAcquireStatus *Progress) : LockFD(-1), Queues(0), Workers(0),
 			   Configs(0), Log(NULL), ToFetch(0),
 			   Debug(_config->FindB("Debug::pkgAcquire",false)),
 			   Running(false)
 {
+   Initialize();
+   SetLog(Progress);
+}
+void pkgAcquire::Initialize()
+{
    string const Mode = _config->Find("Acquire::Queue-Mode","host");
    if (strcasecmp(Mode.c_str(),"host") == 0)
       QueueMode = QueueHost;
    if (strcasecmp(Mode.c_str(),"access") == 0)
       QueueMode = QueueAccess;
-   SetLog(Progress);
+
+   // chown the auth.conf file as it will be accessed by our methods
+   std::string const SandboxUser = _config->Find("APT::Sandbox::User");
+   if (getuid() == 0 && SandboxUser.empty() == false) // if we aren't root, we can't chown, so don't try it
+   {
+      struct passwd const * const pw = getpwnam(SandboxUser.c_str());
+      struct group const * const gr = getgrnam("root");
+      if (pw != NULL && gr != NULL)
+      {
+	 std::string const AuthConf = _config->FindFile("Dir::Etc::netrc");
+	 if(AuthConf.empty() == false && RealFileExists(AuthConf) &&
+	       chown(AuthConf.c_str(), pw->pw_uid, gr->gr_gid) != 0)
+	    _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of file %s failed", SandboxUser.c_str(), AuthConf.c_str());
+      }
+   }
 }
 									/*}}}*/
 // Acquire::GetLock - lock directory and prepare for action		/*{{{*/
@@ -81,21 +96,16 @@ static bool SetupAPTPartialDirectory(std::string const &grand, std::string const
 	 CreateAPTDirectoryIfNeeded(parent, partial) == false)
       return false;
 
-   if (getuid() == 0) // if we aren't root, we can't chown, so don't try it
+   std::string const SandboxUser = _config->Find("APT::Sandbox::User");
+   if (getuid() == 0 && SandboxUser.empty() == false) // if we aren't root, we can't chown, so don't try it
    {
-      std::string SandboxUser = _config->Find("APT::Sandbox::User");
-      struct passwd *pw = getpwnam(SandboxUser.c_str());
-      struct group *gr = getgrnam("root");
+      struct passwd const * const pw = getpwnam(SandboxUser.c_str());
+      struct group const * const gr = getgrnam("root");
       if (pw != NULL && gr != NULL)
       {
          // chown the partial dir
          if(chown(partial.c_str(), pw->pw_uid, gr->gr_gid) != 0)
             _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of directory %s failed", SandboxUser.c_str(), partial.c_str());
-         // chown the auth.conf file
-         std::string const AuthConf = _config->FindFile("Dir::Etc::netrc");
-	 if(AuthConf.empty() == false && RealFileExists(AuthConf) &&
-	       chown(AuthConf.c_str(), pw->pw_uid, gr->gr_gid) != 0)
-	    _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of file %s failed", SandboxUser.c_str(), AuthConf.c_str());
       }
    }
    if (chmod(partial.c_str(), 0700) != 0)
diff --git a/apt-pkg/acquire.h b/apt-pkg/acquire.h
index a1a192d5f..f33362922 100644
--- a/apt-pkg/acquire.h
+++ b/apt-pkg/acquire.h
@@ -378,6 +378,8 @@ class pkgAcquire
     */
    virtual ~pkgAcquire();
 
+   private:
+   APT_HIDDEN void Initialize();
 };
 
 /** \brief Represents a single download source from which an item