From f48df7585c1a5de2bbeda4189aa52fadfd6bb62c Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 10 Feb 2025 23:33:06 +0000 Subject: [PATCH] refactor(windows): de-duplicate Win32 error codes (#8071) The errors returned from Win32 API calls are currently duplicated in several places. To makes it error-prone to handle them correctly. With this PR, we de-duplicate this and add proper docs and links for further reading to them. We also fix a case where we would currently fail to set IP addresses for our tunnel interface if the IP stack is not supported. --- .../src/tun_device_manager/windows.rs | 20 ++++---- rust/bin-shared/src/windows.rs | 50 ++++++++++++++++--- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/rust/bin-shared/src/tun_device_manager/windows.rs b/rust/bin-shared/src/tun_device_manager/windows.rs index c4dd6fb7b..7a9512ee9 100644 --- a/rust/bin-shared/src/tun_device_manager/windows.rs +++ b/rust/bin-shared/src/tun_device_manager/windows.rs @@ -1,3 +1,4 @@ +use crate::windows::error::{NOT_FOUND, NOT_SUPPORTED, OBJECT_EXISTS}; use crate::windows::TUNNEL_UUID; use crate::TUNNEL_NAME; use anyhow::{Context as _, Result}; @@ -28,7 +29,6 @@ use windows::Win32::{ }, Networking::WinSock::{ADDRESS_FAMILY, AF_INET, AF_INET6}, }; -use windows_core::HRESULT; use wintun::Adapter; /// The ring buffer size used for Wintun. @@ -78,9 +78,6 @@ impl TunDeviceManager { .luid .context("Cannot set IPs prior to creating an adapter")?; - tracing::debug!("Setting our IPv4 = {}", ipv4); - tracing::debug!("Setting our IPv6 = {}", ipv6); - // SAFETY: Both NET_LUID_LH unions should be the same. We're just copying out // the u64 value and re-wrapping it, since wintun doesn't refer to the windows // crate's version of NET_LUID_LH. @@ -122,7 +119,6 @@ impl TunDeviceManager { // It's okay if this blocks until the route is added in the OS. fn add_route(route: IpNetwork, iface_idx: u32) { - const DUPLICATE_ERR: u32 = 0x80071392; let entry = forward_entry(route, iface_idx); // SAFETY: Windows shouldn't store the reference anywhere, it's just a way to pass lots of arguments at once. And no other thread sees this variable. @@ -133,7 +129,7 @@ fn add_route(route: IpNetwork, iface_idx: u32) { }; // We expect set_routes to call add_route with the same routes always making this error expected - if e.code().0 as u32 == DUPLICATE_ERR { + if e.code() == OBJECT_EXISTS { return; } @@ -142,7 +138,6 @@ fn add_route(route: IpNetwork, iface_idx: u32) { // It's okay if this blocks until the route is removed in the OS. fn remove_route(route: IpNetwork, iface_idx: u32) { - const ELEMENT_NOT_FOUND: u32 = 0x80070490; let entry = forward_entry(route, iface_idx); // SAFETY: Windows shouldn't store the reference anywhere, it's just a way to pass lots of arguments at once. And no other thread sees this variable. @@ -153,7 +148,7 @@ fn remove_route(route: IpNetwork, iface_idx: u32) { return; }; - if e.code().0 as u32 == ELEMENT_NOT_FOUND { + if e.code() == NOT_FOUND { return; } @@ -387,7 +382,7 @@ fn try_set_mtu(luid: NET_LUID_LH, family: ADDRESS_FAMILY, mtu: u32) -> Result<() // SAFETY: TODO if let Err(error) = unsafe { GetIpInterfaceEntry(&mut row) }.ok() { - if family == AF_INET6 && error.code() == windows_core::HRESULT::from_win32(0x80070490) { + if family == AF_INET6 && error.code() == NOT_FOUND { tracing::debug!(?family, "Couldn't set MTU, maybe IPv6 is disabled."); } else { tracing::warn!(?family, "Couldn't set MTU: {}", err_with_src(&error)); @@ -406,7 +401,7 @@ fn try_set_mtu(luid: NET_LUID_LH, family: ADDRESS_FAMILY, mtu: u32) -> Result<() } fn try_set_ip(luid: NET_LUID_LH, ip: IpAddr) -> Result<()> { - const ERROR_OBJECT_ALREADY_EXISTS: HRESULT = HRESULT::from_win32(0x80071392); + tracing::debug!(%ip, "Setting tunnel interface IP"); // Safety: Docs don't mention anything in regards to safety of this function. let mut row = unsafe { @@ -435,7 +430,10 @@ fn try_set_ip(luid: NET_LUID_LH, ip: IpAddr) -> Result<()> { // Safety: Docs don't mention anything about safety other than having to use `InitializeUnicastIpAddressEntry` and we did that. match unsafe { CreateUnicastIpAddressEntry(&row) }.ok() { Ok(()) => {} - Err(e) if e.code() == ERROR_OBJECT_ALREADY_EXISTS => {} + Err(e) if e.code() == NOT_SUPPORTED => { + tracing::debug!(%ip, "IP stack not supported"); + } + Err(e) if e.code() == OBJECT_EXISTS => {} Err(e) => { return Err(anyhow::Error::new(e).context("Failed to create `UnicastIpAddressEntry`")) } diff --git a/rust/bin-shared/src/windows.rs b/rust/bin-shared/src/windows.rs index e7acba394..b14e07873 100644 --- a/rust/bin-shared/src/windows.rs +++ b/rust/bin-shared/src/windows.rs @@ -36,6 +36,48 @@ pub const CREATE_NO_WINDOW: u32 = 0x08000000; /// This ends up in registry keys and tunnel management. pub const TUNNEL_UUID: Uuid = Uuid::from_u128(0xe924_5bc1_b8c1_44ca_ab1d_c6aa_d4f1_3b9c); +/// Error codes returned from Windows APIs. +/// +/// For details, see the Windows Error reference: . +/// +/// ## tl;dr +/// +/// - Windows _result_ codes are 32-bit numbers. +/// - The most-signficant bit indicates an error, if set. Hence all results here start with an `8`. +/// - We can ignore the next 4 bits. +/// - The "7" indicates the facility. In our case, this means Win32 APIs. +/// - The last 4 numbers (i.e. 16 bit) are the actual error code. +/// +/// ## Adding new codes +/// +/// We create the error codes using the [`windows_core::HRESULT::from_win32`] constructor which sets these bits correctly. +/// The doctests make sure we actually construct the error that we'll see in logs. +/// Being able to search for these with full-text search is important for maintenance. +pub mod error { + use windows_core::HRESULT; + + /// Win32 error code objects that don't exist (like network adapters). + /// + /// ``` + /// assert_eq!(firezone_bin_shared::windows::error::NOT_FOUND.0 as u32, 0x80070490) + /// ``` + pub const NOT_FOUND: HRESULT = HRESULT::from_win32(0x0490); + + /// Win32 error code for objects that already exist (like routing table entries). + /// + /// ``` + /// assert_eq!(firezone_bin_shared::windows::error::OBJECT_EXISTS.0 as u32, 0x80071392) + /// ``` + pub const OBJECT_EXISTS: HRESULT = HRESULT::from_win32(0x1392); + + /// Win32 error code for unsupported operations (like setting an IPv6 address without an IPv6 stack). + /// + /// ``` + /// assert_eq!(firezone_bin_shared::windows::error::NOT_SUPPORTED.0 as u32, 0x80070032) + /// ``` + pub const NOT_SUPPORTED: HRESULT = HRESULT::from_win32(0x0032); +} + #[derive(clap::ValueEnum, Clone, Copy, Debug)] pub enum DnsControlMethod { /// Explicitly disable DNS control. @@ -160,8 +202,6 @@ struct RoutingTableEntry { impl RoutingTableEntry { /// Creates a new routing table entry by using the given prototype and overriding the route. fn create(route: IpAddr, mut prototype: MIB_IPFORWARD_ROW2) -> io::Result { - const DUPLICATE_ERR: u32 = 0x80071392; - let prefix = &mut prototype.DestinationPrefix; match route { IpAddr::V4(x) => { @@ -178,7 +218,7 @@ impl RoutingTableEntry { unsafe { CreateIpForwardEntry2(&prototype) } .ok() .or_else(|e| { - if e.code().0 as u32 == DUPLICATE_ERR { + if e.code() == error::OBJECT_EXISTS { Ok(()) } else { Err(io::Error::other(e)) @@ -198,8 +238,6 @@ impl RoutingTableEntry { impl Drop for RoutingTableEntry { fn drop(&mut self) { - const NOT_FOUND: u32 = 0x80070490; - let iface_idx = self.entry.InterfaceIndex; // Safety: The entry we stored is valid. @@ -208,7 +246,7 @@ impl Drop for RoutingTableEntry { return; }; - if e.code().0 as u32 == NOT_FOUND { + if e.code() == error::NOT_FOUND { return; }