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.
develop
Pauli 2018-06-12 17:53:43 +03:00
parent 19a169ba65
commit 1acb60daa2
4 changed files with 31 additions and 14 deletions

@ -730,8 +730,7 @@ Console::Console()
} }
Console::~Console() Console::~Console()
{ {
if(inited) assert(!inited);
shutdown();
if(wlock) if(wlock)
delete wlock; delete wlock;
if(d) if(d)
@ -768,11 +767,6 @@ bool Console::shutdown(void)
if(!d) if(!d)
return true; return true;
lock_guard <recursive_mutex> g(*wlock); 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]); close(d->exit_pipe[1]);
return true; return true;
} }
@ -854,8 +848,16 @@ int Console::lineedit(const std::string & prompt, std::string & output, CommandH
{ {
lock_guard <recursive_mutex> g(*wlock); lock_guard <recursive_mutex> g(*wlock);
int ret = -2; int ret = -2;
if(inited) if(inited) {
ret = d->lineedit(prompt,output,wlock,ch); 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; return ret;
} }

@ -285,7 +285,10 @@ namespace DFHack
INPUT_RECORD rec; INPUT_RECORD rec;
DWORD count; DWORD count;
lock->unlock(); lock->unlock();
ReadConsoleInputA(console_in, &rec, 1, &count); if (ReadConsoleInputA(console_in, &rec, 1, &count) != 0) {
lock->lock();
return -2;
}
lock->lock(); lock->lock();
if (rec.EventType != KEY_EVENT || !rec.Event.KeyEvent.bKeyDown) if (rec.EventType != KEY_EVENT || !rec.Event.KeyEvent.bKeyDown)
continue; continue;

@ -130,6 +130,7 @@ struct Core::Private
Core::Cond core_cond; Core::Cond core_cond;
thread::id df_suspend_thread; thread::id df_suspend_thread;
int df_suspend_depth; int df_suspend_depth;
std::thread iothread;
Private() { Private() {
df_suspend_depth = 0; df_suspend_depth = 0;
@ -1476,6 +1477,12 @@ void fIOthread(void * iodata)
} }
} }
Core::~Core()
{
if (d->iothread.joinable())
con.shutdown();
delete d;
}
Core::Core() : Core::Core() :
d{new Private}, d{new Private},
@ -1739,12 +1746,12 @@ bool Core::Init()
{ {
cerr << "Starting IO thread.\n"; cerr << "Starting IO thread.\n";
// create IO thread // create IO thread
new thread(fIOthread, (void *) temp); d->iothread = std::thread{fIOthread, (void*)temp};
} }
else else
{ {
cerr << "Starting dfhack.init thread.\n"; std::cerr << "Starting dfhack.init thread.\n";
new thread(fInitthread, (void *) temp); d->iothread = std::thread{fInitthread, (void*)temp};
} }
cerr << "Starting DF input capture thread.\n"; cerr << "Starting DF input capture thread.\n";
@ -2339,9 +2346,14 @@ void Core::onStateChange(color_ostream &out, state_change_event event)
handleLoadAndUnloadScripts(out, event); handleLoadAndUnloadScripts(out, event);
} }
// FIXME: needs to terminate the IO threads and properly dismantle all the machinery involved.
int Core::Shutdown ( void ) 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) if(errorstate)
return true; return true;
errorstate = 1; errorstate = 1;
@ -2358,7 +2370,6 @@ int Core::Shutdown ( void )
} }
allModules.clear(); allModules.clear();
memset(&(s_mods), 0, sizeof(s_mods)); memset(&(s_mods), 0, sizeof(s_mods));
con.shutdown();
return -1; return -1;
} }

@ -198,6 +198,7 @@ namespace DFHack
DFHack::Console con; DFHack::Console con;
Core(); Core();
~Core();
struct Private; struct Private;
Private *d; Private *d;