fix(tauri): deprecate Manager::unmanage to fix use-after-free

[WSH032]

close #12721

As a reference, here is another implementation that retains Manager::unmanage.
For semantic compatibility, I don't think it's ideal in terms of performance and ergonomics.

I prefer the solution of deprecating Manager::unmanage, as the need to remove state seems to be a rare requirement (I personally can't think of a use case, and you can always achieve it with Mutex and Option).

[github-actions]

Package Changes Through c7d380e

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

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.2.0	2.3.0
tauri-utils	2.1.1	2.2.0
tauri-bundler	2.2.3	2.2.4
tauri-runtime	2.3.0	2.4.0
tauri-runtime-wry	2.3.0	2.4.0
tauri-codegen	2.0.4	2.0.5
tauri-macros	2.0.4	2.0.5
tauri-plugin	2.0.4	2.0.5
tauri-build	2.0.5	2.0.6
tauri	2.2.5	2.3.0
@tauri-apps/cli	2.2.7	2.3.0
tauri-cli	2.2.7	2.3.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

[FabianLars]

I totally get the sentiment here but completely removing it like this is a bit awkward considering that this api was added on user request. I've never dealt with such issues though so idk. would love to hear some more opinions here.

p.s. to be clear, i'm 100% with you on the deprecation itself, just very unsure about making it noop

[WSH032]

just very unsure about making it noop

maybe add a new unsafe fn unsafe_unmanage ? and let unmanage as alias of unsafe_unmanage ?

[amrbashir]

how about using an Arc internally instead of a box?

[WSH032]

how about using an Arc internally instead of a box?

Do you mean replacing Box with Arc based on this PR, or providing another implementation using Arc to mitigate the use-after-free issue?

[amrbashir]

same implementation as now, but instead of using a Box, we use Arc and on each .get() we clone the arc and increase its strong counter, this means even if the user unmanage a state, they would still have valid values until all Arcs are dropped.

[WSH032]

Then the implementation of State::inner would be broken. We can choose only one between unmanager and State::inner.

ref impl

[amrbashir]

I left a few comments on that implementation but it would be better to just add it here so we can discuss it better

[WSH032]

follow #12745 (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.

@amrbashir Do you mean to keep the original unsafe behavior of unmanage? And what about #[deprecated]?

[amrbashir]

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.

@amrbashir Do you mean to keep the original unsafe behavior of unmanage? And what about #[deprecated]?

yeah keep the original unsafe behavior for now, keep #[deprecated] that explains why this is unsafe, and note that it maybe removed in the future.

[WSH032]

ping @amrbashir

[WSH032]

rust 1.85 seems add more clippy lint ruels

[sftse]

Soundness is excluded from the stability guarantees of Rust, as was the breaking change in the stdlib of making fn set_var unsafe because there was no other way. Misstating soundness is not optional.

[FabianLars]

Soundness is excluded from the stability guarantees of Rust

got a source for that? i do remember something like that but can't find anything rn.

as was the breaking change in the stdlib of making fn set_var unsafe because there was no other way

except that this change is "opt-in" by being tied to edition 2024 (meaning in edition 2021 it's not an unsafe fn).

Misstating soundness is not optional.

I don't understand what that means, including the phrasing before your edit.

[sftse]

This isn't tied to edition, but to stdlib version. Upgrading the version will result in breakage, it was just that the edition boundary was as good as any for such an exceptional change. Retaining the 2021 edition in the Cargo.toml will not prevent breakage.

I decided to phrase it more diplomatically, but a function that in its public api is declared as safe is misstating its soundness if it can be used to obtain UB.

[FabianLars]

This isn't tied to edition, but to stdlib version. Upgrading the version will result in breakage, it was just that the edition boundary was as good as any for such an exceptional change. Retaining the 2021 edition in the Cargo.toml will not prevent breakage.

assuming we're still talking about just set_var then that doesn't seem to be true. I use 1.86 in a edition 2021 project and set_var is not an unsafe fn. If i change the edition it starts complaining. Literally just tested it. Apologies if i misunderstood the stdlib version part.

I decided to phrase it more diplomatically, but a function that in its public api is declared as safe is misstating its soundness if it can be used to obtain UB.

Ah got it. Yeah, that's fair and i'd agree

[sftse]

Excuse my mistake, I never checked and was going by the documentation that states it is unsafe regardless of version. That is a bit surprising, thanks for surfacing that.

The current API seems like it could remain safe, I'm working on a PR.