From 34ab5ff7300b7cd5ffc0895dc21e2172e70c3540 Mon Sep 17 00:00:00 2001 From: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com> Date: Sun, 18 Feb 2024 15:56:45 +0100 Subject: [PATCH] Fixes #6626: Throw if Lv2 object CTORs fail (#6951) On plugin instantiation failure, `Lv2Proc::m_valid` was being set to false. However, `Lv2Proc::run` did not evaluate `m_valid` and still called `lilv_instance_run`, which caused undefined behavior, including crashes. This bug fixes this by not even create such zombie classes, and instead `throw`s right away. The throws are caught in `lmms_plugin_main`, as suggested in the PR discussion and as the VST3 approach. --- include/Lv2ControlBase.h | 4 --- include/Lv2Proc.h | 4 --- plugins/Lv2Effect/Lv2Effect.cpp | 10 +++++--- plugins/Lv2Effect/Lv2Effect.h | 2 -- plugins/Lv2Effect/Lv2FxControls.cpp | 7 ++---- plugins/Lv2Instrument/Lv2Instrument.cpp | 33 +++++++++++-------------- plugins/Lv2Instrument/Lv2Instrument.h | 2 -- src/core/lv2/Lv2ControlBase.cpp | 28 ++++++--------------- src/core/lv2/Lv2Proc.cpp | 16 +++++------- 9 files changed, 37 insertions(+), 69 deletions(-) diff --git a/include/Lv2ControlBase.h b/include/Lv2ControlBase.h index 2d44f0ecf..9bfb40f87 100644 --- a/include/Lv2ControlBase.h +++ b/include/Lv2ControlBase.h @@ -102,9 +102,6 @@ protected: Lv2ControlBase& operator=(const Lv2ControlBase&) = delete; - //! Must be checked after ctor or reload - bool isValid() const { return m_valid; } - /* overrides */ @@ -149,7 +146,6 @@ private: //! fulfill LMMS' requirement of having stereo input and output std::vector> m_procs; - bool m_valid = true; bool m_hasGUI = false; unsigned m_channelsPerProc; diff --git a/include/Lv2Proc.h b/include/Lv2Proc.h index 1259aeede..65bd90698 100644 --- a/include/Lv2Proc.h +++ b/include/Lv2Proc.h @@ -78,8 +78,6 @@ public: ~Lv2Proc() override; void reload(); void onSampleRateChanged(); - //! Must be checked after ctor or reload - bool isValid() const { return m_valid; } /* port access @@ -173,8 +171,6 @@ protected: void shutdownPlugin(); private: - bool m_valid = true; - const LilvPlugin* m_plugin; LilvInstance* m_instance = nullptr; Lv2Features m_features; diff --git a/plugins/Lv2Effect/Lv2Effect.cpp b/plugins/Lv2Effect/Lv2Effect.cpp index 9c21b3f2a..eef6305cc 100644 --- a/plugins/Lv2Effect/Lv2Effect.cpp +++ b/plugins/Lv2Effect/Lv2Effect.cpp @@ -24,6 +24,7 @@ #include "Lv2Effect.h" +#include #include "Lv2SubPluginFeatures.h" @@ -109,9 +110,12 @@ extern "C" PLUGIN_EXPORT Plugin *lmms_plugin_main(Model *_parent, void *_data) { using KeyType = Plugin::Descriptor::SubPluginFeatures::Key; - auto eff = new Lv2Effect(_parent, static_cast(_data)); - if (!eff->isValid()) { delete eff; eff = nullptr; } - return eff; + try { + return new Lv2Effect(_parent, static_cast(_data)); + } catch (const std::runtime_error& e) { + qCritical() << e.what(); + return nullptr; + } } } diff --git a/plugins/Lv2Effect/Lv2Effect.h b/plugins/Lv2Effect/Lv2Effect.h index 3bcded355..a28182132 100644 --- a/plugins/Lv2Effect/Lv2Effect.h +++ b/plugins/Lv2Effect/Lv2Effect.h @@ -41,8 +41,6 @@ public: initialization */ Lv2Effect(Model* parent, const Descriptor::SubPluginFeatures::Key* _key); - //! Must be checked after ctor or reload - bool isValid() const { return m_controls.isValid(); } bool processAudioBuffer( sampleFrame* buf, const fpp_t frames ) override; EffectControls* controls() override { return &m_controls; } diff --git a/plugins/Lv2Effect/Lv2FxControls.cpp b/plugins/Lv2Effect/Lv2FxControls.cpp index 3ec7dbe23..72c387ba7 100644 --- a/plugins/Lv2Effect/Lv2FxControls.cpp +++ b/plugins/Lv2Effect/Lv2FxControls.cpp @@ -38,11 +38,8 @@ Lv2FxControls::Lv2FxControls(class Lv2Effect *effect, const QString& uri) : EffectControls(effect), Lv2ControlBase(this, uri) { - if (isValid()) - { - connect(Engine::audioEngine(), &AudioEngine::sampleRateChanged, - this, [this](){onSampleRateChanged();}); - } + connect(Engine::audioEngine(), &AudioEngine::sampleRateChanged, + this, &Lv2FxControls::onSampleRateChanged); } diff --git a/plugins/Lv2Instrument/Lv2Instrument.cpp b/plugins/Lv2Instrument/Lv2Instrument.cpp index 841b8a89a..316829327 100644 --- a/plugins/Lv2Instrument/Lv2Instrument.cpp +++ b/plugins/Lv2Instrument/Lv2Instrument.cpp @@ -76,19 +76,16 @@ Lv2Instrument::Lv2Instrument(InstrumentTrack *instrumentTrackArg, Instrument(instrumentTrackArg, &lv2instrument_plugin_descriptor, key), Lv2ControlBase(this, key->attributes["uri"]) { - if (Lv2ControlBase::isValid()) - { - clearRunningNotes(); + clearRunningNotes(); - connect(instrumentTrack()->pitchRangeModel(), SIGNAL(dataChanged()), - this, SLOT(updatePitchRange()), Qt::DirectConnection); - connect(Engine::audioEngine(), &AudioEngine::sampleRateChanged, - this, [this](){onSampleRateChanged();}); + connect(instrumentTrack()->pitchRangeModel(), SIGNAL(dataChanged()), + this, SLOT(updatePitchRange()), Qt::DirectConnection); + connect(Engine::audioEngine(), &AudioEngine::sampleRateChanged, + this, &Lv2Instrument::onSampleRateChanged); - // now we need a play-handle which cares for calling play() - auto iph = new InstrumentPlayHandle(this, instrumentTrackArg); - Engine::audioEngine()->addPlayHandle(iph); - } + // now we need a play-handle which cares for calling play() + auto iph = new InstrumentPlayHandle(this, instrumentTrackArg); + Engine::audioEngine()->addPlayHandle(iph); } @@ -134,11 +131,6 @@ void Lv2Instrument::onSampleRateChanged() -bool Lv2Instrument::isValid() const { return Lv2ControlBase::isValid(); } - - - - void Lv2Instrument::saveSettings(QDomDocument &doc, QDomElement &that) { Lv2ControlBase::saveSettings(doc, that); @@ -321,9 +313,12 @@ extern "C" PLUGIN_EXPORT Plugin *lmms_plugin_main(Model *_parent, void *_data) { using KeyType = Plugin::Descriptor::SubPluginFeatures::Key; - auto ins = new Lv2Instrument(static_cast(_parent), static_cast(_data)); - if (!ins->isValid()) { delete ins; ins = nullptr; } - return ins; + try { + return new Lv2Instrument(static_cast(_parent), static_cast(_data)); + } catch (const std::runtime_error& e) { + qCritical() << e.what(); + return nullptr; + } } } diff --git a/plugins/Lv2Instrument/Lv2Instrument.h b/plugins/Lv2Instrument/Lv2Instrument.h index 5e255e0df..de41dc958 100644 --- a/plugins/Lv2Instrument/Lv2Instrument.h +++ b/plugins/Lv2Instrument/Lv2Instrument.h @@ -61,8 +61,6 @@ public: ~Lv2Instrument() override; void reload(); void onSampleRateChanged(); - //! Must be checked after ctor or reload - bool isValid() const; /* load/save diff --git a/src/core/lv2/Lv2ControlBase.cpp b/src/core/lv2/Lv2ControlBase.cpp index 64cdc51fd..5741866e9 100644 --- a/src/core/lv2/Lv2ControlBase.cpp +++ b/src/core/lv2/Lv2ControlBase.cpp @@ -59,7 +59,7 @@ Lv2ControlBase::Lv2ControlBase(Model* that, const QString &uri) : else { qCritical() << "No Lv2 plugin found for URI" << uri; - m_valid = false; + throw std::runtime_error("No Lv2 plugin found for given URI"); } } @@ -77,26 +77,14 @@ void Lv2ControlBase::init(Model* meAsModel) while (channelsLeft > 0) { std::unique_ptr newOne = std::make_unique(m_plugin, meAsModel); - if (newOne->isValid()) - { - channelsLeft -= std::max( - 1 + static_cast(newOne->inPorts().m_right), - 1 + static_cast(newOne->outPorts().m_right)); - Q_ASSERT(channelsLeft >= 0); - m_procs.push_back(std::move(newOne)); - } - else - { - qCritical() << "Failed instantiating LV2 processor"; - m_valid = false; - channelsLeft = 0; - } - } - if (m_valid) - { - m_channelsPerProc = DEFAULT_CHANNELS / m_procs.size(); - linkAllModels(); + channelsLeft -= std::max( + 1 + static_cast(newOne->inPorts().m_right), + 1 + static_cast(newOne->outPorts().m_right)); + Q_ASSERT(channelsLeft >= 0); + m_procs.push_back(std::move(newOne)); } + m_channelsPerProc = DEFAULT_CHANNELS / m_procs.size(); + linkAllModels(); } diff --git a/src/core/lv2/Lv2Proc.cpp b/src/core/lv2/Lv2Proc.cpp index 158196fdb..77177a1c0 100644 --- a/src/core/lv2/Lv2Proc.cpp +++ b/src/core/lv2/Lv2Proc.cpp @@ -467,7 +467,7 @@ void Lv2Proc::initPlugin() << "(URI:" << lilv_node_as_uri(lilv_plugin_get_uri(m_plugin)) << ")"; - m_valid = false; + throw std::runtime_error("Failed to create Lv2 processor"); } } @@ -476,16 +476,12 @@ void Lv2Proc::initPlugin() void Lv2Proc::shutdownPlugin() { - if (m_valid) - { - lilv_instance_deactivate(m_instance); - lilv_instance_free(m_instance); - m_instance = nullptr; + lilv_instance_deactivate(m_instance); + lilv_instance_free(m_instance); + m_instance = nullptr; - m_features.clear(); - m_options.clear(); - } - m_valid = true; + m_features.clear(); + m_options.clear(); }