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

Make sure Mutex and RwLock can't be re-locked on the same thread #33861

Merged
merged 5 commits into from
Jun 3, 2016

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented May 25, 2016

@Amanieu
Copy link
Member Author

Amanieu commented May 25, 2016

I don't have any tests for this because I'm not sure how I can write a test to ensure that an operation successfully deadlocks...

pub struct RWLock { inner: UnsafeCell<libc::pthread_rwlock_t> }
pub struct RWLock {
inner: UnsafeCell<libc::pthread_rwlock_t>,
write_locked: Cell<bool>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically accessed concurrently so could this switch to being an UnsafeCell as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't Cell also allow concurrent access? Neither of them have the Sync marker, but that doesn't matter since that marker added to RWLock 4 lines later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantically the usage here is fine, but Cell is not Sync and it's technically used in an unsafe fashion here. It's not normally safe to access it on many threads, but the readers will read it concurrently here.

But yeah this would just to be a bit "more correct", it wouldn't actually change anything about the implementation.

@alexcrichton
Copy link
Member

Thanks @Amanieu! My digging around indicates, though, that this only may fix the problem for rwlocks. The standard no longer explicitly says that relocking is undefined behavior, but it doesn't say what the behavior is beyond that it might deadlock. In that sense it could be possible that this is still undefined, in which case we may need to find another solution.

In any case though this seems like the best solution for now as it should be guaranteed to fix mutexes and it likely fixes rwlocks on all implementations.

@@ -30,6 +30,20 @@ impl Mutex {
Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
}
#[inline]
pub unsafe fn init(&mut self) {
// We need to set the mutex type to PTHREAD_MUTEX_NORMAL to avoid
// undefined behavior when re-locking in the same thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this expand a bit and link to the lock elision issue and perhaps a link to the standard as well? Something along the lines of the default being DEFAULT which has recursive locking as undefined behavior, but we need to guarantee somehow that a recursive lock never returns (either deadlock or panic)

@alexcrichton
Copy link
Member

This actually seems subtle enough that we should probably add some tests. The semantics here aren't actually really testing for deadlock more so that the second lock never returns. Perhaps a test could be added like:

let lock = ..;
let _l1 = lock.lock();
let _l2 = lock.lock();
println!("oops!");

The test can then exec itself, capturing output. If after something like a second has passed and the process hasn't exited we can also send it a kill signal. Then the test could just be that the output doesn't contain "oops!".

Not exactly a foolproof test, but we should run it dozens of times a day on bors, so if it's ever spurious we'll pick up on that and it'll be pretty obvious what the fix is :)

@Amanieu Amanieu force-pushed the lock_elision_fix branch 3 times, most recently from efc8732 to b003f2e Compare May 26, 2016 05:23
@Amanieu
Copy link
Member Author

Amanieu commented May 26, 2016

OK this should handle all of the cases, but the code is now quite ugly :/

@alexcrichton
Copy link
Member

@bors: r+ 8999243b4c57807d599630fc78ad8268701c02eb

Thanks @Amanieu!

bors added a commit that referenced this pull request May 28, 2016
Rollup of 15 pull requests

- Successful merges: #33820, #33821, #33822, #33824, #33825, #33831, #33832, #33848, #33849, #33852, #33854, #33856, #33859, #33860, #33861
- Failed merges:
@bors
Copy link
Contributor

bors commented May 29, 2016

⌛ Testing commit 8999243 with merge 85c9847...

@bors
Copy link
Contributor

bors commented May 29, 2016

💔 Test failed - auto-linux-64-opt-rustbuild

@eddyb
Copy link
Member

eddyb commented May 29, 2016

@bors retry

@bors
Copy link
Contributor

bors commented May 30, 2016

⌛ Testing commit 8999243 with merge 1312a32...

@bors
Copy link
Contributor

bors commented May 30, 2016

💔 Test failed - auto-linux-cross-opt

@Amanieu
Copy link
Member Author

Amanieu commented May 31, 2016

Updated libc, should work now

@alexcrichton
Copy link
Member

@bors: r+ 5708cf7b691b752798098ce1bf3117a682f76668

@bors
Copy link
Contributor

bors commented May 31, 2016

⌛ Testing commit 5708cf7 with merge be4ac33...

@bors
Copy link
Contributor

bors commented May 31, 2016

💔 Test failed - auto-linux-64-cross-armhf

@alexcrichton
Copy link
Member

@bors: reetry

On Tue, May 31, 2016 at 3:37 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-armhf
http://buildbot.rust-lang.org/builders/auto-linux-64-cross-armhf/builds/466


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33861 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95CYrjNFJEWjKtXzBu2xx6ZGfg-R5ks5qHLhBgaJpZM4ImKt7
.

@alexcrichton
Copy link
Member

@bors: retry

On Tue, May 31, 2016 at 4:14 PM, Alex Crichton alex@alexcrichton.com
wrote:

@bors: reetry

On Tue, May 31, 2016 at 3:37 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-armhf
http://buildbot.rust-lang.org/builders/auto-linux-64-cross-armhf/builds/466


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33861 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD95CYrjNFJEWjKtXzBu2xx6ZGfg-R5ks5qHLhBgaJpZM4ImKt7
.

@Manishearth
Copy link
Member

Travis fails

@eddyb
Copy link
Member

eddyb commented Jun 1, 2016

@Manishearth Travis is busted again, apt-get can't get llvm, AFAICT.

@bors
Copy link
Contributor

bors commented Jun 2, 2016

⌛ Testing commit 5708cf7 with merge 03dd3a9...

@bors
Copy link
Contributor

bors commented Jun 2, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

The only applies to pthread mutexes. We solve this by creating the
mutex with the PTHREAD_MUTEX_NORMAL type, which guarantees that
re-locking from the same thread will deadlock.
This is allowed by POSIX and can happen on glibc with processors
that support hardware lock elision.
@Amanieu
Copy link
Member Author

Amanieu commented Jun 2, 2016

Fixed the test on windows

@sfackler
Copy link
Member

sfackler commented Jun 3, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jun 3, 2016

📌 Commit fc4b356 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 3, 2016

⌛ Testing commit fc4b356 with merge 9552bcd...

bors added a commit that referenced this pull request Jun 3, 2016
Make sure Mutex and RwLock can't be re-locked on the same thread

Fixes #33770

r? @alexcrichton
@bors bors merged commit fc4b356 into rust-lang:master Jun 3, 2016
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

Successfully merging this pull request may close these issues.

6 participants