Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
xmh0511 opened this issue Apr 24, 2024 · 7 comments · Fixed by #124387
Closed

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

xmh0511 opened this issue Apr 24, 2024 · 7 comments · Fixed by #124387
Labels
A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@xmh0511
Copy link

xmh0511 commented Apr 24, 2024

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

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

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.

@xmh0511 xmh0511 added the C-bug Category: This is a bug. label Apr 24, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 24, 2024
@workingjubilee
Copy link
Contributor

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
Copy link

NichtsHsu commented Apr 24, 2024

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
Copy link
Contributor

@NichtsHsu No, you should not do that.

@NichtsHsu
Copy link

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
Copy link
Contributor

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

@workingjubilee
Copy link
Contributor

workingjubilee commented Apr 25, 2024

@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 workingjubilee added A-thread-locals Area: Thread local storage (TLS) T-libs Relevant to the library team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Apr 25, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 25, 2024
@workingjubilee
Copy link
Contributor

( probably. )

jhpratt added a commit to jhpratt/rust that referenced this issue Apr 27, 2024
…-thread-locals, r=joboet

thread_local: be excruciatingly explicit in dtor code

Use raw pointers to accomplish internal mutability, and clearly split references where applicable. This reduces the likelihood that any of these parts are misunderstood, either by humans or the compiler's optimizations.

Fixes rust-lang#124317

r? `@joboet`
@bors bors closed this as completed in 7c5213c Apr 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 27, 2024
Rollup merge of rust-lang#124387 - workingjubilee:use-raw-pointers-in-thread-locals, r=joboet

thread_local: be excruciatingly explicit in dtor code

Use raw pointers to accomplish internal mutability, and clearly split references where applicable. This reduces the likelihood that any of these parts are misunderstood, either by humans or the compiler's optimizations.

Fixes rust-lang#124317

r? ``@joboet``
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 28, 2024
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants