Segmentation Fault when closing ADS-B panel while Sampling Device is still running

[ChiefGokhlayeh]

I noticed a segmentation fault whenever closing the ADS-B Demodulator panel while having the sampling device still running.

Steps to reproduce:

1. Open sdrangel
2. Select "Sampling Device" TestSource
3. Add ADS-B Demodulator to Channels
4. Start TestSource
5. Close ADS-B Demodulator panel without stopping TestSource

If TestSource is stopped before closing ADS-B Demodulator everything continues to work fine.

I traced the issue down to an "use-after-delete" "use-after-half-delete" by DSPDeviceSourceEngine:

sdrangel/sdrbase/dsp/dspdevicesourceengine.cpp

Lines 595 to 597 in 3e9b4a4

	if(m_state == StRunning) {
	sink->stop();
	}

In this case sink is the already half deleted ADSBDemod object. The delete is called before that by DeviceUISet::handleChannelGUIClosing() on UI-thread:

sdrangel/sdrgui/device/deviceuiset.cpp

Line 439 in de23efe

it->m_channelAPI->destroy();

-- Tested on latest master following v6.13.0.

[ChiefGokhlayeh]

It appears this use-after-delete isn't unique to ADS-B Demod. I found the same fault-type in other plugins, such as Broadcast FM or AM Demodulator. sink->stop(); gets called after sink already got deleted.

EDIT: My initial assumption of a use-after-delete was wrong. In fact, sink->stop() is called during destruction of ADSBDemod, via:

sdrangel/plugins/channelrx/demodadsb/adsbdemod.cpp

Lines 89 to 90 in 3e9b4a4

	m_deviceAPI->removeChannelSinkAPI(this);
	m_deviceAPI->removeChannelSink(this);

So the issue simply boils down to a "wrong order of member destruction"-fault. m_workers, m_basebandSink and m_thread should be deleted last to resolve the segmentation fault.

I'll raise a PR.

[f4exb]

The issue is that ADSBDemod::stop is called several times. When the demod is closed via GUI (the "X" button) there is no other way to do this than through the ChannelGUI::closeEvent. However DSPDeviceSourceEngine::removeSink also calls ADSBDemod::stop which is legitimate as well. This is the sequence of events shown in the log right before the crash:

2021-06-05 06:43:25.054 (D) ChannelGUI::closeEvent                                                                                                                                                                                          
2021-06-05 06:43:25.054 (D) ADSBDemod::stop                                                                                                                                                                                                 
2021-06-05 06:43:25.054 (D) ADSBDemodSink::stopWorker: Stopping worker                                                                                                                                                                      
2021-06-05 06:43:25.061 (D) ADSBDemodSink::stopWorker: Worker stopped                                                                                                                                                                       
2021-06-05 06:43:25.062 (D) DSPDeviceSourceEngine::removeSink:  ADSBDemod                                                                                                                                                                   
2021-06-05 06:43:25.062 (D) DSPDeviceSourceEngine::handleSynchronousMessages:  DSPRemoveBasebandSampleSink                                                                                                                                  
2021-06-05 06:43:25.062 (D) ADSBDemod::stop                                                                                                                                                                                                 
2021-06-05 06:43:25.062 (W) QObject::disconnect: No such signal QObject::messageEnqueued()                                                                                                                                                  
Segmentation fault (core dumped)

This yields the last warning message that looks inconsistent because MessageQueue does have a MessageEnqueued() signal. But in this case this comes from the fact that this signal was already disconnected.

In fact ADSBDemodSink::stopWork should handle the condition whether it is running or not. Rewriting it like this solves the problem:

void ADSBDemodWorker::stopWork()
{
    QMutexLocker mutexLocker(&m_mutex);

    if (!m_running) {
        return;
    }

    m_heartbeatTimer.stop();
    disconnect(&m_inputMessageQueue, SIGNAL(messageEnqueued()), this, SLOT(handleInputMessages()));
    m_running = false;
}

[f4exb]

Note: m_basebandSink->stopWork is also called twice via ADSBDemod::stop which in turn calls ADSBDemodSink::stopWorker twice. However in this case the running condition is handled properly:

void ADSBDemodSink::stopWorker()
{
    if (m_worker.isRunning())
    {
        qDebug() << "ADSBDemodSink::stopWorker: Stopping worker";
        m_worker.requestInterruption();
        // Worker may be blocked waiting for a buffer
        for (int i = 0; i < m_buffers; i++)
        {
            if (m_bufferRead[i].available() == 0)
                m_bufferRead[i].release(1);
        }
        m_worker.wait();
        // If this is called from ADSBDemod, we need to also
        // make sure baseband sink thread isnt blocked in processOneSample
        for (int i = 0; i < m_buffers; i++)
        {
            if (m_bufferWrite[i].available() == 0)
                m_bufferWrite[i].release(1);
        }
        qDebug() << "ADSBDemodSink::stopWorker: Worker stopped";
    }
}