Crash when unplugging DMX USB

[matthijskooijman]

I get a segfault when unplugging an Enttec DMX USB Pro configured for input. I'm running on git master (3973c82f60dbb2e84545bfc536db01d490387101), this issue did not occur on the latest 4.11.2 release. The bug happens both with a classic DMX USB PRO as well as with a MK2. The issue seems to be very reproducible.

There's a slight oddity in my setup (I did a make install to a non-system folder and only set up a symlink for the plugins), but I do not think this is the cause. I wanted to try the opensuse autobuilds for master, but those are down for maintanance apparently.

I'm including debug output and a backtrace below.

Plug in USB device:

void HotPlugMonitor::emitDeviceAdded(uint, uint) 1027 24577
void DMXUSB::slotDeviceAdded(uint, uint) "403" "6001"
virtual void HPMPrivate::run() Unhandled udev action: "bind"
static QList<DMXInterface*> LibFTDIInterface::interfaces(QList<DMXInterface*>) DMX USB VID: "403" PID: "6001"
static QList<DMXInterface*> LibFTDIInterface::interfaces(QList<DMXInterface*>) DMX USB serial:  "EN099032" name: "DMX USB PRO" vendor: "ENTTEC"
[setOutputsNumber] base line: 0 m_outputLines: 1
void IOPluginCache::slotConfigurationChanged()
[InputOutputPatchEditor] Fill tree for universe:  0
[InputOutputPatchEditor] Fill tree for universe:  0
void HIDPlugin::slotDeviceAdded(uint, uint) "403" "6001"
void MidiPlugin::slotDeviceAdded(uint, uint) "403" "6001"
void MidiEnumerator::rescan()
void MidiEnumeratorPrivate::rescan()
ALSA Port name:  "Midi Through Port-0"
ALSA Port name:  "Midi Through Port-0"
ALSA Port name:  "Receiver"
ALSA Port name:  "X-TOUCH COMPACT MIDI 1"
ALSA Port name:  "X-TOUCH COMPACT MIDI 1"
void Peperoni::slotDeviceAdded(uint, uint) "403" "6001"
void Peperoni::slotDeviceAdded(uint, uint) not a Peperoni device

Enable input in I/O tab:

[Universe] setInputPatch - ID: 0 , plugin: "DMX USB" , input: 0 , profile: "None"
InputPatch::set - plugin: "DMX USB" , line: 0 , profile: "None"
[QLCIOPlugin] setting lines: 0 0 4294967295
virtual bool DMXUSBWidget::open(quint32, bool) Line: 0 , open inputs: 1 , open outputs: 0
virtual bool DMXUSBWidget::open(quint32, bool) Interface correctly opened and configured
bool EnttecDMXUSBPro::extractSerial() "DMX USB PRO" gave malformed serial reply: "0" "0" "0" "0" "0" "0" "0" "0" "0"
[New Thread 0x7fff17fff700 (LWP 9971)]
Time is late by 321491332 nanoseconds
virtual void MasterTimerPrivate::run() MasterTimer is running late!
INPUT thread started
virtual void EnttecDMXUSBProInput::run() Got unrecognized label: 10
Value at 1 changed to "73"
Value at 4 changed to "23"
write 4 23
write 1 73

Unplug USB:

virtual void HPMPrivate::run() Unhandled udev action: "unbind"
void HotPlugMonitor::emitDeviceRemoved(uint, uint) 1027 24577
void DMXUSB::slotDeviceRemoved(uint, uint) "403" "6001"
virtual EnttecDMXUSBPro::~EnttecDMXUSBPro()
void EnttecDMXUSBPro::stopOutputThread()
virtual bool LibFTDIInterface::close() "DMX USB PRO" usb bulk read failed

Thread 18 "EnttecDMXUSBPro" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff17fff700 (LWP 9971)]
0x00007fffd4342506 in EnttecDMXUSBProInput::run (this=0x55555604b250) at enttecdmxusbpro.cpp:559
559         if ((byte = m_interface->readByte(&ok)) != ENTTEC_PRO_START_OF_MSG)
(gdb) bt
#0  0x00007fffd4342506 in EnttecDMXUSBProInput::run() (this=0x55555604b250) at enttecdmxusbpro.cpp:559
#1  0x00007ffff606d16d in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x00007ffff3ee16db in start_thread (arg=0x7fff17fff700) at pthread_create.c:463
#3  0x00007ffff575088f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) p *this
$1 = {<QThread> = {<No data fields>}, static staticMetaObject = {d = {
      superdata = 0x7ffff6700a40 <QThread::staticMetaObject>, 
      stringdata = 0x7fffd4352e80 <qt_meta_stringdata_EnttecDMXUSBProInput>, 
      data = 0x7fffd4352f40 <qt_meta_data_EnttecDMXUSBProInput>, 
      static_metacall = 0x7fffd434eaa0 <EnttecDMXUSBProInput::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)>, relatedMetaObjects = 0x0, extradata = 0x0}}, m_interface = 0x5555561ffab0, m_running = true}
(gdb) p m_interface
$2 = (DMXInterface *) 0x5555561ffab0
(gdb) p *m_interface
$3 = {_vptr.DMXInterface = 0x0, m_serial = {static null = {<No data fields>}, d = 0x555555c0ed50}, m_name = {
    static null = {<No data fields>}, d = 0x555555c37730}, m_vendor = {static null = {<No data fields>}, 
    d = 0x5555562219b0}, m_vendorID = 1027, m_productID = 24577, m_id = 0, static FTDIVID = 1027, 
  static ATMELVID = 1003, static MICROCHIPVID = 1240, static FTDIPID = 24577, static DMX4ALLPID = 51280, 
  static NANODMXPID = 8216, static EUROLITEPID = 64099, static ELECTROTASPID = 0}

It looks like m_interface got corrupted, with the vtable (_vptr) being zero. I've also seen versions of this crash where the v_ptr is not zero, and there is one extra unknown frame in the backtrace, so I suppose the vptr is invalid but not zero in those cases.

I've added some debug output:

virtual void HPMPrivate::run() Unhandled udev action: "unbind"
void HotPlugMonitor::emitDeviceRemoved(uint, uint) 1027 24577
void DMXUSB::slotDeviceRemoved(uint, uint) "403" "6001"
virtual EnttecDMXUSBPro::~EnttecDMXUSBPro()
void EnttecDMXUSBPro::stopOutputThread()
virtual bool LibFTDIInterface::close() "DMX USB PRO" usb bulk read failed

Thread 18 "EnttecDMXUSBPro" received signal SIGSEGV, Segmentation fault.

Looking at the code, I suspect that the interface is freed (by DMXUSBWidget::~DMXUSBWidget()) before the input thread is stopped. It seems that the input thread is only freed / stopped by EnttecDMXUSBPro::close(), so perhaps that does not run in the USB disconnected case?

I added some debug output to EnttecDMXUSBPro::close and DMXUSBWidget::~DMXUSBWidget(), which gives the below output, confirming my suspicion.

virtual void HPMPrivate::run() Unhandled udev action: "unbind"
void HotPlugMonitor::emitDeviceRemoved(uint, uint) 1027 24577
void DMXUSB::slotDeviceRemoved(uint, uint) "403" "6001"
virtual EnttecDMXUSBPro::~EnttecDMXUSBPro()
void EnttecDMXUSBPro::stopOutputThread()
virtual DMXUSBWidget::~DMXUSBWidget()
virtual bool LibFTDIInterface::close() "DMX USB PRO" usb bulk read failed

If I add cleanup of the input thread to the EnttecDMXUSBPro destructor, things seem to work. I'm not quite sure this is a proper solution, though (I'm duplicating this code, so it should at least go into a method, I suppose). Perhaps it would be better to make sure close is called, I really have no clue :-)

Here's what I tried:

--- a/plugins/dmxusb/src/enttecdmxusbpro.cpp
+++ b/plugins/dmxusb/src/enttecdmxusbpro.cpp
@@ -41,6 +41,12 @@ EnttecDMXUSBPro::~EnttecDMXUSBPro()
 {
     qDebug() << Q_FUNC_INFO;
     stopOutputThread();
+    if (m_inputThread)
+    {
+       disconnect(m_inputThread, SIGNAL(dataReady(QByteArray,bool)), this, SLOT(slotDataReceived(QByteArray,bool)));
+       delete m_inputThread;
+       m_inputThread = NULL;
+    }
 }

 DMXUSBWidget::Type EnttecDMXUSBPro::type() const

With that, the ouput becomes:

virtual void HPMPrivate::run() Unhandled udev action: "unbind"
void HotPlugMonitor::emitDeviceRemoved(uint, uint) 1027 24577
void DMXUSB::slotDeviceRemoved(uint, uint) "403" "6001"
virtual EnttecDMXUSBPro::~EnttecDMXUSBPro()
void EnttecDMXUSBPro::stopOutputThread()
virtual EnttecDMXUSBProInput::~EnttecDMXUSBProInput()
void EnttecDMXUSBProInput::stopInputThread()
INPUT thread terminated
virtual DMXUSBWidget::~DMXUSBWidget()
virtual bool LibFTDIInterface::close() "DMX USB PRO" usb bulk read failed
[Thread 0x7fff17fff700 (LWP 15939) exited]
void IOPluginCache::slotConfigurationChanged()
[InputOutputPatchEditor] Fill tree for universe:  0
[InputOutputPatchEditor] Fill tree for universe:  0
void HIDPlugin::slotDeviceRemoved(uint, uint) "403" "6001"
void MidiPlugin::slotDeviceRemoved(uint, uint) "403" "6001"
void MidiEnumerator::rescan()
void MidiEnumeratorPrivate::rescan()
ALSA Port name:  "Midi Through Port-0"
ALSA Port name:  "Midi Through Port-0"
ALSA Port name:  "Receiver"
ALSA Port name:  "X-TOUCH COMPACT MIDI 1"
ALSA Port name:  "X-TOUCH COMPACT MIDI 1"
void Peperoni::slotDeviceRemoved(uint, uint) "403" "6001"
void Peperoni::slotDeviceRemoved(uint, uint) not a Peperoni device

[mcallegari]

@matthijskooijman Thanks for the accurate report. Indeed there was an issue in the destructor. Fixed now

[matthijskooijman]

Hm, I wonder about your fix. You're now calling close() passing a line of 0. AFAICS this should be a global line number, not a device line number, so this is effectively just a dummy (and potentially invalid) line number. In addition to stopping threads, the close() method will also call DMXUSBWidget::close(), which will actually try to do things with the line. Since it mostly does internal bookkeeping, this is probably not a problem for this particular case (when we're already destructing things anyway), but future changes to DMXUSBWidget::close() might cause problems.

Also, if this widget is not the first one (e.g. m_inputBaseLine or m_outputBaseLine is non-zero, I believe the subtraction in e.g. this line will overflow, and a "Trying to close an out of bounds input line !" warning will be shown).

Reading the code, I noticed another bug in there: currently EnttecDMXUSBPro::close always closes the output thread, regardless of any remaining open lines. On my Pro Mk2, that has 2 output lines, this means that if I enable both outputs, and then uncheck one in the I/O tab of QLC, the output thread is stopped and both outputs stop working. Fixing this (by checking for any remaining open lines before stopping the thread) would also mean that the fix for the original issue to stop working (since that would then require all open lines to be properly closed with proper line numbers before the threads would be stopped), which is another indication that the fix for the original issue is not entirerly correct.

[mcallegari]

We're in the destructor, which means QLC+ is shutting down or the USB device has been unplugged. Therefore, DMXUSBWidget is destroyed as well. If it makes you feel better, close calls can be replaced with

    close(m_inputBaseLine, true);
    close(m_outputBaseLine, false);

As for multiple outputs open/close while running, I'll check that.

[matthijskooijman]

If it makes you feel better, close calls can be replaced with

At least that would prevent the warning from being shown, I guess :-)