Unsoundness: TOCTOU allows `Port` and `Transport` to outlive the `Client`

[t1mlange]

Hi,

the following safe Rust program exhibits UB:

use std::{sync::Arc, time::Duration};

fn main() -> anyhow::Result<()> {
    for _ in 0..1000 { // just to increase the chance of hitting
        let mut handles = vec![];
        let in_port;
        let transport;
        {
            let (client, _) = jack::Client::new("test_client", jack::ClientOptions::default())?;
            in_port = client.register_port("input", jack::AudioIn::default())?;
            transport = Arc::new(client.transport());
            println!("Actual Count: {:?}", in_port.connected_count());
            // println!("Actual Query: {:?}", transport.query());
            for i in 15..25 {
                let in_port = in_port.clone_unowned();
                let transport = transport.clone();
                handles.push(std::thread::spawn(move || {
                    std::thread::sleep(Duration::from_millis(i));
                    println!("Race: {:?}", in_port.connected_count());
                    // println!("Race: {:?}", transport.query());
                }));
            }
        }
        
        for handle in handles {
            handle.join().ok();
        }
    }

    Ok(())
}

$ cargo r
Actual Count: Ok(0)
Race: Err(ClientIsNoLongerAlive)
Race: Err(ClientIsNoLongerAlive)
Race: Err(ClientIsNoLongerAlive)
Race: Err(ClientIsNoLongerAlive)
Race: Err(ClientIsNoLongerAlive)
Race: Ok(18446744073709551615) // I guess UAFs on connected_count()?
Race: Ok(18446744073709551615)
Race: Ok(18446744073709551615)
Race: Ok(18446744073709551615)
Race: Ok(18446744073709551615)
Actual Count: Ok(0)
Race: Err(ClientIsNoLongerAlive)
[...]

If you replace the connected_count() call with name() (or uncomment the Tranport lines), it's also possible to trigger a segfault.

I noticed that you already thought about how to ensure that a Port/Transport cannot outlive a Client by having a weak reference and checking before each use whether the Client behind the weak ref is still alive. However, that doesn't actually do what you want because you instantly drop the strong reference after upgrading. If you want to keep the "weak ref and runtime error if the lifetimes are wrong"-behavior, you need to hold the strong reference for as long as you use the port:

- fn check_client_life(&self) -> Result<(), Error> {
+ fn check_client_life(&self) -> Result<Arc<Client>, Error> {
     self.client_life
         .upgrade()
-        .map(|_| ())
         .ok_or(Error::ClientIsNoLongerAlive)
 }

You could also just exchange the weak ref for a strong one in the first place. May I ask why you did choose to have a weak reference here?

Kind Regards
Tim

[wmedrano]

The weak reference is in case clients want to ensure that things are properly closed by deactivate. So, its to prevent people from accidentally leaving a client active.

In practice, I'm not sure if it matters but its probably safer not to change existing behavior.