From 9eb6a5446a4e35f48ad22a5b70a74a8badb9fa0d Mon Sep 17 00:00:00 2001
From: Petros Angelatos <petrosagg@gmail.com>
Date: Thu, 10 Apr 2025 11:09:31 +0300
Subject: [PATCH 1/2] sync::mpsc: add miri reproducer of double free
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
---
library/std/src/sync/mpmc/list.rs | 5 +++
.../miri/tests/pass/issues/issue-139553.rs | 45 +++++++++++++++++++
2 files changed, 50 insertions(+)
create mode 100644 src/tools/miri/tests/pass/issues/issue-139553.rs
diff --git a/library/std/src/sync/mpmc/list.rs b/library/std/src/sync/mpmc/list.rs
index d88914f529142..2dd8f41226d63 100644
--- a/library/std/src/sync/mpmc/list.rs
+++ b/library/std/src/sync/mpmc/list.rs
@@ -213,6 +213,11 @@ impl<T> Channel<T> {
.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 {
diff --git a/src/tools/miri/tests/pass/issues/issue-139553.rs b/src/tools/miri/tests/pass/issues/issue-139553.rs
new file mode 100644
index 0000000000000..97cb9e1a8057c
--- /dev/null
+++ b/src/tools/miri/tests/pass/issues/issue-139553.rs
@@ -0,0 +1,45 @@
+//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-compare-exchange-weak-failure-rate=0
+use std::sync::mpsc::channel;
+use std::thread;
+
+/// This test aims to trigger a race condition that causes a double free in the unbounded channel
+/// implementation. The test relies on a particular thread scheduling to happen as annotated by the
+/// comments below.
+fn main() {
+ let (s1, r) = channel::<u64>();
+ let s2 = s1.clone();
+
+ let t1 = thread::spawn(move || {
+ // 1. The first action executed is an attempt to send the first value in the channel. This
+ // will begin to initialize the channel but will stop at a critical momement as
+ // indicated by the `yield_now()` call in the `start_send` method of the implementation.
+ let _ = s1.send(42);
+ // 4. The sender is re-scheduled and it finishes the initialization of the channel by
+ // setting head.block to the same value as tail.block. It then proceeds to publish its
+ // value but observes that the channel has already disconnected (due to the concurrent
+ // call of `discard_all_messages`) and aborts the send.
+ });
+ std::thread::yield_now();
+
+ // 2. A second sender attempts to send a value while the channel is in a half-initialized
+ // state. Here, half-initialized means that the `tail.block` pointer points to a valid block
+ // but `head.block` is still null. This condition is ensured by the yield of step 1. When
+ // this call returns the channel state has tail.index != head.index, tail.block != NULL, and
+ // head.block = NULL.
+ s2.send(42).unwrap();
+ // 3. This thread continues with dropping the one and only receiver. When all receivers are
+ // gone `discard_all_messages` will attempt to drop all currently sent values and
+ // de-allocate all the blocks. If `tail.block != NULL` but `head.block = NULL` the
+ // implementation waits for the initializing sender to finish by spinning/yielding.
+ drop(r);
+ // 5. This thread is rescheduled and `discard_all_messages` observes the head.block pointer set
+ // by step 4 and proceeds with deallocation. In the problematic version of the code
+ // `head.block` is simply read via an `Acquire` load and not swapped with NULL. After this
+ // 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
+ // 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.
+}
From b9e2ac5c7b1d6bb3b6f5fdfe0819eaf7e95bf7ff Mon Sep 17 00:00:00 2001
From: Petros Angelatos <petrosagg@gmail.com>
Date: Tue, 8 Apr 2025 22:37:25 +0300
Subject: [PATCH 2/2] 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 <petrosagg@gmail.com>
---
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(-)
diff --git a/library/std/src/sync/mpmc/list.rs b/library/std/src/sync/mpmc/list.rs
index 2dd8f41226d63..1c6acb29e375f 100644
--- a/library/std/src/sync/mpmc/list.rs
+++ b/library/std/src/sync/mpmc/list.rs
@@ -569,9 +569,15 @@ impl<T> Channel<T> {
// 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 97cb9e1a8057c..119d589d1eada 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.