CString::into_raw() trigger miri
Stargateur opened this issue · 12 comments
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.
/cc @RalfJung
The code in question:
https://github.com/rust-lang/rust/blob/78ca1bda3522b14bc033/src/libstd/ffi/c_str.rs#L605
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
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 uniquenessWhat 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()
}
}That's basically a transmute, but with even fewer compiler-level checks. At that point I'd just recommend transmuting self to Box<[u8]>.
Isn't transmuting T to MaybeUninit<T> always valid?
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)
}
}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.
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)
}
}@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)
}
}So we have 2 clean solutions that doesn't trigger miri what do we pick ?