diff --git a/.changes/fix-state-manager-unsoundness.md b/.changes/fix-state-manager-unsoundness.md new file mode 100644 index 000000000000..f1281bf41f1d --- /dev/null +++ b/.changes/fix-state-manager-unsoundness.md @@ -0,0 +1,5 @@ +--- +tauri: 'minor:bug' +--- + +Fix `use-after-free` unsoundness for using `State::inner` after `Manager::unmanage`, see tauri-apps/tauri#12721 for details. diff --git a/bench/tests/files_transfer/src-tauri/src/main.rs b/bench/tests/files_transfer/src-tauri/src/main.rs index 3563d86850bf..a0a81e15497d 100644 --- a/bench/tests/files_transfer/src-tauri/src/main.rs +++ b/bench/tests/files_transfer/src-tauri/src/main.rs @@ -15,7 +15,7 @@ fn app_should_close(exit_code: i32) { #[command] async fn read_file(app: AppHandle) -> Result { let path = app - .path() + .path_arc() .resolve(".tauri_3mb.json", BaseDirectory::Home) .map_err(|e| e.to_string())?; let contents = read(path).map_err(|e| e.to_string())?; diff --git a/crates/tauri/src/app.rs b/crates/tauri/src/app.rs index 57fc7a00ac56..de9dd9d6b80e 100644 --- a/crates/tauri/src/app.rs +++ b/crates/tauri/src/app.rs @@ -1981,7 +1981,7 @@ tauri::Builder::default() if let crate::utils::config::WebviewInstallMode::FixedRuntime { path } = &app.manager.config().bundle.windows.webview_install_mode { - if let Ok(resource_dir) = app.path().resource_dir() { + if let Ok(resource_dir) = app.path_arc().resource_dir() { std::env::set_var( "WEBVIEW2_BROWSER_EXECUTABLE_FOLDER", resource_dir.join(path), diff --git a/crates/tauri/src/ipc/authority.rs b/crates/tauri/src/ipc/authority.rs index 3a0985f87699..2af6df501325 100644 --- a/crates/tauri/src/ipc/authority.rs +++ b/crates/tauri/src/ipc/authority.rs @@ -854,7 +854,7 @@ impl ScopeManager { key: &str, ) -> crate::Result> { match self.global_scope_cache.try_get::>() { - Some(cached) => Ok(cached.inner().clone()), + Some(cached) => Ok((*cached).clone()), None => { let mut allow = Vec::new(); let mut deny = Vec::new(); @@ -891,7 +891,7 @@ impl ScopeManager { ) -> crate::Result> { let cache = self.command_cache.get(key).unwrap(); match cache.try_get::>() { - Some(cached) => Ok(cached.inner().clone()), + Some(cached) => Ok((*cached).clone()), None => { let resolved_scope = self .command_scope diff --git a/crates/tauri/src/lib.rs b/crates/tauri/src/lib.rs index 44d29258ac3e..8e5bd36419d7 100644 --- a/crates/tauri/src/lib.rs +++ b/crates/tauri/src/lib.rs @@ -193,7 +193,7 @@ use std::{ borrow::Cow, collections::HashMap, fmt::{self, Debug}, - sync::MutexGuard, + sync::{Arc, MutexGuard}, }; use utils::assets::{AssetKey, CspHash, EmbeddedAssets}; @@ -710,12 +710,33 @@ pub trait Manager: sealed::ManagerBase { self.manager().state().set(state) } - /// Removes the state managed by the application for T. Returns the state if it was actually removed. + /// Removes the state managed by the application for T. Returns the state if it was actually removed and no other [Arc] clones exist. + /// + ///
+ /// + /// To fix [tauri-apps/tauri#12721], the internal structure of [State] changed in version `2.3`. + /// So This method is equivalent to [Manager::unmanage_arc] and [Arc::into_inner] now, + /// which means that even if the state is actually removed, this method will return None if there are other Arc clones of the state. + /// Please refer to [[Arc::into_inner]] for details. + /// + /// [tauri-apps/tauri#12721]: https://github.com/tauri-apps/tauri/issues/12721 + /// + ///
+ #[deprecated(since = "2.3.0", note = "use `unmanage_arc` instead")] fn unmanage(&self) -> Option where T: Send + Sync + 'static, { - self.manager().state().unmanage() + let state = self.unmanage_arc::()?; + Arc::into_inner(state) + } + + /// Removes the state managed by the application for T. Returns the state if it was actually removed. + fn unmanage_arc(&self) -> Option> + where + T: Send + Sync + 'static, + { + self.manager().state().unmanage_arc() } /// Retrieves the managed state for the type `T`. @@ -750,20 +771,33 @@ pub trait Manager: sealed::ManagerBase { /// Gets the managed [`Env`]. fn env(&self) -> Env { - self.state::().inner().clone() + (*self.state::()).clone() } /// Gets the scope for the asset protocol. #[cfg(feature = "protocol-asset")] fn asset_protocol_scope(&self) -> scope::fs::Scope { - self.state::().inner().asset_protocol.clone() + self.state::().asset_protocol.clone() } - /// The path resolver. + /// The path resolver.(deprecated) + /// + ///
+ /// + /// This method will cause a memory leak, See [State::inner] for details. + /// + ///
+ #[deprecated(since = "2.3.0", note = "use `Manager::path_arc` instead")] fn path(&self) -> &crate::path::PathResolver { + #[expect(deprecated)] self.state::>().inner() } + /// The path resolver. + fn path_arc(&self) -> Arc> { + self.state::>().into_inner() + } + /// Adds a capability to the app. /// /// Note that by default every capability file in the `src-tauri/capabilities` folder diff --git a/crates/tauri/src/manager/webview.rs b/crates/tauri/src/manager/webview.rs index 8bade86ebde2..4767560f2e53 100644 --- a/crates/tauri/src/manager/webview.rs +++ b/crates/tauri/src/manager/webview.rs @@ -518,7 +518,7 @@ impl WebviewManager { // but we do respect user-specification #[cfg(any(target_os = "linux", target_os = "windows"))] if pending.webview_attributes.data_directory.is_none() { - let local_app_data = manager.path().resolve( + let local_app_data = manager.path_arc().resolve( &app_manager.config.identifier, crate::path::BaseDirectory::LocalData, ); diff --git a/crates/tauri/src/scope/fs.rs b/crates/tauri/src/scope/fs.rs index 31829f224fec..8045b14f6457 100644 --- a/crates/tauri/src/scope/fs.rs +++ b/crates/tauri/src/scope/fs.rs @@ -181,7 +181,7 @@ impl Scope { ) -> crate::Result { let mut allowed_patterns = HashSet::new(); for path in scope.allowed_paths() { - if let Ok(path) = manager.path().parse(path) { + if let Ok(path) = manager.path_arc().parse(path) { push_pattern(&mut allowed_patterns, path, Pattern::new)?; } } @@ -189,7 +189,7 @@ impl Scope { let mut forbidden_patterns = HashSet::new(); if let Some(forbidden_paths) = scope.forbidden_paths() { for path in forbidden_paths { - if let Ok(path) = manager.path().parse(path) { + if let Ok(path) = manager.path_arc().parse(path) { push_pattern(&mut forbidden_patterns, path, Pattern::new)?; } } diff --git a/crates/tauri/src/state.rs b/crates/tauri/src/state.rs index 2f1092e6ce02..b49465dee27a 100644 --- a/crates/tauri/src/state.rs +++ b/crates/tauri/src/state.rs @@ -4,10 +4,11 @@ use std::{ any::{Any, TypeId}, - cell::UnsafeCell, collections::HashMap, hash::BuildHasherDefault, - sync::Mutex, + marker::PhantomData, + ops::Deref, + sync::{Arc, Mutex}, }; use crate::{ @@ -15,33 +16,62 @@ use crate::{ Runtime, }; +type Wrapper = Arc; + /// 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); +pub struct State<'r, T: Send + Sync + 'static>( + Wrapper, + // 👇 TODO: just for 2.x semantic compatibility, we can't remove `'r` for now + PhantomData<&'r ()>, +); 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`. + /// + ///
+ /// + /// To fix [tauri-apps/tauri#12721], the internal structure of [State] changed in version `2.3`. + /// So This method is equivalent to [Self::into_inner] and [Arc::into_raw] now, which means it may cause memory leaks. + /// Please refer to [[Arc::into_raw]] for related limitations. + /// + /// [tauri-apps/tauri#12721]: https://github.com/tauri-apps/tauri/issues/12721 + /// + ///
#[inline(always)] + #[deprecated( + since = "2.3.0", + note = "will cause memory leak, use `into_inner` or `Deref::deref` instead" + )] pub fn inner(&self) -> &'r T { + let ptr = Arc::into_raw(self.0.clone()); + // SAFETY: this ptr is valid, because we just created it; + // and the lifetime is 'static (it's leaked), so it's safe to return it. + unsafe { &*ptr } + } + + /// Retrieve the inner value of the [State]. + /// For `2.0` semantic compatibility, we cannot directly return [Arc] in [crate::Manager::state]. + pub fn into_inner(self) -> Arc { self.0 } } -impl std::ops::Deref for State<'_, T> { +impl Deref for State<'_, T> { type Target = T; #[inline(always)] fn deref(&self) -> &T { - self.0 + Deref::deref(&self.0) } } impl Clone for State<'_, T> { fn clone(&self) -> Self { - State(self.0) + State(Wrapper::clone(&self.0), PhantomData) } } @@ -97,12 +127,13 @@ impl std::hash::Hasher for IdentHash { } } -type TypeIdMap = HashMap, BuildHasherDefault>; +/// The `key` must equal to `value.type_id()`, see the safety doc in methods of [StateManager] for details. +type TypeIdMap = HashMap, BuildHasherDefault>; /// The Tauri state manager. #[derive(Debug)] pub struct StateManager { - map: Mutex>, + map: Mutex, } // SAFETY: data is accessed behind a lock @@ -116,37 +147,32 @@ 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 R, R>(&self, f: F) -> R { - let mut map = self.map.lock().unwrap(); - let map = map.get_mut(); - f(map) - } - pub(crate) fn set(&self, state: T) -> bool { - self.with_map_mut(|map| { - let type_id = TypeId::of::(); - let already_set = map.contains_key(&type_id); - if !already_set { - map.insert(type_id, Box::new(state) as Box); - } - !already_set - }) + let mut map = self.map.lock().unwrap(); + let type_id = TypeId::of::>(); + let already_set = map.contains_key(&type_id); + if !already_set { + map.insert( + type_id, + // SAFETY: keep the type of the key is the same as the type of the value, + // see following methods for details. + Box::new(Wrapper::new(state)) as Box, + ); + } + !already_set } - pub(crate) fn unmanage(&self) -> Option { - self.with_map_mut(|map| { - let type_id = TypeId::of::(); - map - .remove(&type_id) - .and_then(|ptr| ptr.downcast().ok().map(|b| *b)) - }) + pub(crate) fn unmanage_arc(&self) -> Option> { + let mut map = self.map.lock().unwrap(); + let type_id = TypeId::of::>(); + let ptr = map.remove(&type_id)?; + let value = unsafe { + ptr + .downcast::>() + // 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 +184,16 @@ impl StateManager { /// Gets the state associated with the specified type. pub fn try_get(&self) -> Option> { - self.with_map_ref(|map| { - map - .get(&TypeId::of::()) - .and_then(|ptr| ptr.downcast_ref::()) - .map(State) - }) + let map = self.map.lock().unwrap(); + let type_id = TypeId::of::>(); + let ptr = map.get(&type_id)?; + let value = unsafe { + ptr + .downcast_ref::>() + // SAFETY: the type of the key is the same as the type of the value + .unwrap_unchecked() + }; + Some(State(Wrapper::clone(value), PhantomData)) } } @@ -197,8 +227,8 @@ mod tests { let state = StateManager::new(); assert!(state.set(1u32)); assert_eq!(*state.get::(), 1); - assert!(state.unmanage::().is_some()); - assert!(state.unmanage::().is_none()); + assert!(state.unmanage_arc::().is_some()); + assert!(state.unmanage_arc::().is_none()); assert_eq!(state.try_get::(), None); assert!(state.set(2u32)); assert_eq!(*state.get::(), 2); diff --git a/examples/api/src-tauri/tauri-plugin-sample/src/lib.rs b/examples/api/src-tauri/tauri-plugin-sample/src/lib.rs index db923fa129a0..d4e85198385c 100644 --- a/examples/api/src-tauri/tauri-plugin-sample/src/lib.rs +++ b/examples/api/src-tauri/tauri-plugin-sample/src/lib.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: MIT use serde::Deserialize; -use std::path::PathBuf; +use std::{path::PathBuf, sync::Arc}; use tauri::{ plugin::{Builder, TauriPlugin}, Manager, Runtime, @@ -28,12 +28,12 @@ pub use error::*; /// Extensions to [`tauri::App`], [`tauri::AppHandle`] and [`tauri::Window`] to access the sample APIs. pub trait SampleExt { - fn sample(&self) -> &Sample; + fn sample(&self) -> Arc>; } impl> crate::SampleExt for T { - fn sample(&self) -> &Sample { - self.state::>().inner() + fn sample(&self) -> Arc> { + self.state::>().into_inner() } } diff --git a/examples/resources/src-tauri/src/main.rs b/examples/resources/src-tauri/src/main.rs index 42640ce61de1..047876cefe29 100644 --- a/examples/resources/src-tauri/src/main.rs +++ b/examples/resources/src-tauri/src/main.rs @@ -15,7 +15,7 @@ fn main() { tauri::Builder::default() .setup(move |app| { let path = app - .path() + .path_arc() .resolve("assets/index.js", tauri::path::BaseDirectory::Resource) .unwrap();