Repository: tauri-apps/tauri (language: rust)

Issue:
[bug] `StateManager`  is unsound
### Describe the bug

```rust
fn foo(app: tauri::App) {
    use tauri::Manager;

    #[derive(Debug)]
    struct Foo(i32);

    app.manage(Foo(0));
    let state = app.state::<Foo>().inner();
    let mut returned_state = app.unmanage::<Foo>().unwrap();
    returned_state.0 = 42;

    println!("{}", state.0); // 👈 encountered a dangling reference (use-after-free)
}
```

While retaining a reference to the state, removing the managed state from the manager will result in a dangling pointer.

### Reproduction

Here is an equivalent implementation, run it with [miri](https://github.com/rust-lang/miri) (miri cannot directly run tauri):

```rust
use std::{
    any::{Any, TypeId},
    cell::UnsafeCell,
    collections::HashMap,
    hash::BuildHasherDefault,
    sync::{Arc, Mutex},
};

/// A guard for a state value.
///
/// See [`Manager::manage`](`crate::Manager::manage`) for usage examples.
pub struct State<'r, T: Send + Sync + 'static>(&'r T);

impl<'r, T: Send + Sync + 'static> State<'r, T> {
    /// Retrieve a borrow to the underlying value with a lifetime of `'r`.
    /// Using this method is typically unnecessary as `State` implements
    /// [`std::ops::Deref`] with a [`std::ops::Deref::Target`] of `T`.
    #[inline(always)]
    pub fn inner(&self) -> &'r T {
        self.0
    }
}

impl<T: Send + Sync + 'static> std::ops::Deref for State<'_, T> {
    type Target = T;

    #[inline(always)]
    fn deref(&self) -> &T {
        self.0
    }
}

impl<T: Send + Sync + 'static> Clone for State<'_, T> {
    fn clone(&self) -> Self {
        State(self.0)
    }
}

impl<T: Send + Sync + 'static + PartialEq> PartialEq for State<'_, T> {
    fn eq(&self, other: &Self) -> bool {
        self.0 == other.0
    }
}

impl<T: Send + Sync + std::fmt::Debug> std::fmt::Debug for State<'_, T> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_tuple("State").field(&self.0).finish()
    }
}

#[derive(Default)]
struct IdentHash(u64);

impl std::hash::Hasher for IdentHash {
    fn finish(&self) -> u64 {
        self.0
    }

    fn write(&mut self, bytes: &[u8]) {
        for byte in bytes {
            self.write_u8(*byte);
        }
    }

    fn write_u8(&mut self, i: u8) {
        self.0 = (self.0 << 8) | (i as u64);
    }

    fn write_u64(&mut self, i: u64) {
        self.0 = i;
    }
}

type TypeIdMap = HashMap<TypeId, Box<dyn Any>, BuildHasherDefault<IdentHash>>;

/// The Tauri state manager.
#[derive(Debug)]
pub struct StateManager {
    map: Mutex<UnsafeCell<TypeIdMap>>,
}

// SAFETY: data is accessed behind a lock
unsafe impl Sync for StateManager {}
unsafe impl Send for StateManager {}

impl StateManager {
    pub(crate) fn new() -> Self {
        Self {
            map: Default::default(),
        }
    }

    fn with_map_ref<'a, F: FnOnce(&'a TypeIdMap) -> R, R>(&'a self, f: F) -> R {
        let map = self.map.lock().unwrap();
        let map = map.get();
        // SAFETY: safe to access since we are holding a lock
        f(unsafe { &*map })
    }

    fn with_map_mut<F: FnOnce(&mut TypeIdMap) -> R, R>(&self, f: F) -> R {
        let mut map = self.map.lock().unwrap();
        let map = map.get_mut();
        f(map)
    }

    pub(crate) fn set<T: Send + Sync + 'static>(&self, state: T) -> bool {
        self.with_map_mut(|map| {
            let type_id = TypeId::of::<T>();
            let already_set = map.contains_key(&type_id);
            if !already_set {
                map.insert(type_id, Box::new(state) as Box<dyn Any>);
            }
            !already_set
        })
    }

    pub(crate) fn unmanage<T: Send + Sync + 'static>(&self) -> Option<T> {
        self.with_map_mut(|map| {
            let type_id = TypeId::of::<T>();
            map.remove(&type_id)
                .and_then(|ptr| ptr.downcast().ok().map(|b| *b))
        })
    }

    /// Gets the state associated with the specified type.
    pub fn get<T: Send + Sync + 'static>(&self) -> State<'_, T> {
        self.try_get()
            .expect("state: get() when given type is not managed")
    }

    /// Gets the state associated with the specified type.
    pub fn try_get<T: Send + Sync + 'static>(&self) -> Option<State<'_, T>> {
        self.with_map_ref(|map| {
            map.get(&TypeId::of::<T>())
                .and_then(|ptr| ptr.downcast_ref::<T>())
                .map(State)
        })
    }
}

fn main() {
    #[derive(Debug)]
    struct Foo(i32);

    let state_manager = Arc::new(StateManager::new());

    state_manager.set(Foo(0)); // manage the state
    let state = state_manager.try_get::<Foo>().unwrap().inner(); // then get the state
    let mut returned_state = state_manager.unmanage::<Foo>().unwrap(); // unmanage the state
    returned_state.0 = 42;

    println!("{}", state.0); // 👈 encountered a dangling reference (use-after-free)
}
```

### Expected behavior

The unsoundness comes from this line: <https://github.com/tauri-apps/tauri/blob/3f680588cd5ada3a44245fd77b5b87d7ee4c478e/crates/tauri/src/state.rs#L123>.

It allows retaining a reference to the Hashmap value after the `MutexGuard` is dropped.

### Full `tauri info` output

```text
rust 1.82 stable
tauri 2.2.5
```

### Stack trace

`cargo +nightly miri run`

```text
error: Undefined Behavior: out-of-bounds pointer use: alloc1105 has been freed, so this pointer is dangling
   --> src/main.rs:152:20
    |
152 |     println!("{}", state.0); // 👈 encountered a dangling reference (use-after-free)
    |                    ^^^^^^^ out-of-bounds pointer use: alloc1105 has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc1105 was allocated here:
   --> src/main.rs:111:37
    |
    |                                     ^^^^^^^^^^^^^^^
help: alloc1105 was deallocated here:
   --> src/main.rs:121:62
    |
121 |                 .and_then(|ptr| ptr.downcast().ok().map(|b| *b))
    |                                                              ^
    = note: BACKTRACE (of the first span):
    = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error
```

### Additional context

_No response_


Discussion / hints:
Due to semantic compatibility, it seems that it can't be fixed 😰, especially since users can still hold `&Foo` after `drop(state)` (via `State::inner`).

It seems we can only deprecate `Manager::unmanage` to let users know about its unsoundness.

Fix patch:
diff --git a/.changes/fix-state-manager-unsoundness.md b/.changes/fix-state-manager-unsoundness.md
new file mode 100644
index 000000000000..409ce7962002
--- /dev/null
+++ b/.changes/fix-state-manager-unsoundness.md
@@ -0,0 +1,5 @@
+---
+tauri: 'minor:bug'
+---
+
+Deprecate `Manager::unmanage` to fix `use-after-free` unsoundness, see tauri-apps/tauri#12721 for details.
diff --git a/crates/tauri/src/lib.rs b/crates/tauri/src/lib.rs
index 44d29258ac3e..db9ab432c7a0 100644
--- a/crates/tauri/src/lib.rs
+++ b/crates/tauri/src/lib.rs
@@ -711,11 +711,31 @@ pub trait Manager<R: Runtime>: sealed::ManagerBase<R> {
   }
 
   /// Removes the state managed by the application for T. Returns the state if it was actually removed.
+  ///
+  /// <div class="warning">
+  ///
+  /// This method is *UNSAFE* and calling it will cause previously obtained references through
+  /// [Manager::state] and [State::inner] to become dangling references.
+  ///
+  /// It is currently deprecated and may be removed in the future.
+  ///
+  /// If you really want to unmanage a state, use [std::sync::Mutex] and [Option::take] to wrap the state instead.
+  ///
+  /// See [tauri-apps/tauri#12721] for more information.
+  ///
+  /// [tauri-apps/tauri#12721]: https://github.com/tauri-apps/tauri/issues/12721
+  ///
+  /// </div>
+  #[deprecated(
+    since = "2.3.0",
+    note = "This method is unsafe, since it can cause dangling references."
+  )]
   fn unmanage<T>(&self) -> Option<T>
   where
     T: Send + Sync + 'static,
   {
-    self.manager().state().unmanage()
+    // The caller decides to break the safety here, then OK, just let it go.
+    unsafe { self.manager().state().unmanage() }
   }
 
   /// Retrieves the managed state for the type `T`.
diff --git a/crates/tauri/src/state.rs b/crates/tauri/src/state.rs
index 2f1092e6ce02..a1b1058fca32 100644
--- a/crates/tauri/src/state.rs
+++ b/crates/tauri/src/state.rs
@@ -4,9 +4,9 @@
 
 use std::{
   any::{Any, TypeId},
-  cell::UnsafeCell,
   collections::HashMap,
   hash::BuildHasherDefault,
+  pin::Pin,
   sync::Mutex,
 };
 
@@ -97,18 +97,17 @@ impl std::hash::Hasher for IdentHash {
   }
 }
 
-type TypeIdMap = HashMap<TypeId, Box<dyn Any>, BuildHasherDefault<IdentHash>>;
+/// Safety:
+/// - The `key` must equal to `(*value).type_id()`, see the safety doc in methods of [StateManager] for details.
+/// - Once you insert a value, you can't remove/mutated/move it anymore, see [StateManager::try_get] for details.
+type TypeIdMap = HashMap<TypeId, Pin<Box<dyn Any + Sync + Send>>, BuildHasherDefault<IdentHash>>;
 
 /// The Tauri state manager.
 #[derive(Debug)]
 pub struct StateManager {
-  map: Mutex<UnsafeCell<TypeIdMap>>,
+  map: Mutex<TypeIdMap>,
 }
 
-// SAFETY: data is accessed behind a lock
-unsafe impl Sync for StateManager {}
-unsafe impl Send for StateManager {}
-
 impl StateManager {
   pub(crate) fn new() -> Self {
     Self {
@@ -116,37 +115,38 @@ impl StateManager {
     }
   }
 
-  fn with_map_ref<'a, F: FnOnce(&'a TypeIdMap) -> R, R>(&'a self, f: F) -> R {
-    let map = self.map.lock().unwrap();
-    let map = map.get();
-    // SAFETY: safe to access since we are holding a lock
-    f(unsafe { &*map })
-  }
-
-  fn with_map_mut<F: FnOnce(&mut TypeIdMap) -> R, R>(&self, f: F) -> R {
-    let mut map = self.map.lock().unwrap();
-    let map = map.get_mut();
-    f(map)
-  }
-
   pub(crate) fn set<T: Send + Sync + 'static>(&self, state: T) -> bool {
-    self.with_map_mut(|map| {
-      let type_id = TypeId::of::<T>();
-      let already_set = map.contains_key(&type_id);
-      if !already_set {
-        map.insert(type_id, Box::new(state) as Box<dyn Any>);
-      }
-      !already_set
-    })
+    let mut map = self.map.lock().unwrap();
+    let type_id = TypeId::of::<T>();
+    let already_set = map.contains_key(&type_id);
+    if !already_set {
+      let ptr = Box::new(state) as Box<dyn Any + Sync + Send>;
+      let pinned_ptr = Box::into_pin(ptr);
+      map.insert(
+        type_id,
+        // SAFETY: keep the type of the key is the same as the type of the value，
+        // see [try_get] methods for details.
+        pinned_ptr,
+      );
+    }
+    !already_set
   }
 
-  pub(crate) fn unmanage<T: Send + Sync + 'static>(&self) -> Option<T> {
-    self.with_map_mut(|map| {
-      let type_id = TypeId::of::<T>();
-      map
-        .remove(&type_id)
-        .and_then(|ptr| ptr.downcast().ok().map(|b| *b))
-    })
+  /// SAFETY: Calling this method will move the `value`,
+  /// which will cause references obtained through [Self::try_get] to dangle.
+  pub(crate) unsafe fn unmanage<T: Send + Sync + 'static>(&self) -> Option<T> {
+    let mut map = self.map.lock().unwrap();
+    let type_id = TypeId::of::<T>();
+    let pinned_ptr = map.remove(&type_id)?;
+    // SAFETY: The caller decides to break the immovability/safety here, then OK, just let it go.
+    let ptr = unsafe { Pin::into_inner_unchecked(pinned_ptr) };
+    let value = unsafe {
+      ptr
+        .downcast::<T>()
+        // SAFETY: the type of the key is the same as the type of the value
+        .unwrap_unchecked()
+    };
+    Some(*value)
   }
 
   /// Gets the state associated with the specified type.
@@ -158,12 +158,18 @@ impl StateManager {
 
   /// Gets the state associated with the specified type.
   pub fn try_get<T: Send + Sync + 'static>(&self) -> Option<State<'_, T>> {
-    self.with_map_ref(|map| {
-      map
-        .get(&TypeId::of::<T>())
-        .and_then(|ptr| ptr.downcast_ref::<T>())
-        .map(State)
-    })
+    let map = self.map.lock().unwrap();
+    let type_id = TypeId::of::<T>();
+    let ptr = map.get(&type_id)?;
+    let value = unsafe {
+      ptr
+        .downcast_ref::<T>()
+        // SAFETY: the type of the key is the same as the type of the value
+        .unwrap_unchecked()
+    };
+    // SAFETY: We ensure the lifetime of `value` is the same as [StateManager] and `value` will not be mutated/moved.
+    let v_ref = unsafe { &*(value as *const T) };
+    Some(State(v_ref))
   }
 }
 
@@ -197,8 +203,9 @@ mod tests {
     let state = StateManager::new();
     assert!(state.set(1u32));
     assert_eq!(*state.get::<u32>(), 1);
-    assert!(state.unmanage::<u32>().is_some());
-    assert!(state.unmanage::<u32>().is_none());
+    // safety: the reference returned by `try_get` is already dropped.
+    assert!(unsafe { state.unmanage::<u32>() }.is_some());
+    assert!(unsafe { state.unmanage::<u32>() }.is_none());
     assert_eq!(state.try_get::<u32>(), None);
     assert!(state.set(2u32));
     assert_eq!(*state.get::<u32>(), 2);


Test patch: