Application Crashes when scrolling through Annotation Markers with the mouse on PC version

[abelouso]

Version: 7.19.0
Steps to reproduce:
-- Create several annotation markers (or load from CSV).
-- Select show annotations from the pull down
-- Point mouse pointer to the wheel to switch through markers, Scroll up and down, the index hits -1 and the application crashes

GDB output of the crash:

hread 1 "sdrangel" received signal SIGSEGV, Segmentation fault.

0x00007ffff7e64520 in SpectrumMarkersDialog::displayAnnotationMarker (this=0x55555811c550) at sdrangel/sdrgui/gui/spectrummarkersdialog.cpp:219

219	        qint64 frequency = m_annotationMarkers[m_annotationMarkerIndex].m_startFrequency +

(gdb) bt

#0  0x00007ffff7e64520 in SpectrumMarkersDialog::displayAnnotationMarker() (this=0x55555811c550) at sdrangel/sdrgui/gui/spectrummarkersdialog.cpp:219

#1  0x00007ffff7d952c3 in SpectrumMarkersDialog::qt_metacall(QMetaObject::Call, int, void**) (this=0x55555811c550, _c=QMetaObject::InvokeMetaMethod, _id=37, _a=0x7fffffffcf10)

    at sdrangel/bulid/sdrgui/sdrgui_autogen/DMHXEJ42XS/moc_spectrummarkersdialog.cpp:362

#2  0x00007ffff61304e5 in  () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#3  0x00007ffff6ce3aa2 in QAbstractSlider::valueChanged(int) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#4  0x00007ffff6ce422a in QAbstractSlider::setValue(int) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#5  0x00007ffff6ce4d25 in  () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#6  0x00007ffff6ce4ea2 in QAbstractSlider::wheelEvent(QWheelEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#7  0x00007ffff6c2d4ee in QWidget::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#8  0x00007ffff6bea713 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#9  0x00007ffff6bf2833 in QApplication::notify(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#10 0x00007ffff60f8e3a in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#11 0x00007ffff6c487d2 in  () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#12 0x00007ffff6c4a1ba in  () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#13 0x00007ffff6bea713 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5

#14 0x00007ffff60f8e3a in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#15 0x00007ffff64dd117 in QGuiApplicationPrivate::processWheelEvent(QWindowSystemInterfacePrivate::WheelEvent*) () at /lib/x86_64-linux-gnu/libQt5Gui.so.5

#16 0x00007ffff64b6a2c in QWindowSystemInterface::sendWindowSystemEvents(QFlags) () at /lib/x86_64-linux-gnu/libQt5Gui.so.5

#17 0x00007ffff14ecd6e in  () at /lib/x86_64-linux-gnu/libQt5XcbQpa.so.5

#18 0x00007ffff43a4d3b in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0

#19 0x00007ffff43fa258 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0

#20 0x00007ffff43a23e3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0

#21 0x00007ffff61520b8 in QEventDispatcherGlib::processEvents(QFlags) () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#22 0x00007ffff60f775b in QEventLoop::exec(QFlags) () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#23 0x00007ffff60ffcf4 in QCoreApplication::exec() () at /lib/x86_64-linux-gnu/libQt5Core.so.5

#24 0x0000555555557425 in runQtApplication(int, char**, qtwebapp::LoggerWithFile*) (argc=, argv=, logger=0x555555800710) at sdrangel/app/main.cpp:211

#25 0x000055555555691b in main(int, char**) (argc=1, argv=0x7fffffffdd28) at sdrangel/app/main.cpp:248

Suggested fix (git patch format):

 git diff --patch
diff --git a/sdrgui/gui/spectrummarkersdialog.cpp b/sdrgui/gui/spectrummarkersdialog.cpp
index f2e905e06..a31a826e1 100644
--- a/sdrgui/gui/spectrummarkersdialog.cpp
+++ b/sdrgui/gui/spectrummarkersdialog.cpp
@@ -608,6 +608,7 @@ void SpectrumMarkersDialog::on_aMakerDuplicate_clicked()
     m_annotationMarkers.push_back(m_annotationMarkers[m_annotationMarkerIndex]);
     ui->aMarker->setMaximum(m_annotationMarkers.size() - 1);
     m_annotationMarkerIndex = m_annotationMarkers.size() - 1;
+    if (m_annotationMarkerIndex < 0) m_annotationMarkerIndex = 0;
     displayAnnotationMarker();
     emit updateAnnotations();
 }
@@ -682,6 +683,10 @@ void SpectrumMarkersDialog::on_aMarker_valueChanged(int value)
     if (m_annotationMarkers.size() == 0) {
         return;
     }
+    if (value< 0 || value>= m_annotationMarkers.size())
+    {
+        return;
+    }
 
     m_annotationMarkerIndex = value;
     displayAnnotationMarker();
@@ -692,6 +697,7 @@ void SpectrumMarkersDialog::on_aMarkerAdd_clicked()
     m_annotationMarkers.append(SpectrumAnnotationMarker());
     m_annotationMarkers.back().m_startFrequency = m_centerFrequency;
     m_annotationMarkerIndex = m_annotationMarkers.size() - 1;
+    if (m_annotationMarkerIndex < 0) m_annotationMarkerIndex = 0;
     ui->aMarker->setMaximum(m_annotationMarkers.size() - 1);
     ui->aMarker->setMinimum(0);
     displayAnnotationMarker();
@@ -707,6 +713,8 @@ void SpectrumMarkersDialog::on_aMarkerDel_clicked()
     m_annotationMarkers.removeAt(m_annotationMarkerIndex);
     m_annotationMarkerIndex = m_annotationMarkerIndex < m_annotationMarkers.size() ?
         m_annotationMarkerIndex : m_annotationMarkerIndex - 1;
+        
+    if (m_annotationMarkerIndex < 0) m_annotationMarkerIndex = 0;
     ui->aMarker->setMaximum(m_annotationMarkers.size() - 1);
     displayAnnotationMarker();
     emit updateAnnotations();

[srcejon]

Can't reproduce on Windows or Ubuntu 22.04.

Don't know why the widget would signal a value of -1 anyway - in the code pasted, you can see ui->aMarker->setMinimum(0); an the .ui file has a minimum of 0 as well.

Perhaps a sign of something else going wrong?

[abelouso]

Before I fixed it in my repo, I could reliably duplicate the issue by scrolling the wheel down until the value goes negative In general, independently of whether the crash is reproducible, the method

void SpectrumMarkersDialog::on_aMarker_valueChanged(int value)

must check for the value to be valid before assigning it to the class' member variable, independently of what checks may or may not exist upstream of the call.

[srcejon]

in general ... must check for the value to be valid before assigning it to the class' member variable, independently of what checks may or may not exist upstream of the call.

Must it?

If Qt guarantees that the slots are only called with valid values, then it's a waste of CPU/typing to check it again.

If Qt doesn't guarantee it, then there's probably a lot of other places that need to be changed as well.

Looks like the Qt code should apply the bounds before signalling:

https://codebrowser.dev/qt5/qtbase/src/widgets/widgets/qabstractslider.cpp.html#_ZN15QAbstractSlider8setValueEi

[abelouso]

It should, according to any good software engineering practices. Any inputs of a public method should be checked for validity, if possible. It must in my field of software engineering, otherwise bad things could happen. You may not believe me that I could replicate a crash, but I could. Otherwise I would not be wasting anybody's time reporting it. Since it did crash for me (repeatedly and consistently, enough for me to find and fix the issue), and gdb reported that the value of m_annotationMarkerIndex was -1, somehow it got assigned that. It's possible, the method was called directly (outside of Qt's signal-slot mechanism) or the mouse wheel interface combined with custom circular spin element does not properly setup boundaries, the crush does occur and to fix it, is to guard the values of m_annotationMarkerIndex each time it is being set.

Would you like me to provide the exact call trace of -1 being set to m_annotationMarkerIndex. I was hoping to avoid that work as it would distract me from my main goal of figuring out a signal. I assumed, I provided enough information for a quick and easy fix that would improve the overall quality of this marvelous app.

[srcejon]

I believe it crashed, I just want to understand why you're getting -1, so we know if that is indeed the most appropriate fix and where else it might need to be applied.

Yes, a call trace would help, thanks.

[abelouso]

I rolled back my changes and attempted to duplicate the issue to get the stack trace. I was unable to do so. I will continue working with markers to see if I can identify the sequence that causes the crash.

[github-actions]

This issue is going to be closed due to inactivity