OwningRef is in conflict with `noalias`

[RalfJung]

See the discussion starting here: It is possible to trigger UB in safe code using owning-ref.

Specifically, the assertion in the following snippet by @comex fails, even though it should succeed:

extern crate owning_ref; // 0.4.0
use owning_ref::OwningRef;
use std::cell::Cell;

fn helper(owning_ref: OwningRef<Box<Cell<u8>>, Cell<u8>>) -> u8 {
    owning_ref.as_owner().set(10);
    owning_ref.set(20);
    owning_ref.as_owner().get() // should return 20
}

fn main() {
    let val: Box<Cell<u8>> = Box::new(Cell::new(25));
    let owning_ref = OwningRef::new(val);
    let res = helper(owning_ref);
    assert_eq!(res, 20); // assertion fails!
}

[est31]

@RalfJung is rental affected, too?

[RalfJung]

I don't know, haven't looked at that yet.

[RalfJung]

But given it mentions "self-referential structs", I heavily expect it to be affected, just like self-referential generators. (For the latter we have UB but no "miscompilation" due to that, yet.)

[danielhenrymantilla]

Here is a playground link showing how to fix OwningRef so that the code of the OP is sound. I hope it can be generalized to solving all the non-aliasing issues, while letting the current API of the crate remain unchanged.

The main lines of the solution are:

• another unsafe marker trait, for smart pointer owners that can be aliased; this is, for instance, the case of [A]Rc, but not of Box:

  /// Marker trait for a pointer type that is allowed to have its
  /// pointee aliased (except when dropped).
  pub
  unsafe
  trait AliasableOwner : Deref {}
  
  unsafe
  impl<T : ?Sized> AliasableOwner for Rc<T> {}
  
  unsafe
  impl<T : ?Sized> AliasableOwner for Arc<T> {}

• a new smart pointer type to act as a Box-equivalent, except for its not having a no-aliasing guarantee:

  use ::std::{ptr::NonNull, ops::Deref};
  
  unsafe
  impl<T : ?Sized> AliasableOwner for NonUniqueBox<T> {}
  
  // Invariant: points to a valid `Box`-allocated `T`,
  // which may be aliased except when dropped
  pub
  struct NonUniqueBox<T : ?Sized> (
      NonNull<T>, // covariance is fine because ownership
  );
  
  impl<T : ?Sized> From<Box<T>> for NonUniqueBox<T> {
      #[inline]
      fn from (ptr: Box<T>) -> Self
      {
          Self(unsafe {
              NonNull::new_unchecked(Box::into_raw(ptr))
          })
      }
  }
  
  impl<T : ?Sized> Drop for NonUniqueBox<T> {
      fn drop (self: &'_ mut Self)
      {
          unsafe {
              // By now all the aliasing must have ended,
              // and we have recovered an unaliased pointer
              let _ = Box::from_raw(self.0.as_ptr());
          }
      }
  }
  
  impl<T : ?Sized> Deref for NonUniqueBox<T> {
      type Target = T;
  
      #[inline]
      fn deref (self: &'_ Self) -> &'_ T
      {
          unsafe { self.0.as_ref() }
      }
  }

It passes MIRI and generates the following LLVM IR for helper:

; Function Attrs: nounwind nonlazybind uwtable
define i8 @helper(i8* nonnull, i8*) unnamed_addr #2 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
bb10:
  store i8 10, i8* %0, align 1
  store i8 20, i8* %1, align 1
  %.val5 = load i8, i8* %0, align 1
  tail call void @__rust_dealloc(i8* nonnull %0, i64 1, i64 1) #5
  ret i8 %.val5
}

• There is also a version with i8* align 1 dereferenceable(1) instead of i8* nonnull, although I am less confident about its correctness (it uses &'a T instead of NonNull<T>, so it feels hacky...)

• compare it to the LLVM-IR generated for the OP's code:

  ; Function Attrs: nounwind nonlazybind uwtable
  define i8 @helper(i8* noalias align 1 dereferenceable(1), i8*) unnamed_addr #2 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
  bb8:
    store i8 20, i8* %1, align 1
    tail call void @__rust_dealloc(i8* nonnull %0, i64 1, i64 1) #6
    ret i8 10
  }

[RalfJung]

Cool! That sounds like it brings the right ingredients.

It passes MIRI

Notice that Miri is currently very permissive for raw pointers. It allows more than LLVM noalias does. Fixing that is blocked on https://github.com/rust-lang/rfcs/pull/2582.

[bluss]

Sorry to ask here, but would the same problem exist if the Box was replaced with a Vec (of one element)? Or does the Vec count as an aliasable owner?

[comex]

AFAIK, it should but currently doesn't. In theory it should be possible to apply noalias to any Unique<T>, which both Box and Vec use for their pointers, but instead the compiler special-cases Box itself. @RalfJung may have more information about that.

[HeroicKatora]

If it is not possible to store and pass a meaningful pointer next to a 'noalias' tagged original pointer, maybe a way to handle this would be to utilize the 'stability' of the dereference operation and store a typed offset from the dereferenced value instead. Then the pointer stored right now would instead be restored by dereferencing the original pointer and then offsetting it with the stored offset. owning-ref can not assert inbounds on the pointer operation (but maybe the std Generators could, for references which it knows to point into the locals). However, llvm is still able to make full use of the noalias on the owned Box in contrast to the other proposed solution where we lose that attribute for all other purposes as well.

Troubles: The offset is not a pure offset for T: ?Sized. Here is a sketch for T: Sized. The llvm IR for helper:

; Function Attrs: nounwind nonlazybind uwtable
define i8 @helper(i8* noalias align 1 dereferenceable(1), i64) unnamed_addr #2 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
bb8:
  store i8 10, i8* %0, align 1
  %2 = getelementptr i8, i8* %0, i64 %1
  store i8 20, i8* %2, align 1
  %.val = load i8, i8* %0, align 1
  tail call void @__rust_dealloc(i8* nonnull %0, i64 1, i64 1) #5
  ret i8 %.val
}

[danielhenrymantilla]

@HeroicKatora that's, by the way, what ::rel-ptr does.

[RalfJung]

AFAIK, it should but currently doesn't. In theory it should be possible to apply noalias to any Unique, which both Box and Vec use for their pointers, but instead the compiler special-cases Box itself.

Agreed.

[avitex]

Hello folks. Kimundi has very kindly given me control over his repository. I'll be having a look at this in coming weeks, but I would like to make sure I understand the changes to fix this fully.

[msizanoen1]

Interestingly, this code works as intended:

use owning_ref::OwningRef;
use std::cell::Cell;

fn helper(r: &mut OwningRef<Box<Cell<i32>>, Cell<i32>>) -> i32 {
    r.as_owner().set(20);
    r.set(10);
    r.as_owner().get()
}

fn main() {
    assert_eq!(helper(&mut OwningRef::new(Box::new(Cell::new(0)))), 10);
}

Also this:

use owning_ref::OwningRef;
use std::cell::Cell;

fn helper(r: Box<OwningRef<Box<Cell<i32>>, Cell<i32>>>) -> i32 {
    r.as_owner().set(20);
    r.set(10);
    r.as_owner().get()
}

fn main() {
    assert_eq!(helper(Box::new(OwningRef::new(Box::new(Cell::new(0))))), 10);
}

So it looks like the UB is gone when passing by pointer. cc @RalfJung This probably explains why generator does not miscompile.

[RalfJung]

So it looks like the UB is gone when passing by pointer.

All I can see is that the compiler doesn't exploit the UB any more. How do you conclude that there is no UB?

[bluss]

<this comment was on the wrong issue.. moving it. I'm sorry!>