diff --git a/DeviceAdapters/NIDAQ/NIDAQ.cpp b/DeviceAdapters/NIDAQ/NIDAQ.cpp index dca769804..8721a48ad 100644 --- a/DeviceAdapters/NIDAQ/NIDAQ.cpp +++ b/DeviceAdapters/NIDAQ/NIDAQ.cpp @@ -320,30 +320,78 @@ int NIDAQHub::InitializeImpl() } +void NIDAQHub::DestroyMonitoringThread(InputMonitoringThread*& t) +{ + if (t == 0) + return; + t->Stop(); + t->Join(); + delete t; + t = 0; +} + + +void NIDAQHub::DestroyMonitoringThread(TraceMonitoringThread*& t) +{ + if (t == 0) + return; + t->Stop(); + t->Join(); + delete t; + t = 0; +} + + int NIDAQHub::Shutdown() +{ + // Wrapper: see NIDAQHub::Initialize(). Shutdown() is also called from + // ~NIDAQHub(), where an escaping exception means immediate std::terminate. + try + { + return ShutdownImpl(); + } + catch (const std::exception& e) + { + LogMessage(std::string("EXCEPTION in NIDAQHub::Shutdown: ") + + typeid(e).name() + ": " + e.what()); + return DEVICE_ERR; + } + catch (...) + { + LogMessage("Unknown (non-standard) C++ exception in NIDAQHub::Shutdown"); + return DEVICE_ERR; + } +} + + +int NIDAQHub::ShutdownImpl() { if (!initialized_) return DEVICE_OK; - mThread_->Stop(); - mThread_->wait(); - delete mThread_; + // Destroy both threads first, so that no worker can still be running + // (and dereferencing this hub) while the members below are torn down. + // + // Order matters: the trace thread must go first. On exit its svc() calls + // hub_->FinishTrace(), which dereferences mThread_, so destroying mThread_ + // while the trace thread is still running is a null dereference. + DestroyMonitoringThread(tThread_); + DestroyMonitoringThread(mThread_); int err = StopTask(aoTask_); physicalAOChannels_.clear(); aoChannelSequences_.clear(); - if (doHub8_ != 0) - delete doHub8_; - else if (doHub16_ != 0) - delete doHub16_; - else if (doHub32_ != 0) - delete doHub32_; - - tThread_->Stop(); - tThread_->wait(); - delete tThread_; + // Independent deletes: the previous if/else-if chain leaked doHub16_ and + // doHub32_ whenever doHub8_ was non-null, which is the normal case since + // InitializeImpl() allocates all three together. Deleting null is a no-op. + delete doHub8_; + doHub8_ = 0; + delete doHub16_; + doHub16_ = 0; + delete doHub32_; + doHub32_ = 0; initialized_ = false; return err; @@ -964,9 +1012,7 @@ int NIDAQHub::StartAIMeasuringForPort(NIAnalogInputPort* port) int err; if (measuringTrace_) { - tThread_->Stop(); - tThread_->wait(); - delete tThread_; + DestroyMonitoringThread(tThread_); tThread_ = new TraceMonitoringThread(this); err = tThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, @@ -974,9 +1020,7 @@ int NIDAQHub::StartAIMeasuringForPort(NIAnalogInputPort* port) } else { - mThread_->Stop(); - mThread_->wait(); - delete mThread_; + DestroyMonitoringThread(mThread_); mThread_ = new InputMonitoringThread(this); err = mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_); @@ -998,9 +1042,7 @@ int NIDAQHub::StopAIMeasuringForPort(NIAnalogInputPort* port) if (measuringTrace_) { - tThread_->Stop(); - tThread_->wait(); - delete tThread_; + DestroyMonitoringThread(tThread_); tThread_ = new TraceMonitoringThread(this); int err = DEVICE_OK; @@ -1012,9 +1054,7 @@ int NIDAQHub::StopAIMeasuringForPort(NIAnalogInputPort* port) } else { - mThread_->Stop(); - mThread_->wait(); - delete mThread_; + DestroyMonitoringThread(mThread_); mThread_ = new InputMonitoringThread(this); @@ -1062,11 +1102,15 @@ std::string NIDAQHub::GetPhysicalChannelListForMeasuring(std::vectorStop(); - mThread_->wait(); + // Destroy the trace thread before the input thread: a trace thread that is + // still running calls hub_->FinishTrace() on exit, which dereferences + // mThread_. (This was previously a bare "delete tThread_", which destroyed + // a possibly still running thread without stopping or joining it first.) + DestroyMonitoringThread(tThread_); + DestroyMonitoringThread(mThread_); + mThread_ = new InputMonitoringThread(this); - delete tThread_; tThread_ = new TraceMonitoringThread(this); int err = tThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_, (float) traceFrequency_, traceAmount_, (int) physicalAIChannels_.size()); @@ -1077,12 +1121,12 @@ int NIDAQHub::StartTrace() int NIDAQHub::StopTrace() { - tThread_->Stop(); - tThread_->wait(); + DestroyMonitoringThread(tThread_); + tThread_ = new TraceMonitoringThread(this); measuringTrace_ = false; - - delete mThread_; + // Previously a bare "delete mThread_" (see StartTrace()). + DestroyMonitoringThread(mThread_); mThread_ = new InputMonitoringThread(this); int err = DEVICE_OK; if (physicalAIChannels_.size() > 0) @@ -1092,6 +1136,9 @@ int NIDAQHub::StopTrace() } +// Called from TraceMonitoringThread::svc(), i.e. on the trace thread itself. +// It must therefore never stop or join tThread_: doing so would make the trace +// thread join itself and deadlock. Only mThread_ may be touched here. int NIDAQHub::FinishTrace() { measuringTrace_ = false; @@ -1228,10 +1275,8 @@ int NIDAQHub::OnExpectedMaxVoltsIn(MM::PropertyBase* pProp, MM::ActionType eAct) double temp_max = 5.0; pProp->Get(temp_max); expectedMaxVoltsIn_ = (float) temp_max; - - mThread_->Stop(); - mThread_->wait(); - delete mThread_; + + DestroyMonitoringThread(mThread_); mThread_ = new InputMonitoringThread(this); if (physicalAIChannels_.size() > 1) mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_); @@ -1253,9 +1298,7 @@ int NIDAQHub::OnExpectedMinVoltsIn(MM::PropertyBase* pProp, MM::ActionType eAct) pProp->Get(temp_min); expectedMinVoltsIn_ = (float) temp_min; - mThread_->Stop(); - mThread_->wait(); - delete mThread_; + DestroyMonitoringThread(mThread_); mThread_ = new InputMonitoringThread(this); if (physicalAIChannels_.size() > 1) mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_); @@ -1339,7 +1382,7 @@ NIDAQDOHub::NIDAQDOHub(NIDAQHub* hub) : diTask_(0), doTask_(0), hub_(hub) { if (typeid(Tuint) == typeid(uInt8)) portWidth_ = 8; - else if (typeid(Tuint) == typeid(uInt8)) + else if (typeid(Tuint) == typeid(uInt16)) portWidth_ = 16; else if (typeid(Tuint) == typeid(uInt32)) portWidth_ = 32; @@ -1678,6 +1721,7 @@ template class NIDAQDOHub; InputMonitoringThread::InputMonitoringThread(NIDAQHub* hub) : stop_(false), + started_(false), aiTask_(NULL) { hub_ = hub; @@ -1687,7 +1731,32 @@ InputMonitoringThread::InputMonitoringThread(NIDAQHub* hub) : InputMonitoringThread::~InputMonitoringThread() { Stop(); - wait(); + // Join rather than letting ~MMDeviceThreadBase detach: svc() dereferences + // hub_, so a detached worker outliving the hub is a use-after-free. + Join(); +} + + +void InputMonitoringThread::Join() +{ + if (started_) + { + // Clear first: if wait() throws anyway, a later Join() must not retry. + started_ = false; + wait(); + } +} + + +// Clear aiTask_ and reset the handle, for use on Start()'s error paths. The +// task was already created there, so simply returning would leak it. +void InputMonitoringThread::ClearTask() +{ + if (aiTask_ != NULL) + { + DAQmxClearTask(aiTask_); + aiTask_ = NULL; + } } @@ -1696,13 +1765,22 @@ int InputMonitoringThread::Start(std::string AIChannelList, float minVal, float stop_ = false; int err = DAQmxCreateTask("AnalogInputReadTask", &aiTask_); if (err != DEVICE_OK) + { + // DAQmx leaves the handle unspecified on failure; do not clear it. + aiTask_ = NULL; return err; - + } + err = DAQmxCreateAIVoltageChan(aiTask_, AIChannelList.c_str(), "", DAQmx_Val_RSE, minVal, maxVal, DAQmx_Val_Volts, NULL); if (err != DEVICE_OK) + { + ClearTask(); return err; + } activate(); + // Only after activate() returns without throwing is there a thread to join. + started_ = true; return DEVICE_OK; } @@ -1730,6 +1808,7 @@ int InputMonitoringThread::svc() TraceMonitoringThread::TraceMonitoringThread(NIDAQHub* hub) : stop_(false), + started_(false), totalAmount_(0), numberOfChannels_(0), CSVheader_("") @@ -1742,7 +1821,31 @@ TraceMonitoringThread::TraceMonitoringThread(NIDAQHub* hub) : TraceMonitoringThread::~TraceMonitoringThread() { Stop(); - wait(); + // See ~InputMonitoringThread(). + Join(); +} + + +void TraceMonitoringThread::Join() +{ + if (started_) + { + // Clear first: if wait() throws anyway, a later Join() must not retry. + started_ = false; + wait(); + } +} + + +// See InputMonitoringThread::ClearTask(). DAQmxClearTask() also stops a task +// that was already started, so this is valid after DAQmxStartTask() too. +void TraceMonitoringThread::ClearTask() +{ + if (aiTask_ != NULL) + { + DAQmxClearTask(aiTask_); + aiTask_ = NULL; + } } @@ -1751,39 +1854,64 @@ int TraceMonitoringThread::Start(std::string AIChannelList, float minVal, float stop_ = false; int err = DAQmxCreateTask("AnalogInputReadTask", &aiTask_); if (err != DEVICE_OK) + { + // DAQmx leaves the handle unspecified on failure; do not clear it. + aiTask_ = NULL; return err; + } + // Every error path below must clear aiTask_, which was created above. CSVheader_ = "Time, " + AIChannelList; err = DAQmxCreateAIVoltageChan(aiTask_, AIChannelList.c_str(), "", DAQmx_Val_RSE, minVal, maxVal, DAQmx_Val_Volts, NULL); if (err != DEVICE_OK) + { + ClearTask(); return err; + } timestep_ = 1 / frequency; err = DAQmxSetSampClkRate(aiTask_, frequency); if (err != DEVICE_OK) + { + ClearTask(); return err; + } err = DAQmxSetSampQuantSampMode(aiTask_, DAQmx_Val_FiniteSamps); if (err != DEVICE_OK) + { + ClearTask(); return err; + } totalAmount_ = numberOfSamples; err = DAQmxSetSampQuantSampPerChan(aiTask_, numberOfSamples); if (err != DEVICE_OK) + { + ClearTask(); return err; + } err = DAQmxSetSampTimingType(aiTask_, DAQmx_Val_SampClk); if (err != DEVICE_OK) + { + ClearTask(); return err; + } path_ += boost::posix_time::to_iso_string(boost::posix_time::second_clock::local_time()) + ".csv"; numberOfChannels_ = numberOfChannels; err = DAQmxStartTask(aiTask_); if (err != DEVICE_OK) + { + ClearTask(); return err; + } activate(); + // Only after activate() returns without throwing is there a thread to join. + started_ = true; return DEVICE_OK; } diff --git a/DeviceAdapters/NIDAQ/NIDAQ.h b/DeviceAdapters/NIDAQ/NIDAQ.h index 9fb27af6a..01df1c01f 100644 --- a/DeviceAdapters/NIDAQ/NIDAQ.h +++ b/DeviceAdapters/NIDAQ/NIDAQ.h @@ -165,13 +165,26 @@ class InputMonitoringThread : public MMDeviceThreadBase int Start(std::string AIChannelList, float minVal, float maxVal); void Stop() { stop_ = true; } + // Join the thread, but only if it was actually started. MMDeviceThreadBase + // exposes no joinable(), and its wait() (== std::thread::join()) throws + // std::system_error when the thread was never activated, so we track that + // ourselves. Always use this instead of calling wait() directly. + void Join(); + bool IsStarted() const { return started_; } + private: + // Clear aiTask_ (if any) and reset the handle. Used on Start()'s error + // paths, where the task has already been created. + void ClearTask(); + NIDAQHub* hub_; TaskHandle aiTask_; bool stop_; + // True only between a successful activate() and the matching Join(). + bool started_; }; @@ -185,8 +198,15 @@ class TraceMonitoringThread : public MMDeviceThreadBase int Start(std::string AIChannelList, float minVal, float maxVal, float frequency, int numberOfSamples, int numberOfChannels); void Stop() { stop_ = true; } + // See InputMonitoringThread::Join(). + void Join(); + bool IsStarted() const { return started_; } + private: + // See InputMonitoringThread::ClearTask(). + void ClearTask(); + NIDAQHub* hub_; TaskHandle aiTask_; @@ -197,6 +217,8 @@ class TraceMonitoringThread : public MMDeviceThreadBase std::string CSVheader_; float timestep_; bool stop_; + // True only between a successful activate() and the matching Join(). + bool started_; }; // Forward declaration needed for NIDAQ hub @@ -220,11 +242,12 @@ class NIDAQHub : public HubBase, NIDAQHub(); virtual ~NIDAQHub(); - // Initialize() is a thin exception-catching wrapper around InitializeImpl(), - // which holds the actual initialization logic. + // Initialize() and Shutdown() are thin exception-catching wrappers around + // InitializeImpl() and ShutdownImpl(), which hold the actual logic. virtual int Initialize(); int InitializeImpl(); virtual int Shutdown(); + int ShutdownImpl(); virtual void GetName(char* name) const; virtual bool Busy() { return false; } @@ -281,6 +304,13 @@ class NIDAQHub : public HubBase, int StopTrace(); int FinishTrace(); + // Stop, join (only if actually started), delete and null a monitoring + // thread. Safe to call when the pointer is null or the thread was never + // activated. Takes the pointer by reference so the caller's member is + // always nulled; never delete these threads by hand. + void DestroyMonitoringThread(InputMonitoringThread*& t); + void DestroyMonitoringThread(TraceMonitoringThread*& t); + // Action handlers int OnDevice(MM::PropertyBase* pProp, MM::ActionType eAct); int OnMaxSequenceLength(MM::PropertyBase* pProp, MM::ActionType eAct);