Merge branch 'win-fix-st-mutex-drop-order'

4 files changed, 25 insertions, 13 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 928615214c..02e6cb3188 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -54,6 +54,7 @@ Line wrap the file at 100 chars.                                              Th
   Previously, the installer would abort.
 - Revert to using netsh for DNS config, as some Windows builds did not deal with changes correctly.
   `TALPID_DNS_MODULE` can be used to override this.
+- Fix deadlock that could occur when the default route changed while initializing split tunneling.
 
 ### Changed
 - Update Electron from 19.0.13 to 21.1.1.
diff --git a/talpid-core/src/split_tunnel/windows/mod.rs b/talpid-core/src/split_tunnel/windows/mod.rs
index 83fb4727e7..9e091e8f01 100644
--- a/talpid-core/src/split_tunnel/windows/mod.rs
+++ b/talpid-core/src/split_tunnel/windows/mod.rs
@@ -15,7 +15,7 @@ use std::{
     path::{Path, PathBuf},
     sync::{
         atomic::{AtomicBool, Ordering},
-        mpsc as sync_mpsc, Arc, Mutex, RwLock, Weak,
+        mpsc as sync_mpsc, Arc, Mutex, MutexGuard, RwLock, Weak,
     },
     time::Duration,
 };
@@ -639,7 +639,7 @@ impl SplitTunnel {
 
         self._route_change_callback = None;
         let moved_context_mutex = context_mutex.clone();
-        let mut context = context_mutex.lock().unwrap();
+        let context = context_mutex.lock().unwrap();
         let callback = self
             .runtime
             .block_on(
@@ -653,15 +653,29 @@ impl SplitTunnel {
                     })),
             )
             .map(Some)
+            // NOTE: This cannot fail if a callback is created. If that assumption is wrong, this
+            // could deadlock if the dropped callback is invoked (see `init_context`).
             .map_err(|_| Error::RegisterRouteChangeCallback)?;
 
-        context.initialize_internet_addresses()?;
-        context.register_ips()?;
+        Self::init_context(context)?;
         self._route_change_callback = callback;
 
         Ok(())
     }
 
+    fn init_context(
+        mut context: MutexGuard<'_, SplitTunnelDefaultRouteChangeHandlerContext>,
+    ) -> Result<(), Error> {
+        // NOTE: This should remain a separate function. Dropping the context after `callback`
+        // causes a deadlock if `split_tunnel_default_route_change_handler` is called at the same
+        // time (i.e. if a route change has occurred), since it waits on the context and
+        // `CallbackHandle::drop` also waits for `split_tunnel_default_route_change_handler`
+        // to complete.
+
+        context.initialize_internet_addresses()?;
+        context.register_ips()
+    }
+
     /// Instructs the driver to stop redirecting tunnel traffic and INADDR_ANY.
     pub fn clear_tunnel_addresses(&mut self) -> Result<(), Error> {
         self._route_change_callback = None;
diff --git a/talpid-routing/src/windows/mod.rs b/talpid-routing/src/windows/mod.rs
index 7c0e2bd739..b4831e9b3f 100644
--- a/talpid-routing/src/windows/mod.rs
+++ b/talpid-routing/src/windows/mod.rs
@@ -117,7 +117,7 @@ impl RouteManagerHandle {
                 response_tx,
             ))
             .map_err(|_| Error::RouteManagerDown)?;
-        response_rx.await.map_err(|_| Error::ManagerChannelDown)?
+        Ok(response_rx.await.map_err(|_| Error::ManagerChannelDown)?)
     }
 
     /// Applies the given routes while the route manager is running.
@@ -143,7 +143,7 @@ pub enum RouteManagerCommand {
     AddRoutes(HashSet<RequiredRoute>, oneshot::Sender<Result<()>>),
     GetMtuForRoute(IpAddr, oneshot::Sender<Result<u16>>),
     ClearRoutes,
-    RegisterDefaultRouteChangeCallback(Callback, oneshot::Sender<Result<CallbackHandle>>),
+    RegisterDefaultRouteChangeCallback(Callback, oneshot::Sender<CallbackHandle>),
     Shutdown,
 }
 
@@ -180,7 +180,7 @@ impl RouteManager {
             {
                 return Err(Error::RouteManagerDown);
             }
-            result_rx.await.map_err(|_| Error::ManagerChannelDown)?
+            Ok(result_rx.await.map_err(|_| Error::ManagerChannelDown)?)
         } else {
             Err(Error::RouteManagerDown)
         }
diff --git a/talpid-routing/src/windows/route_manager.rs b/talpid-routing/src/windows/route_manager.rs
index 691f39f453..2e9f032180 100644
--- a/talpid-routing/src/windows/route_manager.rs
+++ b/talpid-routing/src/windows/route_manager.rs
@@ -458,18 +458,15 @@ impl RouteManagerInternal {
         Ok(())
     }
 
-    pub fn register_default_route_changed_callback(
-        &self,
-        callback: Callback,
-    ) -> Result<CallbackHandle> {
+    pub fn register_default_route_changed_callback(&self, callback: Callback) -> CallbackHandle {
         let (nonce, callbacks) = &mut *self.callbacks.lock().unwrap();
         let old_nonce = *nonce;
         callbacks.insert(old_nonce, callback);
         *nonce = nonce.wrapping_add(1);
-        Ok(CallbackHandle {
+        CallbackHandle {
             nonce: old_nonce,
             callbacks: self.callbacks.clone(),
-        })
+        }
     }
 
     fn default_route_change<'a>(