From 1acb60daa2cff9303aa11953d77131b7794da0fe Mon Sep 17 00:00:00 2001
From: Pauli <suokkos@gmail.com>
Date: Tue, 12 Jun 2018 17:53:43 +0300
Subject: [PATCH] Prevent data races during console/init thread shutdown

There is a minor chance that console or init thread would access already
freed memory when core is shutting down and cleaning up state. To avoid
any danger of having random bugs caused by the potential data race I
decided to make sure the shutdown code waits for the thread to exit
first.

Windows change is completely untested. It is purely based on msdn
documentation.
---
 library/Console-posix.cpp   | 18 ++++++++++--------
 library/Console-windows.cpp |  5 ++++-
 library/Core.cpp            | 21 ++++++++++++++++-----
 library/include/Core.h      |  1 +
 4 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/library/Console-posix.cpp b/library/Console-posix.cpp
index 9c244de10..a2f59077d 100644
--- a/library/Console-posix.cpp
+++ b/library/Console-posix.cpp
@@ -730,8 +730,7 @@ Console::Console()
 }
 Console::~Console()
 {
-    if(inited)
-        shutdown();
+    assert(!inited);
     if(wlock)
         delete wlock;
     if(d)
@@ -768,11 +767,6 @@ bool Console::shutdown(void)
     if(!d)
         return true;
     lock_guard <recursive_mutex> g(*wlock);
-    if(d->rawmode)
-        d->disable_raw();
-    d->print("\n");
-    inited = false;
-    // kill the thing
     close(d->exit_pipe[1]);
     return true;
 }
@@ -854,8 +848,16 @@ int Console::lineedit(const std::string & prompt, std::string & output, CommandH
 {
     lock_guard <recursive_mutex> g(*wlock);
     int ret = -2;
-    if(inited)
+    if(inited) {
         ret = d->lineedit(prompt,output,wlock,ch);
+        if (ret == -2) {
+            // kill the thing
+            if(d->rawmode)
+                d->disable_raw();
+            d->print("\n");
+            inited = false;
+        }
+    }
     return ret;
 }
 
diff --git a/library/Console-windows.cpp b/library/Console-windows.cpp
index 3caacd924..f3771f4ff 100644
--- a/library/Console-windows.cpp
+++ b/library/Console-windows.cpp
@@ -285,7 +285,10 @@ namespace DFHack
                 INPUT_RECORD rec;
                 DWORD count;
                 lock->unlock();
-                ReadConsoleInputA(console_in, &rec, 1, &count);
+                if (ReadConsoleInputA(console_in, &rec, 1, &count) != 0) {
+                    lock->lock();
+                    return -2;
+                }
                 lock->lock();
                 if (rec.EventType != KEY_EVENT || !rec.Event.KeyEvent.bKeyDown)
                     continue;
diff --git a/library/Core.cpp b/library/Core.cpp
index fccf3fa0e..d584cb891 100644
--- a/library/Core.cpp
+++ b/library/Core.cpp
@@ -130,6 +130,7 @@ struct Core::Private
     Core::Cond core_cond;
     thread::id df_suspend_thread;
     int df_suspend_depth;
+    std::thread iothread;
 
     Private() {
         df_suspend_depth = 0;
@@ -1476,6 +1477,12 @@ void fIOthread(void * iodata)
     }
 }
 
+Core::~Core()
+{
+    if (d->iothread.joinable())
+        con.shutdown();
+    delete d;
+}
 
 Core::Core() :
     d{new Private},
@@ -1739,12 +1746,12 @@ bool Core::Init()
     {
         cerr << "Starting IO thread.\n";
         // create IO thread
-        new thread(fIOthread, (void *) temp);
+        d->iothread = std::thread{fIOthread, (void*)temp};
     }
     else
     {
-        cerr << "Starting dfhack.init thread.\n";
-        new thread(fInitthread, (void *) temp);
+        std::cerr << "Starting dfhack.init thread.\n";
+        d->iothread = std::thread{fInitthread, (void*)temp};
     }
 
     cerr << "Starting DF input capture thread.\n";
@@ -2339,9 +2346,14 @@ void Core::onStateChange(color_ostream &out, state_change_event event)
     handleLoadAndUnloadScripts(out, event);
 }
 
-// FIXME: needs to terminate the IO threads and properly dismantle all the machinery involved.
 int Core::Shutdown ( void )
 {
+    // Make sure the console thread shutdowns before clean up to avoid any
+    // unlikely data races.
+    if (d->iothread.joinable()) {
+        con.shutdown();
+        d->iothread.join();
+    }
     if(errorstate)
         return true;
     errorstate = 1;
@@ -2358,7 +2370,6 @@ int Core::Shutdown ( void )
     }
     allModules.clear();
     memset(&(s_mods), 0, sizeof(s_mods));
-    con.shutdown();
     return -1;
 }
 
diff --git a/library/include/Core.h b/library/include/Core.h
index a8bbb9c41..6e3bd718e 100644
--- a/library/include/Core.h
+++ b/library/include/Core.h
@@ -198,6 +198,7 @@ namespace DFHack
         DFHack::Console con;
 
         Core();
+        ~Core();
 
         struct Private;
         Private *d;