The implementation in `fast_local.rs` seems to violate pointer aliasing rule

[xmh0511]

rust/library/std/src/sys/thread_local/fast_local.rs

Line 210 in c1feb3e

unsafe { register_dtor(self as *const _ as *mut u8, destroy_value::<T>) };

unsafe fn try_register_dtor(&self) -> bool {
        match self.dtor_state.get() {
            DtorState::Unregistered => {
                // SAFETY: dtor registration happens before initialization.
                // Passing `self` as a pointer while using `destroy_value<T>`
                // is safe because the function will build a pointer to a
                // Key<T>, which is the type of self and so find the correct
                // size.
                unsafe { register_dtor(self as *const _ as *mut u8, destroy_value::<T>) };  // #1
                self.dtor_state.set(DtorState::Registered);
                true
            }
            DtorState::Registered => {
                // recursively initialized
                true
            }
            DtorState::RunningOrHasRun => false,
        }
    }

At #1, acquire a pointer of type * mut u8 from a shared reference &Key<T>, and subsequently, in the destroy_value, we reinterpret * mut u8 as * mut Key<T> and use it as a mutable pointer

rust/library/std/src/sys/thread_local/fast_local.rs

Line 240 in c1feb3e

let value = (*ptr).inner.take();

unsafe extern "C" fn destroy_value<T>(ptr: *mut u8) {
    let ptr = ptr as *mut Key<T>;

    // SAFETY:
    //
    // The pointer `ptr` has been built just above and comes from
    // `try_register_dtor` where it is originally a Key<T> coming from `self`,
    // making it non-NUL and of the correct type.
    //
    // Right before we run the user destructor be sure to set the
    // `Option<T>` to `None`, and `dtor_state` to `RunningOrHasRun`. This
    // causes future calls to `get` to run `try_initialize_drop` again,
    // which will now fail, and return `None`.
    //
    // Wrap the call in a catch to ensure unwinding is caught in the event
    // a panic takes place in a destructor.
    if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(|| unsafe {
        let value = (*ptr).inner.take();  // #2
        (*ptr).dtor_state.set(DtorState::RunningOrHasRun);
        drop(value);
    })) {
        rtabort!("thread local panicked on drop");
    }
}

At #2 the method take of the field inner requires & mut self as defined in

rust/library/std/src/sys/thread_local/mod.rs

Line 98 in c1feb3e

pub unsafe fn take(&mut self) -> Option<T> {

The whole process can be that we use &Key<T> to acquire a & mut Key<T> and call a mutable method on that variable, which seems problematic.

[workingjubilee]

The code is sound if it uses UnsafeCell::get, and all the fields inside that type is covered by UnsafeCell. It should use UnsafeCell::get or UnsafeCell::raw_get instead of pointer-casting.

[NichtsHsu]

The code is sound if it uses UnsafeCell::get, and all the fields inside that type is covered by UnsafeCell. It should use UnsafeCell::get or UnsafeCell::raw_get instead of pointer-casting.

Edit: I think what I want to ask about is the following example, which is almost identical to the original question and passes miri's check:

use std::cell::Cell;

struct S(Cell<i32>);
struct U(S);

fn main() {
    let u = U(S(Cell::new(10)));
    let p = &u as *const _ as *mut U;
    let s = unsafe { &mut (*p).0 };
    println!("{}", s.0.take());
}

[workingjubilee]

@NichtsHsu No, you should not do that.

[NichtsHsu]

So this is what is happening in the standard library:

There is a structure like Key { inner: LazyKeyInner(UnsafeCell), ... };

In Key::try_register_dtor, &self is converted to *mut u8 and passed to register_dtor;

In destroy_value, *mut u8 is converted to ptr: *mut Key, then (*ptr).inner.take() is called.

LazyKeyInner::take requires &mut self, so it is actually LazyKeyInner::take(&mut (*ptr).inner), which means that a &mut LazyKeyInner is born from a &Key.

In the entire process, we haven’t touched on anything related to UnsafeCell. And, I also believe that containing an UnsafeCell is not a reason for LazyKeyInner to be treated differently.

[workingjubilee]

Yes, LazyKeyInner::take should be fn(&self) -> Option<T> and not fn(&mut self) -> Option<T>.

[workingjubilee]

@NichtsHsu No, you should not do that.

I should clarify that my "should not" here is that it can potentially pass miri, but my understanding is that it is brittle and relying on the operational semantics being a certain way, rather than being a "necessarily true" interpretation of Rust's semantics. Programs which are slight modifications easily become unsound, and libstd unfortunately has slight modifications done to it all the time, so...

aiui this code currently passes miri because we do run miri on libstd's tests, but is not what I would consider "durable".

[workingjubilee]

( probably. )