Crash when closing the FreeDV Demodulator window

[AsciiWolf]

Closing the ADS-B Demodulator window causes SDRangel (version 7.22.2 on Linux x86_64) to crash.

SDRangel compiled with Qt6:

malloc(): mismatching next->prev_size (unsorted)

SDRangel compiled with Qt5:

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

[AsciiWolf]

Same issue with the FreeDV Demodulator.

[srcejon]

Tried compiling with address sanitizer - running FreeDV on Linux exits with:

==3217==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x616000ab0c00 at pc 0x79351341715f bp 0x7934e69fef80 sp 0x7934e69fef70
READ of size 2 at 0x616000ab0c00 thread T53 (QThread)
#0 0x79351341715e in FreeDVDemodSink::pushSampleToDV(short) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodsink.cpp:298
#1 0x793513416b01 in FreeDVDemodSink::processOneSample(std::complex&) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodsink.cpp:262
#2 0x793513415e82 in FreeDVDemodSink::feed(__gnu_cxx::__normal_iterator<Sample const*, std::vector<Sample, std::allocator > > const&, __gnu_cxx::__normal_iterator<Sample const*, std::vector<Sample, std::allocator > > const&) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodsink.cpp:207
#3 0x79354e98fed8 in DownChannelizer::feed(__gnu_cxx::__normal_iterator<Sample const*, std::vector<Sample, std::allocator > > const&, __gnu_cxx::__normal_iterator<Sample const*, std::vector<Sample, std::allocator > > const&) /opt/build/sdrangel/sdrbase/dsp/downchannelizer.cpp:57
#4 0x793513410eac in FreeDVDemodBaseband::handleData() /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodbaseband.cpp:83
#5 0x7935134132f1 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (FreeDVDemodBaseband::)()>::call(void (FreeDVDemodBaseband::)(), FreeDVDemodBaseband*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:152
#6 0x7935134130ec in void QtPrivate::FunctionPointer<void (FreeDVDemodBaseband::)()>::call<QtPrivate::List<>, void>(void (FreeDVDemodBaseband::)(), FreeDVDemodBaseband*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:185
#7 0x793513412f8b in QtPrivate::QSlotObject<void (FreeDVDemodBaseband::)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase, QObject*, void**, bool*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:418
#8 0x79354b6e741d in QObject::event(QEvent*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e741d)
#9 0x79354c36c712 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x16c712)
#10 0x79354b6b9e39 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b9e39)
#11 0x79354b6bcf26 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2bcf26)
#12 0x79354b713a66 (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x313a66)
#13 0x79354a520d3a in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x55d3a)
#14 0x79354a5762b7 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0xab2b7)
#15 0x79354a51e3e2 in g_main_context_iteration (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x533e2)
#16 0x79354b7130b7 in QEventDispatcherGlib::processEvents(QFlagsQEventLoop::ProcessEventsFlag) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x3130b7)
#17 0x79354b6b875a in QEventLoop::exec(QFlagsQEventLoop::ProcessEventsFlag) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b875a)
#18 0x79354b4cbaf1 in QThread::exec() (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xcbaf1)
#19 0x79354b4ccca0 (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xccca0)
#20 0x79354ac94ac2 in start_thread nptl/pthread_create.c:442
#21 0x79354ad2684f (/lib/x86_64-linux-gnu/libc.so.6+0x12684f)

0x616000ab0c00 is located 0 bytes to the right of 640-byte region [0x616000ab0980,0x616000ab0c00)
allocated by thread T53 (QThread) here:
#0 0x7935518b6357 in operator new[](unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:102
#1 0x793513419081 in FreeDVDemodSink::applyFreeDVMode(FreeDVDemodSettings::FreeDVMode) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodsink.cpp:464
#2 0x7935134121d0 in FreeDVDemodBaseband::applySettings(FreeDVDemodSettings const&, bool) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodbaseband.cpp:187
#3 0x793513411689 in FreeDVDemodBaseband::handleMessage(Message const&) /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodbaseband.cpp:120
#4 0x7935134113c6 in FreeDVDemodBaseband::handleInputMessages() /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodbaseband.cpp:106
#5 0x7935133f70fb in FreeDVDemodBaseband::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /opt/build/sdrangel/build/plugins/channelrx/demodfreedv/demodfreedv_autogen/EWIEGA46WW/moc_freedvdemodbaseband.cpp:88
#6 0x79354b6e741d in QObject::event(QEvent*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e741d)
#7 0x79354c36c712 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x16c712)

Thread T53 (QThread) created by T49 (QThread) here:
#0 0x793551858685 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:216
#1 0x79354b4cc752 in QThread::start(QThread::Priority) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xcc752)

Thread T49 (QThread) created by T0 here:
#0 0x793551858685 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:216
#1 0x79354b4cc752 in QThread::start(QThread::Priority) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xcc752)

SUMMARY: AddressSanitizer: heap-buffer-overflow /opt/build/sdrangel/plugins/channelrx/demodfreedv/freedvdemodsink.cpp:298 in FreeDVDemodSink::pushSampleToDV(short)
Shadow bytes around the buggy address:

Seems to relate to the following line of code:

 int nout = freedv_rx(m_freeDV, m_speechOut, m_modIn);
 ...
 for (int i = 0; i < nout; i++)
     while (!m_audioResampler.upSample(m_speechOut[i], audioSample)) {

Index i extends beyond size of m_speechOut. Size of m_speechOut is calculated from:

int nSpeechSamples = freedv_get_n_speech_samples(m_freeDV);

I think this should probably be:

int nSpeechSamples = freedv_get_n_max_speech_samples(m_freeDV);

[srcejon]

That doesn't appear to be causing the crash though. I do see in the console:

QThread: Destroyed while thread is still running

Probably just need to call stop()

[srcejon]

Calling stop in FreeDVDemod::~FreeDVDemod fixes QThread: Destroyed while thread is still running

Next problem is FreeDVDemod::stop is called after FreeDVDemod::~FreeDVDemod (i.e. after it's been deleted) from DSPDeviceSourceEngine::handleMessage (presumably handling a DSPRemoveBasebandSampleSink)

[srcejon]

I think the problem relates to the changes for #2159 and only happens when closing a demod window while device is running. It can probably happen to more than just the FreeDV demod, as it's a generic race condition.

In the destructor for demods, we have:

     m_deviceAPI->removeChannelSink(this);

This calls DSPDeviceSourceEngine::removeSink(), which sends a DSPRemoveBasebandSampleSink message to itself, which has:

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

Resulting in the demods::stop() method being called.

Before the changes for #2159, DSPDeviceSourceEngine::removeSink() blocked while the message was handled, before returning to the demod's destructor. Now that isn't the case, it's possible that the call to sink->stop() occurs after the destructor has finished. (So we have a call to a method of a deleted object).

We don't want to add blocking back in, so I think we probably want to prevent the DSPRemoveBasebandSampleSink handler from calling sink->stop(), when it has been called from a destructor.

Is it even necessary to have that call to stop at all? Most of the destructors seem to call stop themselves. The other case where removeSink() is called is when removing device sets, and in that case stopAcquisition() should have been called first. Any thoughts @f4exb?

[f4exb]

Is it even necessary to have that call to stop at all? Most of the destructors seem to call stop themselves.

Yes I suppose this is superfluous and may come from an old design.

[srcejon]

Was having a look for other paths that may call it, and there is another, from DeviceAPI::renumerateChannels().

However, it does actually appear to be problematic for that anyway. If we try to move a Channel between two Device Sets that are both running, then stopping the channel appears to cause SDRangel to get in to a bad state that can't be recovered from. Not stopping a channel that moves to a Device Set that isn't running doesn't seem to be a problem. But doesn't seem correct...

[f4exb]

If this is too problematic we have to remove the feature of moving channels between device sets.

[srcejon]

Should be fixed in 7.22.5