rust: Add Send bound to SecureSessionTransport trait #898
+30
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parent struct SecureSession expects to implement Send at all times. However, it would be unsound if SecureSessionTransport does not implement Send. This is a blunder in the original implementation. Well, let's hope that all reasonable implementations are already Send, and just require SecureSessionTransport to be Send. I'd rather make 0.15.1 later with some ugly generic fix if some users show up with a genuine use-case for !Send transport callbacks. That said, SecureSession could be made to implement Send based on the type of SecureSessionTransport it uses. However, I don't want to get generics into the mix unless I have to, since that might turn out to be another breaking change.
Because the first "unsafe impl" went so well, let's add another one. This makes sure that the first "unsafe impl" is well-founded: that is, all fields are actually Send -- compiler does not check that, trusting our "unsafe" but recent Clippy does, and it complained about it. With "Box<dyn SecureSessionTransport>" being Send now, the only remaining thing that is !Send is secure_session_user_callbacks_t, because of that "void* user_context" pointer in it. The pointer points to SecureSessionContext struct itself, which make it not only unsafe to send across threads, it's unsafe to move it at all. That's why we put it in a Box which stays in the same place on heap until SecureSession is dropped. I believe this should be safe now.
ilammy
added
compatibility
|
Ha-ha, "all users", said I. Our own integration tests are those affected users, it seems. |
vixentael
approved these changes
Jan 19, 2022
|
Should we update Thread Safety guidelines too? |
Make sure that newly added Send bound it satisfied. The closures we use are safe to Send across thread boundaries, but the utilities did not preserve this bound as they themselves use type erasure.
I think it will be okay as is, no need to add any extra warnings there. Contrary to C++, if a Rust application tries to use |
Rust rules! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New Clippy 1.58 has found an unsoundness in RustThemis:
unsafe implforSecureSessionmake itSend– safe to pass over to a different thread – but in fact it's not safe to do that unlessSecureSessionTransportimplementation used bySecureSessionis alsoSend, which was expected but never required. Well, now it is required.This would break application code that uses
SecureSessionTransportimplementations that are!Send(e.g., usingRcinside, or raw pointers). This is not safe ifSecureSessionis moved to another thread, but it should be okay if it stays on the same thread as its transport.I believe that most applications will not be affected by this change. If someone is really out there using
!Sendtransports, they'd come complaining, then I'd decide what to do: either tell them “you're doing it wrong”, or make a patch release that make it possible toSecureSessionto be!Sendas well.For now, my goal is to fix a warning from Clippy on CI.
Checklist