From 9eb6a5446a4e35f48ad22a5b70a74a8badb9fa0d Mon Sep 17 00:00:00 2001 From: Petros Angelatos Date: Thu, 10 Apr 2025 11:09:31 +0300 Subject: sync::mpsc: add miri reproducer of double free Signed-off-by: Petros Angelatos --- library/std/src/sync/mpmc/list.rs | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'library') diff --git a/library/std/src/sync/mpmc/list.rs b/library/std/src/sync/mpmc/list.rs index d88914f5291..2dd8f41226d 100644 --- a/library/std/src/sync/mpmc/list.rs +++ b/library/std/src/sync/mpmc/list.rs @@ -213,6 +213,11 @@ impl Channel { .compare_exchange(block, new, Ordering::Release, Ordering::Relaxed) .is_ok() { + // This yield point leaves the channel in a half-initialized state where the + // tail.block pointer is set but the head.block is not. This is used to + // facilitate the test in src/tools/miri/tests/pass/issues/issue-139553.rs + #[cfg(miri)] + crate::thread::yield_now(); self.head.block.store(new, Ordering::Release); block = new; } else { -- cgit 1.4.1-3-g733a5 From b9e2ac5c7b1d6bb3b6f5fdfe0819eaf7e95bf7ff Mon Sep 17 00:00:00 2001 From: Petros Angelatos Date: Tue, 8 Apr 2025 22:37:25 +0300 Subject: sync::mpsc: prevent double free on `Drop` This PR is fixing a regression introduced by #121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR https://github.com/crossbeam-rs/crossbeam/pull/1187 Signed-off-by: Petros Angelatos --- library/std/src/sync/mpmc/list.rs | 8 +++++++- src/tools/miri/tests/pass/issues/issue-139553.rs | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 'library') diff --git a/library/std/src/sync/mpmc/list.rs b/library/std/src/sync/mpmc/list.rs index 2dd8f41226d..1c6acb29e37 100644 --- a/library/std/src/sync/mpmc/list.rs +++ b/library/std/src/sync/mpmc/list.rs @@ -569,9 +569,15 @@ impl Channel { // In that case, just wait until it gets initialized. while block.is_null() { backoff.spin_heavy(); - block = self.head.block.load(Ordering::Acquire); + block = self.head.block.swap(ptr::null_mut(), Ordering::AcqRel); } } + // After this point `head.block` is not modified again and it will be deallocated if it's + // non-null. The `Drop` code of the channel, which runs after this function, also attempts + // to deallocate `head.block` if it's non-null. Therefore this function must maintain the + // invariant that if a deallocation of head.block is attemped then it must also be set to + // NULL. Failing to do so will lead to the Drop code attempting a double free. For this + // reason both reads above do an atomic swap instead of a simple atomic load. unsafe { // Drop all messages between head and tail and deallocate the heap-allocated blocks. diff --git a/src/tools/miri/tests/pass/issues/issue-139553.rs b/src/tools/miri/tests/pass/issues/issue-139553.rs index 97cb9e1a805..119d589d1ea 100644 --- a/src/tools/miri/tests/pass/issues/issue-139553.rs +++ b/src/tools/miri/tests/pass/issues/issue-139553.rs @@ -38,7 +38,7 @@ fn main() { // call returns the channel state has tail.index = head.index, tail.block = NULL, and // head.block != NULL. t1.join().unwrap(); - // 6. The last sender (s1) is dropped here which also attempts to cleanup any data in the + // 6. The last sender (s2) is dropped here which also attempts to cleanup any data in the // channel. It observes `tail.index = head.index` and so it doesn't attempt to cleanup any // messages but it also observes that `head.block != NULL` and attempts to deallocate it. // This is however already deallocated by `discard_all_messages`, leading to a double free. -- cgit 1.4.1-3-g733a5