Merge branch 'document-and-lint-all-usages-of-unsafe-des-1459'

Author: hulthe

Cargo.lock

@@ -74,6 +74,7 @@ single_use_lifetimes = "warn"
 
 [workspace.lints.clippy]
 unused_async = "deny"
+undocumented_unsafe_blocks = "warn"
 
 [workspace.dependencies]
 hickory-proto = "0.24.3"

Cargo.toml

@@ -1,6 +1,5 @@
 //! Forward [NSEvent]s from macOS to node.
 #![cfg(target_os = "macos")]
-#![warn(clippy::undocumented_unsafe_blocks)]
 
 use std::ptr::NonNull;
 use std::sync::{mpsc, Arc, Mutex};

desktop/packages/nseventforwarder/nseventforwarder-rs/lib.rs

@@ -1,4 +1,6 @@
 #![cfg(not(target_os = "android"))]
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use std::{
     ffi::{CStr, CString},
     net::SocketAddr,

mullvad-api/src/ffi/mod.rs

@@ -1,5 +1,4 @@
 //! Install signal handlers to catch critical program faults and log them. See [`enable`].
-#![warn(clippy::undocumented_unsafe_blocks)]
 
 use libc::siginfo_t;
 use nix::sys::signal::{sigaction, SaFlags, SigAction, SigHandler, SigSet, Signal};

mullvad-daemon/src/exception_logging/unix.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)]
+
 use mullvad_paths::log_dir;
 use std::{
     borrow::Cow,

mullvad-daemon/src/exception_logging/win.rs

@@ -5,6 +5,8 @@
 //! must be checked so that the user can be directed to approve the launch
 //! daemon in the system settings.
 
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use libc::c_longlong;
 use objc2::{class, msg_send, runtime::AnyObject, Encode, Encoding, RefEncode};
 use std::ffi::CStr;

mullvad-daemon/src/macos_launch_daemon.rs

@@ -229,8 +229,7 @@ async fn create_daemon(log_dir: Option<PathBuf>) -> Result<Daemon, String> {
 
 #[cfg(unix)]
 fn running_as_admin() -> bool {
-    let uid = unsafe { libc::getuid() };
-    uid == 0
+    nix::unistd::Uid::current().is_root()
 }
 
 #[cfg(windows)]

mullvad-daemon/src/main.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 }
     }
 }
 

mullvad-daemon/src/migrations/mod.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use crate::cli;
 use mullvad_daemon::{runtime::new_multi_thread, DaemonShutdownHandle};
 use std::{

mullvad-daemon/src/system_service.rs

@@ -1,4 +1,6 @@
 #![cfg(target_os = "ios")]
+#![allow(clippy::undocumented_unsafe_blocks)]
+
 mod api_client;
 mod encrypted_dns_proxy;
 mod ephemeral_peer_proxy;

mullvad-ios/src/lib.rs

@@ -47,9 +47,14 @@ impl fmt::Debug for Interface {
         match self {
             Self::Name(arg0) => f.debug_tuple("Name").field(arg0).finish(),
 
-            // SAFETY: u64 is valid for all bit patterns, so reading the union as a u64 is safe.
             #[cfg(target_os = "windows")]
-            Self::Luid(arg0) => f.debug_tuple("Luid").field(&unsafe { arg0.Value }).finish(),
+            Self::Luid(arg0) => f
+                .debug_tuple("Luid")
+                .field(
+                    // SAFETY: u64 is valid for all bit patterns, so reading the union as a u64 is safe.
+                    &unsafe { arg0.Value },
+                )
+                .finish(),
 
             #[cfg(target_os = "macos")]
             Self::Index(arg0) => f.debug_tuple("Luid").field(arg0).finish(),

mullvad-leak-checker/src/lib.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use crate::{Error, Result};
 use once_cell::sync::OnceCell;
 use std::{
@@ -362,6 +364,7 @@ fn adjust_token_privilege(
     privilege: &WideCStr,
     enable: bool,
 ) -> std::io::Result<()> {
+    // SAFETY: LUID is a C struct and can safely be zeroed.
     let mut privilege_luid: LUID = unsafe { mem::zeroed() };
 
     if unsafe { LookupPrivilegeValueW(ptr::null(), privilege.as_ptr(), &mut privilege_luid) } == 0 {

mullvad-paths/src/windows.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use parking_lot::Mutex;
 use std::{
     collections::{BTreeSet, HashMap},

talpid-core/src/dns/macos.rs

@@ -62,6 +62,7 @@ impl DnsApi {
         std::thread::spawn(move || {
             let begin = Instant::now();
 
+            // SAFETY: this function is trivially safe to call
             let result = if unsafe { (DnsFlushResolverCache)() } != 0 {
                 let elapsed = begin.elapsed();
                 if elapsed >= FLUSH_TIMEOUT {

talpid-core/src/dns/windows/dnsapi.rs

@@ -2,6 +2,7 @@
 //! <https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-setinterfacednssettings>,
 //! it requires at least Windows 10, build 19041. For that reason, use run-time linking and fall
 //! back on other methods if it is not available.
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
 
 use crate::dns::{DnsMonitorT, ResolvedDnsConfig};
 use once_cell::sync::OnceCell;

talpid-core/src/dns/windows/iphlpapi.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use crate::dns::{DnsMonitorT, ResolvedDnsConfig};
 use std::{
     ffi::OsString,

talpid-core/src/dns/windows/netsh.rs

@@ -164,8 +164,12 @@ fn flush_dns_cache() -> Result<(), Error> {
 /// Obtain a string representation for a GUID object.
 fn string_from_guid(guid: &GUID) -> String {
     let mut buffer = [0u16; 40];
-    let length = unsafe { StringFromGUID2(guid, &mut buffer[0] as *mut _, buffer.len() as i32 - 1) }
-        as usize;
+
+    let length =
+        // SAFETY: `guid` and `buffer` are valid references.
+        // StringFromGUID2 won't write past the end of the provided length.
+        unsafe { StringFromGUID2(guid, buffer.as_mut_ptr(), buffer.len() as i32 - 1) } as usize;
+
     // cannot fail because `buffer` is large enough
     assert!(length > 0);
     let length = length - 1;

talpid-core/src/dns/windows/tcpip.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use crate::{dns::ResolvedDnsConfig, tunnel::TunnelMetadata};
 
 use std::{ffi::CStr, io, net::IpAddr, ptr, sync::LazyLock};

talpid-core/src/firewall/windows/mod.rs

@@ -1,27 +1,16 @@
-use std::{
-    ffi::{self, CString},
-    io,
-};
+use nix::{errno::Errno, net::if_::if_nametoindex};
 
 /// Converts an interface name into the corresponding index.
 pub fn iface_index(name: &str) -> Result<libc::c_uint, IfaceIndexLookupError> {
-    let c_name = CString::new(name)
-        .map_err(|e| IfaceIndexLookupError::InvalidInterfaceName(name.to_owned(), e))?;
-    let index = unsafe { libc::if_nametoindex(c_name.as_ptr()) };
-    if index == 0 {
-        Err(IfaceIndexLookupError::InterfaceLookupError(
-            name.to_owned(),
-            io::Error::last_os_error(),
-        ))
-    } else {
-        Ok(index)
-    }
+    if_nametoindex(name).map_err(|error| IfaceIndexLookupError {
+        interface_name: name.to_owned(),
+        error,
+    })
 }
 
 #[derive(Debug, thiserror::Error)]
-pub enum IfaceIndexLookupError {
-    #[error("Invalid network interface name: {0}")]
-    InvalidInterfaceName(String, #[source] ffi::NulError),
-    #[error("Failed to get index for interface {0}")]
-    InterfaceLookupError(String, #[source] io::Error),
+#[error("Failed to get index for interface {interface_name}: {error}")]
+pub struct IfaceIndexLookupError {
+    pub interface_name: String,
+    pub error: Errno,
 }

talpid-core/src/linux/mod.rs

@@ -24,8 +24,6 @@ pub struct BroadcastListener {
     _notify_tx: Arc<UnboundedSender<Connectivity>>,
 }
 
-unsafe impl Send for BroadcastListener {}
-
 impl BroadcastListener {
     pub async fn start(
         notify_tx: UnboundedSender<Connectivity>,

talpid-core/src/offline/windows.rs

@@ -132,16 +132,17 @@ impl Bpf {
             return Err(Error::InterfaceNameTooLong);
         }
 
+        // SAFETY: `name_bytes` cannot exceed the size of `ifr_name`
         unsafe {
-            // SAFETY: `name_bytes` cannot exceed the size of `ifr_name`
             std::ptr::copy_nonoverlapping(
                 name_bytes.as_ptr(),
                 &mut ifr.ifr_name as *mut _ as *mut _,
                 name_bytes.len(),
-            );
-            // SAFETY: The fd is valid for the lifetime of `self`, and `ifr` has a valid interface
-            ioctl!(self.file.as_raw_fd(), BIOCSETIF, &ifr)
-        }
+            )
+        };
+
+        // SAFETY: The fd is valid for the lifetime of `self`, and `ifr` has a valid interface
+        unsafe { ioctl!(self.file.as_raw_fd(), BIOCSETIF, &ifr) }
     }
 
     /// Enable or disable immediate mode (BIOCIMMEDIATE)

talpid-core/src/split_tunnel/macos/bpf.rs

@@ -20,7 +20,7 @@ use pnet_packet::{
     MutablePacket, Packet,
 };
 use std::{
-    ffi::{c_uint, CStr},
+    ffi::c_uint,
     io::{self, IoSlice, Write},
     net::{IpAddr, Ipv4Addr, Ipv6Addr},
 };
@@ -887,8 +887,14 @@ impl PacketCodec for PktapCodec {
             return None;
         }
 
-        let iface = unsafe { CStr::from_ptr(header.pth_ifname.as_ptr() as *const _) };
-        if iface.to_bytes() != self.interface.as_bytes() {
+        // cast the array from [i8] to [u8] to enable comparison with String::as_bytes
+        let iface = header.pth_ifname.map(|b| b as u8);
+        // get the interface name by splitting on the first null byte (if any)
+        let iface = iface
+            .split(|&b| b == 0)
+            .next()
+            .expect("split will yield at least one element");
+        if iface != self.interface.as_bytes() {
             return None;
         }
 

talpid-core/src/split_tunnel/macos/tun.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 use super::windows::{
     get_device_path, get_process_creation_time, get_process_device_path, open_process,
     ProcessAccess,

talpid-core/src/split_tunnel/windows/driver.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
+
 mod driver;
 mod path_monitor;
 mod service;

talpid-core/src/split_tunnel/windows/mod.rs

@@ -1,4 +1,5 @@
 //! Utilities for working with windows on Windows.
+#![allow(clippy::undocumented_unsafe_blocks)] // Remove me if you dare.
 
 use std::{os::windows::io::AsRawHandle, ptr, sync::Arc, thread};
 use tokio::sync::broadcast;

talpid-core/src/window.rs

@@ -15,3 +15,5 @@ libc = "0.2"
 talpid-types = { path = "../talpid-types" }
 socket2 = { workspace = true, features = ["all"] }
 log = { workspace = true }
+thiserror = { workspace = true }
+nix = { version = "0.29", features = ["net"] }