Repository navigation
Ref parameter incorrectly decorated with noalias attribute #63787
Description
The following library:
use std::cell::*;
pub fn break_it(rc: &RefCell<i32>, r: Ref<'_, i32>) {
// `r` has a shared reference, it is passed in as argument and hence
// a barrier is added that marks this memory as read-only for the entire
// duration of this function.
drop(r);
// *oops* here we can mutate that memory.
*rc.borrow_mut() = 2;
}generates IR for break_it that starts like
; playground::break_it
; Function Attrs: noinline nonlazybind uwtable
define void @_ZN10playground8break_it17hb8a9beeb2affda19E(
{ i64, i32 }* align 8 dereferenceable(16) %rc,
i32* noalias nocapture readonly align 4 dereferenceable(4) %r.0,
i64* nocapture align 8 dereferenceable(8) %r.1)
unnamed_addr #1 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {Note the noalias. That is incorrect, this function violates the noalias assumptions by mutating the data r points to through a pointer (rc) not derived from c. Here's a caller causing bad aliasing:
pub fn main() {
let rc = RefCell::new(0);
break_it(&rc, rc.borrow())
}RefMut has the same problem when -Zmutable-noalias is set.
Activity
gnzlbg commentedon Aug 21, 2019
cc @eddyb
bjorn3 commentedon Aug 21, 2019
Maybe pour an
UnsafeCellintoRefinstead of&T?RalfJung commentedon Aug 21, 2019
It should probably use a raw pointer (something I have been arguing for for years now ;). For
RefMut,UnsafeCellwouldn't even help.gnzlbg commentedon Aug 21, 2019
Changing
Refvalue field tovalue: &'b std::cell::UnsafeCell<T>fixes this.RalfJung commentedon Aug 21, 2019
Also see #60076:
vec_deque::Drainhas a similar problem.It does, but I think a raw pointer is the better fix.
gnzlbg commentedon Aug 21, 2019
Why?
The value field inside
RefCellis of typeUnsafeCell<T>, notT, so IMORefhas a bug by using&Tinstead of&UnsafeCell<T>. Using a raw pointer (e.g.NonNull<T>) would mean that we lose thedereferenceableannotation.comex commentedon Aug 21, 2019
Proof of concept miscompilation: playground link
Edit: To be fair, it works based on the “
a == bimplies the compiler can replacebwitha” optimization, which can be unsound all by itself in some cases involving pointer arithmetic or allocation/deallocation... but eh, this code does neither.eddyb commentedon Aug 22, 2019
In rust-lang/unsafe-code-guidelines#125 (comment) I asked:
But it seems like the preferred solution is to change
Refto not contain a reference.@RalfJung Why not? It would have to be
&UnsafeCell, not&mut UnsafeCell, but I expect it to work.38 remaining items
RalfJung commentedon Jul 30, 2021
Yes I think that would be the best fix (and I've argued for that change even before we knew about this problem^^)
BurntSushi commentedon May 13, 2022
cc @rust-lang/libs What's stopping us from fixing this? It looks like a low hanging unsound bug. :)
RalfJung commentedon May 13, 2022
One problem is deciding whether this should be fixed on the lang level or libs level...
BurntSushi commentedon May 13, 2022
It looks like we could at least fix this at the libs level without much difficulty, right? Then the lang team can decide whether they want to fix this on their end or not. If they do, I assume we could then revert the library fix.
Although I will say that I don't quite grok the lang side fix for this.
RalfJung commentedon May 13, 2022
Yeah, I think there are several good reasons to use raw pointers (or rather,
NonNull) rather than references here.On the lang side, we could not add
noaliasto newtyped references... but that might also be a bad idea since it makes newtypes less powerful.cuviper commentedon May 13, 2022
I can work on the lib side.
@rfcbot claim
cuviper commentedon May 13, 2022
Please see #97027.
Rollup merge of rust-lang#97027 - cuviper:yesalias-refcell, r=thomcc
Rollup merge of rust-lang#97027 - cuviper:yesalias-refcell, r=thomcc
cuviper commentedon Jul 30, 2022
Now that the library fix has landed, are we still considering lang changes, or can we close this?
RalfJung commentedon Jul 30, 2022
I think the lang side of this is not final yet, but this is not the right issue to track that. We have rust-lang/unsafe-code-guidelines#125 and this will have to be part of the discussion of the aliasing model, once we get there.