clarify partially initialized Mutex issues #53108
Merged
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
|
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Someone should also check if I used the right conditions for UB of |
|
Woah |
|
Okay I think I handled both cases: |
|
cc @rust-lang/libs: Can you please assign someone familiar with the Mutexs in |
alexcrichton
approved these changes
Aug 7, 2018
src/libstd/io/lazy.rs
Outdated
| /// Safety: `init` must not call `get` on the variable that is being | ||
| /// initialized. | ||
| pub const unsafe fn new(init: fn() -> Arc<T>) -> Lazy<T> { | ||
| // `lock` is never initialized fully, so it is UB to attempt to |
There was a problem hiding this comment.
This threw me for a spin when I first read it, could it perhaps be reworded? In the Rust sense we fully initialize the lock (aka put valid bytes in place) but what this comment is alluding to is the POSIX-specific behavior tweak in the mutex implementation where Mutex::new isn't as safe as an init'd mutex. Could this perhaps be reworded as:
lockis not valid to use in a reentrant fashion, causing UB if that happens;
for more information see comments in
src/libstd/sys/unix/args.rs
Outdated
| @@ -80,6 +80,8 @@ mod imp { | |||
|
|
|||
| static mut ARGC: isize = 0; | |||
| static mut ARGV: *const *const u8 = ptr::null(); | |||
| // `ENV_LOCK` is never initialized fully, so it is UB to attempt to | |||
There was a problem hiding this comment.
Same comment as bove to the wording "initialized fully" here
src/libstd/sys/unix/mutex.rs
Outdated
| // Might be moved to a different address, so it is better to avoid | ||
| // initialization of potentially opaque OS data before it landed. | ||
| // Be very careful using this newly constructed `Mutex`, it should | ||
| // be initialized by calling `init()` first! |
There was a problem hiding this comment.
Could this be tweaked as well to not use the word "initialized" here? I think it makes sense but it just threw me personally for a spin, I had to go read all the comments in init to understand this.
src/libstd/sys/unix/os.rs
Outdated
| @@ -33,6 +33,8 @@ use sys::fd; | |||
| use vec; | |||
|
|
|||
| const TMPBUF_SZ: usize = 128; | |||
| // `ENV_LOCK` is never initialized fully, so it is UB to attempt to | |||
There was a problem hiding this comment.
Same comment as bove to the wording "initialized fully" here
src/libstd/sys_common/at_exit_imp.rs
Outdated
| @@ -23,6 +23,8 @@ type Queue = Vec<Box<dyn FnBox()>>; | |||
| // on poisoning and this module needs to operate at a lower level than requiring | |||
| // the thread infrastructure to be in place (useful on the borders of | |||
| // initialization/destruction). | |||
| // `LOCK` is never initialized fully, so it is UB to attempt to | |||
There was a problem hiding this comment.
Same comment as bove to the wording "initialized fully" here
src/libstd/sys_common/mutex.rs
Outdated
| pub const fn new() -> Mutex { Mutex(imp::Mutex::new()) } | ||
|
|
||
| /// Prepare the mutex for use. | ||
| /// | ||
| /// This should be called once the mutex is at a stable memory address. | ||
| /// Behavior is undefined unless this is called before any other operation. |
There was a problem hiding this comment.
Could this be clarified to say something like "If this function is called, behavior is undefined ..." to be clear that it's not undefined behavior to not call this method, only to call the method after you've already done other operations.
| @@ -161,6 +161,8 @@ impl StaticKey { | |||
| // Additionally a 0-index of a tls key hasn't been seen on windows, so | |||
| // we just simplify the whole branch. | |||
| if imp::requires_synchronized_create() { | |||
| // `INIT_LOCK` is never initialized fully, so it is UB to attempt to | |||
There was a problem hiding this comment.
Same comment as bove to the wording "initialized fully" here
src/libstd/thread/mod.rs
Outdated
| @@ -940,6 +940,8 @@ pub struct ThreadId(u64); | |||
| impl ThreadId { | |||
| // Generate a new unique thread ID. | |||
| fn new() -> ThreadId { | |||
| // `GUARD` is never initialized fully, so it is UB to attempt to | |||
There was a problem hiding this comment.
Same comment as bove to the wording "initialized fully" here
…apable state of the mutex
|
Done. I hope I got them all. |
|
@bors: r+ Thanks! |
|
📌 Commit 25db842 has been approved by |
bors
added
S-waiting-on-bors
|
@bors: delegate=RalfJung |
|
✌️ @RalfJung can now approve this pull request |
|
⌛ Testing commit 25db842 with merge 43565febd4a71e979b5ebb49b9e7d08b135b499c... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors |
|
⌛ Trying commit 25db842 with merge 06a7c82b17d56f280d19d4a5d78ede395a7463ed... |
|
Dang, typo... @bors retry |
bors
added a commit
that referenced
this pull request
Aug 9, 2018
clarify partially initialized Mutex issues Using a `sys_common::mutex::Mutex` without calling `init` is dangerous, and yet there are some places that do this. I tried to find all of them and add an appropriate comment about reentrancy. I found two places where (I think) reentrancy can actually occur, and was not able to come up with an argument for why this is okay. Someone who knows `io::lazy` and/or `sys_common::at_exit_imp` should have a careful look at this.
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r=alexcrichton |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 25db842 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 9, 2018
clarify partially initialized Mutex issues Using a `sys_common::mutex::Mutex` without calling `init` is dangerous, and yet there are some places that do this. I tried to find all of them and add an appropriate comment about reentrancy. I found two places where (I think) reentrancy can actually occur, and was not able to come up with an argument for why this is okay. Someone who knows `io::lazy` and/or `sys_common::at_exit_imp` should have a careful look at this.
|
☀️ Test successful - status-appveyor, status-travis |
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.
Using a
sys_common::mutex::Mutexwithout callinginitis dangerous, and yet there are some places that do this. I tried to find all of them and add an appropriate comment about reentrancy.I found two places where (I think) reentrancy can actually occur, and was not able to come up with an argument for why this is okay. Someone who knows
io::lazyand/orsys_common::at_exit_impshould have a careful look at this.