Skip to content
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

Potential soundness issue #1

Open
Pointerbender opened this issue Oct 27, 2022 · 2 comments
Open

Potential soundness issue #1

Pointerbender opened this issue Oct 27, 2022 · 2 comments

Comments

@Pointerbender
Copy link

Hi! I came across your crate on crates.io because I wanted to do something similar to UnsafeAliasCell and I was curious about how others might solve the same issue :) I may have spotted a potential soundness issue in your approach, which I wanted to let you know about.

Currently the implementation of UnsafeAliasCell relies on it being !Unpin in order for LLVM to omit the noalias attribute:

#[repr(transparent)]
pub struct UnsafeAliasCell<T: ?Sized> {
// need to list _pin before inner, so T can be !Sized.
_pin: PhantomPinned,
inner: UnsafeCell<T>,
}

As far as I understand, rustc and miri currently do this as an implementation detail to work-around a known soundness issue with self-referential structs (e.g. async generators), but it is not a stable API guarantee to the outside world. This means that at some point in the future, Rust may start to emit noalias again for types that are !Unpin and then current users of this crate will have a soundness bug in them retroactively. I was warned of the same danger some time ago (relevant Zulip thread).

We were chatting on Zulip about something similar very recently, in case it helps for additional inspiration on how to work-around this soundness issue. Although I'm not sure if there exists a proper one-size-fits-all solution yet without additional support from the standard library. You might be able to partially leverage the MaybeUninit<T> type to strip the noalias attribute from pointees T (pointees T as in e.g. Box<T> and &mut T, where we distinguish between the pointer and pointee parts of the type), but a big downside is that it also strips a lot of other useful LLVM attributes as well. I'm not sure if this would work for regular struct fields like i32 or the pointers themselves. I hope this feedback is useful and might help prevent some UB :)

@y86-dev
Copy link
Owner

y86-dev commented Nov 1, 2022

Thanks for reaching out! When creating this crate, I was aware that I am relying on compiler internal implementation details. My original use for this crate was pinned initialization. Now I have created a different solution, so I do not really have a use for this anymore. I do not like using MaybeUninit<T>, because as you already said it also strips other attributes as well.

I should probably yank the current version just to be extra sure (do you have any other solutions?).

If you would like to have the name of this crate, I can transfer it to you. If you want to create a better solution.

@Pointerbender
Copy link
Author

do you have any other solutions?

All sub-optimal solutions that I could think of are:

  1. Use MaybeUninit. Downsides: this approach strips to many llvm attributes and I think it only works on pointees, but not on owned data. For example, if we have a &mut MaybeUninit<T> I believe this asserts noalias on the T. A RFC about a new MaybeDangling<T> type in the core library was launched very recently to deal with the first drawback.
  2. Use MaybeUninit/MaybeDangling in combination with a Box pointer indirection as the inner field (e.g. inner: MaybeDangling<Box<T>>). The down-side is that this will add allocation overhead and is not compatible with #[no_std] without alloc. Also it only works on non-pointer types.
  3. Use specialization. This feature is still unstable and such an approach would be hard to maintain due to the many special types out there. But we could use it to specialize on the UnsafeAliasCell::get() method iff there exists a way to soundly suppress noalias on its contents. For Box<T> and &mut T I'm not sure if an implementation of UnsafeAliasCell should/could be defined at all, because what is the meaning of a &mut UnsafeAliasCell<&mut T>, for example? For owned values it could call UnsafeCell::get() internally without storing the pointer, but any derived pointers would still get invalidated if a mutable reference to the parent is taken out. I haven't explored this option much further yet, because it has too many drawbacks already :)
  4. Note that MaybeDangling by itself is not able to offer a full solution regarding noalias either: a MaybeDangling<P> and a &mut UnsafeCellMut<T> would be different complementary types, because here P is a pointer type (e.g. Box<T> or &mut T), and T should be a pointee. To make a newtype that works in all generic cases, it would probably look something like pub struct UnsafeEverything<T> { inner: MaybeDangling<UnsafeAliasCell<UnsafeCell<T>>> } (if all of these types existed in the core library, that is) and then probably specialization is still required for whether T is a pointer or not, so maybe this should not even be a single type for everything.

If you would like to have the name of this crate, I can transfer it to you. If you want to create a better solution.

Thank you for the kind offer! :) Unfortunately, to me it looks like any "zero-cost" and sound solution could only be implemented at the compiler level. There has been some speculation about adding an UnsafeAliasCell type to the core library for a while now, but so far these have not materialized into a (pre-)RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants