From b3038b458c35a620fbc07b65a7261693f6e68e0e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 5 May 2024 23:20:45 +0200 Subject: [PATCH] Fix race condition in RunLoopEventHandler --- CHANGELOG.md | 2 ++ src/wrapper/vst3/view.rs | 43 ++++++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5e3ff14..e986fe8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ state is to list breaking changes. ### Fixed +- Fixed a race condition in the VST3 GUI event loop on Linux. This could cause + panics with certain versions of Carla. - The CPAL backend now correctly handles situations where it receives fewer samples than configured. - Fixed the handling of multichannel audio in the CPAL backend. diff --git a/src/wrapper/vst3/view.rs b/src/wrapper/vst3/view.rs index 55601d7d..fd0fd255 100644 --- a/src/wrapper/vst3/view.rs +++ b/src/wrapper/vst3/view.rs @@ -225,12 +225,8 @@ impl RunLoopEventHandler

{ self.tasks.push(task)?; // We need to use a Unix domain socket to let the host know to call our event handler. In - // theory eventfd would be more suitable here, but Ardour does not support that. - // XXX: This can technically lead to a race condition if the host is currently calling - // `on_fd_is_set()` on another thread and the task has already been popped and executed - // and this value has not yet been written to the socket. Doing it the other way around - // gets you the other situation where the event handler could be run without the task - // being posted yet. In practice this won't cause any issues however. + // theory eventfd would be more suitable here, but Ardour does not support that. This is + // read again in `Self::on_fd_is_set()`. let notify_value = 1i8; const NOTIFY_VALUE_SIZE: usize = std::mem::size_of::(); assert_eq!( @@ -472,21 +468,34 @@ impl IPlugViewContentScaleSupport for WrapperView

{ #[cfg(target_os = "linux")] impl IEventHandler for RunLoopEventHandler

{ unsafe fn on_fd_is_set(&self, _fd: FileDescriptor) { + // There should be a one-to-one correlation to bytes being written to `self.socket_read_fd` + // and events being pushed to `self.tasks`, but because the process of pushing a task and + // notifying this thread through the socket is not atomic we can't reliably just read a byte + // from this socket for every task we process. For instance, if `Self::post_task()` gets + // called while this loop is already running, it could happen that we pop and execute the + // task before the byte gets written to the socket. To avoid this, we'll clear the socket + // upfront, and then execute the tasks afterwards. If this situation does happen, then the + // worst thing that can happen is that this function is called a second time while + // `self.tasks()` is already empty. + let mut notify_value = [0; 32]; + loop { + let read_result = libc::read( + self.socket_read_fd, + &mut notify_value as *mut _ as *mut c_void, + std::mem::size_of_val(¬ify_value), + ); + + // If after the first loop the socket contains no more data, then the `read()` call will + // return -1 and `errno` will have been set to `EAGAIN` + if read_result <= 0 { + break; + } + } + // This gets called from the host's UI thread because we wrote some bytes to the Unix domain // socket. We'll read that data from the socket again just to make REAPER happy. while let Some(task) = self.tasks.pop() { self.inner.execute(task, true); - - let mut notify_value = 1i8; - const NOTIFY_VALUE_SIZE: usize = std::mem::size_of::(); - assert_eq!( - libc::read( - self.socket_read_fd, - &mut notify_value as *mut _ as *mut c_void, - NOTIFY_VALUE_SIZE - ), - NOTIFY_VALUE_SIZE as isize - ); } } }