From: Soeren Apel Date: Mon, 5 Mar 2018 22:45:46 +0000 (+0100) Subject: Fix #1132 by passing segment IDs, not segment instances X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=7f894d958;p=pulseview.git 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 5aec9ba8..7746ab40 100644 --- 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()); - - samples_added(lsegment, i, i + ConversionBlockSize); + samples_added(lsegment->segment_id(), i, i + ConversionBlockSize); i += ConversionBlockSize; } @@ -497,7 +496,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l shared_ptr 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) { @@ -516,8 +515,7 @@ void SignalBase::convert_single_segment(AnalogSegment *asegment, LogicSegment *l &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; } @@ -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()); - samples_added(lsegment, i, end_sample); + samples_added(lsegment->segment_id(), i, end_sample); } // 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(segment); + samples_added(s->segment_id(), start_sample, end_sample); } void SignalBase::on_min_max_changed(float min, float max) diff --git a/pv/data/signalbase.hpp b/pv/data/signalbase.hpp index 7375c1db..47c593fa 100644 --- a/pv/data/signalbase.hpp +++ b/pv/data/signalbase.hpp @@ -297,7 +297,7 @@ Q_SIGNALS: 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); diff --git a/pv/views/viewbase.cpp b/pv/views/viewbase.cpp index caed41ee..11ff7a27 100644 --- a/pv/views/viewbase.cpp +++ b/pv/views/viewbase.cpp @@ -80,8 +80,8 @@ void ViewBase::clear_signalbases() for (shared_ptr 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(); @@ -93,8 +93,8 @@ void ViewBase::add_signalbase(const shared_ptr signalbase) 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 @@ -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; - data::Segment* s = qobject_cast(segment); - - if (s->segment_id() != current_segment_) + if (segment_id != current_segment_) return; if (!delayed_view_updater_.isActive()) diff --git a/pv/views/viewbase.hpp b/pv/views/viewbase.hpp index 5d7d2adb..923d47fb 100644 --- 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: - 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();