Race condition in `NotificationHandler` callbacks

[iitalics]

I have a very simple JACK app that creates MIDI in/audio out ports, and I noticed getting interleaved logs which indicate that the NotificationHandler API is not properly thread safe.

struct AppNotificationHandler;

impl jack::NotificationHandler for AppNotificationHandler {
    fn thread_init(&self, client: &jack::Client) {
        info!(client_name = client.name(), "Thread init");
        let port_list = client.ports(None, None, jack::PortFlags::empty());
        for (i, port) in port_list.into_iter().enumerate() {
            debug!(i, ?port, "Port list");
        }
    }

    fn port_registration(
        &mut self,
        client: &jack::Client,
        port: jack::PortId,
        is_registered: bool,
    ) {
        debug!("port_registration");
        let port = client.port_by_id(port).unwrap();
        if !client.is_mine(&port) {
            return;
        }
        let port_name = port.name();
        let port_name = port_name.as_deref().unwrap_or("(port)");
        debug!(port=?port_name, is_registered);
    }
}

example logging output:

2025-08-21T01:49:03.248280Z DEBUG beeps: port_registration
2025-08-21T01:49:03.248292Z DEBUG beeps: port="beeps_app:input" is_registered=true
2025-08-21T01:49:03.248299Z DEBUG beeps: port_registration
2025-08-21T01:49:03.248300Z  INFO beeps: Thread init client_name="beeps_app"
2025-08-21T01:49:03.248301Z DEBUG beeps: port="beeps_app:output" is_registered=true
2025-08-21T01:49:03.248315Z DEBUG beeps: Port list i=0 port="Built-in Audio Analog Stereo:capture_FL"
2025-08-21T01:49:03.248320Z DEBUG beeps: Port list i=1 port="Built-in Audio Analog Stereo:capture_FR"

it should be impossible for port_registration() and thread_init() to become interleaved since the former takes &mut self which should give exclusive access to the referenced object

[iitalics]

it looks like there are not really any protections against the callbacks being called from multiple threads, but rather these functions immediately assume the notification handler is safe to use

rust-jack/src/client/callbacks.rs

Lines 342 to 353 in 73c4305

	unsafe extern "C" fn port_registration<N, P>(
	port_id: PortId,
	register: libc::c_int,
	data: *mut libc::c_void,
	) where
	N: 'static + Send + Sync + NotificationHandler,
	P: 'static + Send + ProcessHandler,
	{
	let res = catch_unwind(|| {
	let Some(ctx) = CallbackContext::<N, P>::from_raw(data) else {
	return;
	};

rust-jack/src/client/callbacks.rs

Lines 527 to 536 in 73c4305

	pub unsafe fn from_raw<'a>(ptr: *mut libc::c_void) -> Option<&'a mut CallbackContext<N, P>> {
	debug_assert!(!ptr.is_null());
	let obj_ptr = ptr as *mut CallbackContext<N, P>;
	let obj_ref = &mut *obj_ptr;
	if obj_ref.is_valid_for_callback.load(Ordering::Relaxed) {
	Some(obj_ref)
	} else {
	None
	}
	}

according to std::thread::current().id(), thread_init() is called on a different thread than port_registration().

[iitalics]

I believe that many of these &mut are undefined behavior, but it would be a massively breaking change to modify the signatures of all of the trait methods

[iitalics]

for completeness, here's a program that, non-deterministically, either 1) errors from failed unwrap, 2) succeeds, 3) segfaults

use jack::*;
use std::thread::sleep;
use std::time::Duration;

struct Notify {
    name: Option<&'static str>,
}

impl NotificationHandler for Notify {
    fn thread_init(&self, _: &Client) {
        let name = self.name.as_ref().unwrap();
        for i in 0..100 {
            println!("{}={}", *name, i);
        }
    }

    fn port_registration(&mut self, _: &Client, _: PortId, _: bool) {
        let name = self.name.as_ref().unwrap();
        for i in 0..100 {
            println!("{}={}", *name, i);
        }
        self.name = None;
    }
}

fn main() {
    let (client, _) = Client::new("app", Default::default()).unwrap();
    client.register_port("input", MidiIn::default()).unwrap();
    let notify = Notify { name: Some("i") };
    let client = client.activate_async(notify, ()).unwrap();
    sleep(Duration::from_millis(100));
    client.deactivate().unwrap();
}

compiled with opt-level = 2

$ cargo run 

...

i=64
i=65
i=66
i=67
i=68
i=69
i=70
i=71
i=72
i=73
i=74
i=75
i=76
i=77
i=78
i=79
i=80
i=81
i=99
Segmentation fault (core dumped)

[wmedrano]

What do you think of wrapping NotificationHandler in a mutex?

• The performance critical ProcessHandler is separate so its fine.
• Probably won't break any existing users.
• If needed, we can design a new handler mechanism with separate state per element.

🤔 in practice, I wonder if its only thread_init that's the problem. In theory, JACK doesn't really like to specify thread safety (when i checked years ago) and prefers to leave that to the backend. The major backends being JACK2 and Pipewire.

[iitalics]

i agree that thread_init looks like the problem. if it had a separate handler that might solve the issue, but that is a pretty big api change. wrapping in a mutex would certainly be viable but i can't really speak to the possible performance ramifications so that is the main potential issue.

i think making all of these methods &self is more conceptually sound given that the notification handler needs to be Sync already, and i took a look at cpal and all of the state they store in their notification handler is already wrapped in synchronization mechanisms, so they would be able to change their methods pretty easily. however, breaking consumers isn't a great thing to do, so maybe that would be appropriate for a future release while the immediate fix is to use a mutex.