-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Ref parameter incorrectly decorated with noalias
attribute
#63787
Comments
cc @eddyb |
Maybe pour an |
It should probably use a raw pointer (something I have been arguing for for years now ;). For |
Changing |
Also see #60076:
It does, but I think a raw pointer is the better fix. |
Why? The value field inside |
Proof of concept miscompilation: playground link Edit: To be fair, it works based on the “ |
In rust-lang/unsafe-code-guidelines#125 (comment) I asked:
But it seems like the preferred solution is to change
@RalfJung Why not? It would have to be |
Hm okay that might work. That seems real ugly though.
IMO raw pointers are more honest, as laid out years ago. The lifetime in that reference is a lie. |
Nominated for lang team as well. @rust-lang/lang the interesting question from a lang team perspective here is to decide whether this is a library bug or a language bug. I.e., is the code wrong and the |
Well, we have other types such as With such types, the pointer can be deallocated by More importantly, * Though Vec can't actually get |
I would argue for the opposite: if an allocation is freed and the underlying memory is recycled to satisfy another allocation later on, the later allocation should result in a pointer of different provenance from the one that was freed, regardless of the physical address reuse. Each new allocation should be considered a fresh allocation for the purposes of the abstract machine (to justify the different provenance). This is somewhat realized today by allocation functions returning This should not lead to any issues because all uses of the "old" allocation must happen before it is freed, and all uses of the new allocation must happen after it was allocated. If these two time spans don't overlap, everything is fine. It's a bit weird that pointers to the same memory address but with different provenance are floating around next to each other, but this can happen even in safe code with e.g. int-to-pointer casts or Note that all of this is completely independent of allocator internals and data structures, which are neither relevant nor usually visible to optimizers. Freeing an allocation to which one has a safe pointer or reference is problematic for other reasons, namely
I wrote above why I think
FWIW this is more a consequence of deliberate choices in the Rust ABI(s) to pass structs of two scalars as those scalars but not do the same for structs with more fields. It's neither platform-specific nor otherwise externally-imposed. Also, if we had usable "local |
They're visible if LTO is turned on, but unless the allocator is trivial, the optimizer is nowhere near smart enough to notice that newly allocated pointers might not be 'based on' the previously freed versions. However, assuming that the optimizer will never be smart enough to notice something is a risky strategy in the long term. :)
Well, it's a combination of two issues mentioned in the paper. In 4.6 it discusses the problems with propagating pointer equalities in GVN – that is, in a code path where In 4.8, it discusses how newly allocated pointers can be equal to previously freed ones, albeit mostly in the context of wanting to optimize the comparison to
To be more concrete, here is an example 'miscompilation' (but not really, because it cheats) based on propagating pointer equalities across a deallocation and reallocation. Under the model proposed by the twin allocation paper, the miscompilation is the optimizer's fault rather than Rust's; I believe the authors' proof-of-concept LLVM modification would have prevented it from occurring, by disabling GVN for pointers outside of limited scenarios where it's known to be safe. The steps:
Since LLVM knows the new and old boxes' pointers are equal at the point of the load, it treats it as a load from the old box... and since the old box is As I said, the example cheats. This optimization can only happen if the pointer doesn't escape from the function, and calling However, there are other circumstances where this could theoretically happen without UB. Suppose that instead of a struct DetachedRc<T> {
refcount: *mut i32,
ptr: *const T,
} In this case, But, assuming (Some practical issues: (1) you would have to make LLVM somehow know statically that the victim function cannot drop the last reference, perhaps by asserting on the refcount, and (2) you'd be working with immutable data, whereas my PoC involves mutation; |
Another argument for using raw pointers: not only use std::cell::*;
pub fn break_it(rc: &RefCell<Option<Box<i32>>>, r: Ref<'_, i32>) {
drop(r);
*rc.borrow_mut() = None;
}
pub fn main() {
let rc = RefCell::new(Some(Box::new(0)));
break_it(&rc, Ref::map(rc.borrow(), |x| &**x.as_ref().unwrap()))
} The memory Of course, this could also be viewed as more evidence that
I don't think that would be sufficient in general. For example, consider a silly version of One could say this is silly, but I am not sure interpreting "fuzzy" signals like "is there a Btw,
That's a very different Custom allocators are a very underexplored topic, and the LLVM "twin allocation" paper did not explore them either, that one assumed a built-in allocator. The paper does not even mention I don't think that pointer helps us determine how to model LLVM argument-position
"escaping" is not an Abstract Machine operation, it is an approximation used by compiler. So it can never be used to argue for correctness.
Does this mean what I wrote at #60076 (comment) is wrong? I saw a I have no idea why you are talking about custom allocators and return-position |
👍
Oh, yeah, the
I don't know why @comex brought it up here, I don't think it's related (nor am I convinced there is any issue to begin with). In any case I've opened rust-lang/unsafe-code-guidelines#196 to discuss it separately. |
Can we answer this question without an aliasing model ? Independently of who is at fault, fixing this at the "library level", by using raw pointers, would be a small and localized change, that we could justify by trying to avoid relying on unspecified details of the aliasing model. Fixing this at the language level, by removing struct Foo<'a, T>(&'a mut T, &'a mut T);
let foo = Foo { ... };
fn before<'a, T>(x: Foo<'a, T>) { ...x.0/x.1 aren't noalias... }
before(foo);
fn after<'a, T>(x0: &'a mut T, x1: &'a mut T) { ...x0,x1 are noalias... }
let Foo(x0, x1) = foo;
after(x0, x1); |
This would be putting a constraint on the aliasing model. We already have many those constraints (e.g. from code where everyone agrees
Not really. At least, I don't view whether E.g., we say that |
They have the same calling convention (the values are passed in the same way from caller to callee and vice-versa), but they are different types, just like
This choice must be made depending on Layout/ABI/PointerKind/etc. |
Is this issue still applicable? When trying the original example on 1.56-nightly (which AFAIK has mutable-noalias on by default) I get the following: ; playground::break_it
; Function Attrs: nonlazybind uwtable
define internal void @_ZN10playground8break_it17h8910f8f2e3383d74E({ i64, i32 }* align 8 dereferenceable(16) %rc, i32* align 4 dereferenceable(4) %r.0, i64* 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 !dbg !797 { |
AFAIK (I've also edited both the issue description and your comment to add syntax highlighting via ```llvm - sadly I really wish LLVM did any kind of multi-line formatting, it's really annoying to have to use horizontal scroll to read these things) |
Interesting! So when did this change, and what exactly are the rules for |
I believe the last changes to these rules was in #82834: rust/compiler/rustc_middle/src/ty/layout.rs Lines 2340 to 2368 in cfc856a
|
Ah... |
By "behave as before", you mean you're still seeing an invalid ; playground::break_it
; Function Attrs: noinline nonlazybind uwtable
define internal fastcc void @_ZN10playground8break_it17hc2b3a560f5f197e6E({ i64, i32 }* align 8 dereferenceable(16) %rc, i64* nocapture align 8 dereferenceable(8) %r.1) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality { What is the exact output that you're seeing, and with which version? |
Make sure to add |
@bstrie I tried this and the 2nd argument of
Though strangely there is a third argument...? Ah I guess this is using Same for
I would be very surprised if LLVM ever added |
Indeed the original example for some reason looks very different, I think LLVM might be optimizing away an unused function argument? It's a binary crate, so very aggressive optimizations are possible because LLVM "knows" all the call sites. Here's a playground reproducer though, using a library crate. I will update the original text. |
@bjorn3 Ah just opened this on desktop and I can see the code - I'm not really happy with non-codegen parts of the compiler relying on opt-level, but I can kinda see how you couldn't avoid the |
@RalfJung Ah I remember now, I removed the body because for some reason I was getting things I didn't want to see in the LLVM IR, codegen'd, and ended up with something like this: https://godbolt.org/z/aooY9jbMa If you add
Ah, fair, I was mostly noticing the |
So to be clear, is the fix for this as straightforward as changing |
Yes I think that would be the best fix (and I've argued for that change even before we knew about this problem^^) |
cc @rust-lang/libs What's stopping us from fixing this? It looks like a low hanging unsound bug. :) |
One problem is deciding whether this should be fixed on the lang level or libs level... |
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. |
Yeah, I think there are several good reasons to use raw pointers (or rather, On the lang side, we could not add |
I can work on the lib side. |
Please see #97027. |
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias` When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`. Fixes the library side of rust-lang#63787, but we still might want to explore language solutions there.
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias` When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`. Fixes the library side of rust-lang#63787, but we still might want to explore language solutions there.
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias` When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`. Fixes the library side of rust-lang#63787, but we still might want to explore language solutions there.
Now that the library fix has landed, are we still considering lang changes, or can we close this? |
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. |
The following library:
generates IR for
break_it
that starts likeNote the
noalias
. That is incorrect, this function violates the noalias assumptions by mutating the datar
points to through a pointer (rc
) not derived fromc
. Here's a caller causing bad aliasing:RefMut
has the same problem when-Zmutable-noalias
is set.Cc @rkruppe @comex
The text was updated successfully, but these errors were encountered: