Data reachable from thread-local storage of the main thread should not be considered leaked

[lukechu10]

Miri does not detect any leak for the following code since the reference is stored in a static variable and is therefore accessible for the whole duration of the program.

use std::sync::Mutex;

static REF: Mutex<Option<&'static i32>> = Mutex::new(None);

pub fn main() {
    let a = 123;
    let b = Box::new(a);
    let r = Box::leak(b);
    println!("{r}");
    *REF.lock().unwrap() = Some(r);
}

However, replacing the static with a thread_local!, Miri complains about a memory leak:

use std::cell::Cell;

// static REF: Mutex<Option<&'static i32>> = Mutex::new(None);

pub fn main() {
    let a = 123;
    let b = Box::new(a);
    let r = Box::leak(b);
    println!("{r}");

    thread_local! {
        static REF: Cell<Option<&'static i32>> = Cell::new(None);
    }

    REF.with(|cell| {
        cell.set(Some(r));
    })
}

[RalfJung]

Yes, that is by design. Threads come and go and so having a reference pointed-to by a thread local after the thread disappeared and all its thread-local storage got cleaned up is a leak.

[saethlin]

In Rust, static items require a const initializer and thus are not initialized at runtime, and they also are never dropped. This avoids the static initialization fiasco that C++ suffers from.

And yeah as Ralf points out thread-locals aren't leaked. So I'm not sure, which side of this comparison do you think is confusing?

[lukechu10]

Ah ok that makes sense. However, Valgrind doesn't detect a leak in the thread_local case. Is there a reason for this disparity between Valgrind and miri?

[RalfJung]

I have no idea how valgrind decides what to make a leak or not. 🤷

Does it consider this a leak? IMO it clearly should:

use std::cell::Cell;

pub fn main() {
    thread_local! {
        static REF: Cell<Option<&'static i32>> = Cell::new(None);
    }

    std::thread::spawn(|| REF.with(|cell| {
        let a = 123;
        let b = Box::new(a);
        let r = Box::leak(b);
        cell.set(Some(r));
    })).join().unwrap();
    
    // Imagine the program running for a long time while the thread is gone
    // and this memory still sits around, unused -- leaked.
}

[lukechu10]

Does it consider this a leak? IMO it clearly should:

I just tried it and yes it is considered a leak by both Valgrind and Miri.

Also in my original example, Valgrind marks the memory as "still reachable".

[RalfJung]

Okay so looks like valgrind differentiates main thread TLS leaks and other TLS leaks. I guess this makes sense since main thread TLS will always stick around until the end of the program. We could in principle do the same in Miri, I think.

[RalfJung]

This should actually be fairly easy to do: before running the leak check (around here), mark the main thread (thread ID 0) data of all TlsEntry (these things are in tls.rs) as 'static roots' by adding them to this vector.

[max-heller]

This should actually be fairly easy to do: before running the leak check (around here), mark the main thread (thread ID 0) data of all TlsEntry (these things are in tls.rs) as 'static roots' by adding them to this vector.

I tried this approach and am running into some issues:

• Thread locals are not yet initialized before and are destroyed after here, so we can't get their AllocIds right before the leakage check. I worked around this by hooking into TlsData::store_tls() to store AllocIds in static_roots when TLS is stored.

• After adding the AllocIds to static_roots, the example that should work still fails. I'm guessing this is because information about the contents of the allocation has been deleted by the time the leakage check runs, but I'm not sure how to work around this.

Any ideas? My attempt is here

[saethlin]

Thread locals are not yet initialized before and are destroyed after

Maybe you could try sticking something deeper into the scheduler code? What you probably want to do is hook into the transition from running the main thread to TlsDtors.

the example that should work still fails

Based on the logging you added, you're adding something but it's the wrong thing.

I'm guessing this is because information about the contents of the allocation has been deleted by the time the leakage check runs

That possible. I traced the AllocId that your logs say are added as a static root (./miri run demo.rs -Zmiri-track-alloc-id=1000) and it seems to point into some indirection in the standard library LocalKey impl. The standard library's thread-locals are notoriously complicated: rust-lang/rust#110897

So yeah I wouldn't be surprised if there is some extra layer of indirection that gets torn down by TLS destructors. So there are two possible fixes for this, which both may or may not work:

• Do the traversal that the leak checker does right before TLS destructors run, so that you snag everything reachable through main thread TLS and add all of that to the static roots
• Move the leak check to before TLS destructors run

I feel like the first one has lower chance of going wrong somehow?

[RalfJung]

Do the traversal that the leak checker does right before TLS destructors run, so that you snag everything reachable through main thread TLS and add all of that to the static roots

That doesn't sound right. Static roots already apply recursively, everything reachable from then is not considered leaked.

There are 2 kinds of thread-local state (thread-local statics and the OS-provided "TLS keys"), and they both get cleaned up on thread exit:

miri/src/shims/tls.rs

Line 274 in 538479b

this.machine.tls.delete_all_thread_tls(this.get_active_thread());

miri/src/eval.rs

Line 246 in 538479b

this.terminate_active_thread()?;

Marking the main thread data as static roots needs to happen before that cleanup.

[max-heller]

There are 2 kinds of thread-local state (thread-local statics and the OS-provided "TLS keys"), and they both get cleaned up on thread exit:

Thanks, I was missing the former. Added those as static roots before they're destroyed and verified that the Cell-containing thread-local is indeed stored as a static root:

     Running `target/debug/miri tests/pass/issues/issue-miri-2881.rs -Zmiri-track-alloc-id=1397`
note: tracking was triggered
  --> tests/pass/issues/issue-miri-2881.rs:8:5
   |
8  | /     thread_local! {
9  | |         static REF: Cell<Option<&'static i32>> = Cell::new(None);
10 | |     }
   | |_____^ created thread-local static allocation of 24 bytes (alignment 8 bytes) with id 1397
   |
...
note: inside `main`
  --> tests/pass/issues/issue-miri-2881.rs:12:5
   |
12 | /     REF.with(move |cell| {
13 | |         cell.set(Some(r));
14 | |     })
   | |______^
   = note: this note originates in the macro `$crate::thread::local_impl::thread_local_inner` which comes from the expansion of the macro `thread_local` (in Nightly builds, run with -Z macro-backtrace for more info)

note: tracking was triggered
  |
  = note: freed allocation with id 1397
  = note: (no span available)

but still no luck in having the leak check acknowledge the box as reachable:

error: memory leaked: alloc1338 (Rust heap, size: 4, align: 4), allocated here:
...
note: inside `main`
   --> tests/pass/issues/issue-miri-2881.rs:5:13
    |
5   |     let b = Box::new(a);
    |             ^^^^^^^^^^^

[oli-obk]

Could you open a PR? That makes it easier to review and track the changes you do.