Loom does not consider dropping UnsafeCell to be a write #349

e00E · 2024-04-23T21:14:25Z

Consider the following code:

#[test]
fn foo() {
    loom::model(|| {
        let a: loom::cell::UnsafeCell<()> = Default::default();
        let first_ref = a.get();
        let second_ref = a.get_mut();
    });
}

The test fails because loom correctly detects that mutable access is not allowed while there is already immutable access.

Now change the last line of the code:

#[test]
fn foo() {
    loom::model(|| {
        let a: loom::cell::UnsafeCell<()> = Default::default();
        let first_ref = a.get();
        std::mem::drop(a);
    });
}

Loom no longer detects an error.

This is surprising because dropping could be considered a write. For example, if instead of () I was using a type with a custom Drop implementation, then I would get a mutable reference to self. This write doesn't go through UnsafeCell but it still leads to the reference guarantees being violated.

This behavior might be intentional because the documentation for ConstPtr says that Loom doesn't track liveness. I'm not sure. I did not find an existing issue about this topic so I created one. If the behavior is intentional, then this issue can be closed and can serve as documentation for the reason.

The text was updated successfully, but these errors were encountered:

hawkw · 2024-04-23T21:27:43Z

This behavior might be intentional because the documentation for ConstPtr says that Loom doesn't track liveness. I'm not sure. I did not find an existing issue about this topic so I created one. If the behavior is intentional, then this issue can be closed and can serve as documentation for the reason.

Loom doesn't track liveness of pointers in the general case, because that would be very challenging to implement in loom (and is arguably better served by other tools, such as miri), but, IMO, that doesn't mean we shouldn't handle this specific case. We probably should treat dropping an UnsafeCell as a mutable access, IMO.

The code that we're removing calls UnsafeCell::with_mut with the argument `std::mem::drop`. This is misleading because the use of `drop` has no effect. `with_mut` takes an argument of type `impl FnOnce(*mut T) -> R`. The argument to the argument function is a pointer. Dropping a pointer has no effect. The comment above the first instance of this pattern claims that this releases some resource. This is false because the call has no effect. The intention might have been to drop the value behind the pointer. If this did happen, it would be a bug because the resource (`waker`) would be dropped again at the end of the function when the containing object is dropped. I looked through the history of this code. This code originally called `with_mut` with the argument `|_| ()`. Calling `with_mut` with an argument function that does nothing has a side effect when testing with loom. When testing with loom, the code uses loom's UnsafeCell type instead of std's. The intention of the code was likely to make use of that side effect because we expect to have exclusive access here as we are going to drop the containing object. The side effect is that loom checks that Rust's reference uniqueness properties are upheld. Instead of removing the whole code, I considered changing `drop` back to `|_|()` and documenting what I wrote above. I decided on removing the code because nowhere else do we use `with_mut` in this way and the purpose of this check would be better served in loom directly as part of UnsafeCell's Drop implementation. I created an issue about this in loom [1]. [1] tokio-rs/loom#349

e00E · 2024-04-24T08:57:01Z

I've thought about the implementation of this. The naive (is there another?) way to implement this is to implement Drop and get a mutable reference while dropping. That way you detect other active references. This is awkward for two reasons:

You restrict the type's use relative to std's UnsafeCell. When Drop is implemented, drop check applies, making some uses that worked before no longer work.
into_inner stops working because you can't destructure Self anymore. We can fix that by storing an Option.

Darksonn · 2024-04-24T09:29:04Z

Let's try with a Drop implementation for now. As for into_inner, we can implement it unsafely.

The code that we're removing calls UnsafeCell::with_mut with the argument `std::mem::drop`. This is misleading because the use of `drop` has no effect. `with_mut` takes an argument of type `impl FnOnce(*mut T) -> R`. The argument to the argument function is a pointer. Dropping a pointer has no effect. The comment above the first instance of this pattern claims that this releases some resource. This is false because the call has no effect. The intention might have been to drop the value behind the pointer. If this did happen, it would be a bug because the resource (`waker`) would be dropped again at the end of the function when the containing object is dropped. I looked through the history of this code. This code originally called `with_mut` with the argument `|_| ()`. Calling `with_mut` with an argument function that does nothing has a side effect when testing with loom. When testing with loom, the code uses loom's UnsafeCell type instead of std's. The intention of the code was likely to make use of that side effect because we expect to have exclusive access here as we are going to drop the containing object. The side effect is that loom checks that Rust's reference uniqueness properties are upheld. To continue to check this, I have only removed the use of `drop` while keeping `with_mut`. It would be even better to have loom check this implicitly when UnsafeCell is dropped. I created an issue about this in loom [1]. [1] tokio-rs/loom#349

Dropping an UnsafeCell or calling into_inner can be seen as mutable access, which is not allowed to happen while another borrow exists. The downside of this change is the added Drop implementation of UnsafeCell. This restricts some uses of UnsafeCell that were previously allowed related to drop check. fixes tokio-rs#349

Dropping an UnsafeCell or calling into_inner can be seen as mutable access, which is not allowed to happen while another borrow exists. The downside of this change is the added Drop implementation of UnsafeCell. This restricts some uses of UnsafeCell that were previously allowed related to drop check. I removed one of the tests because it stops working with this change due to double panic. Fixing it is awkward. I went with an approach using unsafe. It is possible to implement this safely but it has too much overhead. We would have to wrap the data field with Option to allow safely taking. This is fine but we would also need to Box it because the type parameter T is ?Sized, which cannot be put into Option. fixes tokio-rs#349

The code that we're removing calls UnsafeCell::with_mut with the argument `std::mem::drop`. This is misleading because the use of `drop` has no effect. `with_mut` takes an argument of type `impl FnOnce(*mut T) -> R`. The argument to the argument function is a pointer. Dropping a pointer has no effect. The comment above the first instance of this pattern claims that this releases some resource. This is false because the call has no effect. The intention might have been to drop the value behind the pointer. If this did happen, it would be a bug because the resource (`waker`) would be dropped again at the end of the function when the containing object is dropped. I looked through the history of this code. This code originally called `with_mut` with the argument `|_| ()`. Calling `with_mut` with an argument function that does nothing has a side effect when testing with loom. When testing with loom, the code uses loom's UnsafeCell type instead of std's. The intention of the code was likely to make use of that side effect because we expect to have exclusive access here as we are going to drop the containing object. The side effect is that loom checks that Rust's reference uniqueness properties are upheld. To continue to check this, I have only removed the use of `drop` while keeping `with_mut`. It would be even better to have loom check this implicitly when UnsafeCell is dropped. I created an issue about this in loom [1]. Links: tokio-rs/loom#349 [1]

Dropping an UnsafeCell or calling into_inner can be seen as mutable access, which is not allowed to happen while another borrow exists. The downside of this change is the added Drop implementation of UnsafeCell. This restricts some uses of UnsafeCell that were previously allowed related to drop check. I went with an approach using unsafe. It is possible to implement this safely but it has too much overhead. We would have to wrap the data field with Option to allow safely taking. This is fine but we would also need to Box it because the type parameter T is ?Sized, which cannot be put into Option. fixes tokio-rs#349

The code that we're removing calls UnsafeCell::with_mut with the argument `std::mem::drop`. This is misleading because the use of `drop` has no effect. `with_mut` takes an argument of type `impl FnOnce(*mut T) -> R`. The argument to the argument function is a pointer. Dropping a pointer has no effect. The comment above the first instance of this pattern claims that this releases some resource. This is false because the call has no effect. The intention might have been to drop the value behind the pointer. If this did happen, it would be a bug because the resource (`waker`) would be dropped again at the end of the function when the containing object is dropped. I looked through the history of this code. This code originally called `with_mut` with the argument `|_| ()`. Calling `with_mut` with an argument function that does nothing has a side effect when testing with loom. When testing with loom, the code uses loom's UnsafeCell type instead of std's. The intention of the code was likely to make use of that side effect because we expect to have exclusive access here as we are going to drop the containing object. The side effect is that loom checks that Rust's reference uniqueness properties are upheld. To continue to check this, I have only removed the use of `drop` while keeping `with_mut`. It would be even better to have loom check this implicitly when UnsafeCell is dropped. I created an issue about this in loom [1]. Links: tokio-rs/loom#349 [1]

e00E mentioned this issue Apr 23, 2024

runtime: remove misleading use of UnsafeCell::with_mut tokio-rs/tokio#6513

Merged

e00E mentioned this issue Apr 24, 2024

Panic when accessing loom exectution state from spawned thread during unwinding #350

Closed

e00E linked a pull request Apr 24, 2024 that will close this issue

Detect simultaneous borrow and drop of UnsafeCell #351

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loom does not consider dropping UnsafeCell to be a write #349

Loom does not consider dropping UnsafeCell to be a write #349

e00E commented Apr 23, 2024

hawkw commented Apr 23, 2024

e00E commented Apr 24, 2024

Darksonn commented Apr 24, 2024

Loom does not consider dropping UnsafeCell to be a write #349

Loom does not consider dropping UnsafeCell to be a write #349

Comments

e00E commented Apr 23, 2024

hawkw commented Apr 23, 2024

e00E commented Apr 24, 2024

Darksonn commented Apr 24, 2024