Arc::drop has a (potentially) dangling shared ref

@Amanieu

Discovered by @Amanieu on IRLO. Quoting their report:

Arc::drop contains this code:

if self.inner().strong.fetch_sub(1, Release) != 1 {
    return;
}

Once the current thread (Thread A) has decremented the reference count, Thread B could come in and free the ArcInner.

The problem becomes apparent when you look at the implementation of fetch_sub:

pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type {
    unsafe { atomic_sub(self.v.get(), val, order) }
    // HERE
}

Note the point marked HERE: at this point we have released our claim to the Arc (as in, decremented the count), which means that Thread B might have freed the ArcInner. However the &self still points to the strong reference count in the ArcInner -- so &self dangles.

Other instances of this:

Soundness of std::sync::Once #65796

Potential fixes:

Provide a version of fetch_sub that takes a raw pointer, and use that.
Provide a version of UnsafeCell that opts-out of the dereferenceable attribute, maybe even make that the default behavior for UnsafeCell. Also see What about: volatile accesses and memory-mapped IO unsafe-code-guidelines#33 for another use-case for a non-dereferenceable shared ref.

Turns out Clang has a similar issue, it uses dereferenceable for C++ references and that's not correct because the pointee might get deallocated during the course of the function. Hence another possible fix might be to use dereferenceable_on_entry instead of dereferenceable for &UnsafeCell<T> once that attribute exists.

It's a little spooky to see a situation where a region of code could do something undefined without an unsafe block. However, as the 'Nomocon notes, this is to be expected when working around unsafe code. A soundly designed library will encapsulate this potential unsafety, but it's not possible or even desirable to protect closely neighboring safe code from the potential of UB.

Anyway, in this twilight zone dangling pointers are not prohibited. Dereferencing dangling pointers is. Exposing somebody else's unsuspecting code to a dangling pointer is also very forbidden. But it's fine for dangling pointers to exist as long as they are dead.

Remember: "dead" means "will not be accessed by anything below this point." Other languages don't even have a concept of reference lifetimes.

Once the current thread (Thread A) has decremented the reference count, Thread B could come in and free the ArcInner.

That can happen. But if it does, Thread A is guaranteed to fetch a reference count greater than 1, which then causes Arc::drop to return early. self dangles but it isn't dereferenced while dangling, so no problem.

It would be undefined behavior for any method to be called after drop. But we also know that this won't happen, because "drop is the last method call" is one of the invariants.

Honestly, I think you're a little too attached to the "Stacked Borrows" model - this is a situation where it doesn't work and rather than trying to understand why the model should be changed, you're arguing that the source code of stdlib should be declared unsound. If Rust continues in the tradition of LLVM, the uniqueness of &mut really means something like "I solemnly swear that the addresses derived from this reference will not be derived by any other means until after the last use of this reference."

The immediate problem is not about Rust, safe or unsafe, or any proposed formal semantics for it. rustc emits IR that has technically undefined behavior, because there is a pointer that is claimed to be dereferenceable for the duration of a function but points to memory that is (potentially) deallocated during the execution of the function. That this is not a Rust-specific problem can also be seen by the fact that Clang ('s C++ mode) is affected by it too, as linked earlier. This specific instance is not terribly likely to cause miscompilations, but others are, and in any case "UB that's unlikely to cause problems in practice" is not a good way to build a reliable language implementation.

Aside from that, there is the task of justifying the LLVM IR that rustc emits with Rust-level rules -- at minimum, in every case where emitted LLVM IR has UB, the input Rust code also needs to have UB to be able to claim rustc is correct. This issue points out a case where previous attempts at justifying the dereferenceable attribute that rustc emits are not sufficient. There are multiple possible remedies for that, none of which are about stacked borrows and most of which modify how rustc emits IR (emitting the attribute in fewer cases, or emitting a weaker attribute instead in some cases) to make the existing code well-defined without modification.

Option one:

Provide a version of fetch_sub that takes a raw pointer, and use that.

for resolving this bug was tried in here: #58611

@jamesmunns

I'm nominating this issue -- and tagging it for @rust-lang/lang -- in response to some comments by @jamesmunns in today's Project Leadership Sync meeting. In particular, @jamesmunns pointed out that this issue around the deferenceable attribute has potentially large repercussions in the embedded space, and cited @japaric's RFC as well as a number of issues around the embedded space (see the minutes).

I've got to dig a bit deeper here but it seems like it'd be good to check-in on the current thinking here. I will try to catch up on the RFC and some of the proposed changes and see if I can post any notes.

Without having done so, I would say that, as far as I know, we still want to keep the dereferenceable attribute, so I expect some form of these changes will be required.

(I suppose omething like the [dereferencable on entry proposal(https://lists.llvm.org/pipermail/llvm-dev/2018-July/124555.html) might also be good, does anyone know if that's made progress? I'm going to assume not.)

@nikomatsakis

@nikomatsakis I have done some work around this. But actually that extended visible API surface by one method. It resolves the problem at a cost of that.

I could be wrong, but it seems like LLVM now supports nofree and nosync function attributes, which would solve the issue. https://lists.llvm.org/pipermail/llvm-dev/2018-July/124555.html

nofree and nosync exist now, but as far as I can tell the switch from

dereferenceable applies through the entire function

to

dereferenceable applies on function entry (use nofree and nosync information to recover analysis power)

has not yet happened.

Merge #855 #858

Auto merge of rust-lang#98017 - RalfJung:dereferenceable, r=nikic

@RalfJung

@RalfJung Can we solve this by changing Drop trait API?

Something like this:

trait Drop{
   fn drop(&mut self);
   
   fn drop_ptr(mut self: NonNull<Self>){
        unsafe{
            // Only case when direct call to `Drop::drop` is allowed.
            Drop::drop(self.as_mut());
        }
   }
}

All existing code would just work because it delegates to old methods that accept the reference.
For cases like discussed ITT, user can implement it like this:

impl Drop for MyTypeWithPtr{
   fn drop(&mut self){ unreachable!("We use only pointer based deallocation") }
   
   fn drop_ptr(mut self: NonNull<Self>){
        // Since we created references inside function, they are not dereferencable.
        // And we can check all our conditions using pointer based API
        // to avoid having living references to data freed in separate thread.
   }
}

This way we would solve issues caused by dropping code like this without giving up dereferencable on &UnsafeCell<X> arguments.

It's not the reference in drop that caused the trouble. It's the &self reference in this AtomucIsize method:

pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type {
    unsafe { atomic_sub(self.v.get(), val, order) }
    // HERE
}

Anyway, with #98017 having landed, I think we can finally close this issue. :) We barely managed to do it in less than 4 years. :D

Use pointers instead of `&self` in `Latch::set`

Merge #1011

Navigation Menu

Repository navigation

Arc::drop has a (potentially) dangling shared ref #55005

55 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Arc::drop has a (potentially) dangling shared ref #55005

Description

Activity

RalfJung commented on Oct 12, 2018

RalfJung commented on Dec 9, 2018

jClaireCodesStuff commented on Jan 6, 2019

hanna-kruppe commented on Jan 6, 2019

vertexclique commented on Feb 21, 2019

nikomatsakis commented on Oct 23, 2019

vertexclique commented on Oct 23, 2019

64 commented on Oct 23, 2019

hanna-kruppe commented on Oct 23, 2019

55 remaining items

SoniEx2 commented on Jul 4, 2022

RalfJung commented on Jul 4, 2022

SoniEx2 commented on Jul 4, 2022

RalfJung commented on Jul 4, 2022

SoniEx2 commented on Jul 4, 2022

RalfJung commented on Jul 4, 2022

AngelicosPhosphoros commented on Jul 24, 2022

RalfJung commented on Jul 24, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions