We want to keep track of the state of a transaction overall to base
future decisions on it, but as a pre-requirement we have to make sure
that a transaction isn't commited twice (which happened if the download
of InRelease failed and Release takes over).

It also happened to create empty commits after a transaction was already
aborted in cases in which the Release files were rejected.

This isn't effecting security at the moment, but to ensure this isn't
happening again and can never be bad a bunch of fatal error messages are
added to make regressions on this front visible.

apt-pkg/acquire-item.cc

@@ -281,6 +281,12 @@ bool pkgAcquire::Item::QueueURI(pkgAcquire::ItemDesc &Item)
    for a hashsum mismatch to happen which helps nobody) */
 bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item)
 {
+   if (TransactionManager->State != TransactionStarted)
+   {
+      if (_config->FindB("Debug::Acquire::Transaction", false))
+	 std::clog << "Skip " << Target.URI << " as transaction was already dealt with!" << std::endl;
+      return false;
+   }
    std::string const FinalFile = GetFinalFilename();
    if (TransactionManager != NULL && TransactionManager->IMSHit == true &&
 	 FileExists(FinalFile) == true)
@@ -362,6 +368,7 @@ bool pkgAcqTransactionItem::TransactionState(TransactionStates const state)
    bool const Debug = _config->FindB("Debug::Acquire::Transaction", false);
    switch(state)
    {
+      case TransactionStarted: _error->Fatal("Item %s changed to invalid transaction start state!", Target.URI.c_str()); break;
       case TransactionAbort:
 	 if(Debug == true)
 	    std::clog << "  Cancel: " << DestFile << std::endl;
@@ -430,7 +437,7 @@ bool pkgAcqTransactionItem::TransactionState(TransactionStates const state)
 	 } else {
 	    if(Debug == true)
 	       std::clog << "rm " << DestFile << " # " << DescURI() << std::endl;
-	    if (RemoveFile("TransactionCommit", DestFile) == false)
+	    if (RemoveFile("TransItem::TransactionCommit", DestFile) == false)
 	       return false;
 	 }
 	 break;
@@ -451,6 +458,7 @@ bool pkgAcqIndex::TransactionState(TransactionStates const state)
 
    switch (state)
    {
+      case TransactionStarted: _error->Fatal("AcqIndex %s changed to invalid transaction start state!", Target.URI.c_str()); break;
       case TransactionAbort:
 	 if (Stage == STAGE_DECOMPRESS_AND_VERIFY)
 	 {
@@ -462,7 +470,7 @@ bool pkgAcqIndex::TransactionState(TransactionStates const state)
 	 break;
       case TransactionCommit:
 	 if (EraseFileName.empty() == false)
-	    RemoveFile("TransactionCommit", EraseFileName);
+	    RemoveFile("AcqIndex::TransactionCommit", EraseFileName);
 	 break;
    }
    return true;
@@ -474,6 +482,7 @@ bool pkgAcqDiffIndex::TransactionState(TransactionStates const state)
 
    switch (state)
    {
+      case TransactionStarted: _error->Fatal("Item %s changed to invalid transaction start state!", Target.URI.c_str()); break;
       case TransactionCommit:
 	 break;
       case TransactionAbort:
@@ -510,6 +519,41 @@ class APT_HIDDEN NoActionItem : public pkgAcquire::Item			/*{{{*/
    }
 };
 									/*}}}*/
+class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem		/*{{{*/
+/* This class ensures that a file which was configured but isn't downloaded
+   for various reasons isn't kept in an old version in the lists directory.
+   In a way its the reverse of NoActionItem as it helps with removing files
+   even if the lists-cleanup is deactivated. */
+{
+   public:
+   virtual std::string DescURI() const APT_OVERRIDE {return Target.URI;};
+   virtual HashStringList GetExpectedHashes()  const APT_OVERRIDE {return HashStringList();};
+
+   CleanupItem(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager, IndexTarget const &Target) :
+      pkgAcqTransactionItem(Owner, TransactionManager, Target)
+   {
+      Status = StatDone;
+      DestFile = GetFinalFileNameFromURI(Target.URI);
+   }
+   bool TransactionState(TransactionStates const state) APT_OVERRIDE
+   {
+      switch (state)
+      {
+	 case TransactionStarted:
+	    break;
+	 case TransactionAbort:
+	    break;
+	 case TransactionCommit:
+	    if (_config->FindB("Debug::Acquire::Transaction", false) == true)
+	       std::clog << "rm " << DestFile << " # " << DescURI() << std::endl;
+	    if (RemoveFile("TransItem::TransactionCommit", DestFile) == false)
+	       return false;
+	    break;
+      }
+      return true;
+   }
+};
+									/*}}}*/
 
 // Acquire::Item::Item - Constructor					/*{{{*/
 APT_IGNORE_DEPRECATED_PUSH
@@ -835,7 +879,7 @@ pkgAcqMetaBase::pkgAcqMetaBase(pkgAcquire * const Owner,
       IndexTarget const &DataTarget)
 : pkgAcqTransactionItem(Owner, TransactionManager, DataTarget), d(NULL),
    IndexTargets(IndexTargets),
-   AuthPass(false), IMSHit(false)
+   AuthPass(false), IMSHit(false), State(TransactionStarted)
 {
 }
 									/*}}}*/
@@ -851,10 +895,20 @@ void pkgAcqMetaBase::AbortTransaction()
    if(_config->FindB("Debug::Acquire::Transaction", false) == true)
       std::clog << "AbortTransaction: " << TransactionManager << std::endl;
 
+   switch (TransactionManager->State)
+   {
+      case TransactionStarted: break;
+      case TransactionAbort: _error->Fatal("Transaction %s was already aborted and is aborted again", TransactionManager->Target.URI.c_str()); return;
+      case TransactionCommit: _error->Fatal("Transaction %s was already aborted and is now commited", TransactionManager->Target.URI.c_str()); return;
+   }
+   TransactionManager->State = TransactionAbort;
+
    // ensure the toplevel is in error state too
    for (std::vector<pkgAcqTransactionItem*>::iterator I = Transaction.begin();
         I != Transaction.end(); ++I)
    {
+      if ((*I)->Status != pkgAcquire::Item::StatFetching)
+	 Owner->Dequeue(*I);
       (*I)->TransactionState(TransactionAbort);
    }
    Transaction.clear();
@@ -884,6 +938,14 @@ void pkgAcqMetaBase::CommitTransaction()
    if(_config->FindB("Debug::Acquire::Transaction", false) == true)
       std::clog << "CommitTransaction: " << this << std::endl;
 
+   switch (TransactionManager->State)
+   {
+      case TransactionStarted: break;
+      case TransactionAbort: _error->Fatal("Transaction %s was already commited and is now aborted", TransactionManager->Target.URI.c_str()); return;
+      case TransactionCommit: _error->Fatal("Transaction %s was already commited and is again commited", TransactionManager->Target.URI.c_str()); return;
+   }
+   TransactionManager->State = TransactionCommit;
+
    // move new files into place *and* remove files that are not
    // part of the transaction but are still on disk
    for (std::vector<pkgAcqTransactionItem*>::iterator I = Transaction.begin();
@@ -1086,10 +1148,12 @@ void pkgAcqMetaBase::QueueIndexes(bool const verify)			/*{{{*/
       // than invent an entirely new flag we would need to carry for all of eternity.
       if (Target->Option(IndexTarget::ARCHITECTURE) == "all")
       {
-	 if (TransactionManager->MetaIndexParser->IsArchitectureSupported("all") == false)
-	    continue;
-	 if (TransactionManager->MetaIndexParser->IsArchitectureAllSupportedFor(*Target) == false)
+	 if (TransactionManager->MetaIndexParser->IsArchitectureSupported("all") == false ||
+	       TransactionManager->MetaIndexParser->IsArchitectureAllSupportedFor(*Target) == false)
+	 {
+	    new CleanupItem(Owner, TransactionManager, *Target);
 	    continue;
+	 }
       }
 
       bool trypdiff = Target->OptionBool(IndexTarget::PDIFFS);
@@ -1099,13 +1163,17 @@ void pkgAcqMetaBase::QueueIndexes(bool const verify)			/*{{{*/
 	 {
 	    // optional targets that we do not have in the Release file are skipped
 	    if (Target->IsOptional)
+	    {
+	       new CleanupItem(Owner, TransactionManager, *Target);
 	       continue;
+	    }
 
 	    std::string const &arch = Target->Option(IndexTarget::ARCHITECTURE);
 	    if (arch.empty() == false)
 	    {
 	       if (TransactionManager->MetaIndexParser->IsArchitectureSupported(arch) == false)
 	       {
+		  new CleanupItem(Owner, TransactionManager, *Target);
 		  _error->Notice(_("Skipping acquire of configured file '%s' as repository '%s' doesn't support architecture '%s'"),
 			Target->MetaKey.c_str(), TransactionManager->Target.Description.c_str(), arch.c_str());
 		  continue;
@@ -1114,7 +1182,10 @@ void pkgAcqMetaBase::QueueIndexes(bool const verify)			/*{{{*/
 	       // ignore silently as this is pretty much the same as just shipping an empty file.
 	       // if we don't know which architectures are supported, we do NOT ignore it to notify user about this
 	       if (TransactionManager->MetaIndexParser->IsArchitectureSupported("*undefined*") == false)
+	       {
+		  new CleanupItem(Owner, TransactionManager, *Target);
 		  continue;
+	       }
 	    }
 
 	    Status = StatAuthError;
@@ -1124,11 +1195,21 @@ void pkgAcqMetaBase::QueueIndexes(bool const verify)			/*{{{*/
 	 else
 	 {
 	    auto const hashes = GetExpectedHashesFor(Target->MetaKey);
-	    if (hashes.usable() == false && hashes.empty() == false)
+	    if (hashes.empty() == false)
 	    {
-	       _error->Warning(_("Skipping acquire of configured file '%s' as repository '%s' provides only weak security information for it"),
+	       if (hashes.usable() == false)
+	       {
+		  new CleanupItem(Owner, TransactionManager, *Target);
+		  _error->Warning(_("Skipping acquire of configured file '%s' as repository '%s' provides only weak security information for it"),
 			Target->MetaKey.c_str(), TransactionManager->Target.Description.c_str());
-	       continue;
+		  continue;
+	       }
+	       // empty files are skipped as acquiring the very small compressed files is a waste of time
+	       else if (hashes.FileSize() == 0)
+	       {
+		  new CleanupItem(Owner, TransactionManager, *Target);
+		  continue;
+	       }
 	    }
 	 }
 
@@ -1350,6 +1431,15 @@ string pkgAcqMetaClearSig::Custom600Headers() const
    return Header;
 }
 									/*}}}*/
+void pkgAcqMetaClearSig::Finished()					/*{{{*/
+{
+   if(_config->FindB("Debug::Acquire::Transaction", false) == true)
+      std::clog << "Finished: " << DestFile <<std::endl;
+   if(TransactionManager != NULL && TransactionManager->State == TransactionStarted &&
+      TransactionManager->TransactionHasError() == false)
+      TransactionManager->CommitTransaction();
+}
+									/*}}}*/
 bool pkgAcqMetaClearSig::VerifyDone(std::string const &Message,		/*{{{*/
 	 pkgAcquire::MethodConfig const * const Cnf)
 {
@@ -1509,15 +1599,6 @@ void pkgAcqMetaIndex::Failed(string const &Message,
    }
 }
 									/*}}}*/
-void pkgAcqMetaIndex::Finished()					/*{{{*/
-{
-   if(_config->FindB("Debug::Acquire::Transaction", false) == true)
-      std::clog << "Finished: " << DestFile <<std::endl;
-   if(TransactionManager != NULL &&
-      TransactionManager->TransactionHasError() == false)
-      TransactionManager->CommitTransaction();
-}
-									/*}}}*/
 std::string pkgAcqMetaIndex::DescURI() const				/*{{{*/
 {
    return Target.URI;
@@ -2721,9 +2802,8 @@ void pkgAcqIndex::StageDownloadDone(string const &Message)
    // methods like file:// give us an alternative (uncompressed) file
    else if (Target.KeepCompressed == false && AltFilename.empty() == false)
    {
-      if (CurrentCompressionExtension != "uncompressed")
-	 DestFile.erase(DestFile.length() - (CurrentCompressionExtension.length() + 1));
       Filename = AltFilename;
+      EraseFileName.clear();
    }
    // Methods like e.g. "file:" will give us a (compressed) FileName that is
    // not the "DestFile" we set, in this case we uncompress from the local file
@@ -2753,7 +2833,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message)
       DestFile = "/dev/null";
    }
 
-   if (EraseFileName.empty())
+   if (EraseFileName.empty() && Filename != AltFilename)
       EraseFileName = Filename;
 
    // queue uri for the next stage

apt-pkg/acquire-item.h

@@ -381,6 +381,7 @@ class APT_HIDDEN pkgAcqTransactionItem: public pkgAcquire::Item		/*{{{*/
    pkgAcqMetaClearSig * const TransactionManager;
 
    enum TransactionStates {
+      TransactionStarted,
       TransactionCommit,
       TransactionAbort,
    };
@@ -467,6 +468,7 @@ class APT_HIDDEN pkgAcqMetaBase : public pkgAcqTransactionItem		/*{{{*/
  public:
    // This refers more to the Transaction-Manager than the actual file
    bool IMSHit;
+   TransactionStates State;
 
    virtual bool QueueURI(pkgAcquire::ItemDesc &Item) APT_OVERRIDE;
    virtual HashStringList GetExpectedHashes() const APT_OVERRIDE;
@@ -522,7 +524,6 @@ class APT_HIDDEN pkgAcqMetaIndex : public pkgAcqMetaBase
    virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 		     pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
-   virtual void Finished() APT_OVERRIDE;
 
    /** \brief Create a new pkgAcqMetaIndex. */
    pkgAcqMetaIndex(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager,
@@ -588,6 +589,7 @@ class APT_HIDDEN pkgAcqMetaClearSig : public pkgAcqMetaIndex
    virtual bool VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 		     pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
+   virtual void Finished() APT_OVERRIDE;
 
    /** \brief Create a new pkgAcqMetaClearSig. */
    pkgAcqMetaClearSig(pkgAcquire * const Owner,