as_ptr()'s use of *mut is misleading

[kornelski]

as_ptr() returns a *mut _ pointer. This isn't an error by itself, but under Rust's memory model, this pointer is derived from shared/immutable &self, and therefore doesn't have permission to mutate self, so the mut of the pointer is misleading.

foreign-types/foreign-types-shared/src/lib.rs

Line 89 in 393f6ab

fn as_ptr(&self) -> *mut Self::CType {

I assume this is for convenience, because C APIs aren't diligent about const vs mut distinction. However, this is a gotcha, because C APIs that take *mut may actually mutate the object, and that is UB from Rust's perspective.

Could you add .as_mut_ptr() that takes &mut self to provide a pointer safe for mutation?

[sfackler]

The structure of Opaque is designed to prevent mutation through the *mut from being UB:

foreign-types/foreign-types-shared/src/lib.rs

Line 14 in 393f6ab

pub struct Opaque(PhantomData<UnsafeCell<*mut ()>>);

.

Even for C APIs that are const-correct, C's notion of constness is distinct enough from Rust's that & <-> *const and &mut <-> *mut doesn't really work.

[icmccorm]

I've developed a version of Miri that can execute foreign functions by interpreting LLVM bytecode, and it found a Tree Borrows violation related to mutating through self.as_ptr(). Writing through the mutable pointer created from self.as_ptr() is considered undefined behavior under this model, since this pointer is given a Frozen access tag as a child tag of the implicit &self parameter to as_ptr.

Here's a minimal example that can recreate the error with an unmodified version of Miri, without the FFI. In particular, this error is triggered when mutating through self.as_ptr() within a method that receives &mut self, where self is an instance of ForeignTypeRef.

impl ExampleRef {
    fn invalid_mutation(&mut self) {
        let raw_ptr: *mut ExampleInner = self.as_ptr();
        unsafe {
            (*raw_ptr).inner = 1;
        }
    }
}

Also, I was a bit unsure about what you meant here:

C's notion of constness is distinct enough from Rust's that & <-> *const and &mut <-> *mut doesn't really work.

Are you saying that the compiler's treatment of &T differs enough from * const in C (even under a const-correct API) that this borrowing violation isn't actually problematic in practice?

[sfackler]

The &self is a pointer to UnsafeCell (semantically through a PhantomData). Why is it being tagged as Frozen?

[kornelski]

Could it be because UnsafeCell in PhantomData has zero size, and doesn't exist in memory? If it was Opaque(UnsafeCell<ActualForeignSizedType>) then it would map to an address space.

[madsmtm]

Yeah, I think there's a difference between PhantomData<UnsafeCell<*mut ()>> and UnsafeCell<PhantomData<*mut ()>>, with only the latter registering as using interior mutability

[madsmtm]

I'll note again: rustc currently treats UnsafeCell differently whether it's inside PhantomData or not! See this playground, click "Show LLVM IR", and inspect the result; the function taking &PhantomData<UnsafeCell<*mut ()>> gets marked with noalias readonly, while &UnsafeCell<PhantomData<*mut ()>> gets marked with readnone.

If you add extern types to the mix, you'll see the exact same LLVM IR as for &UnsafeCell<PhantomData<*mut ()>> (LLVM will even merge the functions because of that).

[madsmtm]

See also discussion from rust-lang/unsafe-code-guidelines#236 (comment), this is still not determined

[icmccorm]

If you add extern types to the mix, you'll see the exact same LLVM IR

@madsmtm I think you accidentally used the same link twice.

Based on this pull request, allowing PhantomData to apply interior mutability would be a breaking change, so it's not likely to be resolved in this direction soon. It seems like it would be worthwhile to change Opaque for now until the semantics are settled.

[madsmtm]

I think you accidentally used the same link twice.

Apologies, I've fixed it now.