From f573843f27fc297d21d389c7c4e15495044027ee Mon Sep 17 00:00:00 2001
From: Sebastian Messmer <messmer@cryfs.org>
Date: Thu, 28 Feb 2019 02:29:10 -0800
Subject: [PATCH] Fix --unmount--idle

---
 ChangeLog.txt                             |  1 +
 src/cpp-utils/thread/ThreadSystem.cpp     | 19 ++++++++++++++++++
 src/cryfs-cli/Cli.cpp                     |  6 +++++-
 test/cpp-utils/process/SubprocessTest.cpp | 24 +++++++++++++++++++++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/ChangeLog.txt b/ChangeLog.txt
index 74445b20..db50a259 100644
--- a/ChangeLog.txt
+++ b/ChangeLog.txt
@@ -3,6 +3,7 @@ Version 0.10.1 (unreleased)
 Fixed bugs:
 * If file system migration encounters files or folders with the wrong format in the base directory, it now just ignores them instead of crashing.
 * When trying to migrate a file system from CryFS 0.9.3 or older, show an error message suggesting to first open it with 0.9.10 because we can't load that anymore.
+* The '--unmount-idle' parameter works again
 
 
 Version 0.10.0
diff --git a/src/cpp-utils/thread/ThreadSystem.cpp b/src/cpp-utils/thread/ThreadSystem.cpp
index dc5a62a6..13a0b0a6 100644
--- a/src/cpp-utils/thread/ThreadSystem.cpp
+++ b/src/cpp-utils/thread/ThreadSystem.cpp
@@ -52,15 +52,34 @@ namespace cpputils {
     void ThreadSystem::_stopAllThreadsForRestart() {
         _mutex.lock(); // Is unlocked in the after-fork handler. This way, the whole fork() is protected.
         for (RunningThread &thread : _runningThreads) {
+            if (boost::this_thread::get_id() == thread.thread.get_id()) {
+                // This means fork was called from within one of our _runningThreads.
+                // We cannot wait or ourselves to die.
+                // Forking from within a thread is usually chaos since the forked process only gets a copy
+                // of the calling thread as its new main thread. So we (hopefully) never should do this.
+                // This is, however, a valid pattern when fork() is directly followed by an exec().
+                // So let's just ignore this situation and continue as if nothing happened, assuming an exec()
+                // follows soon.
+                continue;
+            }
             thread.thread.interrupt();
         }
         for (RunningThread &thread : _runningThreads) {
+            if (boost::this_thread::get_id() == thread.thread.get_id()) {
+                // This means fork was called from within one of our _runningThreads. See comment above.
+                continue;
+            }
             thread.thread.join();
         }
     }
 
     void ThreadSystem::_restartAllThreads() {
         for (RunningThread &thread : _runningThreads) {
+            if (thread.thread.joinable()) {
+                // Because all non-self threads have been terminated in _stopAllThreadsForRestart,
+                // this means fork was called from within one of our _runningThreads. See comment above.
+                continue;
+            }
             thread.thread = _startThread(thread.loopIteration, thread.threadName);
         }
         _mutex.unlock(); // Was locked in the before-fork handler
diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp
index b4e7a508..261459f9 100644
--- a/src/cryfs-cli/Cli.cpp
+++ b/src/cryfs-cli/Cli.cpp
@@ -252,7 +252,11 @@ namespace cryfs_cli {
                 ASSERT(_device != none, "File system not ready to be initialized. Was it already initialized before?");
 
                 //TODO Test auto unmounting after idle timeout
-                _idleUnmounter = _createIdleCallback(options.unmountAfterIdleMinutes(), [fs] {fs->stop();});
+                const boost::optional<double> idle_minutes = options.unmountAfterIdleMinutes();
+                _idleUnmounter = _createIdleCallback(idle_minutes, [fs, idle_minutes] {
+                    LOG(INFO, "Unmounting because file system was idle for {} minutes", *idle_minutes);
+                    fs->stop();
+                });
                 if (_idleUnmounter != none) {
                     (*_device)->onFsAction(std::bind(&CallAfterTimeout::resetTimer, _idleUnmounter->get()));
                 }
diff --git a/test/cpp-utils/process/SubprocessTest.cpp b/test/cpp-utils/process/SubprocessTest.cpp
index a5b0e93d..9a1c857c 100644
--- a/test/cpp-utils/process/SubprocessTest.cpp
+++ b/test/cpp-utils/process/SubprocessTest.cpp
@@ -1,6 +1,8 @@
 #include <cpp-utils/process/subprocess.h>
 #include <gtest/gtest.h>
 
+#include <cpp-utils/lock/ConditionBarrier.h>
+
 using cpputils::Subprocess;
 using cpputils::SubprocessError;
 
@@ -98,3 +100,25 @@ TEST(SubprocessTest, Call_error5withoutput_output) {
 TEST(SubprocessTest, Call_error5withoutput_exitcode) {
     EXPECT_EQ(5, Subprocess::call(exit_with_message_and_status("hello", 5)).exitcode);
 }
+
+// TODO Move this test to a test suite for ThreadSystem/LoopThread
+#include <cpp-utils/thread/LoopThread.h>
+TEST(SubprocessTest, CallFromThreadSystemThread) {
+    cpputils::ConditionBarrier barrier;
+
+    cpputils::LoopThread thread(
+        [&barrier] () {
+            auto result = Subprocess::check_call(exit_with_message_and_status("hello", 0));
+            EXPECT_EQ(0, result.exitcode);
+            EXPECT_EQ("hello", result.output);
+
+            barrier.release();
+
+            return false; // don't run loop again
+        },
+        "child_thread"
+    );
+    thread.start();
+    barrier.wait();
+    thread.stop(); // just to make sure it's stopped before the test exits. Returning false above should already stop it, but we don't know when exactly. thread.stop() will block until it's actually stopped.
+}