fn use_n( n: usize ) { /* omitted */ }
struct Foo {
n: usize,
}
impl Foo {
// may cause deallocation of`*self`
fn do_dirty_work( &mut self ) { /* omitted */ }
fn do_work( &mut self ) {
let n = self.n;
self.do_dirty_work();
use_n( n ); // instead of use_n( self.n );
}
}
I think your example omits too much to express your concern. The n: usize will just be copied, so that’s fine, but if you had grabbed a reference instead you wouldn’t be allowed to call the &mut self method.
I’m also not sure what you think do_dirty_stuff could do to “deallocate *self” at all. You can’t move out of a reference, and you can’t manually call Drop. You could ptr::read it out, but that’s unsafe for a reason – even without your explicit use, it would face a double-drop problem.
2 Likes
Perhaps I did not use words accurately. Let me make it clear: "deallocation of *self" means "the memory area pointed by self has been retrieved by the allocator. It does not imply call <Foo as Drop>::drop().
I did omit details and focused on the correctness of “is it enough for memory safety to avoid any access to self after the memory area pointed by it has been retrieved”.
If you are interested in what do_dirty_work() can do, let’s imagine a foo: Foo is an element of Vec<Foo>, and foo.do_dirty_work() causes reallocation of the vec.
And do_dirty_work() may have extra parameters to let it acess to the vec, which is implemented in unsafe blocks.
Hm, if I’m interpreting your posts correctly, then I think the question you’re trying to ask, in terms most of us are more familiar with, is: Given this code snippet:
fn do_work( &mut self ) {
let n = self.n;
self.do_dirty_work();
use_n( n ); // instead of use_n( self.n );
}
Is the compiler allowed to reorder this code such that self.n gets copied after do_dirty_work is called, and does that mean this function is UB if do_dirty_work uninitializes self?
My immediate reaction is that there should be nothing to worry about because do_dirty_work takes a &mut to self, and you obviously can’t have two &muts to the same object at the same time no matter what wacky unsafe code is making use of them. If the compiler was somehow allowed to move the self.n copy down, then you would have two &muts to the same object and do_dirty_work could easily cause UB only using safe code (similar logic applies if n is a non-Copy type and the compiler somehow accepted that). And while you can’t “uninitialize” a variable (in the move semantics sense) using a &mut, you can use a &mut to completely replace the old value with a brand new one with safe code like mem::replace, which often does mean uninitializing/deallocating the old value. So you should only be in trouble if do_dirty_work does unsafe tricks that rely on more than just the guarantees that &mut gives you. But I’m no expert on this stuff.
1 Like
I think this should be UB. You shouldn’t have a live reference to deallocated memory, and &mut self is passed in as an argument and hence live for the entire duration of the fn call. We would prefer this not to depend on whether self is used again after calling do_dirty_work to make optimizations simpler and to enable more of them.
But thanks for the example! This reminded me I need to think about deallocation in Stacked Borrows. I think deallocation should be UB if there are any FnBarrier left on the stack. That would make this program UB.
6 Likes
The problem with making this UB is that it makes it impossible to implement any kind of reference-counted pointer (i.e. the current implementation of Arc would be UB by your definition).
This is because decrementing the reference count is done using AtomicUsize::fetch_sub(1, Release), which requires a &self on the AtomicUsize. The decrement operation effectively acts as a free since another thread could free the object immediately afterwards.
1 Like
I don’t see how this happens. The ArcInner is only freed when both strong and weak counts are zero. The dealloc is done by the drop of the last existing Arc or Weak.
Could you list the operations in the order that is problematic?
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 reference, which means that another thread might have freed the ArcInner. However the &self still points to the strong reference count in the ArcInner.
1 Like
I see the point. However, having a reference to &self that is freed is not UB; it is only UB when you actually use that reference. So, is it the case that HERE contains any code that uses self?
Your point is valid on unsafe code (partial move?). For safe code though, I am with @Ixrec: you basically cannot have violation on this rule in safe Rust. The top post program is perfectly legal if self.n is Copy, but if it is not, it is hardly compile unless using some unsafe trick.
Sure, we are only talking about unsafe code here. 
For safe code we could have way stricter rules, the question is how strict we make the "underlying" rules that safe code approximates (from the safe side).
1 Like
Wow, good catch! You are right. If this e.g. decrements from 2 to 1, but then before fetch_sub returns another thread decrements from 1 to 0 and deallocates, we have an &self pointing to deallocated memory.
But this is a problem. I am not sure what exactly dereferencable means for LLVM, but it seems to me that a pointer to memory that disappears while the function still runs must NOT be marked dereferencable. @hanna-kruppe, what do you think?
My inclination is that there should be a variant of fetch_sub that takes a raw pointer for self, and Arc should use that. Or else we have to remove dereferencable from shared references (maybe just non-Freeze shared references), which seems bad.
1 Like
I don't dare think about how concurrency interacts with this but yes the way you put it it seems like a recipe for miscompiles. Clang also struggles with this, see [llvm-dev] [RFC] A nofree (and nosynch) function attribute: Mixing dereferenceable and delete
1 Like
Thanks for that link, that’s very interesting indeed! https://lists.llvm.org/pipermail/llvm-dev/2018-July/124568.html is particularly nasty…
The summary from that LLVM discussion is that there will be a new dereferenceable_on_entry attribute, which we could set safely here. However, is that what we want? It would certainly significantly weaken what reference types mean, pushing us even more towards a model where references only have to be valid when “used” (or passed as argument, or so; but not until the end of a function).
I think a reasonable change in the stacked borrows model might be to not push any barriers for non-Freeze shared references (i.e., &T where T contains an UnsafeCell) – making shared references with interior mutability even more like raw pointers.
That would however also mean that if a function takes (&mut T, &Cell<U>) the two pointers would actually be allowed to alias (but if both are used, that would still be UB because neither is derived from the other).
I’m not sure whether this would just be papering over one instance of the issue or actually fix it, but:
What about if Atomic*::* were made atomic in and of themselves? (AKA they always inline to the atomic syscall instead of adding a stack frame.)
// Stupid bikeshed std-only syntax to guarantee this behavior
pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type =
unsafe { atomic_sub(self.v.get(), val, order) };
// Alternately, using TCO:
pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type {
unsafe { become atomic_sub(self.v.get(), val, order) }
}
// Or if we can guarantee inlining 101%
#[inline(required)]
pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type {
unsafe { atomic_sub(self.v.get(), val, order) }
}
This would mean that there is no place in the stack frame where the &Atomic* is held while it’s been decremented.
I’m not a fan. Semantics shouldn’t depend on inlining, and it should be possible for users to put a newtype around Atomic* and forward all methods manually and everything should still work.
3 Likes
Once we have &out/&move references it’ll be possible to write:
impl Foo {
fn do_dirty_work(&out self) -> Option<&out Foo> { /* omitted */ }
fn do_work(&out self) -> Option<&out Foo> {
let n = self.n;
let ret = self.do_dirty_work();
use_n( n ); // instead of use_n( self.n );
ret
}
}
do_dirty_work can return None if it deallocates self, otherwise you can
get self back via the Some.
+1. With arbitrary self types we can just implement it as a method on *const Self.
This hasn’t been reported in the bugtracker yet, has it? I filed a new issue at