add safety comments

ClementTsang · ClementTsang · Apr 10, 2023 · Mar 9, 2023 · Mar 11, 2023 · Mar 14, 2023
commit 32e38ae5d847c389ae8e33f860281b5f6158dfa6
diff --git a/src/app/data_harvester/disks/system/linux/partition.rs b/src/app/data_harvester/disks/system/linux/partition.rs
@@ -86,9 +86,12 @@ impl Partition {
             .map_err(|e| anyhow::anyhow!("invalid path: {e:?}"))?;
 
         let mut vfs = mem::MaybeUninit::<libc::statvfs>::uninit();
+
+        // SAFETY: libc call, `path` is a valid C string and buf is a valid pointer to write to.
         let result = unsafe { libc::statvfs(path.as_ptr(), vfs.as_mut_ptr()) };
 
         if result == 0 {
+            // SAFETY: If result is 0, it succeeded, and vfs should be non-null.
             let vfs = unsafe { vfs.assume_init() };
             Ok(Usage::new(vfs))
         } else {

diff --git a/src/app/data_harvester/disks/system/macos/io_kit/io_disks.rs b/src/app/data_harvester/disks/system/macos/io_kit/io_disks.rs
@@ -7,7 +7,7 @@ use super::{bindings::*, IoIterator};
 pub fn get_disks() -> anyhow::Result<IoIterator> {
     let mut media_iter: io_iterator_t = 0;
 
-    // SAFETY: This is a safe syscall,
+    // SAFETY: This is a safe syscall via IOKit, all the arguments should be safe.
     let result = unsafe {
         IOServiceGetMatchingServices(
             kIOMasterPortDefault,

diff --git a/src/app/data_harvester/disks/system/macos/io_kit/io_iterator.rs b/src/app/data_harvester/disks/system/macos/io_kit/io_iterator.rs
@@ -36,6 +36,8 @@ impl Iterator for IoIterator {
 
     fn next(&mut self) -> Option<Self::Item> {
         // Basically, we just stop when we hit 0.
+
+        // SAFETY: IOKit call, the passed argument (an `io_iterator_t`) is what is expected.
         match unsafe { IOIteratorNext(self.0) } {
             0 => None,
             io_object => Some(IoObject::from(io_object)),
@@ -45,6 +47,7 @@ impl Iterator for IoIterator {
 
 impl Drop for IoIterator {
     fn drop(&mut self) {
+        // SAFETY: IOKit call, the passed argument (an `io_iterator_t`) is what is expected.
         let result = unsafe { IOObjectRelease(self.0) };
         assert_eq!(result, kern_return::KERN_SUCCESS);
     }

diff --git a/src/app/data_harvester/disks/system/macos/io_kit/io_object.rs b/src/app/data_harvester/disks/system/macos/io_kit/io_object.rs
@@ -22,6 +22,8 @@ pub struct IoObject(io_object_t);
 impl IoObject {
     /// Returns a typed dictionary with this object's properties.
     pub fn properties(&self) -> anyhow::Result<CFDictionary<CFString, CFType>> {
+        // SAFETY: The IOKit call should be fine, the arguments are safe. The `assume_init` should also be fine, as
+        // we guard against it with a check against `result` to ensure it succeeded.
         unsafe {
             let mut props = mem::MaybeUninit::<CFMutableDictionaryRef>::uninit();
 
@@ -46,6 +48,7 @@ impl IoObject {
     pub fn service_parent(&self) -> anyhow::Result<IoObject> {
         let mut parent: io_registry_entry_t = 0;
 
+        // SAFETY: IOKit call, the arguments should be safe.
         let result = unsafe {
             IORegistryEntryGetParentEntry(self.0, kIOServicePlane.as_ptr().cast(), &mut parent)
         };
@@ -58,6 +61,7 @@ impl IoObject {
     }
 
     // pub fn conforms_to_block_storage_driver(&self) -> bool {
+    //     // SAFETY: IOKit call, the arguments should be safe.
     //     let result =
     //         unsafe { IOObjectConformsTo(self.0, "IOBlockStorageDriver\0".as_ptr().cast()) };
 
@@ -73,6 +77,7 @@ impl From<io_object_t> for IoObject {
 
 impl Drop for IoObject {
     fn drop(&mut self) {
+        // SAFETY: IOKit call, the argument here (an `io_object_t`) should be safe and expected.
         let result = unsafe { IOObjectRelease(self.0) };
         assert_eq!(result, kern_return::KERN_SUCCESS);
     }
@@ -85,16 +90,15 @@ pub fn get_dict(
 
     dict.find(&key)
         .map(|value_ref| {
+            // SAFETY: Only used for debug asserts, system API call that should be safe.
             unsafe {
                 debug_assert!(value_ref.type_of() == CFDictionaryGetTypeID());
             }
 
             // "Casting" `CFDictionary<*const void, *const void>` into a needed dict type
-
-            // TODO: I think that reference to an original dict is still stored somewhere
-            // and it does not decrements here.
             let ptr = value_ref.to_void() as CFDictionaryRef;
 
+            // SAFETY: System API call, it should be safe?
             unsafe { CFDictionary::wrap_under_get_rule(ptr) }
         })
         .ok_or_else(|| anyhow!("missing key"))
@@ -107,10 +111,10 @@ pub fn get_i64(
 
     dict.find(&key)
         .and_then(|value_ref| {
+            // SAFETY: Only used for debug asserts, system API call that should be safe.
             unsafe {
                 debug_assert!(value_ref.type_of() == CFNumberGetTypeID());
             }
-
             value_ref.downcast::<CFNumber>()
         })
         .and_then(|number| number.to_i64())
@@ -124,6 +128,7 @@ pub fn get_string(
 
     dict.find(&key)
         .and_then(|value_ref| {
+            // SAFETY: Only used for debug asserts, system API call that should be safe.
             unsafe {
                 debug_assert!(value_ref.type_of() == CFStringGetTypeID());
             }

diff --git a/src/app/data_harvester/disks/system/other/bindings.rs b/src/app/data_harvester/disks/system/other/bindings.rs
@@ -12,8 +12,12 @@ extern "C" {
 
 /// Returns all the mounts on the system at the moment.
 pub(crate) fn mounts() -> anyhow::Result<Vec<libc::statfs>> {
+    // SAFETY: System API FFI call, arguments should be correct.
     let expected_len = unsafe { getfsstat64(std::ptr::null_mut(), 0, MNT_NOWAIT) };
+
     let mut mounts: Vec<libc::statfs> = Vec::with_capacity(expected_len as usize);
+
+    // SAFETY: System API FFI call, arguments should be correct.
     let result = unsafe {
         getfsstat64(
             mounts.as_mut_ptr(),
@@ -29,6 +33,11 @@ pub(crate) fn mounts() -> anyhow::Result<Vec<libc::statfs>> {
             expected_len, result,
             "Expected {expected_len} statfs entries, but instead got {result} entries",
         );
+
+        // SAFETY: We have a debug assert check, and if `result` is not correct (-1), we check against it.
+        // Otherwise, getfsstat64 should return the number of statfs structures if it succeeded.
+        //
+        // Source: https://man.freebsd.org/cgi/man.cgi?query=getfsstat&sektion=2&format=html
         unsafe {
             mounts.set_len(result as usize);
         }

diff --git a/src/app/data_harvester/disks/system/other/partition.rs b/src/app/data_harvester/disks/system/other/partition.rs
@@ -34,9 +34,11 @@ impl Partition {
         let path = CString::new(self.mount_point().as_os_str().as_bytes())?;
         let mut vfs = std::mem::MaybeUninit::<libc::statvfs>::uninit();
 
+        // SAFETY: System API call. Arguments should be correct.
         let result = unsafe { libc::statvfs(path.as_ptr(), vfs.as_mut_ptr()) };
 
         if result == 0 {
+            // SAFETY: We check that it succeeded (result is 0), which means vfs should be populated.
             Ok(Usage::new(unsafe { vfs.assume_init() }))
         } else {
             bail!("statvfs failed to get the disk usage for disk {path:?}")