Mercurial > hg > sonic-annotator
changeset 366:9f7297c47850
Update plugin handling to follow the smart pointers used in the svcore library. This is awkward in our context, and the outcome isn't a very nice one - but it
does look as if we had the potential for use-after-free in cases where a plugin
was both used "bare" and wrapped in an auto-deleting adapter; those should be
fixed now
author | Chris Cannam |
---|---|
date | Thu, 23 Apr 2020 15:51:55 +0100 |
parents | 5e17f44b2e74 |
children | 93dba46baf35 |
files | repoint-lock.json runner/FeatureExtractionManager.cpp runner/FeatureExtractionManager.h |
diffstat | 3 files changed, 100 insertions(+), 48 deletions(-) [+] |
line wrap: on
line diff
--- a/repoint-lock.json Tue Jan 28 15:41:34 2020 +0000 +++ b/repoint-lock.json Thu Apr 23 15:51:55 2020 +0100 @@ -1,16 +1,16 @@ { "libraries": { "vamp-plugin-sdk": { - "pin": "c42e50a5c297" + "pin": "8ffb8985ae8f" }, "svcore": { - "pin": "1cd161242250" + "pin": "98339fac0faf" }, "checker": { - "pin": "ef64b3f171d9" + "pin": "e839338d3869" }, "piper-vamp-cpp": { - "pin": "f381235a4ba88eac2fa31fc1f7f613cca9707f63" + "pin": "f0d3ab2952b21d287b481759bda986427df10ef7" }, "dataquay": { "pin": "35098262cadd" @@ -31,7 +31,7 @@ "pin": "a0926b93e771" }, "sv-dependency-builds": { - "pin": "d0c2a83c1364" + "pin": "08ae793730bd" } } }
--- a/runner/FeatureExtractionManager.cpp Tue Jan 28 15:41:34 2020 +0000 +++ b/runner/FeatureExtractionManager.cpp Thu Apr 23 15:51:55 2020 +0100 @@ -78,14 +78,25 @@ SVDEBUG << "FeatureExtractionManager::~FeatureExtractionManager: cleaning up" << endl; - for (PluginMap::iterator pi = m_plugins.begin(); - pi != m_plugins.end(); ++pi) { - delete pi->first; - } foreach (AudioFileReader *r, m_readyReaders) { delete r; } + // We need to ensure m_allLoadedPlugins outlives anything that + // holds a shared_ptr to a plugin adapter built from one of the + // raw plugin pointers. So clear these explicitly, in this order, + // instead of allowing it to happen automatically + + m_pluginOutputs.clear(); + m_transformPluginMap.clear(); + m_orderedPlugins.clear(); + m_plugins.clear(); + + // and last + + m_allAdapters.clear(); + m_allLoadedPlugins.clear(); + SVDEBUG << "FeatureExtractionManager::~FeatureExtractionManager: done" << endl; } @@ -194,13 +205,24 @@ transform.setSampleRate(m_sampleRate); } - Plugin *plugin = 0; + shared_ptr<Plugin> plugin = nullptr; // Remember what the original transform looked like, and index // based on this -- because we may be about to fill in the zeros // for step and block size, but we want any further copies with // the same zeros to match this one Transform originalTransform = transform; + + // In a few cases here, after loading the plugin, we create an + // adapter to wrap it. We always give a raw pointer to the adapter + // (that's what the API requires). It is safe to use the raw + // pointer obtained from .get() on the originally loaded + // shared_ptr, so long as we also stash the shared_ptr somewhere + // so that it doesn't go out of scope before the adapter is + // deleted, and we call disownPlugin() on the adapter to prevent + // the adapter from trying to delete it. We have + // m_allLoadedPlugins and m_allAdapters as our stashes of + // shared_ptrs, which share the lifetime of this manager object. if (m_transformPluginMap.find(transform) == m_transformPluginMap.end()) { @@ -221,8 +243,14 @@ << "summary type; sharing its plugin instance" << endl; plugin = i->second; if (transform.getSummaryType() != Transform::NoSummary && - !dynamic_cast<PluginSummarisingAdapter *>(plugin)) { - plugin = new PluginSummarisingAdapter(plugin); + !std::dynamic_pointer_cast<PluginSummarisingAdapter>(plugin)) { + // See comment above about safety of raw pointer here + auto psa = + make_shared<PluginSummarisingAdapter>(plugin.get()); + psa->disownPlugin(); + psa->setSummarySegmentBoundaries(m_boundaries); + m_allAdapters.insert(psa); + plugin = psa; i->second = plugin; } break; @@ -233,8 +261,9 @@ TransformFactory *tf = TransformFactory::getInstance(); - PluginBase *pb = tf->instantiatePluginFor(transform); - plugin = tf->downcastVampPlugin(pb); + shared_ptr<PluginBase> pb = tf->instantiatePluginFor(transform); + plugin = dynamic_pointer_cast<Vamp::Plugin>(pb); + if (!plugin) { //!!! todo: handle non-Vamp plugins too, or make the main --list // option print out only Vamp transforms @@ -243,9 +272,10 @@ if (pb) { SVCERR << "NOTE: (A plugin was loaded, but apparently not a Vamp plugin)" << endl; } - delete pb; return false; } + + m_allLoadedPlugins.insert(pb); // We will provide the plugin with arbitrary step and // block sizes (so that we can use the same read/write @@ -261,21 +291,29 @@ size_t pluginStepSize = plugin->getPreferredStepSize(); size_t pluginBlockSize = plugin->getPreferredBlockSize(); - PluginInputDomainAdapter *pida = 0; + shared_ptr<PluginInputDomainAdapter> pida = nullptr; // adapt the plugin for buffering, channels, etc. if (plugin->getInputDomain() == Plugin::FrequencyDomain) { - pida = new PluginInputDomainAdapter(plugin); - pida->setProcessTimestampMethod(PluginInputDomainAdapter::ShiftData); + // See comment up top about safety of raw pointer here + pida = make_shared<PluginInputDomainAdapter>(plugin.get()); + pida->disownPlugin(); + pida->setProcessTimestampMethod + (PluginInputDomainAdapter::ShiftData); PluginInputDomainAdapter::WindowType wtype = convertWindowType(transform.getWindowType()); pida->setWindowType(wtype); + + m_allAdapters.insert(pida); plugin = pida; } - PluginBufferingAdapter *pba = new PluginBufferingAdapter(plugin); + auto pba = make_shared<PluginBufferingAdapter>(plugin.get()); + pba->disownPlugin(); + + m_allAdapters.insert(pba); plugin = pba; if (transform.getStepSize() != 0) { @@ -290,19 +328,23 @@ transform.setBlockSize(int(pluginBlockSize)); } - plugin = new PluginChannelAdapter(plugin); + auto pca = make_shared<PluginChannelAdapter>(plugin.get()); + pca->disownPlugin(); + + m_allAdapters.insert(pca); + plugin = pca; if (!m_summaries.empty() || transform.getSummaryType() != Transform::NoSummary) { - PluginSummarisingAdapter *adapter = - new PluginSummarisingAdapter(plugin); - adapter->setSummarySegmentBoundaries(m_boundaries); - plugin = adapter; + auto psa = make_shared<PluginSummarisingAdapter>(plugin.get()); + psa->disownPlugin(); + psa->setSummarySegmentBoundaries(m_boundaries); + m_allAdapters.insert(psa); + plugin = psa; } if (!plugin->initialise(m_channels, m_blockSize, m_blockSize)) { SVCERR << "ERROR: Plugin initialise (channels = " << m_channels << ", stepSize = " << m_blockSize << ", blockSize = " << m_blockSize << ") failed." << endl; - delete plugin; return false; } @@ -341,7 +383,7 @@ if (transform.getStepSize() == 0 || transform.getBlockSize() == 0) { - PluginWrapper *pw = dynamic_cast<PluginWrapper *>(plugin); + auto pw = dynamic_pointer_cast<PluginWrapper>(plugin); if (pw) { PluginBufferingAdapter *pba = pw->getWrapper<PluginBufferingAdapter>(); @@ -762,7 +804,7 @@ sv_frame_t latestEndFrame = frameCount; bool haveExtents = false; - foreach (Plugin *plugin, m_orderedPlugins) { + for (auto plugin: m_orderedPlugins) { PluginMap::iterator pi = m_plugins.find(plugin); @@ -826,7 +868,7 @@ sv_frame_t startFrame = earliestStartFrame; sv_frame_t endFrame = latestEndFrame; - foreach (Plugin *plugin, m_orderedPlugins) { + for (auto plugin: m_orderedPlugins) { PluginMap::iterator pi = m_plugins.find(plugin); @@ -898,7 +940,7 @@ RealTime timestamp = RealTime::frame2RealTime(i, m_sampleRate); - foreach (Plugin *plugin, m_orderedPlugins) { + for (auto plugin: m_orderedPlugins) { PluginMap::iterator pi = m_plugins.find(plugin); @@ -938,7 +980,7 @@ lifemgr.destroy(); // deletes reader, data - foreach (Plugin *plugin, m_orderedPlugins) { + for (auto plugin: m_orderedPlugins) { Plugin::FeatureSet featureSet = plugin->getRemainingFeatures(); @@ -948,8 +990,8 @@ if (!m_summaries.empty()) { // Summaries requested on the command line, for all transforms - PluginSummarisingAdapter *adapter = - dynamic_cast<PluginSummarisingAdapter *>(plugin); + auto adapter = + dynamic_pointer_cast<PluginSummarisingAdapter>(plugin); if (!adapter) { SVCERR << "WARNING: Summaries requested, but plugin is not a summarising adapter" << endl; } else { @@ -982,7 +1024,8 @@ } void -FeatureExtractionManager::writeSummaries(QString audioSource, Plugin *plugin) +FeatureExtractionManager::writeSummaries(QString audioSource, + shared_ptr<Plugin> plugin) { // caller should have ensured plugin is in m_plugins PluginMap::iterator pi = m_plugins.find(plugin); @@ -1004,8 +1047,7 @@ continue; } - PluginSummarisingAdapter *adapter = - dynamic_cast<PluginSummarisingAdapter *>(plugin); + auto adapter = dynamic_pointer_cast<PluginSummarisingAdapter>(plugin); if (!adapter) { SVCERR << "FeatureExtractionManager::writeSummaries: INTERNAL ERROR: Summary requested for transform, but plugin is not a summarising adapter" << endl; continue; @@ -1021,7 +1063,7 @@ } void FeatureExtractionManager::writeFeatures(QString audioSource, - Plugin *plugin, + shared_ptr<Plugin> plugin, const Plugin::FeatureSet &features, Transform::SummaryType summaryType) { @@ -1093,7 +1135,7 @@ void FeatureExtractionManager::finish() { - foreach (Plugin *plugin, m_orderedPlugins) { + for (auto plugin: m_orderedPlugins) { PluginMap::iterator pi = m_plugins.find(plugin);
--- a/runner/FeatureExtractionManager.h Tue Jan 28 15:41:34 2020 +0000 +++ b/runner/FeatureExtractionManager.h Thu Apr 23 15:51:55 2020 +0100 @@ -19,6 +19,7 @@ #include <vector> #include <set> #include <string> +#include <memory> #include <QMap> @@ -31,6 +32,7 @@ using std::string; using std::pair; using std::map; +using std::shared_ptr; class FeatureWriter; class AudioFileReader; @@ -76,15 +78,23 @@ private: bool m_verbose; + + // The plugins that we actually "run" may be wrapped versions of + // the originally loaded ones, depending on the adapter + // characteristics we need. Because the wrappers use raw pointers, + // we need to separately retain the originally loaded shared_ptrs + // so that they don't get auto-deleted. Same goes for any wrappers + // that may then be re-wrapped. That's what these are for. + set<shared_ptr<Vamp::PluginBase>> m_allLoadedPlugins; + set<shared_ptr<Vamp::PluginBase>> m_allAdapters; // A plugin may have many outputs, so we can have more than one // transform requested for a single plugin. The things we want to // run in our process loop are plugins rather than their outputs, // so we maintain a map from the plugins to the transforms desired // of them and then iterate through this map - typedef map<Transform, vector<FeatureWriter *> > TransformWriterMap; - typedef map<Vamp::Plugin *, TransformWriterMap> PluginMap; + typedef map<shared_ptr<Vamp::Plugin>, TransformWriterMap> PluginMap; PluginMap m_plugins; // When we run plugins, we want to run them in a known order so as @@ -95,20 +105,19 @@ // that the TransformWriterMap is consistently ordered (because // the key is a Transform which has a proper ordering) so using // this gives us a consistent order across the whole PluginMap - vector<Vamp::Plugin *> m_orderedPlugins; + vector<shared_ptr<Vamp::Plugin>> m_orderedPlugins; // And a map back from transforms to their plugins. Note that - // this is keyed by transform, not transform ID -- two differently - // configured transforms with the same ID must use different - // plugin instances. - - typedef map<Transform, Vamp::Plugin *> TransformPluginMap; + // this is keyed by whole transform structure, not transform ID -- + // two differently configured transforms with the same ID must use + // different plugin instances. + typedef map<Transform, shared_ptr<Vamp::Plugin>> TransformPluginMap; TransformPluginMap m_transformPluginMap; // Cache the plugin output descriptors, mapping from plugin to a // map from output ID to output descriptor. typedef map<string, Vamp::Plugin::OutputDescriptor> OutputMap; - typedef map<Vamp::Plugin *, OutputMap> PluginOutputMap; + typedef map<shared_ptr<Vamp::Plugin>, OutputMap> PluginOutputMap; PluginOutputMap m_pluginOutputs; // Map from plugin output identifier to plugin output index @@ -124,10 +133,11 @@ void extractFeaturesFor(AudioFileReader *reader, QString audioSource); - void writeSummaries(QString audioSource, Vamp::Plugin *); + void writeSummaries(QString audioSource, + std::shared_ptr<Vamp::Plugin>); void writeFeatures(QString audioSource, - Vamp::Plugin *, + std::shared_ptr<Vamp::Plugin>, const Vamp::Plugin::FeatureSet &, Transform::SummaryType summaryType = Transform::NoSummary);