From 27c96135cb8714f3a205810f264a1e0ee3ec24d3 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 30 May 2021 02:06:52 +0200 Subject: [PATCH] Fix thread safety issue with VST2 plugin groups While a VST2 plugin is being initialized in a plugin group, all host callbacks would go over that new plugin instance's sockets. This would cause a race condition if the host processes audio while loading plugins. This issue has been there since the introduction of plugin groups, but it's only noticeable in Bitwig Studio, and only when using over thirty instances of the same plugin in a plugin group. --- CHANGELOG.md | 4 ++++ src/wine-host/bridges/vst2.cpp | 41 +++++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60702ff0..6060059e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,10 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed +- Fixed a longstanding thread safety issue when hosting multiple instances of a + VST2 plugin in a plugin group. This could cause plugins to crash or freeze + when initializing a new instance of a VST2 plugin in a plugin group while + another instance of the same plugin is processing audio. - Fixed yabridge's Wine processes inheriting file descriptors in some situations. This could cause **Ardour** and **Mixbus** to hang when trying to reopen it after a crash. The watchdog timer added in yabridge 3.2.0 also diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 130f4f78..513d6dfe 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -26,9 +26,23 @@ */ using VstEntryPoint = AEffect*(VST_CALL_CONV*)(audioMasterCallback); +/** + * If `plugin->ptr2` is set to this value, then we'll know that `plugin->ptr1` + * is a valid pointer to a `Vst2Bridge` instance. This is needed for when one + * instance of a plugin in a plugin group processes audio while another instance + * of that plugin in the same plugin group is being initialized. In that + * situation we cannot rely on just `current_bridge_instance`, and some plugins + * don't zero initialize these pointers like they should so we also can't rely + * on that. + */ +constexpr size_t yabridge_ptr2_magic = 0xdeadbeef + 420; + /** * This ugly global is needed so we can get the instance of a `Vst2Bridge` class * from an `AEffect` when it performs a host callback during its initialization. + * + * We don't need any locking here because we can only initialize `Vst2Bridge` + * from the main thread anyways. */ Vst2Bridge* current_bridge_instance = nullptr; @@ -92,13 +106,18 @@ host_callback_proxy(AEffect*, int, int, intptr_t, void*, float); * the VST C API. */ Vst2Bridge& get_bridge_instance(const AEffect* plugin) { - // This is needed during the initialization of the plugin since we can only - // add our own pointer after it's done initializing - if (current_bridge_instance) { - return *current_bridge_instance; + if (plugin && reinterpret_cast(plugin->ptr2) == yabridge_ptr2_magic) + [[likely]] { + return *static_cast(plugin->ptr1); } - return *static_cast(plugin->ptr1); + // We can only set this pointer after the plugin has initialized, so when + // the plugin performs a callback during its initialization we'll use the + // current bridge instance set during the Vst2Bridge constructor. This is + // thread safe because VST2 plugins have to be initialized on the main + // thread. + assert(current_bridge_instance); + return *current_bridge_instance; } Vst2Bridge::Vst2Bridge(MainContext& main_context, @@ -149,10 +168,9 @@ Vst2Bridge::Vst2Bridge(MainContext& main_context, sockets.connect(); - // Initialize after communication has been set up We'll try to do the same - // `get_bridge_instance` trick as in `plugin/bridges/vst2.cpp`, but since - // the plugin will probably call the host callback while it's initializing - // we sadly have to use a global here. + // We'll try to do the same `get_bridge_instance()` trick as in + //`plugin/bridges/vst2.cpp`, but since the plugin will probably call the + // host callback while it's initializing we sadly have to use a global here. // Note that this reinterpret cast is not needed at all since the function // pointer types are exactly the same, but clangd will complain otherwise current_bridge_instance = this; @@ -163,9 +181,12 @@ Vst2Bridge::Vst2Bridge(MainContext& main_context, "' failed to initialize."); } - // We only needed this little hack during initialization + // We use `plugin->ptr2` to identify plugins that have already been + // initialized. Otherwise we can run into thread safety issues when a plugin + // is processing audio while another plugin is being initialized. current_bridge_instance = nullptr; plugin->ptr1 = this; + plugin->ptr2 = reinterpret_cast(yabridge_ptr2_magic); // Send the plugin's information to the Linux VST plugin. Any other updates // of this object will be sent over the `dispatcher()` socket. This would be