From 7172a42c679e0bf48537be47fd8005512f2cd02d Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 16 Mar 2020 13:58:49 +0100 Subject: [PATCH] Prevent data races in host callbacks --- README.md | 2 -- src/common/events.h | 16 ++++++++++++++++ src/plugin/host-bridge.cpp | 21 +++++---------------- src/plugin/host-bridge.h | 9 +++------ src/wine-host/plugin-bridge.cpp | 4 ++-- src/wine-host/plugin-bridge.h | 8 ++++++++ 6 files changed, 34 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 647a88ea..b23d865a 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,6 @@ There are a few things that should be done before releasing this, including: - All effect plugins report that they have a sidechain input and multiple outputs. This might be because of the initial value of the `AEffect` struct we pass to the host callbacks during initialization? Not sure. - - Add a similar seaphore to the plugin's `audioMaster` callback handler to - prevent requests from multiple threads being sent at once. - Add missing details if any to the architecture section. - Document what this has been tested on and what does or does not work. - Document wine32 support. diff --git a/src/common/events.h b/src/common/events.h index c4427d47..e36ac232 100644 --- a/src/common/events.h +++ b/src/common/events.h @@ -16,6 +16,8 @@ #pragma once +#include + #include "communication.h" #include "logging.h" @@ -97,6 +99,12 @@ class DefaultDataConverter { * since they follow the same format. See one of those functions for details on * the parameters and return value of this function. * + * @param socket The socket to write over, should be the same socket the other + * endpoint is using to call `passthrough_event()`. + * @param write_semaphore A mutex to ensure that only one thread can write to + * the socket at once. Needed because VST hosts and plugins can and sometimes + * will call the `dispatch()` or `audioMaster()` functions from multiple + * threads at once. * @param data_converter Some struct that knows how to read data from and write * data back to the `data` void pointer. For host callbacks this parameter * contains either a string or a null pointer while `dispatch()` calls might @@ -111,6 +119,7 @@ class DefaultDataConverter { */ template intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, + std::mutex& write_semaphore, D& data_converter, int opcode, int index, @@ -135,7 +144,14 @@ intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, } const Event event{opcode, index, value, option, payload.value()}; + + // Prevent two threads from writing over the socket at the same time. This + // should not be needed, but for instance Bitwig's plugin bridge will + // sometimes repeatedly send events from an off thread that may overlap with + // other `dispatch()` calls. + write_semaphore.lock(); write_object(socket, event); + write_semaphore.unlock(); const auto response = read_object(socket); diff --git a/src/plugin/host-bridge.cpp b/src/plugin/host-bridge.cpp index 935eb790..0a987ccd 100644 --- a/src/plugin/host-bridge.cpp +++ b/src/plugin/host-bridge.cpp @@ -262,8 +262,8 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/, // Allow the plugin to handle its own shutdown const auto return_value = send_event( - host_vst_dispatch, converter, opcode, index, value, data, - option, std::pair(logger, true)); + host_vst_dispatch, dispatch_semaphore, converter, opcode, index, + value, data, option, std::pair(logger, true)); // Boost.Process will send SIGKILL to the Wien host for us when this // class gets destroyed. Because the process is running a few @@ -293,20 +293,9 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/, } // TODO: Maybe reuse buffers here when dealing with chunk data - - // Finish message Bitwig's plugin host sometimes calls the dispatch function - // for opcode 52 from an off thread. This shouldn't be happening, but it - // does. To prevent race conditions from multiple writes over the same - // socket at once we'll make sure that only one thread can send sockets at - // once. These locks are actually only needed around the `write_object()` - // part of `send_event()`. - dispatch_semaphore.lock(); - const auto return_value = - send_event(host_vst_dispatch, converter, opcode, index, value, data, - option, std::pair(logger, true)); - dispatch_semaphore.unlock(); - - return return_value; + return send_event(host_vst_dispatch, dispatch_semaphore, converter, opcode, + index, value, data, option, + std::pair(logger, true)); } void HostBridge::process_replacing(AEffect* /*plugin*/, diff --git a/src/plugin/host-bridge.h b/src/plugin/host-bridge.h index 852c5da6..b87fe711 100644 --- a/src/plugin/host-bridge.h +++ b/src/plugin/host-bridge.h @@ -158,12 +158,9 @@ class HostBridge { std::thread host_callback_handler; /** - * A binary semaphore for preventing the dispatch function from being called - * by two threads at once. This rarely happens and shouldn't event really be - * happening, but in Bitwig's plugin bridge sometimes calls the dispatch - * function with opcode 52 while it's still waiting for another dispatch - * call to return, which would cause two threads to write over the same - * socket at the same time. + * A binary semaphore to prevent race conditions from the dispatch function + * being called by two threads at once. See `send_event()` for more + * information. */ std::mutex dispatch_semaphore; diff --git a/src/wine-host/plugin-bridge.cpp b/src/wine-host/plugin-bridge.cpp index 49c8bab2..7d35a0b8 100644 --- a/src/wine-host/plugin-bridge.cpp +++ b/src/wine-host/plugin-bridge.cpp @@ -268,8 +268,8 @@ intptr_t PluginBridge::host_callback(AEffect* /*plugin*/, void* data, float option) { HostCallbackDataConverter converter(plugin, time_info); - return send_event(vst_host_callback, converter, opcode, index, value, data, - option, std::nullopt); + return send_event(vst_host_callback, host_callback_semaphore, converter, + opcode, index, value, data, option, std::nullopt); } intptr_t VST_CALL_CONV host_callback_proxy(AEffect* effect, diff --git a/src/wine-host/plugin-bridge.h b/src/wine-host/plugin-bridge.h index 9eb90007..6d9701fe 100644 --- a/src/wine-host/plugin-bridge.h +++ b/src/wine-host/plugin-bridge.h @@ -27,6 +27,7 @@ #include #include +#include #include #include "../common/logging.h" @@ -115,6 +116,13 @@ class PluginBridge { */ std::thread process_replacing_handler; + /** + * A binary semaphore to prevent race conditions from the host callback + * function being called by two threads at once. See `send_event()` for more + * information. + */ + std::mutex host_callback_semaphore; + /** * A scratch buffer for sending and receiving data during `process` and * `processReplacing` calls.