0.16 review: four crash-class fixes (PD-1, PD-3, PD-7, SA-1)

src/detail/clap/preset_discovery.cpp

@@ -18,6 +18,7 @@
 
 #include "detail/clap/fsutil.h"
 #include "detail/os/fs.h"
+#include "detail/os/log.h"
 
 namespace Clap
 {
@@ -55,6 +56,24 @@ std::string normalizeExtension(const char *declared)
   return toLower(value);
 }
 
+// CLAP strings are UTF-8, fs::path is OS-native: on Windows fs::path{std::string}
+// decodes through the ANSI code page, and path::string() *throws* for any name
+// that page cannot express. Not fs::u8path(): deprecated in C++20, -Werror.
+fs::path pathFromUtf8(const std::string &utf8)
+{
+#if WIN
+  if (utf8.empty()) return {};
+  const auto size = static_cast<int>(utf8.size());
+  const int length = ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, nullptr, 0);
+  if (length <= 0) return {};
+  std::wstring wide(static_cast<size_t>(length), L'\0');
+  ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, wide.data(), length);
+  return fs::path{std::move(wide)};
+#else
+  return fs::path{utf8};
+#endif
+}
+
 }  // namespace
 
 std::string PresetEntry::displayName() const
@@ -111,7 +130,8 @@ struct PresetIndex::Declarations
     // "If empty or NULL then every file should be matched."
     if (extensions.empty()) return false;
 
-    auto ext = path.extension().string();
+    // u8string(), not string(): see pathFromUtf8.
+    auto ext = path.extension().u8string();
     if (!ext.empty() && ext.front() == '.') ext.erase(ext.begin());
     ext = toLower(ext);
 
@@ -389,21 +409,53 @@ std::shared_ptr<PresetIndex> PresetIndex::forPlugin(const Library *library, cons
 
 void PresetIndex::resetCache()
 {
-  auto &cache = indexCache();
-  std::lock_guard<std::mutex> lock(cache.mutex);
-  cache.map.clear();
+  // Refcount or not: clearing the map alone leaves the thread of an index a
+  // wrapper still holds running inside a module about to be unloaded.
+  std::vector<std::shared_ptr<PresetIndex>> dropped;
+  {
+    auto &cache = indexCache();
+    std::lock_guard<std::mutex> lock(cache.mutex);
+    for (auto &[key, index] : cache.map) dropped.push_back(index);
+    cache.map.clear();
+  }
+  for (auto &index : dropped) index->abandon();
 }
 
 PresetIndex::~PresetIndex()
+{
+  abandon();
+}
+
+void PresetIndex::abandon()
 {
   _abandon.store(true);
+  // join() leaves the thread non-joinable, so a second call is a no-op.
   if (_thread.joinable()) _thread.join();
 }
 
 void PresetIndex::start(const Library *library, const std::string &pluginId)
 {
   const auto *factory = library->_pluginFactoryPresetDiscovery;
-  _thread = std::thread([this, factory, pluginId]() { crawl(factory, pluginId); });
+  _thread = std::thread(
+      [this, factory, pluginId]()
+      {
+        // Bare std::thread: an escaping exception is std::terminate.
+        try
+        {
+          crawl(factory, pluginId);
+        }
+        catch (const std::exception &e)
+        {
+          LOGINFO("preset crawl for '{}' aborted: {}", pluginId, e.what());
+        }
+        catch (...)
+        {
+          LOGINFO("preset crawl for '{}' aborted by a non-standard exception", pluginId);
+        }
+
+        // On every path, or waitUntilComplete() and the listeners never fire.
+        finish();
+      });
 }
 
 void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::string pluginId)
@@ -460,7 +512,7 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str
 
         // A FILE location may be a directory to crawl or a single file.
         std::error_code ec;
-        const fs::path root{location.location};
+        const fs::path root = pathFromUtf8(location.location);
 
         if (fs::is_regular_file(root, ec))
         {
@@ -494,7 +546,8 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str
           if (!declarations.matchesExtension(it->path())) continue;
           if (++seen > kMaxPresetsPerLocation) break;
 
-          const auto path = it->path().string();
+          // u8string(): what from_location() expects back, and cannot throw.
+          const auto path = it->path().u8string();
           receiver.location = path;
           provider->get_metadata(provider, location.kind, path.c_str(), &receiver.receiver);
           receiver.finishFile();
@@ -522,7 +575,7 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str
                      });
   }
 
-  finish();
+  // finish() is called by the thread body in start(), so it also runs on throw.
 }
 
 bool PresetIndex::waitUntilComplete(unsigned timeoutMs)
@@ -545,13 +598,38 @@ void PresetIndex::finish()
   }
   _completionCv.notify_all();
 
-  std::vector<Listener> toCall;
+  // Looked up one at a time as its turn comes, never snapshotted: a copy cannot
+  // be recalled, so a listener removed meanwhile would still be called, possibly
+  // on a destroyed instance. Called outside the lock - a listener may re-enter.
+  // ceiling: later registrations are called by addCompletionListener() instead.
+  uint64_t last = 0;
+  uint64_t ceiling;
   {
     std::lock_guard<std::mutex> lock(_listenerMutex);
-    for (auto &[token, listener] : _listeners) toCall.push_back(listener);
+    ceiling = _nextListenerToken - 1;
   }
-  for (auto &listener : toCall)
+  for (;;)
+  {
+    Listener listener;
+    {
+      std::lock_guard<std::mutex> lock(_listenerMutex);
+      auto it = std::find_if(_listeners.begin(), _listeners.end(), [&](const auto &pair)
+                             { return pair.first > last && pair.first <= ceiling; });
+      if (it == _listeners.end()) break;
+      last = it->first;
+      listener = it->second;
+      _listenerInCall = last;
+      _listenerInCallThread = std::this_thread::get_id();
+    }
+
     if (listener) listener();
+
+    {
+      std::lock_guard<std::mutex> lock(_listenerMutex);
+      _listenerInCall = 0;
+    }
+    _listenerCv.notify_all();
+  }
 }
 
 std::vector<PresetEntry> PresetIndex::presets() const
@@ -617,10 +695,15 @@ uint64_t PresetIndex::addCompletionListener(Listener listener)
 
 void PresetIndex::removeCompletionListener(uint64_t token)
 {
-  std::lock_guard<std::mutex> lock(_listenerMutex);
+  std::unique_lock<std::mutex> lock(_listenerMutex);
   _listeners.erase(std::remove_if(_listeners.begin(), _listeners.end(),
                                   [token](const auto &pair) { return pair.first == token; }),
                    _listeners.end());
+
+  // Erasing cannot recall a call finish() is making right now, so wait it out -
+  // except for a listener removing itself, which would wait on its own thread.
+  if (_listenerInCall == token && _listenerInCallThread != std::this_thread::get_id())
+    _listenerCv.wait(lock, [this, token]() { return _listenerInCall != token; });
 }
 
 }  // namespace Clap

src/detail/clap/preset_discovery.h

@@ -111,8 +111,10 @@ class PresetIndex
   // thread, including concurrently.
   static std::shared_ptr<PresetIndex> forPlugin(const Library *library, const std::string &pluginId);
 
-  // Drops every cached index. For test harnesses and for a host that unloads
-  // the library; the shared_ptrs handed out stay valid.
+  // Drops every cached index and joins its crawl thread, held shared_ptrs or
+  // not (those stay valid, just finished). Must run before the hosted CLAP is
+  // deinit()ed - the crawl is inside that module - and not from a static
+  // destructor, where Windows' loader lock keeps the thread from exiting.
   static void resetCache();
 
   ~PresetIndex();
@@ -158,6 +160,10 @@ class PresetIndex
   // the caller's thread) if it already has. Use it to tell a host its preset
   // list changed. The token lets a wrapper unregister in its destructor -
   // without that, a callback could outlive the instance it captured.
+  // removeCompletionListener() returns only once the listener is neither running
+  // nor able to run again, which is what makes removal from a destructor safe. A
+  // listener may re-enter the index and remove itself, but must not wait on the
+  // thread that may be removing it, nor drop the index's last reference.
   using Listener = std::function<void()>;
   uint64_t addCompletionListener(Listener listener);
   void removeCompletionListener(uint64_t token);
@@ -168,6 +174,8 @@ class PresetIndex
   void start(const Library *library, const std::string &pluginId);
   void crawl(const clap_preset_discovery_factory_t *factory, std::string pluginId);
   void finish();
+  // Stops the crawl and waits for it. Idempotent.
+  void abandon();
 
   // The receiver and indexer callbacks the provider talks to. They live here
   // so the whole conversation with the plugin is in one place.
@@ -183,8 +191,13 @@ class PresetIndex
   std::condition_variable _completionCv;
 
   std::mutex _listenerMutex;
-  std::vector<std::pair<uint64_t, Listener>> _listeners;
+  std::vector<std::pair<uint64_t, Listener>> _listeners;  // in token order
   uint64_t _nextListenerToken{1};
+  // Which listener finish() is running (0: none) and on which thread, so
+  // removeCompletionListener() can wait for that one. Guarded by _listenerMutex.
+  uint64_t _listenerInCall{0};
+  std::thread::id _listenerInCallThread;
+  std::condition_variable _listenerCv;
 
   std::thread _thread;
   std::atomic<bool> _abandon{false};

src/detail/standalone/macos/AppDelegate.mm

@@ -261,14 +261,20 @@ - (void)applicationWillTerminate:(NSNotification *)aNotification
   freeaudio::clap_wrapper::standalone::getStandaloneHost()->displayAudioError = nullptr;
   freeaudio::clap_wrapper::standalone::getStandaloneHost()->onRequestResize = nullptr;
 
-  auto plugin = freeaudio::clap_wrapper::standalone::getMainPlugin();
-
-  if (plugin && plugin->_ext._gui)
+  // Scoped so this shared_ptr copy is released before mainFinish, which
+  // deinit()s the entry: were it the last owner, ~Plugin would then call
+  // _plugin->destroy() on a deinited - dynamically, unloaded - entry.
   {
-    plugin->_ext._gui->hide(plugin->_plugin);
-    plugin->_ext._gui->destroy(plugin->_plugin);
+    auto plugin = freeaudio::clap_wrapper::standalone::getMainPlugin();
+
+    if (plugin && plugin->_ext._gui)
+    {
+      plugin->_ext._gui->hide(plugin->_plugin);
+      plugin->_ext._gui->destroy(plugin->_plugin);
+    }
   }
 
+  // Before mainFinish: the callback dereferences getMainPlugin() unchecked.
   [self.requestCallbackTimer invalidate];
   self.requestCallbackTimer = nil;
 

src/wrapasvst3_entry.cpp

@@ -66,6 +66,7 @@ using namespace Steinberg::Vst;
 //------------------------------------------------------------------------
 
 #include "detail/clap/fsutil.h"
+#include "detail/clap/preset_discovery.h"
 #include "detail/vst3/categories.h"
 #include "clap_proxy.h"
 
@@ -102,6 +103,11 @@ Clap::Library &hostedClapLibrary()
 static Steinberg::ModuleTerminator gReleaseHostedClapLibrary(
     []()
     {
+      // First: the crawl thread runs inside the hosted .clap and must be joined
+      // before it is deinit()ed and unmapped. And here rather than at static
+      // destruction - the host calls ExitDll(), so no loader lock is held.
+      Clap::PresetIndex::resetCache();
+
       delete gHostedClapLibrary;
       gHostedClapLibrary = nullptr;
     });