diff --git a/rust/Cargo.lock b/rust/Cargo.lock index fb9e01d78..c568f47be 100644 --- 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" diff --git a/rust/bin-shared/Cargo.toml b/rust/bin-shared/Cargo.toml index a621827c8..af9995492 100644 --- 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", diff --git a/rust/bin-shared/src/network_changes/windows.rs b/rust/bin-shared/src/network_changes/windows.rs index 08733e600..319c025c2 100644 --- 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::new() +) -> Result { + 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::new().await +) -> Result { + 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, 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>, - 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 { +impl Worker { + fn new(thread_name: S, func: F) -> Result + where + F: FnOnce(mpsc::Sender<()>, oneshot::Receiver<()>) -> Result<()> + Send + 'static, + S: Into, + { + 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>, +} + +impl WorkerInner { + fn new< + F: FnOnce(mpsc::Sender<()>, oneshot::Receiver<()>) -> Result<()> + Send + 'static, + S: Into, + >( + thread_name: S, + tx: mpsc::Sender<()>, + func: F, + ) -> Result { + 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 { +// +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 { + 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) } diff --git a/rust/gui-client/src-tauri/Cargo.toml b/rust/gui-client/src-tauri/Cargo.toml index 605696554..37319c6a7 100644 --- 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", diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index d6388d26c..0e44a23f5 100644 --- 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::>(); 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"); diff --git a/rust/headless-client/Cargo.toml b/rust/headless-client/Cargo.toml index 79d79d40b..7cb3d1c52 100644 --- 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", diff --git a/rust/headless-client/src/ipc_service/ipc/windows.rs b/rust/headless-client/src/ipc_service/ipc/windows.rs index 52aa507fe..875491a55 100644 --- 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 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.