CString::into_raw() trigger miri

[Stargateur]

use std::ffi::CString;

fn main() {
    let _hello = CString::new("Hello")
        .expect("CString::new failed")
        .into_raw();
}

This simple code should not trigger any error, except a leak of course. But miri report an error before:

error[E0080]: Miri evaluation error: trying to reborrow for Unique, but parent tag <2145> does not have an appropriate item in the borrow stack
   --> /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/ffi/c_str.rs:605:13
    |
605 |             result
    |             ^^^^^^ Miri evaluation error: trying to reborrow for Unique, but parent tag <2145> does not have an appropriate item in the borrow stack
    |
    = note: inside call to `std::ffi::CString::into_inner` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/ffi/c_str.rs:440:23
note: inside call to `std::ffi::CString::into_raw` at src/main.rs:4:18
   --> src/main.rs:4:18
    |
4   |       let _hello = CString::new("Hello")
    |  __________________^
5   | |         .expect("CString::new failed")
6   | |         .into_raw();
    | |___________________^
    = note: inside call to `main` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:64:34
    = note: inside call to closure at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:53
    = note: inside call to closure at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:294:40
    = note: inside call to `std::panicking::try::do_call::<[closure@DefId(1:5878 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:290:5
    = note: inside call to `std::panicking::try::<i32, [closure@DefId(1:5878 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:388:9
    = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:5878 ~ std[82ff]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:25
    = note: inside call to `std::rt::lang_start_internal` at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:64:5
    = note: inside call to `std::rt::lang_start::<()>`

First, I suspected a miri bug but look like the code of CString could be the problem, I don't really understand the code of the into_inner() call by into_raw().

fn into_inner(self) -> Box<[u8]> {
    unsafe {
        let result = ptr::read(&self.inner);
        mem::forget(self);
        result
    }
}

Is this code correct and it's a miri bug or the code is incorrect ?

@matklad as you write the code maybe you want be ping.

[shepmaster]

/cc @RalfJung

[shepmaster]

The code in question:

https://github.com/rust-lang/rust/blob/78ca1bda3522b14bc033/src/libstd/ffi/c_str.rs#L605

[Stargateur]

fn into_inner(self) -> Box<[u8]> {
    unsafe {
        let result = std::mem::MaybeUninit::new(std::ptr::read(&self.inner));
        std::mem::forget(self);
        result.assume_init()
    }
}

Should be ok for me but it's still trigger miri

[RalfJung]

On a first glance this looks like an issue I have seen before, where the problem is that mem::forget is a normal function call. So the std::ptr::read(&self.inner) creates a raw pointer and reads from it, but then the original pointer self gets used again, which invalidates the raw pointer. So this is basically the same as

let mut local = 0;
let x = &mut local;
let raw = x as *mut _; // create raw pointer
some_function(x); // use x, re-asserting that x is unique
let _val = *raw; // use raw pointer -- UB because that would violate x's uniqueness

[danielhenrymantilla]

What about:

fn into_inner(self) -> Box<[u8]> {
    use ::std::{mem::MaybeUninit, ptr};
    unsafe {
        type T = Box<[u8]>;
        let inner = ptr::read(&mut self.inner as *mut T as *const MaybeUninit<T>);
        std::mem::forget(self);
        inner.assume_init()
    }
}

[RalfJung]

That's basically a transmute, but with even fewer compiler-level checks. At that point I'd just recommend transmuting self to Box<[u8]>.

[danielhenrymantilla]

Isn't transmuting T to MaybeUninit<T> always valid?

[RalfJung]

There's in fact a safe method for it, called MaybeUninit::new. ;)

But my suggestion does not involve MaybeUninit.

// self: CString, which is just a newtype around Box<[u8]>.
fn into_inner(self) -> Box<[u8]> {
    unsafe {
        mem::transmute(self)
    }
}

[Centril]

While using mem::transmute + comment seems like the better option in the interim, my long-term preference here would be to just remove the check that doesn't let you destructure Drop-implementing types as discussed in https://internals.rust-lang.org/t/pre-rfc-destructuring-values-that-impl-drop/10450.

[danielhenrymantilla]

There's in fact a safe method for it, called MaybeUninit::new. ;)

This leads to @Stargateur 's suggestion, which seems to trigger Miri nevertheless.

But my suggestion does not involve MaybeUninit.

This indeed solves the problem here, but I was wondering about the more general pattern (See this URLO post):

what if, for instance, CString had a second non ZST field: needing to extract an unaliased pointer from a struct implementing Drop is actually not that easy to do soundly, once transmuting it entirely is not doable;

---

Another idea (again, considering that CString may contain a second field that prevents it from being transmuted into the return value directly):

fn into_inner (self) -> Box<[u8]>
{
    use ::core::{mem::MaybeUninit, ptr};
    let this = MaybeUninit::new(self);
    unsafe {
        ptr::read(&mut (*this.as_mut_ptr()).inner)
    }
}

[spunit262]

@danielhenrymantilla
ManuallyDrop would be more appropriate than MaybeUninit as it more correctly documents what you're doing. Also both MaybeUninit and forget use ManuallyDrop internally.

fn into_inner(self) -> Box<[u8]> {
    let this = mem::ManuallyDrop::new(self);
    unsafe {
        ptr::read(&this.inner)
    }
}

[Stargateur]

So we have 2 clean solutions that doesn't trigger miri what do we pick ?