Make compare_cstr_safe infallible and instead panic

.github/workflows/build-and-test.yml

@@ -18,7 +18,7 @@ jobs:
     strategy:
       matrix:
         # Keep MSRV in sync with rust-version in Cargo.toml as much as possible.
-        rust: [stable, beta, nightly, 1.62.0]
+        rust: [stable, beta, nightly, 1.69.0]
     runs-on: macos-latest
     steps:
       - uses: actions/checkout@v4

CHANGELOG.md

@@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 * Upgrade `ipnetwork` dependency from 0.16 to 0.20. This is a breaking change since
   `ipnetwork` is part of the public API.
 * Upgrade crate to Rust 2021 edition.
+* MSRV bumped to 1.69 due to use of `CStr::from_bytes_until_nul`.
 
 ### Removed
 * Remove `PoolAddrList::to_palist` from the public API. It should never have been exposed.

Cargo.toml

@@ -9,7 +9,7 @@ readme = "README.md"
 keywords = ["pf", "firewall", "macos", "packet", "filter"]
 categories = ["network-programming", "os", "os::macos-apis", "api-bindings"]
 edition = "2021"
-rust-version = "1.62.0"
+rust-version = "1.69.0"
 
 [badges]
 travis-ci = { repository = "mullvad/pfctl-rs" }

src/lib.rs

@@ -62,6 +62,7 @@
 #[macro_use]
 pub extern crate error_chain;
 
+use core::slice;
 use std::{
     ffi::CStr,
     fs::File,
@@ -147,10 +148,23 @@ mod conversion {
 use crate::conversion::*;
 
 /// Internal function to safely compare Rust string with raw C string slice
-fn compare_cstr_safe(s: &str, cchars: &[std::os::raw::c_char]) -> Result<bool> {
-    ensure!(cchars.iter().any(|&c| c == 0), "Not null terminated");
-    let cs = unsafe { CStr::from_ptr(cchars.as_ptr()) };
-    Ok(s.as_bytes() == cs.to_bytes())
+///
+/// # Panics
+///
+/// Panics if `cchars` does not contain any null byte.
+fn compare_cstr_safe(s: &str, c_chars: &[std::os::raw::c_char]) -> bool {
+    // Due to `c_char` being `i8` on macOS, and `CStr` methods taking `u8` data,
+    // we need to convert the slice from `&[i8]` to `&[u8]`
+    let c_chars_ptr = c_chars.as_ptr() as *const u8;
+
+    // SAFETY: We point to the same memory region as `c_chars`,
+    // which is a valid slice, so it's guaranteed to be safe
+    let c_chars_u8 = unsafe { slice::from_raw_parts(c_chars_ptr, c_chars.len()) };
+
+    let cs = CStr::from_bytes_until_nul(c_chars_u8)
+        .expect("System returned C String without terminating null byte");
+
+    s.as_bytes() == cs.to_bytes()
 }
 
 /// Struct communicating with the PF firewall.
@@ -355,7 +369,7 @@ impl PfCtl {
         for i in 0..pfioc_rule.nr {
             pfioc_rule.nr = i;
             ioctl_guard!(ffi::pf_get_rule(self.fd(), &mut pfioc_rule))?;
-            if compare_cstr_safe(name, &pfioc_rule.anchor_call)? {
+            if compare_cstr_safe(name, &pfioc_rule.anchor_call) {
                 return f(pfioc_rule);
             }
         }
@@ -410,44 +424,41 @@ fn setup_pfioc_state_kill(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use assert_matches::assert_matches;
     use std::ffi::CString;
 
     #[test]
+    #[should_panic]
     fn compare_cstr_without_nul() {
         let cstr = CString::new("Hello").unwrap();
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes()) };
-        assert_matches!(
-            compare_cstr_safe("Hello", cchars),
-            Err(ref e) if e.description() == "Not null terminated"
-        );
+        compare_cstr_safe("Hello", cchars);
     }
 
     #[test]
     fn compare_same_strings() {
         let cstr = CString::new("Hello").unwrap();
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes_with_nul()) };
-        assert_matches!(compare_cstr_safe("Hello", cchars), Ok(true));
+        assert!(compare_cstr_safe("Hello", cchars));
     }
 
     #[test]
     fn compare_different_strings() {
         let cstr = CString::new("Hello").unwrap();
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes_with_nul()) };
-        assert_matches!(compare_cstr_safe("olleH", cchars), Ok(false));
+        assert!(!compare_cstr_safe("olleH", cchars));
     }
 
     #[test]
     fn compare_long_short_strings() {
         let cstr = CString::new("veryverylong").unwrap();
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes_with_nul()) };
-        assert_matches!(compare_cstr_safe("short", cchars), Ok(false));
+        assert!(!compare_cstr_safe("short", cchars));
     }
 
     #[test]
     fn compare_short_long_strings() {
         let cstr = CString::new("short").unwrap();
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes_with_nul()) };
-        assert_matches!(compare_cstr_safe("veryverylong", cchars), Ok(false));
+        assert!(!compare_cstr_safe("veryverylong", cchars));
     }
 }