fix(tauri): fix use-after-free unsoundness for state manager
#12745
Conversation
|
ping @amrbashir |
| #[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 } | ||
| } |
There was a problem hiding this comment.
a simple fix for this would be
pub fn inner(&'r self) -> &'r T {
self.0.as_ref()
}In normal cases, this should be a breaking change but since this fixes a memory leak, it should be fine
There was a problem hiding this comment.
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).
So, does tauri really want to introduce a BREAKING CHANGE in 2.x for this? Moreover, this BREAKING CHANGE would make pub fn inner(&'r self) -> &'r T quite useless (almost equivalent to removing it).
There was a problem hiding this comment.
I assume you mean app.state().inner() use-case won't work? in that case I guess your solution is correct, and deref should be used instead.
There was a problem hiding this comment.
Here is my use case for fn inner: https://github.com/WSH032/pytauri/blob/47fe27d6f67c6d490ce753ac7976eb4b8a43cbdd/crates/tauri-plugin-pytauri/src/lib.rs#L103-L109.
With fn inner<'a>(&'a self) -> &'r T, I can return a reference of another type without needing to continue holding State.
If we really want to proceed as mentioned above, we might need to provide a method similar to MutexGuard::map to help convert State<'_, T> to State<'_, U>.
There was a problem hiding this comment.
at this point, I would go with #12723 for now, but instead of completely removing, we just document its pitfall and maybe print a warning in debug builds.
then in v3, we rework the API and introduce .map and remove inner in favor of deref
| }; | ||
|
|
||
| use crate::{ | ||
| ipc::{CommandArg, CommandItem, InvokeError}, | ||
| Runtime, | ||
| }; | ||
|
|
||
| type Wrapper<T> = Arc<T>; |
There was a problem hiding this comment.
This just makes it confusing, let's remove and use Arc directly
| 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<dyn Any + Sync + Send>, |
There was a problem hiding this comment.
we could remove Box and use Arc directly, should avoid a level of indirection
close #12721
another impl to #12723