From 95203356cbe9af853fe74b9cff08ab49579a5224 Mon Sep 17 00:00:00 2001
From: Sebastian Messmer <messmer@cryfs.org>
Date: Sat, 2 Feb 2019 13:00:53 -0800
Subject: [PATCH] Fix destructor race in LeftRight and add additional test
 cases

---
 src/cpp-utils/thread/LeftRight.h        | 31 ++++++++----------
 test/cpp-utils/thread/LeftRightTest.cpp | 43 +++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/src/cpp-utils/thread/LeftRight.h b/src/cpp-utils/thread/LeftRight.h
index 1870eee9..65ddba75 100644
--- a/src/cpp-utils/thread/LeftRight.h
+++ b/src/cpp-utils/thread/LeftRight.h
@@ -31,14 +31,6 @@ private:
 template <class T>
 class LeftRight final {
 public:
-    LeftRight()
-    : _writeMutex()
-    , _foregroundCounterIndex{0}
-    , _foregroundDataIndex{0}
-    , _counters{{{0}, {0}}}
-    , _data{{{}, {}}}
-    , _inDestruction(false) {}
-
     ~LeftRight() {
         // from now on, no new readers/writers will be accepted (see asserts in read()/write())
         _inDestruction = true;
@@ -56,22 +48,25 @@ public:
 
     template <typename F>
     auto read(F&& readFunc) const {
+        detail::IncrementRAII _increment_counter(&_counters[_foregroundCounterIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
+
         if(_inDestruction.load()) {
             throw std::logic_error("Issued LeftRight::read() after the destructor started running");
         }
 
-        detail::IncrementRAII _increment_counter(&_counters[_foregroundCounterIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
         return readFunc(_data[_foregroundDataIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
     }
 
-    // Throwing from write would result in invalid state
+    // Throwing an exception in writeFunc is ok but causes the state to be either the old or the new state,
+    // depending on if the first or the second call to writeFunc threw.
     template <typename F>
     auto write(F&& writeFunc) {
+        std::unique_lock<std::mutex> lock(_writeMutex);
+
         if(_inDestruction.load()) {
             throw std::logic_error("Issued LeftRight::read() after the destructor started running");
         }
 
-        std::unique_lock<std::mutex> lock(_writeMutex);
         return _write(writeFunc);
     }
 
@@ -116,7 +111,7 @@ private:
         _waitForBackgroundCounterToBeZero(localCounterIndex);
 
         /*
-         *4. Switch A/B counters
+         * 4. Switch A/B counters
          *
          * Now that we know all readers on B are really gone, we can switch the counters and have new readers
          * increment A's counter again, which is the correct counter since they're reading A.
@@ -133,7 +128,7 @@ private:
         _waitForBackgroundCounterToBeZero(localCounterIndex);
 
         // 6. Write to B
-        _callWriteFuncOnBackgroundInstance(writeFunc, localDataIndex);
+        return _callWriteFuncOnBackgroundInstance(writeFunc, localDataIndex);
     }
 
     template<class F>
@@ -155,11 +150,11 @@ private:
     }
 
     std::mutex _writeMutex;
-    std::atomic<uint8_t> _foregroundCounterIndex;
-    std::atomic<uint8_t> _foregroundDataIndex;
-    mutable std::array<std::atomic<int32_t>, 2> _counters;
-    std::array<T, 2> _data;
-    std::atomic<bool> _inDestruction;
+    std::atomic<uint8_t> _foregroundCounterIndex = {0};
+    std::atomic<uint8_t> _foregroundDataIndex = {0};
+    mutable std::array<std::atomic<int32_t>, 2> _counters = {{{0}, {0}}};
+    std::array<T, 2> _data = {{{}, {}}};
+    std::atomic<bool> _inDestruction = {false};
 };
 
 }
diff --git a/test/cpp-utils/thread/LeftRightTest.cpp b/test/cpp-utils/thread/LeftRightTest.cpp
index 2c2db300..851d2d47 100644
--- a/test/cpp-utils/thread/LeftRightTest.cpp
+++ b/test/cpp-utils/thread/LeftRightTest.cpp
@@ -30,9 +30,17 @@ TEST(LeftRightTest, givenVector_whenWritingAndReading_thenChangesArePresent) {
     EXPECT_EQ((vector<int>{5, 6}), read);
 }
 
+TEST(LeftRightTest, givenVector_whenWritingReturnsValue_thenValueIsReturned) {
+    LeftRight<vector<int>> obj;
+
+    auto a = obj.write([] (auto&) -> int {return 5;});
+    static_assert(std::is_same<int, decltype(a)>::value, "");
+    EXPECT_EQ(5, a);
+}
+
 TEST(LeftRightTest, readsCanBeConcurrent) {
     LeftRight<int> obj;
-	std::atomic<int> num_running_readers{0};
+    std::atomic<int> num_running_readers{0};
 
     std::thread reader1([&] () {
        obj.read([&] (auto&) {
@@ -157,7 +165,7 @@ TEST(LeftRightTest, whenWriteThrowsException_thenThrowsThrough) {
     );
 }
 
-TEST(LeftRightTest, givenInt_whenWriteThrowsException_thenResetsToOldState) {
+TEST(LeftRightTest, givenInt_whenWriteThrowsExceptionOnFirstCall_thenResetsToOldState) {
     LeftRight<int> obj;
 
     obj.write([](auto& obj) {obj = 5;});
@@ -180,6 +188,37 @@ TEST(LeftRightTest, givenInt_whenWriteThrowsException_thenResetsToOldState) {
     EXPECT_EQ(5, read);
 }
 
+// note: each write is executed twice, on the foreground and background copy.
+// We need to test a thrown exception in either call is handled correctly.
+TEST(LeftRightTest, givenInt_whenWriteThrowsExceptionOnSecondCall_thenKeepsNewState) {
+    LeftRight<int> obj;
+
+    obj.write([](auto& obj) {obj = 5;});
+    bool write_called = false;
+
+    EXPECT_THROW(
+        obj.write([&](auto& obj) {
+            obj = 6;
+            if (write_called) {
+                // this is the second time the write callback is executed
+                throw MyException();
+            } else {
+                write_called = true;
+            }
+        }),
+    MyException
+    );
+
+    // check reading it returns new value
+    int read = obj.read([] (auto& obj) {return obj;});
+    EXPECT_EQ(6, read);
+
+    // check changes are also present in background copy
+    obj.write([] (auto&) {}); // this switches to the background copy
+    read = obj.read([] (auto& obj) {return obj;});
+    EXPECT_EQ(6, read);
+}
+
 TEST(LeftRightTest, givenVector_whenWriteThrowsException_thenResetsToOldState) {
     LeftRight<vector<int>> obj;