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

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