Reduce the scope of unsafe and ways to cause UB

[sftse]

This reduces the number of public API functions that can have bad interactions and cause UB.

This reduces it to only fn inner() and fn unmanage() while previously any combination of methods on StateManager were potentially perilous.

[github-actions]

Package Changes Through c745514

There are 10 changes which include tauri with minor, @tauri-apps/api with minor, tauri-macos-sign with patch, tauri-build with patch, tauri-runtime with minor, tauri-runtime-wry with minor, tauri-bundler with minor, tauri-cli with minor, @tauri-apps/cli with minor, tauri-utils with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package	current	next
@tauri-apps/api	2.10.1	2.11.0
tauri-utils	2.8.3	2.9.0
tauri-macos-sign	2.3.3	2.3.4
tauri-bundler	2.8.1	2.9.0
tauri-runtime	2.10.1	2.11.0
tauri-runtime-wry	2.10.1	2.11.0
tauri-codegen	2.5.5	2.5.6
tauri-macros	2.5.5	2.5.6
tauri-plugin	2.5.4	2.5.5
tauri-build	2.5.6	2.5.7
tauri	2.10.3	2.11.0
@tauri-apps/cli	2.10.1	2.11.0
tauri-cli	2.10.1	2.11.0

Add another change file through the GitHub UI by following this link.

---

Read about change files or the docs at github.com/jbolda/covector

[Legend-Master]

I think originally, the life time alone was enough since the state will be staying for as long as the StateManager, but later on, with the introduction of unmanage, this is no longer true, so now we need to wrap it inside an Arc (#12745 or what this PR tries to do)

Personally, I agree with @WSH032 on this one, and I would probably think we should remove this function in v3

I personally believe that most people do not need the unmanage feature (it seems that tauri itself does not use it also); if users really need unmanage, they can implement it themselves using Mutex and Option::take. This way, users who do not need mutability (unmanage) do not have to pay the performance cost (use Arc instead of Box, and Arc::clone for every get).
https://github.com/tauri-apps/tauri/pull/12745/files#r1962817326

For now though, I think this is fine either ways (a safe implementation for unmanage with a higher overhead vs the current unsafe implementation), just some suggestions on the implementation in this PR:

diff --git a/crates/tauri/src/state.rs b/crates/tauri/src/state.rs
index 37adc8464..69a03afe1 100644
--- a/crates/tauri/src/state.rs
+++ b/crates/tauri/src/state.rs
@@ -6,7 +6,6 @@ use std::{
   any::{Any, TypeId},
   collections::HashMap,
   hash::BuildHasherDefault,
-  marker::PhantomData,
   sync::Arc,
   sync::Mutex,
 };
@@ -20,10 +19,7 @@ use crate::{
 ///
 /// See [`Manager::manage`](`crate::Manager::manage`) for usage examples.
 #[derive(Clone)]
-pub struct State<'r, T: Send + Sync + 'static> {
-  _life: PhantomData<&'r T>,
-  t: Arc<dyn Any + Send + Sync>,
-}
+pub struct State<'r, T: Send + Sync + 'static>(Arc<&'r T>);
 
 impl<'r, T: Send + Sync + 'static> State<'r, T> {
   /// Retrieve a borrow to the underlying value with a lifetime of `'r`.
@@ -31,13 +27,7 @@ impl<'r, T: Send + Sync + 'static> State<'r, T> {
   /// [`std::ops::Deref`] with a [`std::ops::Deref::Target`] of `T`.
   #[inline(always)]
   pub fn inner(&self) -> &'r T {
-    let x: &(dyn Any + Send + Sync) = &*self.t;
-    // SAFETY: everything returned from this function must no longer live when fn unmanage() is called
-    unsafe {
-      &*(x
-        .downcast_ref::<T>()
-        .expect("the type of the key should be same as the type of the value") as *const T)
-    }
+    *self.0
   }
 }
 
@@ -46,19 +36,19 @@ impl<T: Send + Sync + 'static> std::ops::Deref for State<'_, T> {
 
   #[inline(always)]
   fn deref(&self) -> &T {
-    self.inner()
+    &self.0
   }
 }
 
 impl<T: Send + Sync + 'static + PartialEq> PartialEq for State<'_, T> {
   fn eq(&self, other: &Self) -> bool {
-    self.t.downcast_ref::<T>() == other.t.downcast_ref::<T>()
+    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.t).finish()
+    f.debug_tuple("State").field(&self.0).finish()
   }
 }
 
@@ -154,10 +144,11 @@ impl StateManager {
     let map = self.map.lock().unwrap();
     let type_id = TypeId::of::<T>();
     let state = map.get(&type_id)?;
-    Some(State {
-      _life: PhantomData,
-      t: state.clone(),
-    })
+    Some(State(
+      Arc::downcast::<&T>(state.clone())
+        // SAFETY: the type of the key is the same as the type of the value
+        .expect("the type of the key should be same as the type of the value"),
+    ))
   }
 }

also pinging @amrbashir since you did most of the reviews on the last one

[WSH032]

I noticed that many pieces of code/plugins frequently call manager::state, so personally, I care more about the performance aspect. (At least, I think Pin provides the same safety semantics as Arc 🤓)

[sftse]

@@ -154,10 +144,11 @@ impl StateManager {
     let map = self.map.lock().unwrap();
     let type_id = TypeId::of::<T>();
     let state = map.get(&type_id)?;
-    Some(State {
-      _life: PhantomData,
-      t: state.clone(),
-    })
+    Some(State(
+      Arc::downcast::<&T>(state.clone())
+        // SAFETY: the type of the key is the same as the type of the value
+        .expect("the type of the key should be same as the type of the value"),
+    ))
   }
 }

This downcast will fail, the type when it was inserted is T but this attempts to downcast to &T.

let mut map: HashMap<TypeId, Arc<dyn Any + Send + Sync>> = HashMap::new();
map.insert(TypeId::of::<u8>(), Arc::new(0) as Arc<_>);
let arc = map.get(&TypeId::of::<u8>()).unwrap();
assert!(Arc::downcast::<&u8>(arc.clone()).is_err());

There is a way to get this to work with these types, but it defeats the purpose of this PR. Creating the &'r T in fn try_get() means any combination of try_get+unmanage or get+unmanage (and by extension inner+unmanage because inner is a method on the return type of try_get or get) is potentially unsound. The Arc delays creating the reference until the call to inner, meaning if either inner or unmanage are never called, there's no unsoundness.

However, it is a bit contorted, if only to reduce the scope of UB without changing the public API. I don't think the Arc<&'r T> has any benefits over the previous code simply containing &'r T.

I noticed that many pieces of code/plugins frequently call manager::state, so personally, I care more about the performance aspect. (At least, I think Pin provides the same safety semantics as Arc 🤓)

I removed the Pin and the unsafe block because an unsafe block means the safety contract is upheld, but fn unmanage does move the value behind the Pin, violating the contract. This reads as if fn unmanage is unconditionally unsound.

[Legend-Master]

You're right, ignore my suggestions then, I thought the downcast would work but apparently no

[sftse]

@Legend-Master
Do you think we can revisit this PR?

This is a net -3 unsafe blocks that are all not performance critical. The cost is one extra Arc clone per getting of state.

[Legend-Master]

Oh my, I sort of already forget most of what this was about. So essentially we want to see if we should keep unmanage and make it safe or to remove it right?

Looking at https://github.com/orgs/tauri-apps/discussions/10329#discussioncomment-15464163 which I ran into recently as well, it might make sense to just wrap everything in Arc, and we could also avoid having the user to use State<'_, Type> instead of State<Type> everywhere (that might need to wait for the next major though since it's a breaking change)

[sftse]

It was decided to not remove unmanage outright, and not to label it as unsafe. This PR does the best that is possible under those constraints, eliminating as many bad, actually unsound method interactions without changing anything about the public API.

Before this PR, a get followed by an unmanage call is unsound. After this PR, this combination is safe. What is still unsound is get + inner + unmanage but most calls don't need inner and will instead use deref which has the correct lifetime. The only way to produce unsound code is to use inner and unmanage at the same time.

I have other thoughts how to improve state management, but that is for future work, which I'd hope can be ready for v3.

[sftse]

I've added a test that can be run under MIRI to exemplify how this unsoundness can happen in practice, and how this PR fixes it.

First commit adds two unsound ways the API can be called, the test is a bit low-level because using a higher-level API would obscure it unnecessarily.
Run either
MIRIFLAGS='-Zmiri-disable-isolation' cargo miri test t_sound_unmanage
or
MIRIFLAGS='-Zmiri-disable-isolation' cargo miri test t_unsound_unmanage

MIRI will confirm both are unsound. We can run both again at 003c35b86 and only one of them is now unsound. The last commit then removes the unsound test.

[Legend-Master]

I am a bit undecided on this one, especially inner will still have the wrong lifetime after this. I wouldn't mind this if we're having a full rework of the state system in v3 though

@FabianLars @amrbashir any thoughts?

[FabianLars]

I don't have many thoughts tbh. What would speak against this? Even if it's not a full solution, it's still something, no? (tbh all this is a bit over my head at the moment)

[amrbashir]

My two cents is that app.state() can be used in performance critical logic and I would prefer not to add the overhead of Arc even if minimal.

I don't mind if unmanage is unsafe and causes UB in some cases, we should mark that function as unsafe and document the unsafety and soundness issues with it.