Improve object releasers and cleanup usage sites

src/apple/disk.rs

@@ -6,11 +6,11 @@ use crate::sys::{
 };
 use crate::{DiskExt, DiskType};
 
-use core_foundation_sys::array::CFArrayCreate;
 use core_foundation_sys::base::kCFAllocatorDefault;
 use core_foundation_sys::dictionary::{CFDictionaryGetValueIfPresent, CFDictionaryRef};
 use core_foundation_sys::number::{kCFBooleanTrue, CFBooleanRef, CFNumberGetValue};
 use core_foundation_sys::string::{self as cfs, CFStringRef};
+use core_foundation_sys::{array::CFArrayCreate, base::CFIndex};
 
 use libc::c_void;
 
@@ -62,16 +62,20 @@ impl DiskExt for Disk {
 
     fn refresh(&mut self) -> bool {
         unsafe {
-            let requested_properties = build_requested_properties(&[
+            if let Some(requested_properties) = build_requested_properties(&[
                 ffi::kCFURLVolumeAvailableCapacityKey,
                 ffi::kCFURLVolumeAvailableCapacityForImportantUsageKey,
-            ]);
-            match get_disk_properties(&self.volume_url, &requested_properties) {
-                Some(disk_props) => {
-                    self.available_space = get_available_volume_space(&disk_props);
-                    true
+            ]) {
+                match get_disk_properties(&self.volume_url, &requested_properties) {
+                    Some(disk_props) => {
+                        self.available_space = get_available_volume_space(&disk_props);
+                        true
+                    }
+                    None => false,
                 }
-                None => false,
+            } else {
+                sysinfo_debug!("failed to create volume key list, skipping refresh");
+                false
             }
         }
     }
@@ -101,7 +105,7 @@ unsafe fn get_disk_inner() -> Vec<Disk> {
     };
 
     // Create a list of properties about the disk that we want to fetch.
-    let requested_properties = build_requested_properties(&[
+    let requested_properties = match build_requested_properties(&[
         ffi::kCFURLVolumeIsEjectableKey,
         ffi::kCFURLVolumeIsRemovableKey,
         ffi::kCFURLVolumeIsInternalKey,
@@ -111,19 +115,30 @@ unsafe fn get_disk_inner() -> Vec<Disk> {
         ffi::kCFURLVolumeNameKey,
         ffi::kCFURLVolumeIsBrowsableKey,
         ffi::kCFURLVolumeIsLocalKey,
-    ]);
+    ]) {
+        Some(properties) => properties,
+        None => {
+            sysinfo_debug!("failed to create volume key list");
+            return Vec::new();
+        }
+    };
 
     let mut disks = Vec::with_capacity(raw_disks.len());
     for c_disk in raw_disks {
-        let volume_url = CFReleaser::new(
+        let volume_url = match CFReleaser::new(
             core_foundation_sys::url::CFURLCreateFromFileSystemRepresentation(
                 kCFAllocatorDefault,
                 c_disk.f_mntonname.as_ptr() as *const u8,
                 c_disk.f_mntonname.len() as isize,
                 false as u8,
             ),
-        )
-        .unwrap();
+        ) {
+            Some(url) => url,
+            None => {
+                sysinfo_debug!("getfsstat returned incompatible paths");
+                continue;
+            }
+        };
 
         let prop_dict = match get_disk_properties(&volume_url, &requested_properties) {
             Some(props) => props,
@@ -177,14 +192,13 @@ type RetainedCFArray = CFReleaser<core_foundation_sys::array::__CFArray>;
 type RetainedCFDictionary = CFReleaser<core_foundation_sys::dictionary::__CFDictionary>;
 type RetainedCFURL = CFReleaser<core_foundation_sys::url::__CFURL>;
 
-unsafe fn build_requested_properties(properties: &[CFStringRef]) -> RetainedCFArray {
+unsafe fn build_requested_properties(properties: &[CFStringRef]) -> Option<RetainedCFArray> {
     CFReleaser::new(CFArrayCreate(
         ptr::null_mut(),
         properties.as_ptr() as *const *const c_void,
-        properties.len() as isize,
+        properties.len() as CFIndex,
         &core_foundation_sys::array::kCFTypeArrayCallBacks,
     ))
-    .unwrap()
 }
 
 fn get_disk_properties(
@@ -238,15 +252,14 @@ unsafe fn get_dict_value<T, F: FnOnce(*const c_void) -> Option<T>>(
         DictKey::Extern(val) => val,
         #[cfg(target_os = "macos")]
         DictKey::Defined(val) => {
-            _defined = CFReleaser::new(cfs::CFStringCreateWithBytesNoCopy(
+            _defined = CFReleaser::new_unchecked(cfs::CFStringCreateWithBytesNoCopy(
                 kCFAllocatorDefault,
                 val.as_ptr(),
                 val.len() as isize,
                 cfs::kCFStringEncodingUTF8,
                 false as _,
                 core_foundation_sys::base::kCFAllocatorNull,
-            ))
-            .unwrap();
+            ));
 
             _defined.inner()
         }
@@ -319,7 +332,7 @@ unsafe fn new_disk(
     // so we just assume the disk type is an SSD until Rust has a way to conditionally link to
     // IOKit in more recent deployment versions.
     #[cfg(target_os = "macos")]
-    let type_ = crate::sys::inner::disk::get_disk_type(&c_disk);
+    let type_ = crate::sys::inner::disk::get_disk_type(&c_disk).unwrap_or(DiskType::Unknown(-1));
     #[cfg(not(target_os = "macos"))]
     let type_ = DiskType::SSD;
 

src/apple/macos/disk.rs

@@ -8,39 +8,44 @@ use crate::sys::{
 };
 use crate::DiskType;
 
+use core_foundation_sys::base::CFIndex;
 use core_foundation_sys::base::{kCFAllocatorDefault, kCFAllocatorNull};
 use core_foundation_sys::string as cfs;
 
 use std::ffi::CStr;
 
-const UNKNOWN_DISK_TYPE: DiskType = DiskType::Unknown(-1);
-
-pub(crate) fn get_disk_type(disk: &libc::statfs) -> DiskType {
-    let characteristics_string = CFReleaser::new(unsafe {
-        cfs::CFStringCreateWithBytesNoCopy(
+pub(crate) fn get_disk_type(disk: &libc::statfs) -> Option<DiskType> {
+    // Safety: All of the inputs are valid, so this will always
+    // return a valid object.
+    let characteristics_string = unsafe {
+        CFReleaser::new_unchecked(cfs::CFStringCreateWithBytesNoCopy(
             kCFAllocatorDefault,
             ffi::kIOPropertyDeviceCharacteristicsKey.as_ptr(),
-            ffi::kIOPropertyDeviceCharacteristicsKey.len() as isize,
+            ffi::kIOPropertyDeviceCharacteristicsKey.len() as CFIndex,
             cfs::kCFStringEncodingUTF8,
             false as _,
             kCFAllocatorNull,
-        )
-    })
-    .unwrap();
+        ))
+    };
 
     // Removes `/dev/` from the value.
     let bsd_name = unsafe {
         CStr::from_ptr(disk.f_mntfromname.as_ptr())
             .to_bytes()
             .strip_prefix(b"/dev/")
-            .expect("device mount point in unknown format")
+            .or_else(|| {
+                sysinfo_debug!("unknown disk mount path format");
+                None
+            })?
     };
 
+    // We don't need to wrap this in an auto-releaser because the following call to `IOServiceGetMatchingServices`
+    // will take ownership of one retain reference.
     let matching =
         unsafe { ffi::IOBSDNameMatching(ffi::kIOMasterPortDefault, 0, bsd_name.as_ptr().cast()) };
 
     if matching.is_null() {
-        return UNKNOWN_DISK_TYPE;
+        return None;
     }
 
     let mut service_iterator: ffi::io_iterator_t = 0;
@@ -53,34 +58,34 @@ pub(crate) fn get_disk_type(disk: &libc::statfs) -> DiskType {
         )
     } != libc::KERN_SUCCESS
     {
-        return UNKNOWN_DISK_TYPE;
+        return None;
     }
 
-    let service_iterator = IOReleaser::new(service_iterator).unwrap();
+    // Safety: We checked for success, so there is always a valid iterator, even if its empty.
+    let service_iterator = unsafe { IOReleaser::new_unchecked(service_iterator) };
 
     let mut parent_entry: ffi::io_registry_entry_t = 0;
 
     while let Some(mut current_service_entry) =
         IOReleaser::new(unsafe { ffi::IOIteratorNext(service_iterator.inner()) })
     {
         loop {
-            let result = unsafe {
+            if unsafe {
                 ffi::IORegistryEntryGetParentEntry(
                     current_service_entry.inner(),
                     ffi::kIOServicePlane.as_ptr().cast(),
                     &mut parent_entry,
                 )
-            };
-            if result != libc::KERN_SUCCESS {
+            } != libc::KERN_SUCCESS
+            {
                 break;
             }
 
-            current_service_entry = IOReleaser::new(parent_entry).unwrap();
-
-            // There were no more parents left.
-            if parent_entry == 0 {
-                break;
-            }
+            current_service_entry = match IOReleaser::new(parent_entry) {
+                Some(service) => service,
+                // There were no more parents left
+                None => break,
+            };
 
             let properties_result = unsafe {
                 CFReleaser::new(ffi::IORegistryEntryCreateCFProperty(
@@ -104,17 +109,17 @@ pub(crate) fn get_disk_type(disk: &libc::statfs) -> DiskType {
                     _ if medium == ffi::kIOPropertyMediumTypeRotationalKey => Some(DiskType::HDD),
                     _ => None,
                 }) {
-                    return disk_type;
+                    return Some(disk_type);
                 } else {
                     // Many external drive vendors do not advertise their device's storage medium.
                     //
                     // In these cases, assuming that there were _any_ properties about them registered, we fallback
                     // to `HDD` when no storage medium is provided by the device instead of `Unknown`.
-                    return DiskType::HDD;
+                    return Some(DiskType::HDD);
                 }
             }
         }
     }
 
-    UNKNOWN_DISK_TYPE
+    None
 }

src/apple/macos/utils.rs

@@ -1,25 +1,30 @@
 // Take a look at the license at the top of the repository in the LICENSE file.
 
-pub(crate) struct IOReleaser(super::ffi::io_object_t);
+use std::num::NonZeroU32;
+
+type IoObject = NonZeroU32;
+
+pub(crate) struct IOReleaser(IoObject);
 
 impl IOReleaser {
     pub(crate) fn new(obj: u32) -> Option<Self> {
-        if obj == 0 {
-            None
-        } else {
-            Some(Self(obj))
-        }
+        IoObject::new(obj).map(Self)
+    }
+
+    pub(crate) unsafe fn new_unchecked(obj: u32) -> Self {
+        // Chance at catching in-development mistakes
+        debug_assert_ne!(obj, 0);
+        Self(IoObject::new_unchecked(obj))
     }
 
+    #[inline]
     pub(crate) fn inner(&self) -> u32 {
-        self.0
+        self.0.get()
     }
 }
 
 impl Drop for IOReleaser {
     fn drop(&mut self) {
-        if self.0 != 0 {
-            unsafe { super::ffi::IOObjectRelease(self.0 as _) };
-        }
+        unsafe { super::ffi::IOObjectRelease(self.0.get() as _) };
     }
 }

src/apple/utils.rs

@@ -2,35 +2,43 @@
 
 use core_foundation_sys::base::CFRelease;
 use libc::c_char;
+use std::ptr::NonNull;
 
 // A helper using to auto release the resource got from CoreFoundation.
 // More information about the ownership policy for CoreFoundation pelease refer the link below:
 // https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-CJBEJBHH
 #[repr(transparent)]
-pub(crate) struct CFReleaser<T>(*const T);
+pub(crate) struct CFReleaser<T>(NonNull<T>);
 
 impl<T> CFReleaser<T> {
     pub(crate) fn new(ptr: *const T) -> Option<Self> {
-        if ptr.is_null() {
-            None
-        } else {
-            Some(Self(ptr))
-        }
+        // This cast is OK because `NonNull` is a transparent wrapper
+        // over a `*const T`. Additionally, mutability doesn't matter with
+        // pointers here.
+        NonNull::new(ptr as *mut T).map(Self)
+    }
+
+    #[cfg(target_os = "macos")]
+    pub(crate) unsafe fn new_unchecked(ptr: *const T) -> Self {
+        // Chance at catching in-development mistakes
+        debug_assert!(!ptr.is_null());
+        Self(NonNull::new_unchecked(ptr as *mut T))
     }
 
     pub(crate) fn inner(&self) -> *const T {
-        self.0
+        self.0.as_ptr().cast()
     }
 }
 
 impl<T> Drop for CFReleaser<T> {
     fn drop(&mut self) {
-        if !self.0.is_null() {
-            unsafe { CFRelease(self.0 as _) }
-        }
+        unsafe { CFRelease(self.0.as_ptr().cast()) }
     }
 }
 
+// Safety: These are safe to implement because we only wrap non-mutable
+// CoreFoundation types, which are generally threadsafe unless noted
+// otherwise.
 unsafe impl<T> Send for CFReleaser<T> {}
 unsafe impl<T> Sync for CFReleaser<T> {}