Fix #1132 by passing segment IDs, not segment instances

Passing segment instances fails because this creates a race condition.
When a long conversion is taking place, the SignalBase::samples_added
signal is called often but since it's in a separate thread, the calls
are queued and aren't executed immediately. Now if the conversion is
restarted - for example as a result of a changed conversion threshold -
then the segments holding the converted data are destroyed, rendering
the pointers submitted as parameters to samples_added invalid.

Once the signal queue is processed, those invalid pointers will be
accessed and PV segfaults.

Since the signal queue can neither be emptied nor flushed, this
leaves only two sensible choices:
1) Signal samples_added less often, thereby reducing the chance of
signals being queued
2) Supply the segment ID instead of the segment instance as that's
essentially the only thing we currently care about - in fact, the
only user of samples_added (ViewBase::on_samples_added) uses the
instance to query only this

As #1 is only a band-aid and not a waterproof solution, I chose
to go with #2.

diff --git a/pv/data/signalbase.cpp b/pv/data/signalbase.cpp
index 5aec9ba884677e924cb8e0d1d533a62c4ef467b2..7746ab40156fb3e88c2f02ac101ac628a42574f4 100644 (file)
--- a/pv/data/signalbase.cpp
+++ b/pv/data/signalbase.cpp
@@ -482,8 +482,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l
                                        analog->get_logic_via_threshold(threshold, lsamples);
 
                                lsegment->append_payload(logic->data_pointer(), logic->data_length());
                                        analog->get_logic_via_threshold(threshold, lsamples);
 
                                lsegment->append_payload(logic->data_pointer(), logic->data_length());

-
-                               samples_added(lsegment, i, i + ConversionBlockSize);
+                               samples_added(lsegment->segment_id(), i, i + ConversionBlockSize);

                                i += ConversionBlockSize;
                        }
 
                                i += ConversionBlockSize;
                        }
 


@@ -497,7 +496,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l
                        shared_ptr<sigrok::Logic> logic =
                                analog->get_logic_via_threshold(threshold, lsamples);
                        lsegment->append_payload(logic->data_pointer(), logic->data_length());
                        shared_ptr<sigrok::Logic> logic =
                                analog->get_logic_via_threshold(threshold, lsamples);
                        lsegment->append_payload(logic->data_pointer(), logic->data_length());

-                       samples_added(lsegment, i, end_sample);
+                       samples_added(lsegment->segment_id(), i, end_sample);

                }
 
                if (conversion_type_ == A2LConversionBySchmittTrigger) {
                }
 
                if (conversion_type_ == A2LConversionBySchmittTrigger) {


@@ -516,8 +515,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l
                                                &state, lsamples);
 
                                lsegment->append_payload(logic->data_pointer(), logic->data_length());
                                                &state, lsamples);
 
                                lsegment->append_payload(logic->data_pointer(), logic->data_length());

-
-                               samples_added(lsegment, i, i + ConversionBlockSize);
+                               samples_added(lsegment->segment_id(), i, i + ConversionBlockSize);

                                i += ConversionBlockSize;
                        }
 
                                i += ConversionBlockSize;
                        }
 


@@ -532,7 +530,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l
                                analog->get_logic_via_schmitt_trigger(lo_thr, hi_thr,
                                        &state, lsamples);
                        lsegment->append_payload(logic->data_pointer(), logic->data_length());
                                analog->get_logic_via_schmitt_trigger(lo_thr, hi_thr,
                                        &state, lsamples);
                        lsegment->append_payload(logic->data_pointer(), logic->data_length());

-                       samples_added(lsegment, i, end_sample);
+                       samples_added(lsegment->segment_id(), i, end_sample);

                }
 
                // If acquisition is ongoing, start-/endsample may have changed
                }
 
                // If acquisition is ongoing, start-/endsample may have changed


@@ -660,7 +658,8 @@ void SignalBase::on_samples_added(QObject* segment, uint64_t start_sample,
                }
        }
 
                }
        }
 

-       samples_added(segment, start_sample, end_sample);
+       data::Segment* s = qobject_cast<data::Segment*>(segment);
+       samples_added(s->segment_id(), start_sample, end_sample);

 }
 
 void SignalBase::on_min_max_changed(float min, float max)
 }
 
 void SignalBase::on_min_max_changed(float min, float max)

diff --git a/pv/data/signalbase.hpp b/pv/data/signalbase.hpp
index 7375c1dbfb149b360962fee6f0a6e846752132fa..47c593fac7f364423d46e81697771d186c8ec7a0 100644 (file)
--- a/pv/data/signalbase.hpp
+++ b/pv/data/signalbase.hpp
@@ -297,7 +297,7 @@ Q_SIGNALS:
 
        void samples_cleared();
 
 
        void samples_cleared();
 

-       void samples_added(QObject* segment, uint64_t start_sample,
+       void samples_added(uint64_t segment_id, uint64_t start_sample,

                uint64_t end_sample);
 
        void min_max_changed(float min, float max);
                uint64_t end_sample);
 
        void min_max_changed(float min, float max);

diff --git a/pv/views/viewbase.cpp b/pv/views/viewbase.cpp
index caed41eecb56daf0d3b58b1102e90d091c1f7ecb..11ff7a273b6af3a2892fdfcf6c7287f2f99d12c6 100644 (file)
--- a/pv/views/viewbase.cpp
+++ b/pv/views/viewbase.cpp
@@ -80,8 +80,8 @@ void ViewBase::clear_signalbases()
        for (shared_ptr<data::SignalBase> signalbase : signalbases_) {
                disconnect(signalbase.get(), SIGNAL(samples_cleared()),
                        this, SLOT(on_data_updated()));
        for (shared_ptr<data::SignalBase> signalbase : signalbases_) {
                disconnect(signalbase.get(), SIGNAL(samples_cleared()),
                        this, SLOT(on_data_updated()));

-               disconnect(signalbase.get(), SIGNAL(samples_added(QObject*, uint64_t, uint64_t)),
-                       this, SLOT(on_samples_added(QObject*, uint64_t, uint64_t)));
+               disconnect(signalbase.get(), SIGNAL(samples_added(uint64_t, uint64_t, uint64_t)),
+                       this, SLOT(on_samples_added(uint64_t, uint64_t, uint64_t)));

        }
 
        signalbases_.clear();
        }
 
        signalbases_.clear();


@@ -93,8 +93,8 @@ void ViewBase::add_signalbase(const shared_ptr<data::SignalBase> signalbase)
 
        connect(signalbase.get(), SIGNAL(samples_cleared()),
                this, SLOT(on_data_updated()));
 
        connect(signalbase.get(), SIGNAL(samples_cleared()),
                this, SLOT(on_data_updated()));

-       connect(signalbase.get(), SIGNAL(samples_added(QObject*, uint64_t, uint64_t)),
-               this, SLOT(on_samples_added(QObject*, uint64_t, uint64_t)));
+       connect(signalbase.get(), SIGNAL(samples_added(uint64_t, uint64_t, uint64_t)),
+               this, SLOT(on_samples_added(uint64_t, uint64_t, uint64_t)));

 }
 
 #ifdef ENABLE_DECODE
 }
 
 #ifdef ENABLE_DECODE


@@ -152,15 +152,13 @@ void ViewBase::perform_delayed_view_update()
 {
 }
 
 {
 }
 

-void ViewBase::on_samples_added(QObject* segment, uint64_t start_sample,
+void ViewBase::on_samples_added(uint64_t segment_id, uint64_t start_sample,

        uint64_t end_sample)
 {
        (void)start_sample;
        (void)end_sample;
 
        uint64_t end_sample)
 {
        (void)start_sample;
        (void)end_sample;
 

-       data::Segment* s = qobject_cast<data::Segment*>(segment);
-
-       if (s->segment_id() != current_segment_)
+       if (segment_id != current_segment_)

                return;
 
        if (!delayed_view_updater_.isActive())
                return;
 
        if (!delayed_view_updater_.isActive())

diff --git a/pv/views/viewbase.hpp b/pv/views/viewbase.hpp
index 5d7d2adbb181a7cdaca7913404ea3ab3f07fa75e..923d47fb515c2b5d96111f3ec0f074b0e1b339d8 100644 (file)
--- a/pv/views/viewbase.hpp
+++ b/pv/views/viewbase.hpp
@@ -100,7 +100,7 @@ public Q_SLOTS:
        virtual void perform_delayed_view_update();
 
 private Q_SLOTS:
        virtual void perform_delayed_view_update();
 
 private Q_SLOTS:

-       void on_samples_added(QObject* segment, uint64_t start_sample,
+       void on_samples_added(uint64_t segment_id, uint64_t start_sample,

                uint64_t end_sample);
 
        void on_data_updated();
                uint64_t end_sample);
 
        void on_data_updated();