From: Soeren Apel Date: Thu, 3 Sep 2020 19:05:17 +0000 (+0200) Subject: Fix #1596 by making memory management more robust X-Git-Url: https://sigrok.org/gitweb/?a=commitdiff_plain;h=cf1541a18fcd007c9965a3199b9c4f917856b292;hp=d1125d7d9e5830bc1d17636988e6c72f9deaeaf3;p=pulseview.git Fix #1596 by making memory management more robust 1) Fixed use of raw pointers to shared_ptr-managed instances 2) Fixed bug due to newly-introduced shared_from_this 3) More nullptr checks 4) Add muxer thread interrupting --- diff --git a/main.cpp b/main.cpp index b0e631f6..a307159c 100644 --- a/main.cpp +++ b/main.cpp @@ -278,6 +278,7 @@ int main(int argc, char *argv[]) qRegisterMetaType("uint64_t"); qRegisterMetaType("util::Timestamp"); qRegisterMetaType("SharedPtrToSegment"); + qRegisterMetaType>("shared_ptr"); // Prepare the global settings since logging needs them early on pv::GlobalSettings settings; diff --git a/pv/data/decode/decoder.hpp b/pv/data/decode/decoder.hpp index 6e11e83f..86a371f2 100644 --- a/pv/data/decode/decoder.hpp +++ b/pv/data/decode/decoder.hpp @@ -82,7 +82,7 @@ struct DecodeChannel uint16_t id; ///< Global numerical ID for the decode channels in the stack uint16_t bit_id; ///< Tells which bit within a sample represents this channel const bool is_optional; - const pv::data::SignalBase *assigned_signal; + shared_ptr assigned_signal; const QString name, desc; int initial_pin_state; const shared_ptr decoder_; diff --git a/pv/data/decodesignal.cpp b/pv/data/decodesignal.cpp index b6166a4c..12077a96 100644 --- a/pv/data/decodesignal.cpp +++ b/pv/data/decodesignal.cpp @@ -309,7 +309,7 @@ void DecodeSignal::auto_assign_signals(const shared_ptr dec) } if (match) { - ch.assigned_signal = match.get(); + ch.assigned_signal = match; new_assignment = true; } } @@ -322,7 +322,7 @@ void DecodeSignal::auto_assign_signals(const shared_ptr dec) } } -void DecodeSignal::assign_signal(const uint16_t channel_id, const SignalBase *signal) +void DecodeSignal::assign_signal(const uint16_t channel_id, shared_ptr signal) { for (decode::DecodeChannel& ch : channels_) if (ch.id == channel_id) { @@ -340,7 +340,7 @@ int DecodeSignal::get_assigned_signal_count() const { // Count all channels that have a signal assigned to them return count_if(channels_.begin(), channels_.end(), - [](decode::DecodeChannel ch) { return ch.assigned_signal; }); + [](decode::DecodeChannel ch) { return ch.assigned_signal.get(); }); } void DecodeSignal::set_initial_pin_state(const uint16_t channel_id, const int init_state) @@ -399,8 +399,9 @@ int64_t DecodeSignal::get_working_sample_count(uint32_t segment_id) const if (segment_id >= logic_data->logic_segments().size()) return 0; - const shared_ptr segment = logic_data->logic_segments()[segment_id]; - count = min(count, (int64_t)segment->get_sample_count()); + const shared_ptr segment = logic_data->logic_segments()[segment_id]->get_shared_ptr(); + if (segment) + count = min(count, (int64_t)segment->get_sample_count()); } return (no_signals_assigned ? 0 : count); @@ -807,7 +808,7 @@ void DecodeSignal::restore_settings(QSettings &settings) for (const shared_ptr& signal : signalbases) if ((signal->name() == assigned_signal_name) && (signal->type() != SignalBase::DecodeChannel)) - channel->assigned_signal = signal.get(); + channel->assigned_signal = signal; channel->initial_pin_state = settings.value("initial_pin_state").toInt(); @@ -838,8 +839,8 @@ bool DecodeSignal::all_input_segments_complete(uint32_t segment_id) const if (segment_id >= logic_data->logic_segments().size()) return false; - const shared_ptr segment = logic_data->logic_segments()[segment_id]; - if (!segment->is_complete()) + const shared_ptr segment = logic_data->logic_segments()[segment_id]->get_shared_ptr(); + if (segment && !segment->is_complete()) all_complete = false; } @@ -878,8 +879,10 @@ double DecodeSignal::get_input_samplerate(uint32_t segment_id) const continue; try { - const shared_ptr segment = logic_data->logic_segments().at(segment_id); - samplerate = segment->samplerate(); + const shared_ptr segment = + logic_data->logic_segments().at(segment_id)->get_shared_ptr(); + if (segment) + samplerate = segment->samplerate(); } catch (out_of_range&) { // Do nothing } @@ -1006,7 +1009,8 @@ void DecodeSignal::mux_logic_samples(uint32_t segment_id, const int64_t start, c return; // Fetch the channel segments and their data - vector > segments; + bool segment_missing = false; + vector > segments; vector signal_data; vector signal_in_bytepos; vector signal_in_bitpos; @@ -1015,14 +1019,21 @@ void DecodeSignal::mux_logic_samples(uint32_t segment_id, const int64_t start, c if (ch.assigned_signal) { const shared_ptr logic_data = ch.assigned_signal->logic_data(); - shared_ptr segment; + shared_ptr segment; try { - segment = logic_data->logic_segments().at(segment_id); + segment = logic_data->logic_segments().at(segment_id)->get_shared_ptr(); } catch (out_of_range&) { qDebug() << "Muxer error for" << name() << ":" << ch.assigned_signal->name() \ << "has no logic segment" << segment_id; + logic_mux_interrupt_ = true; return; } + + if (!segment) { + segment_missing = true; + break; + } + segments.push_back(segment); uint8_t* data = new uint8_t[(end - start) * segment->unit_size()]; @@ -1034,6 +1045,8 @@ void DecodeSignal::mux_logic_samples(uint32_t segment_id, const int64_t start, c signal_in_bitpos.push_back(bitpos % 8); } + if (segment_missing) + return; shared_ptr output_segment; try { @@ -1042,6 +1055,7 @@ void DecodeSignal::mux_logic_samples(uint32_t segment_id, const int64_t start, c qDebug() << "Muxer error for" << name() << ": no logic mux segment" \ << segment_id << "in mux_logic_samples(), mux segments size is" \ << logic_mux_data_->logic_segments().size(); + logic_mux_interrupt_ = true; return; } @@ -1104,8 +1118,7 @@ void DecodeSignal::logic_mux_proc() // Create initial logic mux segment shared_ptr output_segment = - make_shared(*logic_mux_data_, segment_id, - logic_mux_unit_size_, 0); + make_shared(*logic_mux_data_, segment_id, logic_mux_unit_size_, 0); logic_mux_data_->push_segment(output_segment); output_segment->set_samplerate(get_input_samplerate(0)); @@ -1172,7 +1185,7 @@ void DecodeSignal::logic_mux_proc() void DecodeSignal::decode_data( const int64_t abs_start_samplenum, const int64_t sample_count, - const shared_ptr input_segment) + const shared_ptr input_segment) { const int64_t unit_size = input_segment->unit_size(); const int64_t chunk_sample_count = DecodeChunkLength / unit_size; @@ -1236,8 +1249,9 @@ void DecodeSignal::decode_proc() if (decode_interrupt_) return; - shared_ptr input_segment = logic_mux_data_->logic_segments().front(); - assert(input_segment); + shared_ptr input_segment = logic_mux_data_->logic_segments().front()->get_shared_ptr(); + if (!input_segment) + return; // Create the initial segment and set its sample rate so that we can pass it to SRD create_decode_segment(); @@ -1418,7 +1432,7 @@ void DecodeSignal::connect_input_notifiers() if (!ch.assigned_signal) continue; - const data::SignalBase *signal = ch.assigned_signal; + const data::SignalBase *signal = ch.assigned_signal.get(); connect(signal, &data::SignalBase::samples_cleared, this, &DecodeSignal::on_data_cleared); connect(signal, &data::SignalBase::samples_added, diff --git a/pv/data/decodesignal.hpp b/pv/data/decodesignal.hpp index c9ebb2a6..fad3db78 100644 --- a/pv/data/decodesignal.hpp +++ b/pv/data/decodesignal.hpp @@ -117,7 +117,7 @@ public: const vector get_channels() const; void auto_assign_signals(const shared_ptr dec); - void assign_signal(const uint16_t channel_id, const SignalBase *signal); + void assign_signal(const uint16_t channel_id, shared_ptr signal); int get_assigned_signal_count() const; void set_initial_pin_state(const uint16_t channel_id, const int init_state); @@ -201,7 +201,7 @@ private: void logic_mux_proc(); void decode_data(const int64_t abs_start_samplenum, const int64_t sample_count, - const shared_ptr input_segment); + const shared_ptr input_segment); void decode_proc(); void start_srd_session(); diff --git a/pv/data/logicsegment.cpp b/pv/data/logicsegment.cpp index 22f1d38a..be56c150 100644 --- a/pv/data/logicsegment.cpp +++ b/pv/data/logicsegment.cpp @@ -63,10 +63,24 @@ LogicSegment::LogicSegment(pv::data::Logic& owner, uint32_t segment_id, LogicSegment::~LogicSegment() { lock_guard lock(mutex_); + for (MipMapLevel &l : mip_map_) free(l.data); } +shared_ptr LogicSegment::get_shared_ptr() const +{ + shared_ptr ptr = nullptr; + + try { + ptr = shared_from_this(); + } catch (std::exception& e) { + /* Do nothing, ptr remains a null pointer */ + } + + return ptr ? std::dynamic_pointer_cast(ptr) : nullptr; +} + template void LogicSegment::downsampleTmain(const T*&in, T &acc, T &prev) { diff --git a/pv/data/logicsegment.hpp b/pv/data/logicsegment.hpp index 1f151eeb..2e37ed2d 100644 --- a/pv/data/logicsegment.hpp +++ b/pv/data/logicsegment.hpp @@ -75,6 +75,14 @@ public: virtual ~LogicSegment(); + /** + * Using enable_shared_from_this prevents the normal use of shared_ptr + * instances by users of LogicSegment instances. Instead, shared_ptrs may + * only be created by the instance itself. + * See https://en.cppreference.com/w/cpp/memory/enable_shared_from_this + */ + shared_ptr get_shared_ptr() const; + void append_payload(shared_ptr logic); void append_payload(void *data, uint64_t data_size); diff --git a/pv/data/segment.cpp b/pv/data/segment.cpp index 18aabedb..4533c9b4 100644 --- a/pv/data/segment.cpp +++ b/pv/data/segment.cpp @@ -46,7 +46,6 @@ Segment::Segment(uint32_t segment_id, uint64_t samplerate, unsigned int unit_siz mem_optimization_requested_(false), is_complete_(false) { - lock_guard lock(mutex_); assert(unit_size_ > 0); // Determine the number of samples we can fit in one chunk diff --git a/pv/data/signalbase.cpp b/pv/data/signalbase.cpp index 94e1f6a6..578d908f 100644 --- a/pv/data/signalbase.cpp +++ b/pv/data/signalbase.cpp @@ -304,11 +304,17 @@ void SignalBase::clear_sample_data() shared_ptr SignalBase::analog_data() const { + if (!data_) + return nullptr; + return dynamic_pointer_cast(data_); } shared_ptr SignalBase::logic_data() const { + if (!data_) + return nullptr; + shared_ptr result = dynamic_pointer_cast(data_); if (((conversion_type_ == A2LConversionByThreshold) || diff --git a/pv/data/signalbase.hpp b/pv/data/signalbase.hpp index a55b3fc2..ee927aae 100644 --- a/pv/data/signalbase.hpp +++ b/pv/data/signalbase.hpp @@ -422,4 +422,6 @@ protected: } // namespace data } // namespace pv +Q_DECLARE_METATYPE(shared_ptr); + #endif // PULSEVIEW_PV_DATA_SIGNALBASE_HPP diff --git a/pv/views/trace/decodetrace.cpp b/pv/views/trace/decodetrace.cpp index 2902c936..5b9b3f36 100644 --- a/pv/views/trace/decodetrace.cpp +++ b/pv/views/trace/decodetrace.cpp @@ -1131,10 +1131,9 @@ QComboBox* DecodeTrace::create_channel_selector(QWidget *parent, const DecodeCha for (const shared_ptr &b : sig_list) { assert(b); if (b->logic_data() && b->enabled()) { - selector->addItem(b->name(), - QVariant::fromValue((void*)b.get())); + selector->addItem(b->name(), QVariant::fromValue(b)); - if (ch->assigned_signal == b.get()) + if (ch->assigned_signal == b) selector->setCurrentIndex(selector->count() - 1); } } @@ -1523,8 +1522,8 @@ void DecodeTrace::on_channel_selected(int) QComboBox *cb = qobject_cast(QObject::sender()); // Determine signal that was selected - const data::SignalBase *signal = - (data::SignalBase*)cb->itemData(cb->currentIndex()).value(); + shared_ptr signal = + cb->itemData(cb->currentIndex()).value>(); // Determine decode channel ID this combo box is the channel selector for const uint16_t id = channel_id_map_.at(cb);