Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new #70597
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
It would also be nice if start_thread (not the thread_start below) accepted a Box<dyn FnOnce()> instead of a *mut u8.
|
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 |
vakaras
force-pushed
the
thread_new_double_free_bug_fix
branch
from
d65ab72 to
b9f4e85
Compare
If I understand you correctly, the suggestion would be to move the cast to |
Yes. All of the callers cast to |
One thing that makes me hesitant to make this change is the feeling that there was a reason to structure code in this way. However, I agree that having at least |
|
|
vakaras
force-pushed
the
thread_new_double_free_bug_fix
branch
2 times, most recently
from
31047ff to
5cbca95
Compare
OK, I am convinced. I have updated the PR. While looking into this I noticed that I missed that this file is unix specific and that code for other architectures also has the issue with |
|
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 |
vakaras
force-pushed
the
thread_new_double_free_bug_fix
branch
from
5cbca95 to
753bc7d
Compare
|
This code would be much clearer if it used |
vakaras
force-pushed
the
thread_new_double_free_bug_fix
branch
from
bb4bc63 to
5382347
Compare
Thanks for the suggestion! It really looks much cleaner now. |
src/libstd/sys/cloudabi/thread.rs
Outdated
| assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); | ||
|
|
||
| return if ret != 0 { | ||
| // The thread failed to start and as a result p was not consumed. Therefore, it is | ||
| // safe to reconstruct the box so that it gets deallocated. | ||
| let _ = Box::from_raw(p); |
There was a problem hiding this comment.
| let _ = Box::from_raw(p); | |
| drop(Box::from_raw(p)); |
IMO that is more clear (same for the other places).
bors
added
S-waiting-on-bors
…fix, r=Amanieu,RalfJung Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new While working on concurrency support for Miri, I found that the `libstd::syn::unix::Thread::new` method has two potential problems: double-free and undefined behaviour. **Double-free** could occur if the following events happened (credit for pointing this out goes to @RalfJung): 1. The call to `pthread_create` successfully launched a new thread that executed to completion and deallocated `p`. 2. The call to `pthread_attr_destroy` returned a non-zero value causing the `assert_eq!` to panic. 3. Since `mem::forget(p)` was not yet executed, the destructor of `p` would be executed and cause a double-free. As far as I understand, this code also violates the stacked-borrows aliasing rules and thus would result in **undefined behaviour** if these rules were adopted. The problem is that the ownership of `p` is passed to the newly created thread before the call to `mem::forget`. Since the call to `mem::forget` is still a call, it counts as a use of `p` and triggers UB. This pull request changes the code to use `mem::ManuallyDrop` instead of `mem::forget`. As a consequence, in case of a panic, `p` would be potentially leaked, which while undesirable is probably better than double-free or undefined behaviour.
|
Failed in #70711 (comment), @bors r- |
bors
added
S-waiting-on-author
|
Does anyone know if there is a way for me to run |
|
You can edit src/ci/github-actions/ci.yml to add more targets to the |
Thank you for the tip! I tried another trick: just create empty bash scripts for the missing tools about which I have checked at least one target for each stack overflow handler. Hopefully, this time it goes through. |
|
@bors r=Amanieu,RalfJung |
|
📌 Commit 53aa5a1 has been approved by |
bors
added
S-waiting-on-bors
|
|
They are still used, but they not need to be used... Sorry for not realizing that. |
|
@RalfJung I have deleted the handlers for |
|
Hm, yes that looks reasonable but I'll wait for @Amanieu to confirm. |
Makes sense. By the way, if we are waiting for Amanieu, wouldn't it be better to |
|
@bors r+ |
|
📌 Commit d512b22 has been approved by |
The PR got dequeued automatically the moment you pushed again. ;) |
I see. Thanks for the explanation! |
Rollup of 9 pull requests Successful merges: - rust-lang#69860 (Use associated numeric consts in documentation) - rust-lang#70576 (Update the description of the ticket to point at RFC 1721) - rust-lang#70597 (Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new) - rust-lang#70640 (Hide `task_context` when lowering body) - rust-lang#70641 (Remove duplicated code in trait selection) - rust-lang#70707 (Remove unused graphviz emitter) - rust-lang#70720 (Place TLS initializers with relocations in .tdata) - rust-lang#70735 (Clean up E0502 explanation) - rust-lang#70741 (Add test for rust-lang#59023) Failed merges: r? @ghost
While working on concurrency support for Miri, I found that the
libstd::syn::unix::Thread::newmethod has two potential problems: double-free and undefined behaviour.Double-free could occur if the following events happened (credit for pointing this out goes to @RalfJung):
pthread_createsuccessfully launched a new thread that executed to completion and deallocatedp.pthread_attr_destroyreturned a non-zero value causing theassert_eq!to panic.mem::forget(p)was not yet executed, the destructor ofpwould be executed and cause a double-free.As far as I understand, this code also violates the stacked-borrows aliasing rules and thus would result in undefined behaviour if these rules were adopted. The problem is that the ownership of
pis passed to the newly created thread before the call tomem::forget. Since the call tomem::forgetis still a call, it counts as a use ofpand triggers UB.This pull request changes the code to use
mem::ManuallyDropinstead ofmem::forget. As a consequence, in case of a panic,pwould be potentially leaked, which while undesirable is probably better than double-free or undefined behaviour.