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

@@ -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<std::unique_ptr<Lv2Proc>> m_procs;
 
-	bool m_valid = true;
 	bool m_hasGUI = false;
 	unsigned m_channelsPerProc;
 

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;

plugins/Lv2Effect/Lv2Effect.cpp

@@ -24,6 +24,7 @@
 
 #include "Lv2Effect.h"
 
+#include <QDebug>
 
 #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<const KeyType*>(_data));
-	if (!eff->isValid()) { delete eff; eff = nullptr; }
-	return eff;
+	try {
+		return new Lv2Effect(_parent, static_cast<const KeyType*>(_data));
+	} catch (const std::runtime_error& e) {
+		qCritical() << e.what();
+		return nullptr;
+	}
 }
 
 }

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; }

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);
 }
 
 

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<InstrumentTrack*>(_parent), static_cast<KeyType*>(_data));
-	if (!ins->isValid()) { delete ins; ins = nullptr; }
-	return ins;
+	try {
+		return new Lv2Instrument(static_cast<InstrumentTrack*>(_parent), static_cast<KeyType*>(_data));
+	} catch (const std::runtime_error& e) {
+		qCritical() << e.what();
+		return nullptr;
+	}
 }
 
 }

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

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<Lv2Proc> newOne = std::make_unique<Lv2Proc>(m_plugin, meAsModel);
-		if (newOne->isValid())
-		{
-			channelsLeft -= std::max(
-				1 + static_cast<bool>(newOne->inPorts().m_right),
-				1 + static_cast<bool>(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<bool>(newOne->inPorts().m_right),
+			1 + static_cast<bool>(newOne->outPorts().m_right));
+		Q_ASSERT(channelsLeft >= 0);
+		m_procs.push_back(std::move(newOne));
 	}
+	m_channelsPerProc = DEFAULT_CHANNELS / m_procs.size();
+	linkAllModels();
 }
 
 

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();
 }