Use SIDs as UIDs on Windows

Cargo.toml

@@ -55,7 +55,8 @@ winapi = { version = "0.3.9", features = [
   "shellapi",
   "std",
   "iphlpapi",
-  "winsock2"
+  "winsock2",
+  "sddl",
 ]}
 ntapi = "0.4"
 

md_doc/sid.md

@@ -0,0 +1 @@
+Opaque type encapsulating a Windows SID.

src/common.rs

@@ -702,10 +702,6 @@ macro_rules! uid {
     ($type:ty$(, $trait:ty)?) => {
         xid!(
             /// A user id wrapping a platform specific type.
-            ///
-            /// ⚠️ On windows, `Uid` is actually wrapping a `Box<str>` due to some
-            /// Windows limitations around their users ID. For the time being, `Uid`
-            /// is the user name on this platform.
             Uid,
             $type
             $(, $trait)?
@@ -739,21 +735,14 @@ cfg_if::cfg_if! {
         uid!(libc::uid_t, FromStr);
         gid!(libc::gid_t);
     } else if #[cfg(windows)] {
-        uid!(Box<str>);
+        uid!(crate::windows::Sid);
         gid!(u32);
         // Manual implementation outside of the macro...
         impl FromStr for Uid {
-            type Err = <String as FromStr>::Err;
+            type Err = <crate::windows::Sid as FromStr>::Err;
 
-            fn from_str(t: &str) -> Result<Self, <String as FromStr>::Err> {
-                Ok(Self(t.into()))
-            }
-        }
-        impl TryFrom<usize> for Uid {
-            type Error = <u32 as TryFrom<usize>>::Error;
-
-            fn try_from(t: usize) -> Result<Self, <u32 as TryFrom<usize>>::Error> {
-                Ok(Self(t.to_string().into_boxed_str()))
+            fn from_str(t: &str) -> Result<Self, Self::Err> {
+                Ok(Self(t.parse()?))
             }
         }
     } else {
@@ -1059,8 +1048,16 @@ mod tests {
         use std::convert::TryFrom;
         use std::str::FromStr;
 
-        assert!(crate::Uid::try_from(0usize).is_ok());
-        assert!(crate::Uid::from_str("0").is_ok());
+        #[cfg(not(windows))]
+        {
+            assert!(crate::Uid::try_from(0usize).is_ok());
+            assert!(crate::Uid::from_str("0").is_ok());
+        }
+        #[cfg(windows)]
+        {
+            assert!(crate::Uid::from_str("S-1-5-18").is_ok()); // SECURITY_LOCAL_SYSTEM_RID
+            assert!(crate::Uid::from_str("0").is_err());
+        }
 
         assert!(crate::Gid::try_from(0usize).is_ok());
         assert!(crate::Gid::from_str("0").is_ok());

src/lib.rs

@@ -336,6 +336,25 @@ mod test {
         }
     }
 
+    #[test]
+    fn check_all_process_uids_resolvable() {
+        if System::IS_SUPPORTED {
+            let s = System::new_with_specifics(
+                RefreshKind::new()
+                    .with_processes(ProcessRefreshKind::new().with_user())
+                    .with_users_list(),
+            );
+
+            // For every process where we can get a user ID, we should also be able
+            // to find that user ID in the global user list
+            for process in s.processes().values() {
+                if let Some(uid) = process.user_id() {
+                    assert!(s.get_user_by_id(uid).is_some());
+                }
+            }
+        }
+    }
+
     #[test]
     fn check_system_info() {
         let s = System::new();

src/windows/mod.rs

@@ -5,6 +5,7 @@ mod cpu;
 mod disk;
 mod network;
 mod process;
+mod sid;
 mod system;
 mod tools;
 mod users;
@@ -15,4 +16,5 @@ pub use self::cpu::Cpu;
 pub use self::disk::Disk;
 pub use self::network::{NetworkData, Networks};
 pub use self::process::Process;
+pub use self::sid::Sid;
 pub use self::system::System;

src/windows/process.rs

@@ -1,7 +1,7 @@
 // Take a look at the license at the top of the repository in the LICENSE file.
 
 use crate::sys::system::is_proc_running;
-use crate::sys::utils::to_str;
+use crate::windows::Sid;
 use crate::{
     DiskUsage, Gid, Pid, PidExt, ProcessExt, ProcessRefreshKind, ProcessStatus, Signal, Uid,
 };
@@ -143,29 +143,7 @@ unsafe fn get_process_user_id(
         return None;
     }
 
-    let mut name_use = 0;
-    let mut name = [0u16; 256];
-    let mut domain_name = [0u16; 256];
-    let mut size = 256;
-
-    if winapi::um::winbase::LookupAccountSidW(
-        std::ptr::null_mut(),
-        (*ptu.0).User.Sid,
-        name.as_mut_ptr(),
-        &mut size,
-        domain_name.as_mut_ptr(),
-        &mut size,
-        &mut name_use,
-    ) == 0
-    {
-        sysinfo_debug!(
-            "LookupAccountSidW failed: {:?}",
-            winapi::um::errhandlingapi::GetLastError(),
-        );
-        None
-    } else {
-        Some(Uid(to_str(name.as_mut_ptr()).into_boxed_str()))
-    }
+    Sid::from_psid((*ptu.0).User.Sid).map(Uid)
 }
 
 struct HandleWrapper(HANDLE);
@@ -617,7 +595,6 @@ pub(crate) fn get_start_time(handle: HANDLE) -> u64 {
     }
 }
 
-#[allow(clippy::uninit_vec)]
 unsafe fn ph_query_process_variable_size(
     process_handle: &HandleWrapper,
     process_information_class: PROCESSINFOCLASS,
@@ -642,8 +619,6 @@ unsafe fn ph_query_process_variable_size(
     let mut return_length = return_length.assume_init();
     let buf_len = (return_length as usize) / 2;
     let mut buffer: Vec<u16> = Vec::with_capacity(buf_len + 1);
-    buffer.set_len(buf_len);
-
     status = NtQueryInformationProcess(
         **process_handle,
         process_information_class,
@@ -654,6 +629,7 @@ unsafe fn ph_query_process_variable_size(
     if !NT_SUCCESS(status) {
         return None;
     }
+    buffer.set_len(buf_len);
     buffer.push(0);
     Some(buffer)
 }
@@ -695,24 +671,31 @@ unsafe fn get_region_size(handle: &HandleWrapper, ptr: LPVOID) -> Result<usize,
     Ok((meminfo.RegionSize as isize - ptr.offset_from(meminfo.BaseAddress)) as usize)
 }
 
-#[allow(clippy::uninit_vec)]
 unsafe fn get_process_data(
     handle: &HandleWrapper,
     ptr: LPVOID,
     size: usize,
 ) -> Result<Vec<u16>, &'static str> {
     let mut buffer: Vec<u16> = Vec::with_capacity(size / 2 + 1);
-    buffer.set_len(size / 2);
+    let mut bytes_read = 0;
+
     if ReadProcessMemory(
         **handle,
         ptr as *mut _,
         buffer.as_mut_ptr() as *mut _,
         size,
-        null_mut(),
-    ) != TRUE
+        &mut bytes_read,
+    ) == FALSE
     {
         return Err("Unable to read process data");
     }
+
+    // Documentation states that the function fails if not all data is accessible
+    assert_eq!(bytes_read, size);
+
+    buffer.set_len(size / 2);
+    buffer.push(0);
+
     Ok(buffer)
 }
 

src/windows/sid.rs

@@ -0,0 +1,157 @@
+// Take a look at the license at the top of the repository in the LICENSE file.
+
+use std::{fmt::Display, str::FromStr};
+
+use winapi::{
+    shared::{
+        sddl::{ConvertSidToStringSidW, ConvertStringSidToSidW},
+        winerror::ERROR_INSUFFICIENT_BUFFER,
+    },
+    um::{
+        errhandlingapi::GetLastError,
+        securitybaseapi::{CopySid, GetLengthSid, IsValidSid},
+        winbase::{LocalFree, LookupAccountSidW},
+        winnt::{SidTypeUnknown, LPWSTR, PSID},
+    },
+};
+
+use crate::sys::utils::to_str;
+
+#[doc = include_str!("../../md_doc/sid.md")]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Sid {
+    sid: Vec<u8>,
+}
+
+impl Sid {
+    /// Creates an `Sid` by making a copy of the given raw SID.
+    pub(crate) unsafe fn from_psid(psid: PSID) -> Option<Self> {
+        if psid.is_null() {
+            return None;
+        }
+
+        if IsValidSid(psid) == 0 {
+            return None;
+        }
+
+        let length = GetLengthSid(psid);
+
+        let mut sid = vec![0; length as usize];
+
+        if CopySid(length, sid.as_mut_ptr() as *mut _, psid) == 0 {
+            sysinfo_debug!("CopySid failed: {:?}", GetLastError());
+            return None;
+        }
+
+        // We are making assumptions about the SID internal structure,
+        // and these only hold if the revision is 1
+        // https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-sid
+        // Namely:
+        // 1. SIDs can be compared directly (memcmp).
+        // 2. Following from this, to hash a SID we can just hash its bytes.
+        // These are the basis for deriving PartialEq, Eq, and Hash.
+        // And since we also need PartialOrd and Ord, we might as well derive them
+        // too. The default implementation will be consistent with Eq,
+        // and we don't care about the actual order, just that there is one.
+        // So it should all work out.
+        // Why bother with this? Because it makes the implementation that
+        // much simpler :)
+        assert_eq!(sid[0], 1, "Expected SID revision to be 1");
+
+        Some(Self { sid })
+    }
+
+    /// Retrieves the account name of this SID.
+    pub(crate) fn account_name(&self) -> Option<String> {
+        unsafe {
+            let mut name_len = 0;
+            let mut domain_len = 0;
+            let mut name_use = SidTypeUnknown;
+
+            if LookupAccountSidW(
+                std::ptr::null_mut(),
+                self.sid.as_ptr() as *mut _,
+                std::ptr::null_mut(),
+                &mut name_len,
+                std::ptr::null_mut(),
+                &mut domain_len,
+                &mut name_use,
+            ) == 0
+            {
+                let error = GetLastError();
+                if error != ERROR_INSUFFICIENT_BUFFER {
+                    sysinfo_debug!("LookupAccountSidW failed: {:?}", error);
+                    return None;
+                }
+            }
+
+            let mut name = vec![0; name_len as usize];
+
+            // Reset length to 0 since we're still passing a NULL pointer
+            // for the domain.
+            domain_len = 0;
+
+            if LookupAccountSidW(
+                std::ptr::null_mut(),
+                self.sid.as_ptr() as *mut _,
+                name.as_mut_ptr(),
+                &mut name_len,
+                std::ptr::null_mut(),
+                &mut domain_len,
+                &mut name_use,
+            ) == 0
+            {
+                sysinfo_debug!("LookupAccountSidW failed: {:?}", GetLastError());
+                return None;
+            }
+
+            Some(to_str(name.as_mut_ptr()))
+        }
+    }
+}
+
+impl Display for Sid {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        unsafe fn convert_sid_to_string_sid(sid: PSID) -> Option<String> {
+            let mut string_sid: LPWSTR = std::ptr::null_mut();
+            if ConvertSidToStringSidW(sid, &mut string_sid) == 0 {
+                sysinfo_debug!("ConvertSidToStringSidW failed: {:?}", GetLastError());
+                return None;
+            }
+            let result = to_str(string_sid);
+            LocalFree(string_sid as *mut _);
+            Some(result)
+        }
+
+        let string_sid = unsafe { convert_sid_to_string_sid(self.sid.as_ptr() as *mut _) };
+        let string_sid = string_sid.ok_or(std::fmt::Error)?;
+
+        write!(f, "{string_sid}")
+    }
+}
+
+impl FromStr for Sid {
+    type Err = String;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        unsafe {
+            let mut string_sid: Vec<u16> = s.encode_utf16().collect();
+            string_sid.push(0);
+
+            let mut psid: PSID = std::ptr::null_mut();
+            if ConvertStringSidToSidW(string_sid.as_ptr(), &mut psid) == 0 {
+                return Err(format!(
+                    "ConvertStringSidToSidW failed: {:?}",
+                    GetLastError()
+                ));
+            }
+            let sid = Self::from_psid(psid);
+            LocalFree(psid as *mut _);
+
+            // Unwrapping because ConvertStringSidToSidW should've performed
+            // all the necessary validations. If it returned an invalid SID,
+            // we better fail fast.
+            Ok(sid.unwrap())
+        }
+    }
+}