usbdmx: Fix use after free hazard in AsyncUsb* destructor

Author: DanielG

plugins/usbdmx/AVLdiyD512.cpp

@@ -124,7 +124,6 @@ class AVLdiyAsyncUsbSender : public AsyncUsbSender {
   }
 
   ~AVLdiyAsyncUsbSender() {
-    CancelTransfer();
     delete[] m_control_setup_buffer;
   }
 

plugins/usbdmx/AnymauDMX.cpp

@@ -124,7 +124,6 @@ class AnymaAsyncUsbSender : public AsyncUsbSender {
   }
 
   ~AnymaAsyncUsbSender() {
-    CancelTransfer();
     delete[] m_control_setup_buffer;
   }
 

plugins/usbdmx/AsyncUsbReceiver.cpp

@@ -36,6 +36,8 @@ AsyncUsbReceiver::AsyncUsbReceiver(ola::usb::LibUsbAdaptor *adaptor,
 }
 
 AsyncUsbReceiver::~AsyncUsbReceiver() {
+  ola::thread::MutexLocker locker(&m_mutex);
+  CancelTransfer();
   if (!m_inited_with_handle) {
     m_adaptor->Close(m_usb_handle);
   }

plugins/usbdmx/AsyncUsbSender.cpp

@@ -37,6 +37,8 @@ AsyncUsbSender::AsyncUsbSender(LibUsbAdaptor *adaptor,
 }
 
 AsyncUsbSender::~AsyncUsbSender() {
+  ola::thread::MutexLocker locker(&m_mutex);
+  CancelTransfer();
   m_adaptor->Close(m_usb_handle);
 }
 

plugins/usbdmx/AsyncUsbTransceiverBase.cpp

@@ -63,6 +63,12 @@ void AsyncUsbTransceiverBase::TransferCompleteInternal(
   }
 
   TransferComplete(transfer);
+
+  {
+    ola::thread::MutexLocker locker(&m_mutex);
+    if (m_inflight.empty())
+      m_cond.Signal();
+  }
 }
 
 AsyncUsbTransceiverBase::AsyncUsbTransceiverBase(LibUsbAdaptor *adaptor,
@@ -82,8 +88,6 @@ AsyncUsbTransceiverBase::AsyncUsbTransceiverBase(LibUsbAdaptor *adaptor,
 AsyncUsbTransceiverBase::~AsyncUsbTransceiverBase() {
   ola::thread::MutexLocker locker(&m_mutex);
 
-  CancelTransfer();
-
   m_adaptor->UnrefDevice(m_usb_device);
 
   while (!m_idle.empty()) {
@@ -118,6 +122,13 @@ void AsyncUsbTransceiverBase::CancelTransfer() {
                << m_adaptor->ErrorCodeToString(ret);
     }
   }
+
+  while (!m_inflight.empty()) {
+    /* Wait for cancelled transfers to complete. Not waiting for the
+     * callbacks to happen is undefined behaviour according to libusb
+     * docs. */
+    m_cond.Wait(&m_mutex);
+  }
 }
 
 struct libusb_transfer *AsyncUsbTransceiverBase::CurrentTransfer() {

plugins/usbdmx/AsyncUsbTransceiverBase.h

@@ -101,7 +101,7 @@ class AsyncUsbTransceiverBase {
   virtual void PostTransferHook() {}
 
   /**
-   * @brief Cancel any pending transfers.
+   * @brief Cancel any pending transfers and wait for them to complete.
    */
   void CancelTransfer();
 
@@ -139,6 +139,7 @@ class AsyncUsbTransceiverBase {
   std::queue<struct libusb_transfer*> m_idle;  // GUARDED_BY(m_mutex);
 
   ola::thread::Mutex m_mutex;
+  ola::thread::ConditionVariable m_cond;
 
  private:
   /**

plugins/usbdmx/DMXCProjectsNodleU1.cpp

@@ -335,7 +335,6 @@ class DMXCProjectsNodleU1AsyncUsbReceiver : public AsyncUsbReceiver {
   }
 
   ~DMXCProjectsNodleU1AsyncUsbReceiver() {
-    CancelTransfer();
   }
 
   libusb_device_handle* SetupHandle() {
@@ -390,7 +389,6 @@ class DMXCProjectsNodleU1AsyncUsbSender : public AsyncUsbSender {
   }
 
   ~DMXCProjectsNodleU1AsyncUsbSender() {
-    CancelTransfer();
   }
 
   libusb_device_handle* SetupHandle() {

plugins/usbdmx/DMXCreator512Basic.cpp

@@ -193,7 +193,6 @@ class DMXCreator512BasicAsyncUsbSender : public AsyncUsbSender {
   }
 
   ~DMXCreator512BasicAsyncUsbSender() {
-    CancelTransfer();
   }
 
   libusb_device_handle* SetupHandle() {

plugins/usbdmx/EurolitePro.cpp

@@ -224,7 +224,6 @@ class EuroliteProAsyncUsbSender : public AsyncUsbSender {
   }
 
   ~EuroliteProAsyncUsbSender() {
-    CancelTransfer();
   }
 
   libusb_device_handle* SetupHandle() {

plugins/usbdmx/ShowJockeyDMXU1.cpp

@@ -277,7 +277,6 @@ class ShowJockeyDMXU1AsyncUsbSender : public AsyncUsbSender {
   }
 
   ~ShowJockeyDMXU1AsyncUsbSender() {
-    CancelTransfer();
     delete[] m_tx_frame;
   }