Return errors instead of panicking on strings with interior NUL

Author: wmedrano

CHANGELOG.md

@@ -28,6 +28,8 @@
   Non-UTF-8 property types no longer panic.
 - `Client::property_get`, `property_get_subject` and `property_get_all` no longer panic on
   non-UTF-8 keys, values or types. Invalid bytes are replaced with `U+FFFD`.
+- Added `Error::InteriorNul`. Methods that take strings now return it instead of panicking
+  when a string contains a NUL byte.
 - `PropertyChange` keys are now `Option<Cow<'a, str>>` and `PropertyChangeOwned`
   keys are now `Option<String>`. The key is `None` when all properties of a subject were removed
   (`Client::property_remove_subject`) or all properties were removed

src/client/client_impl.rs

@@ -5,7 +5,7 @@ use std::{ffi, fmt, ptr};
 
 use crate::client::common::CREATE_OR_DESTROY_CLIENT_MUTEX;
 use crate::jack_enums::CodeOrMessage;
-use crate::jack_utils::collect_strs;
+use crate::jack_utils::{collect_strs, to_cstring};
 use crate::properties::PropertyChangeHandler;
 use crate::transport::Transport;
 use crate::{
@@ -120,24 +120,23 @@ impl Client {
             return Err(Error::LibraryError(err.to_string()));
         }
 
+        let client_name = to_cstring(client_name)?;
+        let server_name = server_name.map(to_cstring).transpose()?;
+
         crate::logging::maybe_init_logging();
         let mut status_bits = 0;
-        let client_name = ffi::CString::new(client_name).unwrap();
         let client = match server_name {
             None => unsafe {
                 j::jack_client_open(client_name.as_ptr(), options.bits(), &mut status_bits)
             },
-            Some(server_name) => {
-                let server_name = ffi::CString::new(server_name).unwrap();
-                unsafe {
-                    j::jack_client_open_with_server_name(
-                        client_name.as_ptr(),
-                        options.bits(),
-                        &mut status_bits,
-                        server_name.as_ptr(),
-                    )
-                }
-            }
+            Some(server_name) => unsafe {
+                j::jack_client_open_with_server_name(
+                    client_name.as_ptr(),
+                    options.bits(),
+                    &mut status_bits,
+                    server_name.as_ptr(),
+                )
+            },
         };
         let status = ClientStatus::from_bits(status_bits).unwrap_or_else(ClientStatus::empty);
         if client.is_null() {
@@ -244,7 +243,7 @@ impl Client {
         raw: *mut jack_sys::jack_client_t,
         name: &str,
     ) -> Option<jack_sys::jack_uuid_t> {
-        let name = ffi::CString::new(name).unwrap();
+        let name = ffi::CString::new(name).ok()?;
         let mut uuid: jack_sys::jack_uuid_t = Default::default();
         unsafe {
             let uuid_s = jack_sys::jack_get_uuid_for_client_name(raw, name.as_ptr());
@@ -294,7 +293,7 @@ impl Client {
 
     /// Get the name of a client by its `&str` uuid.
     pub fn name_by_uuid_str(&self, uuid: &str) -> Option<String> {
-        let uuid = ffi::CString::new(uuid).unwrap();
+        let uuid = ffi::CString::new(uuid).ok()?;
         unsafe { self.name_by_uuid_raw(uuid.as_ptr()) }
     }
 
@@ -316,8 +315,14 @@ impl Client {
         type_name_pattern: Option<&str>,
         flags: PortFlags,
     ) -> Vec<String> {
-        let port_name_pattern_cstr = ffi::CString::new(port_name_pattern.unwrap_or("")).unwrap();
-        let type_name_pattern_cstr = ffi::CString::new(type_name_pattern.unwrap_or("")).unwrap();
+        // A pattern containing a NUL byte can't be passed to JACK, and no port name or type
+        // contains one, so nothing matches.
+        let (Ok(port_name_pattern_cstr), Ok(type_name_pattern_cstr)) = (
+            ffi::CString::new(port_name_pattern.unwrap_or("")),
+            ffi::CString::new(type_name_pattern.unwrap_or("")),
+        ) else {
+            return Vec::new();
+        };
         let flags = libc::c_ulong::from(flags.bits());
         unsafe {
             let ports = j::jack_get_ports(
@@ -352,8 +357,8 @@ impl Client {
         if port_spec.jack_port_type().is_empty() {
             return Err(Error::PortRegistrationError(port_name.to_string()));
         }
-        let port_name_c = ffi::CString::new(port_name).unwrap();
-        let port_type_c = ffi::CString::new(port_spec.jack_port_type()).unwrap();
+        let port_name_c = to_cstring(port_name)?;
+        let port_type_c = to_cstring(port_spec.jack_port_type())?;
         let port_flags = port_spec.jack_flags().bits();
         let buffer_size = port_spec.jack_buffer_size();
         let pp = unsafe {
@@ -384,7 +389,8 @@ impl Client {
 
     /// Get a `Port` by its port name.
     pub fn port_by_name(&self, port_name: &str) -> Option<Port<Unowned>> {
-        let port_name = ffi::CString::new(port_name).unwrap();
+        // No port name contains a NUL byte.
+        let port_name = ffi::CString::new(port_name).ok()?;
         let pp = unsafe { j::jack_port_by_name(self.raw(), port_name.as_ptr()) };
         if pp.is_null() {
             None
@@ -412,9 +418,9 @@ impl Client {
         client_bin_name: &str,
         client_args: &str,
     ) -> Result<InternalClientID, Error> {
-        let ffi_client_name = ffi::CString::new(client_name).unwrap();
-        let ffi_client_bin = ffi::CString::new(client_bin_name).unwrap();
-        let ffi_client_args = ffi::CString::new(client_args).unwrap();
+        let ffi_client_name = to_cstring(client_name)?;
+        let ffi_client_bin = to_cstring(client_bin_name)?;
+        let ffi_client_args = to_cstring(client_args)?;
 
         let mut status_bits = 0;
         let options: j::Enum_JackOptions = j::JackLoadName | j::JackLoadInit;
@@ -506,7 +512,7 @@ impl Client {
         port_name: &str,
         enable_monitor: bool,
     ) -> Result<(), Error> {
-        let port_name_cstr = ffi::CString::new(port_name).unwrap();
+        let port_name_cstr = to_cstring(port_name)?;
         let res = unsafe {
             j::jack_port_request_monitor_by_name(
                 self.raw(),
@@ -558,15 +564,15 @@ impl Client {
     /// 3. The port flags of the `destination_port` must include `IS_INPUT`.
     /// 4. Both ports must be owned by active clients.
     ///
-    /// # Panics
-    /// Panics if it is not possible to convert `source_port` or `destination_port` to a `CString`.
+    /// Returns `Err(Error::InteriorNul)` if `source_port` or `destination_port` contains a NUL
+    /// byte.
     pub fn connect_ports_by_name(
         &self,
         source_port: &str,
         destination_port: &str,
     ) -> Result<(), Error> {
-        let source_cstr = ffi::CString::new(source_port).unwrap();
-        let destination_cstr = ffi::CString::new(destination_port).unwrap();
+        let source_cstr = to_cstring(source_port)?;
+        let destination_cstr = to_cstring(destination_port)?;
         let res =
             unsafe { j::jack_connect(self.raw(), source_cstr.as_ptr(), destination_cstr.as_ptr()) };
         match res {
@@ -661,8 +667,8 @@ impl Client {
         source_port: &str,
         destination_port: &str,
     ) -> Result<(), Error> {
-        let source_port = ffi::CString::new(source_port).unwrap();
-        let destination_port = ffi::CString::new(destination_port).unwrap();
+        let source_port = to_cstring(source_port)?;
+        let destination_port = to_cstring(destination_port)?;
         let res = unsafe {
             j::jack_disconnect(self.raw(), source_port.as_ptr(), destination_port.as_ptr())
         };
@@ -674,12 +680,16 @@ impl Client {
 
     /// The buffer size of a port type
     ///
+    /// Returns 0 if `port_type` contains a NUL byte, since no port type does.
+    ///
     /// # Safety
     ///
     /// * This function may only be called in a buffer size callback.
     pub unsafe fn type_buffer_size(&self, port_type: &str) -> usize {
         unsafe {
-            let port_type = ffi::CString::new(port_type).unwrap();
+            let Ok(port_type) = ffi::CString::new(port_type) else {
+                return 0;
+            };
             j::jack_port_type_get_buffer_size(self.raw(), port_type.as_ptr())
         }
     }

src/jack_enums.rs

@@ -33,6 +33,8 @@ pub enum Error {
     ClientIsNoLongerAlive,
     ClientPanicked,
     RingbufferCreateFailed,
+    /// A string argument contained an interior NUL byte, so it cannot be passed to JACK.
+    InteriorNul(String),
     UnknownError {
         error_code: libc::c_int,
     },
@@ -85,6 +87,7 @@ impl std::fmt::Display for Error {
             Error::ClientIsNoLongerAlive => write!(f, "client is no longer alive"),
             Error::ClientPanicked => write!(f, "client notifcation or processor panicked"),
             Error::RingbufferCreateFailed => write!(f, "ringbuffer creation failed"),
+            Error::InteriorNul(s) => write!(f, "string {s:?} contains an interior NUL byte"),
             Error::UnknownError { error_code } => write!(f, "unkown error with code {error_code}"),
         }
     }

src/jack_utils.rs

@@ -1,6 +1,13 @@
 use jack_sys as j;
 use std::ffi;
 
+use crate::Error;
+
+/// Converts `s` to a `CString`, returning `Error::InteriorNul` if `s` contains a NUL byte.
+pub(crate) fn to_cstring(s: &str) -> Result<ffi::CString, Error> {
+    ffi::CString::new(s).map_err(|_| Error::InteriorNul(s.to_string()))
+}
+
 /// Collects strings from an array of c-strings into a Rust vector of strings
 /// and frees the memory pointed to by `ptr`. The end of the array is marked by
 /// the value of the c-string being the null pointer. `ptr` may be `null`, in

src/port/port_impl.rs

@@ -7,6 +7,7 @@ use std::marker::Sized;
 use std::sync::Arc;
 use std::{ffi, fmt, iter};
 
+use crate::jack_utils::to_cstring;
 use crate::{ClientHandle, Error, Frames, LatencyType, PortFlags};
 
 lazy_static! {
@@ -120,7 +121,10 @@ impl<PS> Port<PS> {
     /// name `port_name`.
     pub fn is_connected_to(&self, port_name: &str) -> Result<bool, Error> {
         let res = unsafe {
-            let port_name = ffi::CString::new(port_name).unwrap();
+            // No port name contains a NUL byte.
+            let Ok(port_name) = ffi::CString::new(port_name) else {
+                return Ok(false);
+            };
             j::jack_port_connected_to(self.raw(), port_name.as_ptr())
         };
         match res {
@@ -182,7 +186,7 @@ impl<PS> Port<PS> {
     /// Set's the short name of the port. If the full name is longer than `PORT_NAME_SIZE`, then it
     /// will be truncated.
     pub fn set_name(&mut self, short_name: &str) -> Result<(), Error> {
-        let short_name = ffi::CString::new(short_name).unwrap();
+        let short_name = to_cstring(short_name)?;
         let res = unsafe { j::jack_port_set_name(self.raw(), short_name.as_ptr()) };
         match res {
             0 => Ok(()),
@@ -201,7 +205,7 @@ impl<PS> Port<PS> {
     /// Ports can have up to two aliases - if both are already set, this function will return an
     /// error.
     pub fn set_alias(&mut self, alias: &str) -> Result<(), Error> {
-        let alias = ffi::CString::new(alias).unwrap();
+        let alias = to_cstring(alias)?;
         let res = unsafe { j::jack_port_set_alias(self.raw(), alias.as_ptr()) };
         match res {
             0 => Ok(()),
@@ -213,7 +217,7 @@ impl<PS> Port<PS> {
     ///
     /// After a successful call, `alias` can no longer be used as an alternate name for `self`.
     pub fn unset_alias(&mut self, alias: &str) -> Result<(), Error> {
-        let alias = ffi::CString::new(alias).unwrap();
+        let alias = to_cstring(alias)?;
         let res = unsafe { j::jack_port_unset_alias(self.raw(), alias.as_ptr()) };
         match res {
             0 => Ok(()),

src/properties.rs

@@ -85,6 +85,7 @@ pub use metadata::*;
 mod metadata {
     use super::{PropertyChange, PropertyChangeHandler, j, uuid};
     use crate::Error;
+    use crate::jack_utils::to_cstring;
     use std::{
         collections::HashMap,
         ffi,
@@ -231,7 +232,8 @@ mod metadata {
         /// * `subject` - The subject of the property.
         /// * `key` - The key of the property, a URI String.
         pub fn property_get(&self, subject: uuid, key: &str) -> Option<Property> {
-            let key = ffi::CString::new(key).expect("key to be convert to CString");
+            // No key contains a NUL byte.
+            let key = ffi::CString::new(key).ok()?;
             let mut value: MaybeUninit<*mut ::libc::c_char> = MaybeUninit::uninit();
             let mut typ: MaybeUninit<*mut ::libc::c_char> = MaybeUninit::uninit();
 
@@ -316,12 +318,11 @@ mod metadata {
             key: &str,
             property: &Property,
         ) -> Result<(), Error> {
-            let key = ffi::CString::new(key).expect("to create cstring from key");
-            let value =
-                ffi::CString::new(property.value.as_str()).expect("to create cstring from value");
+            let key = to_cstring(key)?;
+            let value = to_cstring(&property.value)?;
+            let typ = property.typ().map(to_cstring).transpose()?;
             map_error(|| unsafe {
-                if let Some(t) = property.typ() {
-                    let t = ffi::CString::new(t).unwrap();
+                if let Some(t) = &typ {
                     j::jack_set_property(
                         self.raw(),
                         subject,
@@ -348,7 +349,7 @@ mod metadata {
         /// * `subject` - The subject to remove all properties from.
         /// * `key` - The key of the property to be removed. A URI string.
         pub fn property_remove(&self, subject: uuid, key: &str) -> Result<(), Error> {
-            let key = ffi::CString::new(key).expect("to create cstring from key");
+            let key = to_cstring(key)?;
             map_error(|| unsafe { j::jack_remove_property(self.raw(), subject, key.as_ptr()) })
         }
 

src/tests/client.rs

@@ -1,4 +1,4 @@
-use crate::tests::DEFAULT_TEST_CLIENT;
+use crate::{Error::InteriorNul, tests::DEFAULT_TEST_CLIENT};
 
 #[test]
 fn client_can_open() {
@@ -242,3 +242,90 @@ fn non_utf8_client_registration_does_not_stop_processing() {
     assert!(cycles.load(Ordering::Relaxed) > before);
     ac.deactivate().unwrap();
 }
+
+#[test]
+fn client_open_rejects_interior_nul() {
+    let nul = "a\0b";
+    let options = crate::ClientOptions::default();
+    assert_eq!(
+        crate::Client::new(nul, options).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        crate::Client::new_with_server_name("ok", nul, options).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+}
+
+#[test]
+fn client_methods_reject_interior_nul() {
+    let nul = "a\0b";
+    let client = &*DEFAULT_TEST_CLIENT;
+    assert_eq!(
+        client.register_port(nul, crate::AudioIn::default()).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        client.request_monitor_by_name(nul, true).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        client.connect_ports_by_name(nul, "x").err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        client.connect_ports_by_name("x", nul).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        client.disconnect_ports_by_name(nul, "x").err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+    assert_eq!(
+        client.load_internal_client(nul, "x", "").err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+}
+
+#[test]
+fn property_methods_reject_interior_nul() {
+    let nul = "a\0b";
+    let client = &*DEFAULT_TEST_CLIENT;
+    let uuid = client.uuid();
+    let nul_key = client.property_set(uuid, nul, &crate::Property::new("v", None));
+    assert_eq!(nul_key.err(), Some(InteriorNul(nul.to_string())));
+    let nul_value = client.property_set(uuid, "k", &crate::Property::new(nul, None));
+    assert_eq!(nul_value.err(), Some(InteriorNul(nul.to_string())));
+    let nul_type = client.property_set(uuid, "k", &crate::Property::new("v", Some(nul.into())));
+    assert_eq!(nul_type.err(), Some(InteriorNul(nul.to_string())));
+    assert_eq!(
+        client.property_remove(uuid, nul).err(),
+        Some(InteriorNul(nul.to_string()))
+    );
+}
+
+#[test]
+fn lookups_with_interior_nul_find_nothing() {
+    let nul = "a\0b";
+    let client = &*DEFAULT_TEST_CLIENT;
+    let flags = crate::PortFlags::empty();
+    assert!(client.port_by_name(nul).is_none());
+    assert!(client.uuid_of_client_by_name(nul).is_none());
+    assert!(client.name_by_uuid_str(nul).is_none());
+    assert!(client.property_get(client.uuid(), nul).is_none());
+    assert!(client.ports(Some(nul), None, flags).is_empty());
+    assert!(client.ports(None, Some(nul), flags).is_empty());
+}
+
+#[test]
+fn port_methods_reject_interior_nul() {
+    let nul = "a\0b";
+    let (client, _) = crate::Client::new("", crate::ClientOptions::default()).unwrap();
+    let mut port = client
+        .register_port("p", crate::AudioIn::default())
+        .unwrap();
+    assert_eq!(port.set_name(nul), Err(InteriorNul(nul.to_string())));
+    assert_eq!(port.set_alias(nul), Err(InteriorNul(nul.to_string())));
+    assert_eq!(port.unset_alias(nul), Err(InteriorNul(nul.to_string())));
+    assert_eq!(port.is_connected_to(nul), Ok(false));
+}