Skip to content

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Jul 17, 2025

When a struct's Drop::drop impl panics, Rust will still drop the fields of the struct before continuing to unwind1.

glib::thread_guard::ThreadGuard<T> currently holds a value: T, so if a ThreadGuard is dropped on the wrong thread, its drop impl will panic, and then the drop glue will attempt to drop the value: T field before continuing to unwind. This violates the purpose of ThreadGuard as it allows accessing the T on a thread other than the one it was created on (in T's Drop impl).

This PR wraps the value field in ManuallyDrop so it will not be automatically dropped, and drops it manually in ThreadGuard's Drop::drop impl if it is on the correct thread.

Examples showing issue
use glib::thread_guard::{ThreadGuard, thread_id};

pub struct PrintOnDrop(usize);

impl PrintOnDrop {
    pub fn new() -> Self {
        Self(thread_id())
    }
}

impl Drop for PrintOnDrop {
    fn drop(&mut self) {
        println!(
            "dropping value on thread {} that was produced on thread {}",
            thread_id(),
            self.0
        );
    }
}

fn main() {
    let _: ThreadGuard<PrintOnDrop> = std::thread::spawn(|| ThreadGuard::new(PrintOnDrop::new()))
        .join()
        .unwrap();
}

The above program prints ThreadGuard's panic message, and then dropping value on thread 1 that was produced on thread 0, showing that the PrintOnDrop is dropped on a different thread than the one it was created on, despite being in a ThreadGuard.

With this PR, the above program no longer prints the message from PrintOnDrop::drop.

//! Run with `MIRIFLAGS="-Zmiri-many-seeds -Zmiri-ignore-leaks" cargo +nightly miri run`

use std::rc::Rc;

use glib::thread_guard::ThreadGuard;

fn main() {
    let (tx, rx) = std::sync::mpsc::channel();

    std::thread::spawn(move || {
        let rc1 = Rc::new(42);
        tx.send(ThreadGuard::new(rc1.clone())).unwrap();
        drop(rc1);
    });

    let rc2 = rx.recv().unwrap();
    assert!(!rc2.is_owner());

    // UB is here, when `rc2.value` is dropped and races with `rc1` being dropped in the other thread.
}

The above program has UB due to racing non-atomic accesses to Rc's internal reference counts. (It is somewhat nondeterministic whether Miri will catch the racing, since racing is timing sensitive. Using -Zmiri-many-seeds runs it several times with different Miri randomization seeds to test different thread interleavings).

With this PR, Miri does not detect any UB in the above program (on my machine with -Zmiri-many-seeds and rustc 1.90.0-nightly (3014e79f9 2025-07-15)).


Something I noticed that is somewhat unrelated: ThreadGuard::into_inner, if called on the wrong thread, will panic twice and thus abort: first in the assert! in into_inner, and second in the assert! in drop when dropping self due to unwinding from the first panic.

This is not currently changed by this PR, but could be changed by moving the let mut this = ManuallyDrop::new(self); above the assert! in into_inner if that is desired.

Footnotes

  1. currently at least, assuming panic=unwind and not abort, and the thread isn't already panicking, etc

@sdroege
Copy link
Member

sdroege commented Jul 17, 2025

This is not currently changed by this PR, but could be changed by moving the let mut this = ManuallyDrop::new(self); above the assert! in into_inner if that is desired.

I think that would be a good idea

sdroege
sdroege previously approved these changes Jul 17, 2025
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Do you want to include the other change here too?

@zachs18 zachs18 force-pushed the thread-guard-no-drop-value-on-other-thread branch from f079d06 to 5829535 Compare July 17, 2025 18:15
@zachs18
Copy link
Contributor Author

zachs18 commented Jul 17, 2025

Latest push has the into_inner change.

@sdroege sdroege added the needs-backport PR needs backporting to the current stable branch label Jul 18, 2025
@sdroege sdroege merged commit 94c3909 into gtk-rs:main Jul 18, 2025
48 checks passed
@sdroege
Copy link
Member

sdroege commented Jul 18, 2025

Thanks!

@sdroege sdroege added backported PR was backported to the current stable branch and removed needs-backport PR needs backporting to the current stable branch labels Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported PR was backported to the current stable branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants