]> git.lizzy.rs Git - rust.git/commitdiff
Rollup merge of #70597 - vakaras:thread_new_double_free_bug_fix, r=Amanieu
authorMazdak Farrokhzad <twingoow@gmail.com>
Fri, 3 Apr 2020 20:55:05 +0000 (22:55 +0200)
committerGitHub <noreply@github.com>
Fri, 3 Apr 2020 20:55:05 +0000 (22:55 +0200)
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.


Trivial merge