From 57f16d51f4158dce1a49f6d5f5f05f057125b871 Mon Sep 17 00:00:00 2001
From: David Kalnischkies <david@kalnischkies.de>
Date: Tue, 5 Apr 2016 20:56:56 +0200
Subject: 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.
---
 apt-pkg/acquire-item.cc | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

(limited to 'apt-pkg/acquire-item.cc')

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;
-- 
cgit v1.2.3