chore(rust/gui-client/windows): update windows to 0.58 (#6506)

Supersedes #5913 This required a big refactor because `HANDLE` is no longer `Send` and was never supposed to be. So we add a worker thread for listening to DNS changes, since that requires us to hold a `HANDLE` across `await` points and I couldn't find any simpler way to do it. I could add integration tests for this in a future PR that prove the notifiers work by poking the registry or setting DNS servers and seeing if we pick up the changes on time. But setting DNS servers without the tunnel up may be tricky, so I left it out of scope for this PR. ```[tasklist] - [x] Fix force-kill bug ```
2026-01-27 10:18:54 +00:00 · 2024-09-02 13:00:45 -05:00
parent c7620055a9
commit d8f25f9bf8
7 changed files with 191 additions and 85 deletions
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -1825,9 +1825,9 @@ dependencies = [
 "tracing",
 "tun",
 "uuid",
- "windows 0.57.0",
- "windows-core 0.57.0",
- "windows-implement 0.57.0",
+ "windows 0.58.0",
+ "windows-core 0.58.0",
+ "windows-implement 0.58.0",
 "winreg 0.52.0",
 "wintun",
 "zbus",
@@ -1917,7 +1917,7 @@ dependencies = [
 "tracing-subscriber",
 "url",
 "uuid",
- "windows 0.57.0",
+ "windows 0.58.0",
 "winreg 0.52.0",
 "wintun",
 "zip",
@@ -1961,7 +1961,7 @@ dependencies = [
 "tracing-subscriber",
 "url",
 "uuid",
- "windows 0.57.0",
+ "windows 0.58.0",
 "windows-service",
 "winreg 0.52.0",
 ]
@@ -7244,6 +7244,16 @@ dependencies = [
 "windows-targets 0.52.6",
 ]

+[[package]]
+name = "windows"
+version = "0.58.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "dd04d41d93c4992d421894c18c8b43496aa748dd4c081bac0dc93eb0489272b6"
+dependencies = [
+ "windows-core 0.58.0",
+ "windows-targets 0.52.6",
+]
+
 [[package]]
 name = "windows-bindgen"
 version = "0.39.0"
@@ -7271,7 +7281,7 @@ checksum = "4698e52ed2d08f8658ab0c39512a7c00ee5fe2688c65f8c0a4f06750d729f2a6"
 dependencies = [
 "windows-implement 0.56.0",
 "windows-interface 0.56.0",
- "windows-result",
+ "windows-result 0.1.1",
 "windows-targets 0.52.6",
 ]

@@ -7283,7 +7293,20 @@ checksum = "d2ed2439a290666cd67ecce2b0ffaad89c2a56b976b736e6ece670297897832d"
 dependencies = [
 "windows-implement 0.57.0",
 "windows-interface 0.57.0",
- "windows-result",
+ "windows-result 0.1.1",
+ "windows-targets 0.52.6",
+]
+
+[[package]]
+name = "windows-core"
+version = "0.58.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6ba6d44ec8c2591c134257ce647b7ea6b20335bf6379a27dac5f1641fcf59f99"
+dependencies = [
+ "windows-implement 0.58.0",
+ "windows-interface 0.58.0",
+ "windows-result 0.2.0",
+ "windows-strings",
 "windows-targets 0.52.6",
 ]

@@ -7319,6 +7342,17 @@ dependencies = [
 "syn 2.0.72",
 ]

+[[package]]
+name = "windows-implement"
+version = "0.58.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2bbd5b46c938e506ecbce286b6628a02171d56153ba733b6c741fc627ec9579b"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.72",
+]
+
 [[package]]
 name = "windows-interface"
 version = "0.56.0"
@@ -7341,6 +7375,17 @@ dependencies = [
 "syn 2.0.72",
 ]

+[[package]]
+name = "windows-interface"
+version = "0.58.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "053c4c462dc91d3b1504c6fe5a726dd15e216ba718e84a0e46a88fbe5ded3515"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.72",
+]
+
 [[package]]
 name = "windows-metadata"
 version = "0.39.0"
@@ -7356,6 +7401,15 @@ dependencies = [
 "windows-targets 0.52.6",
 ]

+[[package]]
+name = "windows-result"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1d1043d8214f791817bab27572aaa8af63732e11bf84aa21a45a78d6c317ae0e"
+dependencies = [
+ "windows-targets 0.52.6",
+]
+
 [[package]]
 name = "windows-service"
 version = "0.7.0"
@@ -7367,6 +7421,16 @@ dependencies = [
 "windows-sys 0.52.0",
 ]

+[[package]]
+name = "windows-strings"
+version = "0.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4cd9b125c486025df0eabcb585e62173c6c9eddcec5d117d3b6e8c30e2ee4d10"
+dependencies = [
+ "windows-result 0.2.0",
+ "windows-targets 0.52.6",
+]
+
 [[package]]
 name = "windows-sys"
 version = "0.42.0"
--- a/rust/bin-shared/Cargo.toml
+++ b/rust/bin-shared/Cargo.toml
@@ -35,13 +35,13 @@ zbus = "4.4" # Can't use `zbus`'s `tokio` feature here, or it will break toast p
 known-folders = "1.1.0"
 ring = "0.17"
 uuid = { version = "1.10.0", features = ["v4"] }
-windows-core = "0.57.0"
-windows-implement = "0.57.0"
+windows-core = "0.58.0"
+windows-implement = "0.58.0"
 wintun = "0.4.0"
 winreg = "0.52.0"

 [target.'cfg(windows)'.dependencies.windows]
-version = "0.57.0"
+version = "0.58.0"
 features = [
  # For implementing COM interfaces
  "implement",
--- a/rust/bin-shared/src/network_changes/windows.rs
+++ b/rust/bin-shared/src/network_changes/windows.rs
@@ -66,7 +66,8 @@

 use crate::platform::DnsControlMethod;
 use anyhow::{anyhow, Context as _, Result};
-use tokio::sync::mpsc;
+use std::thread;
+use tokio::sync::{mpsc, oneshot};
 use windows::{
    core::{Interface, Result as WinResult, GUID},
    Win32::{
@@ -81,61 +82,73 @@ use windows::{
 // async needed to match Linux
 #[allow(clippy::unused_async)]
 pub async fn new_dns_notifier(
-    _tokio_handle: tokio::runtime::Handle,
+    tokio_handle: tokio::runtime::Handle,
    _method: DnsControlMethod,
-) -> Result<async_dns::DnsNotifier> {
-    async_dns::DnsNotifier::new()
+) -> Result<Worker> {
+    Worker::new("Firezone DNS monitoring worker", move |tx, stopper_rx| {
+        async_dns::worker_thread(tokio_handle, tx, stopper_rx)?;
+        Ok(())
+    })
 }

+// Async on Linux due to `zbus`
+#[allow(clippy::unused_async)]
 pub async fn new_network_notifier(
    _tokio_handle: tokio::runtime::Handle,
    _method: DnsControlMethod,
-) -> Result<NetworkNotifier> {
-    NetworkNotifier::new().await
+) -> Result<Worker> {
+    Worker::new(
+        "Firezone network monitoring worker",
+        move |tx, stopper_rx| {
+            {
+                let com = ComGuard::new()?;
+                let _network_change_listener = Listener::new(&com, tx)?;
+                stopper_rx.blocking_recv().ok();
+            }
+            Ok(())
+        },
+    )
 }

-/// Notifies when we change Wi-Fi networks, change between Wi-Fi and Ethernet, or gain / lose Internet
-pub struct NetworkNotifier {
+/// Container for a worker thread that we can cooperatively stop.
+///
+/// The worker thread emits notifications with no data in them.
+pub struct Worker {
    inner: Option<WorkerInner>,
    rx: mpsc::Receiver<()>,
+    thread_name: String,
 }

-/// Needed so that `Drop` can consume the oneshot Sender and the thread's JoinHandle
-struct WorkerInner {
-    thread: std::thread::JoinHandle<Result<()>>,
-    stopper: tokio::sync::oneshot::Sender<()>,
+impl Drop for Worker {
+    fn drop(&mut self) {
+        self.close()
+            .expect("should be able to close WorkerInner cleanly");
+    }
 }

-impl NetworkNotifier {
-    // Async on Linux due to `zbus`
-    #[allow(clippy::unused_async)]
-    pub async fn new() -> Result<Self> {
+impl Worker {
+    fn new<F, S>(thread_name: S, func: F) -> Result<Self>
+    where
+        F: FnOnce(mpsc::Sender<()>, oneshot::Receiver<()>) -> Result<()> + Send + 'static,
+        S: Into<String>,
+    {
+        let thread_name = thread_name.into();
        let (tx, rx) = mpsc::channel(1);
-
-        let (stopper, stopper_rx) = tokio::sync::oneshot::channel();
-        let thread = {
-            std::thread::Builder::new()
-                .name("Firezone COM worker".into())
-                .spawn(move || {
-                    {
-                        let com = ComGuard::new()?;
-                        let _network_change_listener = Listener::new(&com, tx)?;
-                        stopper_rx.blocking_recv().ok();
-                    }
-                    tracing::debug!("COM worker thread shut down gracefully");
-                    Ok(())
-                })?
-        };
-
+        let inner = WorkerInner::new(thread_name.clone(), tx, func)?;
        Ok(Self {
-            inner: Some(WorkerInner { thread, stopper }),
+            inner: Some(inner),
            rx,
+            thread_name,
        })
    }

    /// Same as `drop`, but you can catch errors
    pub fn close(&mut self) -> Result<()> {
        if let Some(inner) = self.inner.take() {
+            tracing::debug!(
+                thread_name = self.thread_name,
+                "Asking worker thread to stop gracefully"
+            );
            inner
                .stopper
                .send(())
@@ -144,6 +157,7 @@ impl NetworkNotifier {
                Err(e) => std::panic::resume_unwind(e),
                Ok(x) => x?,
            }
+            tracing::debug!("Worker thread stopped gracefully");
        }
        Ok(())
    }
@@ -151,15 +165,35 @@ impl NetworkNotifier {
    // `Result` needed to match Linux
    #[allow(clippy::unnecessary_wraps)]
    pub async fn notified(&mut self) -> Result<()> {
-        self.rx.recv().await;
+        self.rx
+            .recv()
+            .await
+            .context("MPSC receiver from worker thread ended.")?;
        Ok(())
    }
 }

-impl Drop for NetworkNotifier {
-    fn drop(&mut self) {
-        self.close()
-            .expect("should be able to close Worker cleanly");
+/// Needed so that `Drop` can consume the oneshot Sender and the thread's JoinHandle
+struct WorkerInner {
+    stopper: oneshot::Sender<()>,
+    thread: thread::JoinHandle<Result<()>>,
+}
+
+impl WorkerInner {
+    fn new<
+        F: FnOnce(mpsc::Sender<()>, oneshot::Receiver<()>) -> Result<()> + Send + 'static,
+        S: Into<String>,
+    >(
+        thread_name: S,
+        tx: mpsc::Sender<()>,
+        func: F,
+    ) -> Result<Self> {
+        let (stopper, stopper_rx) = tokio::sync::oneshot::channel();
+        let thread = std::thread::Builder::new()
+            .name(thread_name.into())
+            .spawn(move || func(tx, stopper_rx))?;
+
+        Ok(Self { stopper, thread })
    }
 }

@@ -226,6 +260,9 @@ struct Callback {
 }

 impl<'a> Drop for Listener<'a> {
+    // Might never be called. Due to the way the scopes ended up,
+    // we crash the GUI process before we can get back to the main thread
+    // and drop the DNS listeners
    fn drop(&mut self) {
        self.close().unwrap();
    }
@@ -298,7 +335,8 @@ impl<'a> Listener<'a> {
    }
 }

-impl INetworkEvents_Impl for Callback {
+// <https://github.com/microsoft/windows-rs/pull/3065>
+impl INetworkEvents_Impl for Callback_Impl {
    fn NetworkAdded(&self, _networkid: &GUID) -> WinResult<()> {
        Ok(())
    }
@@ -333,10 +371,13 @@ impl Drop for Callback {
 }

 mod async_dns {
-    use anyhow::{Context, Result};
+    use anyhow::{Context as _, Result};
    use futures::FutureExt as _;
    use std::{ffi::c_void, ops::Deref, path::Path, pin::pin};
-    use tokio::sync::mpsc;
+    use tokio::{
+        sync::{mpsc, oneshot},
+        task::LocalSet,
+    };
    use windows::Win32::{
        Foundation::{CloseHandle, BOOLEAN, HANDLE, INVALID_HANDLE_VALUE},
        System::Registry,
@@ -371,32 +412,32 @@ mod async_dns {
        ))
    }

-    pub struct DnsNotifier {
-        listener_4: Listener,
-        listener_6: Listener,
-    }
-
-    impl DnsNotifier {
-        pub fn new() -> Result<Self> {
+    pub fn worker_thread(
+        tokio_handle: tokio::runtime::Handle,
+        tx: mpsc::Sender<()>,
+        stopper_rx: oneshot::Receiver<()>,
+    ) -> Result<()> {
+        let local = LocalSet::new();
+        let task = local.run_until(async move {
            let (key_ipv4, key_ipv6) = open_network_registry_keys()?;
-            let listener_4 = Listener::new(key_ipv4)?;
-            let listener_6 = Listener::new(key_ipv6)?;
+            let mut listener_4 = Listener::new(key_ipv4)?;
+            let mut listener_6 = Listener::new(key_ipv6)?;

-            Ok(Self {
-                listener_4,
-                listener_6,
-            })
-        }
-
-        pub async fn notified(&mut self) -> Result<()> {
-            let mut fut_4 = pin!(self.listener_4.notified().fuse());
-            let mut fut_6 = pin!(self.listener_6.notified().fuse());
-            futures::select! {
-                r = fut_4 => r?,
-                r = fut_6 => r?,
+            let mut stop = pin!(stopper_rx.fuse());
+            loop {
+                let mut fut_4 = pin!(listener_4.notified().fuse());
+                let mut fut_6 = pin!(listener_6.notified().fuse());
+                futures::select! {
+                    _ = stop => break,
+                    _ = fut_4 => tx.try_send(())?,
+                    _ = fut_6 => tx.try_send(())?,
+                }
            }
+
            Ok(())
-        }
+        });
+
+        tokio_handle.block_on(task)
    }

    /// Listens to one registry key for changes. Callers should await `notified`.
@@ -433,7 +474,7 @@ mod async_dns {
            let tx = Box::new(tx);
            let tx_ptr: *const _ = tx.deref();
            let event = unsafe { CreateEventA(None, false, false, None) }?;
-            let mut wait_handle = HANDLE(0isize);
+            let mut wait_handle = HANDLE::default();

            // The docs say that `RegisterWaitForSingleObject` uses "a worker thread" from
            // "the thread pool".
@@ -510,7 +551,7 @@ mod async_dns {
        //
        // > Each time a process calls RegNotifyChangeKeyValue with the same set of parameters, it establishes another wait operation, creating a resource leak. Therefore, check that you are not calling RegNotifyChangeKeyValue with the same parameters until the previous wait operation has completed.
        fn register_callback(&mut self) -> Result<()> {
-            let key_handle = Registry::HKEY(self.key.raw_handle());
+            let key_handle = Registry::HKEY(self.key.raw_handle() as *mut c_void);
            let notify_flags = Registry::REG_NOTIFY_CHANGE_NAME
                | Registry::REG_NOTIFY_CHANGE_LAST_SET
                | Registry::REG_NOTIFY_THREAD_AGNOSTIC;
@@ -531,7 +572,6 @@ mod async_dns {
                .expect("Should be able to `UnregisterWaitEx` in the DNS change listener");
            unsafe { CloseHandle(self.event) }
                .expect("Should be able to `CloseHandle` in the DNS change listener");
-            tracing::debug!("Gracefully closed DNS change listener");
        }
    }

@@ -564,7 +604,7 @@ mod async_dns {
            let notify_flags = Registry::REG_NOTIFY_CHANGE_NAME
                | Registry::REG_NOTIFY_CHANGE_LAST_SET
                | Registry::REG_NOTIFY_THREAD_AGNOSTIC;
-            let key_handle = Registry::HKEY(key.raw_handle());
+            let key_handle = Registry::HKEY(key.raw_handle() as *mut c_void);
            unsafe {
                Registry::RegNotifyChangeKeyValue(key_handle, true, notify_flags, event, true)
            }
--- a/rust/gui-client/src-tauri/Cargo.toml
+++ b/rust/gui-client/src-tauri/Cargo.toml
@@ -73,7 +73,7 @@ winreg = "0.52.0"
 wintun = "0.4.0"

 [target.'cfg(target_os = "windows")'.dependencies.windows]
-version = "0.57.0"
+version = "0.58.0"
 features = [
  "Win32_Foundation",
  "Win32_System_Threading",
--- a/rust/gui-client/src-tauri/src/client/gui.rs
+++ b/rust/gui-client/src-tauri/src/client/gui.rs
@@ -100,7 +100,6 @@ pub(crate) fn run(
        inject_faults: cli.inject_faults,
    };

-    tracing::info!("Setting up Tauri app instance...");
    let (setup_result_tx, mut setup_result_rx) =
        tokio::sync::oneshot::channel::<Result<(), Error>>();
    let app = tauri::Builder::default()
@@ -144,8 +143,6 @@ pub(crate) fn run(
            }
        })
        .setup(move |app| {
-            tracing::info!("Entered Tauri's `setup`");
-
            let setup_inner = move || {
                // Check for updates
                let ctlr_tx_clone = ctlr_tx.clone();
@@ -995,8 +992,13 @@ async fn run_controller(
        // Code down here may not run because the `select` sometimes `continue`s.
    }

+    tracing::debug!("Closing...");
+
+    if let Err(error) = dns_notifier.close() {
+        tracing::error!(?error, "dns_notifier");
+    }
    if let Err(error) = network_notifier.close() {
-        tracing::error!(?error, "com_worker");
+        tracing::error!(?error, "network_notifier");
    }
    if let Err(error) = controller.ipc_client.disconnect_from_ipc().await {
        tracing::error!(?error, "ipc_client");
--- a/rust/headless-client/Cargo.toml
+++ b/rust/headless-client/Cargo.toml
@@ -60,7 +60,7 @@ windows-service = "0.7.0"
 winreg = "0.52.0"

 [target.'cfg(windows)'.dependencies.windows]
-version = "0.57.0"
+version = "0.58.0"
 features = [
    # For DNS control and route control
    "Win32_Foundation",
--- a/rust/headless-client/src/ipc_service/ipc/windows.rs
+++ b/rust/headless-client/src/ipc_service/ipc/windows.rs
@@ -32,7 +32,7 @@ pub(crate) async fn connect_to_service(id: ServiceId) -> Result<ClientStream, Er
            ErrorKind::NotFound => Error::NotFound(path),
            _ => Error::Other(error.into()),
        })?;
-    let handle = HANDLE(stream.as_raw_handle() as isize);
+    let handle = HANDLE(stream.as_raw_handle());
    let mut server_pid: u32 = 0;
    // SAFETY: Windows doesn't store this pointer or handle, and we just got the handle
    // from Tokio, so it should be valid.
@@ -74,7 +74,7 @@ impl Server {
            .connect()
            .await
            .context("Couldn't accept IPC connection from GUI")?;
-        let handle = HANDLE(server.as_raw_handle() as isize);
+        let handle = HANDLE(server.as_raw_handle());
        let mut client_pid: u32 = 0;
        // SAFETY: Windows doesn't store this pointer or handle, and we just got the handle
        // from Tokio, so it should be valid.