Skip to content

Conversation

sftse
Copy link
Contributor

@sftse sftse commented Jun 24, 2025

StaticValue has Send and Sync impls with unclear soundness. StaticValue<T> is a wrapper around Mutex<T>, but Mutex<T> itself is only ever Send or Sync when T: Send. I tried to get something to crash but the 'static lifetime on the closures seems quite restrictive.

To remove any doubts about soundness the involved closures would need an additional Send bound, a breaking change.

This PR removes the Send impl on StaticValue since it is never needed. The questionable Sync impl remains.

sftse added 3 commits June 24, 2025 10:58
StaticValue<T> is simply a wrapper around Mutex<T>.
For Mutex<T> to be Send or Sync the stdlib requires T: Send.
This bound is missing on the Send and Sync impl of StaticValue.
The Send impl is not even needed, remove it.
@sftse sftse requested a review from a team as a code owner June 24, 2025 09:44
@sftse sftse changed the title Remove potentially unsound Send bound Remove potentially dangerous Send bound Jun 24, 2025
@sftse sftse changed the title Remove potentially dangerous Send bound Remove potentially dangerous <code>Send</code> bound Jun 24, 2025
@sftse sftse changed the title Remove potentially dangerous <code>Send</code> bound Remove potentially dangerous Send bound Jun 24, 2025
@lucasfernog
Copy link
Member

what's the advantage of removing the Send bound if Sync is still there?

@sftse
Copy link
Contributor Author

sftse commented Aug 16, 2025

Is removing unsafe code not a general positive?

Copy link
Contributor

Package Changes Through 5549ba3

No changes.

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


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

@lucasfernog
Copy link
Member

lucasfernog commented Aug 16, 2025

well in this case i think removing unsafe code would mean making the StaticValue actually safe to share between threads (which is a breaking change like you said - one that Tauri cannot adhere without a breaking change on its own).
(just removing the Send trait doesn't change the code behavior at all)

@sftse
Copy link
Contributor Author

sftse commented Aug 16, 2025

This PR does what it says on the tin, it removes unsafe code. Whether the behavior changes is a bit tangential, every bit of unsafe requires additional time of reviewers auditing their dependencies.

If T had the appropriate Send bound the StaticValue would not be necessary and could be removed. What alerted me to take a closer look is the combination of Deref and the unsafe impl<T> Sync for StaticValue<T> which translates to no-matter-the-T-make-Mutex<T>-Sync.

Can we hold off on merging? The missing bound on the closure seems unsound, but I can't remember what I got stuck on last time I tried to confirm this.

@sftse
Copy link
Contributor Author

sftse commented Sep 15, 2025

This is good to merge. It would probably make sense to update the bounds and do the breaking change by bounding all closures with Send.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants