Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsound on systems which use pthread mutex? #4

Open
j-baker opened this issue Dec 25, 2023 · 4 comments
Open

Unsound on systems which use pthread mutex? #4

j-baker opened this issue Dec 25, 2023 · 4 comments

Comments

@j-baker
Copy link

j-baker commented Dec 25, 2023

Most unices use futexes, but some use pthread mutexes. For pthread mutexes, it is undefined if unlock happens from a different thread than which the lock occurred. https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_lock.html. This implies that lock guards cannot be Send on such platforms, so while this code is safe Rust, it may not safely call the OS.

@jonhoo
Copy link
Owner

jonhoo commented Jan 3, 2024

Ooof, yes, you're entirely right. Now, I think most Rust platforms now use futexes thanks to the excellent work of Mara Bos (@ m-ou-se, but don't want to ping unnecessarily). Looking through the list, the following targets should be fine as they're using futexes:

  • Linux
  • emscripten
  • wasm / wasm32
  • {Free, Open, DragonFly}BSD
  • Fucshia

Windows doesn't use futexes, but uses SRW locks which are Send.

The big problem is macOS which as far as I can tell is pthread-only.

It's also not clear what the story on Android is around this.

Not sure what the best path forward here is. I can't immediately think of a clever trick to get "something that works" on macOS. We may just have to declare that this library does not work on macOS 👀

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "android"))]
compile_error!("This crate would be unsound on targets with pthread-based locks (macOS, iOS, and Android).");

@j-baker
Copy link
Author

j-baker commented Jan 3, 2024

I think the good news for you is that on MacOS it's defined to explode. https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/pthread_mutex_unlock.3.html#//apple_ref/doc/man/3/pthread_mutex_unlock

This isn't the case on Solaris (where it's defined to return status 0 and have undefined further behaviour).

I think that one safe approach would be to somehow implement !Send on the guardian objects on non-acknowledged-futex-supporting-Rusts. Certainly the one time I used Guardian, there was no threading at play once the lock had been acquired (ymmv!) - rather I was using it to get rid of the borrow of the lock itself, to allow passing the lock up the stack without pollution [0]. And obviously it's fine with Rc (though Rc doesn't make a tonne of sense).
The danger is that it makes it very easy for folks to write code which doesn't then work on MacOS because there are stricter rules on that platform. That's better than your alternative, though!

The restriction may be reasonable on all platforms. Apparently only a few folks use this crate anyway.

You will know better than me a good way to do it (https://docs.rs/negative-impl/latest/negative_impl/?).

[0] - I was implementing a sql connection pool double for tests which handed out a single connection.

@j-baker
Copy link
Author

j-baker commented Jan 3, 2024

one last comment - I checked for Mutex, but obviously there is RwLock too. Not sure how that is implemented across the platforms.

@jonhoo
Copy link
Owner

jonhoo commented Jan 4, 2024

Actually, hang on — the guards already aren't Send (on any platform)! See https://docs.rs/guardian/latest/guardian/struct.ArcMutexGuardian.html#impl-Send-for-ArcMutexGuardian%3CT%3E for example.

So really the observation here is that it could be Send on certain platforms. But then I'm inclined to just leave them as non-Send on any platform 🤔

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

No branches or pull requests

2 participants