Refactor unsafe slightly in mullvad_daemon::migrations

1 files changed, 32 insertions, 12 deletions

diff --git a/mullvad-daemon/src/migrations/mod.rs b/mullvad-daemon/src/migrations/mod.rs
index 635c2f6ebe..eaf3350e0c 100644
--- a/mullvad-daemon/src/migrations/mod.rs
+++ b/mullvad-daemon/src/migrations/mod.rs
@@ -227,15 +227,22 @@ pub(crate) fn migrate_device(
 
 #[cfg(windows)]
 mod windows {
-    use std::{ffi::OsStr, io, os::windows::ffi::OsStrExt, path::Path, ptr};
+    use std::{
+        ffi::OsStr,
+        io,
+        os::windows::ffi::OsStrExt,
+        path::Path,
+        ptr::{self, NonNull},
+    };
     use talpid_types::ErrorExt;
     use tokio::fs;
     use windows_sys::Win32::{
-        Foundation::{LocalFree, ERROR_SUCCESS, HLOCAL, PSID},
+        Foundation::{LocalFree, ERROR_SUCCESS, PSID},
         Security::{
             Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT, SE_OBJECT_TYPE},
             IsWellKnownSid, WinBuiltinAdministratorsSid, WinLocalSystemSid,
-            OWNER_SECURITY_INFORMATION, SECURITY_DESCRIPTOR, SID, WELL_KNOWN_SID_TYPE,
+            OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, SECURITY_DESCRIPTOR, SID,
+            WELL_KNOWN_SID_TYPE,
         },
     };
 
@@ -355,8 +362,8 @@ mod windows {
     }
 
     struct SecurityInformation {
-        security_descriptor: *mut SECURITY_DESCRIPTOR,
-        owner: PSID,
+        security_descriptor: NonNull<SECURITY_DESCRIPTOR>,
+        owner: Option<NonNull<SID>>,
     }
 
     impl SecurityInformation {
@@ -375,9 +382,12 @@ mod windows {
             let mut u16_path: Vec<u16> = object_name.as_ref().encode_wide().collect();
             u16_path.push(0u16);
 
-            let mut security_descriptor = ptr::null_mut();
-            let mut owner = ptr::null_mut();
+            let mut security_descriptor: PSECURITY_DESCRIPTOR = ptr::null_mut();
+            let mut owner: PSID = ptr::null_mut();
 
+            // SAFETY:
+            // - u16_path is a null-terminated UTF-16 string
+            // - The *mut pointers are allowed to be null
             let status = unsafe {
                 GetNamedSecurityInfoW(
                     u16_path.as_ptr(),
@@ -395,25 +405,35 @@ mod windows {
                 return Err(std::io::Error::from_raw_os_error(status as i32));
             }
 
+            let Some(security_descriptor) = NonNull::new(security_descriptor) else {
+                return Err(std::io::Error::other("GetNamedSecurityInfoW returned null"));
+            };
+
             Ok(SecurityInformation {
-                security_descriptor: security_descriptor as *mut _,
-                owner,
+                security_descriptor: security_descriptor.cast::<SECURITY_DESCRIPTOR>(),
+                owner: NonNull::new(owner.cast::<SID>()),
             })
         }
 
         pub fn owner(&self) -> Option<&SID> {
-            unsafe { (self.owner as *const SID).as_ref() }
+            // SAFETY: GetNamedSecurityInfoW promises that this pointer was valid,
+            // and it should stay valid until we deallocate self.security_descriptor.
+            self.owner.map(|ptr| unsafe { ptr.as_ref() })
         }
     }
 
     impl Drop for SecurityInformation {
         fn drop(&mut self) {
-            unsafe { LocalFree(self.security_descriptor as HLOCAL) };
+            // SAFETY: GetNamedSecurityInfoW promises that this pointer was valid,
+            // and we do not deallocate it before this point. Since we have &mut self,
+            // we know that no one else has a reference to security_descriptor.
+            unsafe { LocalFree(self.security_descriptor.as_ptr() as PSECURITY_DESCRIPTOR) };
         }
     }
 
     fn is_well_known_sid(sid: &SID, well_known_sid_type: WELL_KNOWN_SID_TYPE) -> bool {
-        unsafe { IsWellKnownSid(sid as *const SID as *mut _, well_known_sid_type) == 1 }
+        // SAFETY: this function doesn't take ownership of sid, and is trivially safe to call.
+        unsafe { IsWellKnownSid(sid as *const SID as PSID, well_known_sid_type) == 1 }
     }
 }