Decode non-UTF-8 client and port names lossily

Author: wmedrano

CHANGELOG.md

@@ -36,6 +36,8 @@
   bytes are replaced with `U+FFFD`. `PropertyChange` now implements `Clone`.
 - Actually stop processing if buffer size callback returns Quit. This was not
   respected in Pipewire.
+- Use lossy string decoding for non-UTF8 names. May affect reading metadata from
+  non-Rust clients.
 
 ## 0.9.0
 

src/client/callbacks.rs

@@ -352,10 +352,10 @@ unsafe extern "C" fn client_registration<N, P>(
             let Some(ctx) = CallbackContext::<N, P>::from_raw(data) else {
                 return;
             };
-            let name = ffi::CStr::from_ptr(name).to_str().unwrap();
+            let name = ffi::CStr::from_ptr(name).to_string_lossy();
             let register = !matches!(register, 0);
             ctx.notification
-                .client_registration(&ctx.client, name, register);
+                .client_registration(&ctx.client, &name, register);
         });
         if let Err(err) = res {
             if let Some(ctx) = CallbackContext::<N, P>::from_raw(data) {
@@ -410,11 +410,11 @@ where
             let Some(ctx) = CallbackContext::<N, P>::from_raw(data) else {
                 return Control::Quit;
             };
-            let old_name = ffi::CStr::from_ptr(old_name).to_str().unwrap();
-            let new_name = ffi::CStr::from_ptr(new_name).to_str().unwrap();
+            let old_name = ffi::CStr::from_ptr(old_name).to_string_lossy();
+            let new_name = ffi::CStr::from_ptr(new_name).to_string_lossy();
             let c = ctx
                 .notification
-                .port_rename(&ctx.client, port_id, old_name, new_name);
+                .port_rename(&ctx.client, port_id, &old_name, &new_name);
             if c == Control::Quit {
                 ctx.mark_invalid(false);
             }

src/client/client_impl.rs

@@ -37,6 +37,9 @@ pub struct Client(Arc<ClientHandle>);
 /// The client is closed when the last strong reference is dropped.
 pub struct ClientHandle {
     client_ptr: *mut j::jack_client_t,
+    // The client name, decoded lossily once when the client is opened. JACK never renames an
+    // open client, so the cached value stays accurate.
+    name: Box<str>,
     // Keep callback storage alive until after jack_client_close returns.
     property_change_handler: Mutex<Option<Box<dyn PropertyChangeHandler>>>,
 }
@@ -189,12 +192,10 @@ impl Client {
     /// as JACK will may rename a client if necessary (ie: name collision, name too long). The name
     /// will only the be different than the one passed to `Client::new` if the `ClientStatus` was
     /// `NAME_NOT_UNIQUE`.
+    ///
+    /// Bytes that are not valid UTF-8 are replaced with `U+FFFD REPLACEMENT CHARACTER`.
     pub fn name(&self) -> &str {
-        unsafe {
-            let ptr = j::jack_get_client_name(self.raw());
-            let cstr = ffi::CStr::from_ptr(ptr);
-            cstr.to_str().unwrap()
-        }
+        &self.0.name
     }
 
     /// The current maximum size that will every be passed to the process
@@ -264,10 +265,7 @@ impl Client {
         unsafe {
             let uuid_s = j::jack_client_get_uuid(self.raw());
             assert!(!uuid_s.is_null());
-            let uuid = ffi::CStr::from_ptr(uuid_s)
-                .to_str()
-                .expect("uuid is valid string")
-                .to_string();
+            let uuid = ffi::CStr::from_ptr(uuid_s).to_string_lossy().into_owned();
             j::jack_free(uuid_s as _);
             uuid
         }
@@ -280,12 +278,7 @@ impl Client {
             if name_ptr.is_null() {
                 None
             } else {
-                Some(
-                    ffi::CStr::from_ptr(name_ptr)
-                        .to_str()
-                        .expect("name convert to a valid str")
-                        .to_string(),
-                )
+                Some(ffi::CStr::from_ptr(name_ptr).to_string_lossy().into_owned())
             }
         }
     }
@@ -706,8 +699,17 @@ impl Client {
     /// # Safety
     /// It is unsafe to create a `Client` from a raw pointer.
     pub unsafe fn from_raw(p: *mut j::jack_client_t) -> Self {
+        let name = unsafe {
+            let name_ptr = j::jack_get_client_name(p);
+            if name_ptr.is_null() {
+                Box::default()
+            } else {
+                ffi::CStr::from_ptr(name_ptr).to_string_lossy().into()
+            }
+        };
         Client(Arc::new(ClientHandle {
             client_ptr: p,
+            name,
             property_change_handler: Mutex::new(None),
         }))
     }

src/tests/client.rs

@@ -177,3 +177,68 @@ fn client_with_unknown_server_name_fails_to_open() {
         err
     );
 }
+
+/// Opens a client whose name is not valid UTF-8. `Client::new` only accepts `&str`, but other
+/// processes on the server can use any bytes.
+fn open_non_utf8_client(name: &[u8]) -> crate::Client {
+    let name = std::ffi::CString::new(name).unwrap();
+    let mut status = 0;
+    let ptr = unsafe {
+        jack_sys::jack_client_open(name.as_ptr(), jack_sys::JackNoStartServer, &mut status)
+    };
+    assert!(!ptr.is_null(), "jack_client_open failed: {status:#x}");
+    unsafe { crate::Client::from_raw(ptr) }
+}
+
+#[test]
+fn non_utf8_client_name_is_decoded_lossily() {
+    let client = open_non_utf8_client(b"non-utf8-\xff");
+    assert_eq!(client.name(), "non-utf8-\u{fffd}");
+    assert_eq!(
+        client.name_by_uuid(client.uuid()).as_deref(),
+        Some("non-utf8-\u{fffd}")
+    );
+}
+
+#[test]
+fn non_utf8_client_registration_does_not_stop_processing() {
+    use std::sync::atomic::{AtomicUsize, Ordering};
+    use std::sync::{Arc, Mutex};
+
+    struct Registrations(Arc<Mutex<Vec<String>>>);
+    impl crate::NotificationHandler for Registrations {
+        fn client_registration(&mut self, _: &crate::Client, name: &str, is_registered: bool) {
+            if is_registered {
+                self.0.lock().unwrap().push(name.to_string());
+            }
+        }
+    }
+
+    let names = Arc::new(Mutex::new(Vec::new()));
+    let cycles = Arc::new(AtomicUsize::new(0));
+    let process = {
+        let cycles = cycles.clone();
+        crate::contrib::ClosureProcessHandler::new(move |_, _| {
+            cycles.fetch_add(1, Ordering::Relaxed);
+            crate::Control::Continue
+        })
+    };
+    let (client, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap();
+    let ac = client
+        .activate_async(Registrations(names.clone()), process)
+        .unwrap();
+    std::thread::sleep(std::time::Duration::from_millis(100));
+
+    let _other = open_non_utf8_client(b"other-\xfe");
+    std::thread::sleep(std::time::Duration::from_millis(200));
+    assert!(
+        names.lock().unwrap().iter().any(|n| n == "other-\u{fffd}"),
+        "{:?}",
+        names.lock().unwrap()
+    );
+
+    let before = cycles.load(Ordering::Relaxed);
+    std::thread::sleep(std::time::Duration::from_millis(200));
+    assert!(cycles.load(Ordering::Relaxed) > before);
+    ac.deactivate().unwrap();
+}