ensure transaction states are changed only once

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.

2 files changed, 32 insertions, 11 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 1b817df71..d820756ca 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -362,6 +362,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;
@@ -451,6 +452,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)
 	 {
@@ -474,6 +476,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:
@@ -835,7 +838,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,6 +854,14 @@ 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)
@@ -884,6 +895,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();
@@ -1350,6 +1369,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 +1537,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;
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 61f64c3a9..54d998898 100644
--- a/apt-pkg/acquire-item.h
+++ b/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,