From 5b1fbd826b4f89238c1d36013e2cb57488b07657 Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:33:38 +0200 Subject: [PATCH 1/3] SA-1: release the CLAP before mainFinish on macOS quit applicationWillTerminate took a shared_ptr copy of the hosted plugin that stayed alive until the method returned - past mainFinish(), which resets the global and the host's references and then calls entry->deinit(). The local copy was therefore the last owner, so ~Plugin ran destroy() on an already-deinited entry and crashed on exit for any plugin whose deinit tears down global state. Windows already released its reference before mainFinish (see the comment in windows_standalone.cpp's WM_DESTROY handler); the macOS path was missed when that fix landed. Scope the reference to the GUI teardown that needs it so the host's own reset inside mainFinish is the last owner. Not compiled: macOS-only source, developed on Windows. Verified by reading mainFinish (entry.cpp) and ~Plugin (clap_proxy.cpp) rather than by build. --- src/detail/standalone/macos/AppDelegate.mm | 24 +++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/detail/standalone/macos/AppDelegate.mm b/src/detail/standalone/macos/AppDelegate.mm index 9799d845..b93fe71c 100644 --- a/src/detail/standalone/macos/AppDelegate.mm +++ b/src/detail/standalone/macos/AppDelegate.mm @@ -261,14 +261,28 @@ - (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) + // The GUI teardown needs the plugin, so take a reference - but only for the + // duration of this block. getMainPlugin() hands back a shared_ptr copy, and + // that copy must be released *before* mainFinish, which resets the global + // and the host's references and then runs entry->deinit(). Had the copy + // lived to the end of this method it would have been the last owner, and + // Clap::Plugin's destructor would have called _plugin->destroy() on an + // entry that was already deinited - a spec violation, and a use-after-free + // of the library in the dynamically loaded case. Windows does the same + // release-before-mainFinish dance in its WM_DESTROY handler. { - 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); + } } + // Stop the timer before mainFinish: its callback dereferences + // getMainPlugin() without a null check, and after mainFinish the global is + // reset and the StandaloneHost it also reads is gone. [self.requestCallbackTimer invalidate]; self.requestCallbackTimer = nil; From db5915db2b34bc80bdcaa9727eacadbefa4bf73f Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:43:59 +0200 Subject: [PATCH 2/3] PD-1, PD-3, PD-7: make the preset crawl safe to run and to stop Three crash-class bugs in the new preset-discovery subsystem, fixed together because they are one story: the crawl thread was unsafe to run, unsafe to stop, and unsafe to be listened to. PD-1, encoding. fs::path and the CLAP ABI disagree about what a string is: the ABI is UTF-8, fs::path is whatever the OS says. On Windows fs::path{std::string} decodes through the ANSI code page, so a plugin declaring "C:\Users\Jurgen\...\Presets" got a folder that does not exist and the location was silently skipped; and path::string() *throws* std::system_error for any name the code page cannot express, which on a bare thread is std::terminate for the host - one odd filename in a user folder was enough. Conversions now go through a pathFromUtf8() helper and u8string(), the way fsutil.cpp already does everywhere. Deliberately not fs::u8path(), which is deprecated in C++20 and this builds with -Werror; u8string() stays std::string on every configuration because shared_prologue.cmake turns char8_t off whenever the standard is >= 20. The thread body also catches everything now, and still reports completion when it does, so a waiter does not sit out its timeout over a failed crawl. PD-3, lifetime. PresetIndex::resetCache() had no callers at all, and the ModuleTerminator deleted the hosted library - deinit() and unmap - while the crawl thread was inside provider->get_metadata() in that very module. Reaper's in-process rescan is where that shows: either the plugin's globals go out under a running crawl, or the crawl is joined later from the static IndexCache destructor, which on Windows is under DLL_PROCESS_DETACH, where the loader lock keeps the thread from exiting and the scan simply hangs. The terminator now calls resetCache() first. Clearing the map was not enough on its own, because a wrapper still holding a shared_ptr keeps its thread alive, so resetCache() takes the indices out under the cache lock and abandons and joins each one outside it. ExitDll/bundleExit/ModuleExit are called by the host rather than from DllMain, so that join is not under the loader lock. PD-7, listeners. finish() copied the listener list, dropped the lock and then called the copies - and a copy cannot be recalled, so a wrapper whose destructor removed its listener could still be called, on freed memory. Logic and auval dispose an AU about 100 ms after instantiating it, which lands squarely inside a folder crawl. finish() now looks each listener up at its turn and publishes which one it is running and on which thread; removeCompletionListener() waits for exactly that one, and only when the caller is not the crawl thread, so a listener removing itself does not wait on itself. Listeners are still called outside the lock, because they may re-enter the index. Verified: VST3 target builds clean under C++17, clap-wrapper-shared-detail under C++20 (the char8_t-sensitive path), clang-format clean. There are no tests covering PresetIndex, so this is compile-and-reasoning, not runtime proof. Pre-existing and left alone: a listener registered in the exact window where a crawl completes can still be called twice. Both current consumers only set an idempotent atomic flag. --- src/detail/clap/preset_discovery.cpp | 167 +++++++++++++++++++++++++-- src/detail/clap/preset_discovery.h | 38 +++++- src/wrapasvst3_entry.cpp | 16 +++ 3 files changed, 206 insertions(+), 15 deletions(-) diff --git a/src/detail/clap/preset_discovery.cpp b/src/detail/clap/preset_discovery.cpp index dbdf5d67..a256bb01 100644 --- a/src/detail/clap/preset_discovery.cpp +++ b/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,37 @@ std::string normalizeExtension(const char *declared) return toLower(value); } +// The plugin speaks UTF-8 - every string across the CLAP ABI is, and +// Library::load hands the plugin its own path as u8string() - while fs::path +// speaks the OS. On Windows those are two different things: fs::path{std::string} +// decodes through the ANSI code page, so a UTF-8 "C:\Users\Jürgen\...\Presets" +// turns into a folder that does not exist and the location is silently +// skipped; and in the other direction path::string() *throws* +// std::system_error for any name the code page cannot express ("パッド.preset" +// on a Western-locale machine), which on a bare thread is std::terminate for +// the host. fsutil.cpp already goes through native()/u8string() everywhere for +// exactly this reason; this is the one conversion it does not need, UTF-8 in. +// +// Not fs::u8path(): deprecated in C++20, which this project builds with under +// -Werror. +fs::path pathFromUtf8(const std::string &utf8) +{ +#if WIN + if (utf8.empty()) return {}; + const auto size = static_cast(utf8.size()); + const int length = ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, nullptr, 0); + if (length <= 0) return {}; // not UTF-8 after all: no such folder, same as a typo + std::wstring wide(static_cast(length), L'\0'); + ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, wide.data(), length); + return fs::path{std::move(wide)}; +#else + // POSIX filenames are bytes and every platform we ship on treats them as + // UTF-8 - the same assumption mac_helpers.mm makes with + // fs::path{[u fileSystemRepresentation]}. + return fs::path{utf8}; +#endif +} + } // namespace std::string PresetEntry::displayName() const @@ -111,7 +143,9 @@ 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 - an extension is as capable + // of being outside the ANSI code page as the rest of the name. + auto ext = path.extension().u8string(); if (!ext.empty() && ext.front() == '.') ext.erase(ext.begin()); ext = toLower(ext); @@ -389,21 +423,75 @@ std::shared_ptr PresetIndex::forPlugin(const Library *library, cons void PresetIndex::resetCache() { - auto &cache = indexCache(); - std::lock_guard lock(cache.mutex); - cache.map.clear(); + // Take the indices out from under the lock, then stop them outside it. + // Clearing the map alone is not enough: a wrapper that is still alive (a + // host that unloads with instances open, or simply a shared_ptr that has + // not been dropped yet) keeps the index and therefore its thread alive, and + // the whole point of this call is that no crawl thread is left running + // inside a module that is about to be unloaded. So every crawl is abandoned + // and joined explicitly, refcount or not. + // + // The join happens outside cache.mutex, so a crawl thread that is at this + // moment still starting up cannot end up behind it. (It never takes that + // mutex today; the ordering is kept so that stays a non-issue.) + std::vector> dropped; + { + auto &cache = indexCache(); + std::lock_guard 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); + // The crawl checks _abandon between files, so this waits out at most the + // get_metadata() call in flight. join() also renders the thread + // non-joinable, which is what makes a second call (destructor after + // resetCache) 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]() + { + // Nothing may escape this body: it is a bare std::thread, and an + // exception leaving it is std::terminate - the host killed over one + // odd file in a user folder. The conversions in crawl() are chosen not + // to throw (see pathFromUtf8), and everything filesystem-side goes + // through an error_code, so what this catches is the remainder: a + // provider that throws across the C ABI, an allocation failure, a + // standard library that found a way we did not think of. + 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); + } + + // Whatever happened above, this is still a crawl that ended, and it + // must say so: a wrapper blocked in waitUntilComplete() would + // otherwise sit out its whole timeout, and a listener would wait + // forever for a completion that already happened. What has been + // collected so far stays, and stays sorted if crawl() got that far. + finish(); + }); } void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::string pluginId) @@ -460,7 +548,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 +582,9 @@ 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 the plugin expects back in from_location(), and + // the only narrowing that cannot throw on Windows - see pathFromUtf8. + const auto path = it->path().u8string(); receiver.location = path; provider->get_metadata(provider, location.kind, path.c_str(), &receiver.receiver); receiver.finishFile(); @@ -522,7 +612,8 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str }); } - finish(); + // finish() is deliberately not called here but by the thread body in + // start(), so that it also runs when this function is left by exception. } bool PresetIndex::waitUntilComplete(unsigned timeoutMs) @@ -545,13 +636,54 @@ void PresetIndex::finish() } _completionCv.notify_all(); - std::vector toCall; + // Listeners are called one at a time, each looked up under the lock at the + // moment it is its turn, and none of them under the lock. + // + // Not a snapshot copied up front: a copy cannot be recalled, so a wrapper + // that removed its listener while the copy was being worked through would + // still be called - on a destroyed instance, if that removal was its + // destructor. Looking each one up as it comes means a listener removed + // before its turn is simply never seen; and the one in flight is + // published in _listenerInCall so removeCompletionListener() can wait for + // it. Tokens are handed out in increasing order and _listeners is kept in + // insertion order, so "first entry with a token above the last one called" + // is the next listener in line, however the list changed in between. + // + // Not under the lock either: a listener may re-enter the index, and a + // listener that calls add/removeCompletionListener() under a non-recursive + // mutex it already holds is a deadlock. + // + // Only listeners registered before this point are called from here. One + // added afterwards is told by addCompletionListener() itself, because + // _complete is already set - calling it from both would be twice. + uint64_t last = 0; + uint64_t ceiling; { std::lock_guard lock(_listenerMutex); - for (auto &[token, listener] : _listeners) toCall.push_back(listener); + ceiling = _nextListenerToken - 1; } - for (auto &listener : toCall) + for (;;) + { + Listener listener; + { + std::lock_guard 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 lock(_listenerMutex); + _listenerInCall = 0; + } + _listenerCv.notify_all(); + } } std::vector PresetIndex::presets() const @@ -617,10 +749,21 @@ uint64_t PresetIndex::addCompletionListener(Listener listener) void PresetIndex::removeCompletionListener(uint64_t token) { - std::lock_guard lock(_listenerMutex); + std::unique_lock lock(_listenerMutex); _listeners.erase(std::remove_if(_listeners.begin(), _listeners.end(), [token](const auto &pair) { return pair.first == token; }), _listeners.end()); + + // Erasing stops future calls; it does nothing about the one finish() may + // be in the middle of right now, on the crawl thread, into the very object + // whose destructor is calling us. So wait for it - and only for it: a + // wrapper being torn down must not stall behind some other instance's + // callback. The one case that must not wait is the listener removing + // itself from inside its own call, which is on the crawl thread and would + // be waiting for itself. Once the wait returns the token is out of the + // list, so the walk in finish() cannot pick it up again. + if (_listenerInCall == token && _listenerInCallThread != std::this_thread::get_id()) + _listenerCv.wait(lock, [this, token]() { return _listenerInCall != token; }); } } // namespace Clap diff --git a/src/detail/clap/preset_discovery.h b/src/detail/clap/preset_discovery.h index d9cc12d7..31855ad7 100644 --- a/src/detail/clap/preset_discovery.h +++ b/src/detail/clap/preset_discovery.h @@ -111,8 +111,19 @@ class PresetIndex // thread, including concurrently. static std::shared_ptr 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 stops its crawl: every crawl thread has been + // joined by the time this returns, whether or not a wrapper still holds the + // shared_ptr (those stay valid; they just hold a finished index). + // + // This is the last thing that may run before the hosted CLAP is + // deinit()ed and unloaded, and it has to run *then*, not at static + // destruction: the crawl thread is inside provider->get_metadata() in that + // very module, so unloading first leaves it executing unmapped code (Reaper's + // in-process rescan), and joining it later from a static destructor means + // joining under DLL_PROCESS_DETACH on Windows, where the loader lock keeps + // the thread from ever exiting. See the ModuleTerminator in + // wrapasvst3_entry.cpp, which is the caller. Must not be called from a + // crawl thread (a listener, say) - it joins them. static void resetCache(); ~PresetIndex(); @@ -158,6 +169,17 @@ 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() guarantees that when it returns the listener + // is not running and will not run again. If the crawl thread is inside that + // very listener at the moment, the call blocks until it comes back - which + // is what makes "remove in the destructor" actually safe: Logic and auval + // dispose an AU some 100 ms after instantiating it, squarely inside a + // folder crawl, and an erase that cannot recall an in-flight callback would + // let it run on freed memory. Two rules follow for listeners: they may + // re-enter the index, including removing themselves (that is detected and + // does not block), but they must not wait on the thread that may be + // removing them, and they must not drop the last reference to the index. using Listener = std::function; uint64_t addCompletionListener(Listener listener); void removeCompletionListener(uint64_t token); @@ -168,6 +190,9 @@ 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(); + // Tells the crawl to stop at the next file and waits for it. Idempotent; + // what both the destructor and resetCache() do. + 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 +208,15 @@ class PresetIndex std::condition_variable _completionCv; std::mutex _listenerMutex; - std::vector> _listeners; + std::vector> _listeners; // in token order uint64_t _nextListenerToken{1}; + // Which listener finish() is executing right now (0: none), and on which + // thread, so removeCompletionListener() can wait for exactly that one - and + // can tell a listener removing itself apart from a destructor racing it. + // All three are guarded by _listenerMutex. + uint64_t _listenerInCall{0}; + std::thread::id _listenerInCallThread; + std::condition_variable _listenerCv; std::thread _thread; std::atomic _abandon{false}; diff --git a/src/wrapasvst3_entry.cpp b/src/wrapasvst3_entry.cpp index 2ea2ebf6..f7c593c5 100644 --- a/src/wrapasvst3_entry.cpp +++ b/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,21 @@ Clap::Library &hostedClapLibrary() static Steinberg::ModuleTerminator gReleaseHostedClapLibrary( []() { + // The preset index first, and it must be first: its crawl thread runs + // inside the hosted .clap - provider->get_metadata() is the plugin's + // code - so the library must not be deinit()ed and unmapped underneath + // it. That is the same class of bug as the static-destruction note + // above, with the order inverted rather than absent: here the library + // would go before the thread that uses it. Reaper's in-process rescan + // is where it showed - the plugin's globals torn down under a crawl + // still running, or, when the crawl was joined later from the static + // IndexCache destructor under DLL_PROCESS_DETACH, a scan that never + // finished. resetCache() joins every crawl before it returns, and this + // is the point where that is still safe: ExitDll()/bundleExit()/ + // ModuleExit() are called by the host, not from DllMain, so nothing + // holds the loader lock while the join waits. + Clap::PresetIndex::resetCache(); + delete gHostedClapLibrary; gHostedClapLibrary = nullptr; }); From 1d4de064c03ce412edf581a799aa233e97db06b0 Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:09:24 +0200 Subject: [PATCH 3/3] Trim comment noise in the 0.16 crash-class fixes Keep the non-obvious why, drop the narration. --- src/detail/clap/preset_discovery.cpp | 96 ++++------------------ src/detail/clap/preset_discovery.h | 41 +++------ src/detail/standalone/macos/AppDelegate.mm | 16 +--- src/wrapasvst3_entry.cpp | 16 +--- 4 files changed, 36 insertions(+), 133 deletions(-) diff --git a/src/detail/clap/preset_discovery.cpp b/src/detail/clap/preset_discovery.cpp index a256bb01..39bab0b5 100644 --- a/src/detail/clap/preset_discovery.cpp +++ b/src/detail/clap/preset_discovery.cpp @@ -56,33 +56,20 @@ std::string normalizeExtension(const char *declared) return toLower(value); } -// The plugin speaks UTF-8 - every string across the CLAP ABI is, and -// Library::load hands the plugin its own path as u8string() - while fs::path -// speaks the OS. On Windows those are two different things: fs::path{std::string} -// decodes through the ANSI code page, so a UTF-8 "C:\Users\Jürgen\...\Presets" -// turns into a folder that does not exist and the location is silently -// skipped; and in the other direction path::string() *throws* -// std::system_error for any name the code page cannot express ("パッド.preset" -// on a Western-locale machine), which on a bare thread is std::terminate for -// the host. fsutil.cpp already goes through native()/u8string() everywhere for -// exactly this reason; this is the one conversion it does not need, UTF-8 in. -// -// Not fs::u8path(): deprecated in C++20, which this project builds with under -// -Werror. +// 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(utf8.size()); const int length = ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, nullptr, 0); - if (length <= 0) return {}; // not UTF-8 after all: no such folder, same as a typo + if (length <= 0) return {}; std::wstring wide(static_cast(length), L'\0'); ::MultiByteToWideChar(CP_UTF8, 0, utf8.data(), size, wide.data(), length); return fs::path{std::move(wide)}; #else - // POSIX filenames are bytes and every platform we ship on treats them as - // UTF-8 - the same assumption mac_helpers.mm makes with - // fs::path{[u fileSystemRepresentation]}. return fs::path{utf8}; #endif } @@ -143,8 +130,7 @@ struct PresetIndex::Declarations // "If empty or NULL then every file should be matched." if (extensions.empty()) return false; - // u8string(), not string(): see pathFromUtf8 - an extension is as capable - // of being outside the ANSI code page as the rest of the name. + // u8string(), not string(): see pathFromUtf8. auto ext = path.extension().u8string(); if (!ext.empty() && ext.front() == '.') ext.erase(ext.begin()); ext = toLower(ext); @@ -423,17 +409,8 @@ std::shared_ptr PresetIndex::forPlugin(const Library *library, cons void PresetIndex::resetCache() { - // Take the indices out from under the lock, then stop them outside it. - // Clearing the map alone is not enough: a wrapper that is still alive (a - // host that unloads with instances open, or simply a shared_ptr that has - // not been dropped yet) keeps the index and therefore its thread alive, and - // the whole point of this call is that no crawl thread is left running - // inside a module that is about to be unloaded. So every crawl is abandoned - // and joined explicitly, refcount or not. - // - // The join happens outside cache.mutex, so a crawl thread that is at this - // moment still starting up cannot end up behind it. (It never takes that - // mutex today; the ordering is kept so that stays a non-issue.) + // 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> dropped; { auto &cache = indexCache(); @@ -452,10 +429,7 @@ PresetIndex::~PresetIndex() void PresetIndex::abandon() { _abandon.store(true); - // The crawl checks _abandon between files, so this waits out at most the - // get_metadata() call in flight. join() also renders the thread - // non-joinable, which is what makes a second call (destructor after - // resetCache) a no-op. + // join() leaves the thread non-joinable, so a second call is a no-op. if (_thread.joinable()) _thread.join(); } @@ -465,13 +439,7 @@ void PresetIndex::start(const Library *library, const std::string &pluginId) _thread = std::thread( [this, factory, pluginId]() { - // Nothing may escape this body: it is a bare std::thread, and an - // exception leaving it is std::terminate - the host killed over one - // odd file in a user folder. The conversions in crawl() are chosen not - // to throw (see pathFromUtf8), and everything filesystem-side goes - // through an error_code, so what this catches is the remainder: a - // provider that throws across the C ABI, an allocation failure, a - // standard library that found a way we did not think of. + // Bare std::thread: an escaping exception is std::terminate. try { crawl(factory, pluginId); @@ -485,11 +453,7 @@ void PresetIndex::start(const Library *library, const std::string &pluginId) LOGINFO("preset crawl for '{}' aborted by a non-standard exception", pluginId); } - // Whatever happened above, this is still a crawl that ended, and it - // must say so: a wrapper blocked in waitUntilComplete() would - // otherwise sit out its whole timeout, and a listener would wait - // forever for a completion that already happened. What has been - // collected so far stays, and stays sorted if crawl() got that far. + // On every path, or waitUntilComplete() and the listeners never fire. finish(); }); } @@ -582,8 +546,7 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str if (!declarations.matchesExtension(it->path())) continue; if (++seen > kMaxPresetsPerLocation) break; - // u8string(): what the plugin expects back in from_location(), and - // the only narrowing that cannot throw on Windows - see pathFromUtf8. + // 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); @@ -612,8 +575,7 @@ void PresetIndex::crawl(const clap_preset_discovery_factory_t *factory, std::str }); } - // finish() is deliberately not called here but by the thread body in - // start(), so that it also runs when this function is left by exception. + // finish() is called by the thread body in start(), so it also runs on throw. } bool PresetIndex::waitUntilComplete(unsigned timeoutMs) @@ -636,26 +598,10 @@ void PresetIndex::finish() } _completionCv.notify_all(); - // Listeners are called one at a time, each looked up under the lock at the - // moment it is its turn, and none of them under the lock. - // - // Not a snapshot copied up front: a copy cannot be recalled, so a wrapper - // that removed its listener while the copy was being worked through would - // still be called - on a destroyed instance, if that removal was its - // destructor. Looking each one up as it comes means a listener removed - // before its turn is simply never seen; and the one in flight is - // published in _listenerInCall so removeCompletionListener() can wait for - // it. Tokens are handed out in increasing order and _listeners is kept in - // insertion order, so "first entry with a token above the last one called" - // is the next listener in line, however the list changed in between. - // - // Not under the lock either: a listener may re-enter the index, and a - // listener that calls add/removeCompletionListener() under a non-recursive - // mutex it already holds is a deadlock. - // - // Only listeners registered before this point are called from here. One - // added afterwards is told by addCompletionListener() itself, because - // _complete is already set - calling it from both would be twice. + // 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; { @@ -754,14 +700,8 @@ void PresetIndex::removeCompletionListener(uint64_t token) [token](const auto &pair) { return pair.first == token; }), _listeners.end()); - // Erasing stops future calls; it does nothing about the one finish() may - // be in the middle of right now, on the crawl thread, into the very object - // whose destructor is calling us. So wait for it - and only for it: a - // wrapper being torn down must not stall behind some other instance's - // callback. The one case that must not wait is the listener removing - // itself from inside its own call, which is on the crawl thread and would - // be waiting for itself. Once the wait returns the token is out of the - // list, so the walk in finish() cannot pick it up again. + // 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; }); } diff --git a/src/detail/clap/preset_discovery.h b/src/detail/clap/preset_discovery.h index 31855ad7..4d08f582 100644 --- a/src/detail/clap/preset_discovery.h +++ b/src/detail/clap/preset_discovery.h @@ -111,19 +111,10 @@ class PresetIndex // thread, including concurrently. static std::shared_ptr forPlugin(const Library *library, const std::string &pluginId); - // Drops every cached index and stops its crawl: every crawl thread has been - // joined by the time this returns, whether or not a wrapper still holds the - // shared_ptr (those stay valid; they just hold a finished index). - // - // This is the last thing that may run before the hosted CLAP is - // deinit()ed and unloaded, and it has to run *then*, not at static - // destruction: the crawl thread is inside provider->get_metadata() in that - // very module, so unloading first leaves it executing unmapped code (Reaper's - // in-process rescan), and joining it later from a static destructor means - // joining under DLL_PROCESS_DETACH on Windows, where the loader lock keeps - // the thread from ever exiting. See the ModuleTerminator in - // wrapasvst3_entry.cpp, which is the caller. Must not be called from a - // crawl thread (a listener, say) - it joins them. + // 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(); @@ -169,17 +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() guarantees that when it returns the listener - // is not running and will not run again. If the crawl thread is inside that - // very listener at the moment, the call blocks until it comes back - which - // is what makes "remove in the destructor" actually safe: Logic and auval - // dispose an AU some 100 ms after instantiating it, squarely inside a - // folder crawl, and an erase that cannot recall an in-flight callback would - // let it run on freed memory. Two rules follow for listeners: they may - // re-enter the index, including removing themselves (that is detected and - // does not block), but they must not wait on the thread that may be - // removing them, and they must not drop the last reference to the index. + // 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; uint64_t addCompletionListener(Listener listener); void removeCompletionListener(uint64_t token); @@ -190,8 +174,7 @@ 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(); - // Tells the crawl to stop at the next file and waits for it. Idempotent; - // what both the destructor and resetCache() do. + // Stops the crawl and waits for it. Idempotent. void abandon(); // The receiver and indexer callbacks the provider talks to. They live here @@ -210,10 +193,8 @@ class PresetIndex std::mutex _listenerMutex; std::vector> _listeners; // in token order uint64_t _nextListenerToken{1}; - // Which listener finish() is executing right now (0: none), and on which - // thread, so removeCompletionListener() can wait for exactly that one - and - // can tell a listener removing itself apart from a destructor racing it. - // All three are guarded by _listenerMutex. + // 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; diff --git a/src/detail/standalone/macos/AppDelegate.mm b/src/detail/standalone/macos/AppDelegate.mm index b93fe71c..2d625514 100644 --- a/src/detail/standalone/macos/AppDelegate.mm +++ b/src/detail/standalone/macos/AppDelegate.mm @@ -261,15 +261,9 @@ - (void)applicationWillTerminate:(NSNotification *)aNotification freeaudio::clap_wrapper::standalone::getStandaloneHost()->displayAudioError = nullptr; freeaudio::clap_wrapper::standalone::getStandaloneHost()->onRequestResize = nullptr; - // The GUI teardown needs the plugin, so take a reference - but only for the - // duration of this block. getMainPlugin() hands back a shared_ptr copy, and - // that copy must be released *before* mainFinish, which resets the global - // and the host's references and then runs entry->deinit(). Had the copy - // lived to the end of this method it would have been the last owner, and - // Clap::Plugin's destructor would have called _plugin->destroy() on an - // entry that was already deinited - a spec violation, and a use-after-free - // of the library in the dynamically loaded case. Windows does the same - // release-before-mainFinish dance in its WM_DESTROY handler. + // 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. { auto plugin = freeaudio::clap_wrapper::standalone::getMainPlugin(); @@ -280,9 +274,7 @@ - (void)applicationWillTerminate:(NSNotification *)aNotification } } - // Stop the timer before mainFinish: its callback dereferences - // getMainPlugin() without a null check, and after mainFinish the global is - // reset and the StandaloneHost it also reads is gone. + // Before mainFinish: the callback dereferences getMainPlugin() unchecked. [self.requestCallbackTimer invalidate]; self.requestCallbackTimer = nil; diff --git a/src/wrapasvst3_entry.cpp b/src/wrapasvst3_entry.cpp index f7c593c5..35e9ab4c 100644 --- a/src/wrapasvst3_entry.cpp +++ b/src/wrapasvst3_entry.cpp @@ -103,19 +103,9 @@ Clap::Library &hostedClapLibrary() static Steinberg::ModuleTerminator gReleaseHostedClapLibrary( []() { - // The preset index first, and it must be first: its crawl thread runs - // inside the hosted .clap - provider->get_metadata() is the plugin's - // code - so the library must not be deinit()ed and unmapped underneath - // it. That is the same class of bug as the static-destruction note - // above, with the order inverted rather than absent: here the library - // would go before the thread that uses it. Reaper's in-process rescan - // is where it showed - the plugin's globals torn down under a crawl - // still running, or, when the crawl was joined later from the static - // IndexCache destructor under DLL_PROCESS_DETACH, a scan that never - // finished. resetCache() joins every crawl before it returns, and this - // is the point where that is still safe: ExitDll()/bundleExit()/ - // ModuleExit() are called by the host, not from DllMain, so nothing - // holds the loader lock while the join waits. + // 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;