Fix minor panic-unsoundness in CString::clone_into#155707
Fix minor panic-unsoundness in CString::clone_into#155707rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Here is the AI-generated report for this DetailsCString invariant broken during clone_into across unwindDetails
LocationImpactOut-of-bounds write through dangling pointer during unwinding Reproduction steps
Recommended fixThe CString invariant (inner is non-empty and NUL-terminated) must hold at every point where the value can be observed by its Drop impl, including across any operation that may unwind. And here's the proof of concept: Details//! PoC: UB in `<CStr as ToOwned>::clone_into` when the alloc-error
//! handler unwinds.
//!
//! library/alloc/src/ffi/c_str.rs:
//!
//! fn clone_into(&self, target: &mut CString) {
//! let mut b = mem::take(&mut target.inner).into_vec(); // (1)
//! self.to_bytes_with_nul().clone_into(&mut b); // (2) may alloc
//! target.inner = b.into_boxed_slice(); // (3) may alloc
//! }
//!
//! After (1), `target.inner` is `Box::<[u8]>::default()` — a zero-length
//! slice backed by `NonNull::<u8>::dangling()`. This violates CString's
//! Invariant 1 ("length of at least one"). If (2) or (3) unwinds,
//! `target` is later dropped with that empty `inner`, and
//!
//! impl Drop for CString {
//! fn drop(&mut self) {
//! unsafe { *self.inner.get_unchecked_mut(0) = 0; }
//! }
//! }
//!
//! performs an out-of-bounds write through a dangling pointer.
//!
//! ──────────────────────────────────────────────────────────────────────
//! How to run
//! ──────────────────────────────────────────────────────────────────────
//! Put this file at `src/main.rs` in a fresh binary crate.
//!
//! Under Miri (recommended — gives a precise UB diagnostic pointing at
//! `c_str.rs` inside the `Drop` impl):
//!
//! cargo +nightly miri run
//!
//! Native (segfaults writing to address 0x1):
//!
//! cargo +nightly run
//!
//! On nightlies that still have the codegen flag, an equivalent trigger
//! that does not require `feature(alloc_error_hook)` is
//! `RUSTFLAGS="-Zoom=panic" cargo +nightly run` with the
//! `set_alloc_error_hook` call removed.
#![feature(alloc_error_hook)]
use std::alloc::{set_alloc_error_hook, GlobalAlloc, Layout, System};
use std::ffi::{CStr, CString};
use std::sync::atomic::{AtomicBool, Ordering};
/// When `true`, the *next* call into the global allocator returns null
/// (simulating OOM) and atomically clears itself, so that the panic
/// machinery's own subsequent allocations succeed normally.
static FAIL_NEXT: AtomicBool = AtomicBool::new(false);
struct OneShotFail;
unsafe impl GlobalAlloc for OneShotFail {
unsafe fn alloc(&self, l: Layout) -> *mut u8 {
if FAIL_NEXT.swap(false, Ordering::SeqCst) {
core::ptr::null_mut()
} else {
unsafe { System.alloc(l) }
}
}
unsafe fn dealloc(&self, p: *mut u8, l: Layout) {
unsafe { System.dealloc(p, l) }
}
unsafe fn realloc(&self, p: *mut u8, l: Layout, n: usize) -> *mut u8 {
if FAIL_NEXT.swap(false, Ordering::SeqCst) {
core::ptr::null_mut()
} else {
unsafe { System.realloc(p, l, n) }
}
}
}
#[global_allocator]
static A: OneShotFail = OneShotFail;
fn main() {
// Make `handle_alloc_error` unwind instead of abort.
// (Unstable, but panicking from the hook is an explicitly supported
// path per the oom=panic RFC 2116 work — see rust-lang/rust#43596 and
// the discussion on rust-lang/rust#147725.)
set_alloc_error_hook(|l| panic!("oom: {} bytes", l.size()));
// `dest.inner` is a `Box<[u8]>` of length 2: b"x\0".
let mut dest = CString::new("x").unwrap();
// Long enough that `clone_into` must grow the reused buffer.
let src: &CStr = c"this string is long enough to force the Vec to reallocate";
// Arm the failure. There are no allocations between here and the
// `realloc` inside `<[u8]>::clone_into` → `Vec::extend_from_slice`
// → `RawVec::grow_*`:
// * `mem::take` of `Box<[u8]>` produces an empty box (no alloc).
// * `into_vec`, `to_bytes_with_nul`, `Vec::clear` do not alloc.
FAIL_NEXT.store(true, Ordering::SeqCst);
// (1) `dest.inner` is replaced with an empty box.
// (2) Growing `b` from cap 2 to ~59 hits our null-returning realloc;
// `handle_alloc_error` calls our hook, which panics and unwinds.
//
// During unwinding of this frame, `dest` is dropped while
// `dest.inner.len() == 0`, and `Drop` writes through
// `get_unchecked_mut(0)` of an empty slice — UB.
src.clone_into(&mut dest);
unreachable!();
}
// ──────────────────────────────────────────────────────────────────────
// Variant: surviving CString with broken invariant
// ──────────────────────────────────────────────────────────────────────
//
// The Drop write is not the only consequence. If the OOM panic is caught,
// the `CString` survives with `inner.len() == 0`, and *any* later use is
// UB without a further panic. Replace `main` with the body below to see
// Miri flag `as_bytes()` instead of `Drop`:
//
// use std::panic::{catch_unwind, AssertUnwindSafe};
//
// set_alloc_error_hook(|l| panic!("oom: {} bytes", l.size()));
// let mut dest = CString::new("x").unwrap();
// let src: &CStr = c"this string is long enough to force the Vec to reallocate";
//
// FAIL_NEXT.store(true, Ordering::SeqCst);
// let _ = catch_unwind(AssertUnwindSafe(|| src.clone_into(&mut dest)));
//
// // CString::as_bytes does
// // self.inner.get_unchecked(..self.inner.len() - 1)
// // i.e. `..usize::MAX` on a dangling pointer.
// let _boom = dest.as_bytes(); |
This comment has been minimized.
This comment has been minimized.
3eede0e to
b4e013b
Compare
| } | ||
|
|
||
| fn clone_into(&self, target: &mut CString) { | ||
| let mut b = mem::take(&mut target.inner).into_vec(); |
There was a problem hiding this comment.
Would it be okay to just wrap this around another mem::take?
If we mem::take on the &mut CString we're given, it'll replace the target CString with c"".to_owned() as that's the default impl of CString. And then we can mem::take to grab the inner Box<[u8]> and turn it into a Vec, preserving the rest of the code that we had before.
With this a panic midway would still keep the target CString in a valid state (an empty string with nul byte or what it contained previously).
There was a problem hiding this comment.
Yes, we could. I wasn't sure if that actually would be cleaner, though.
There was a problem hiding this comment.
Hmm, I feel like I would prefer a smaller change like that with a comment above saying why we need to do this.
I think it's slightly cleaner seeing the double mem::take than seeing in the else condition:
mem::replace(&mut target.inner, Box::new([0])).into_vec()). We also avoid a bit of redundant code like self.to_bytes_with_nul() and not needing to use target.inner.copy_from_slice(src) because we're reusing the space from our CString's inner Box<[u8]>.
There was a problem hiding this comment.
What about something like the following, to avoid the happy-path allocation:
fn clone_into(&self, target: &mut CString) {
struct SetToNulOnDrop<'a>(&'a mut Box<[u8]>);
impl Drop for SetToNulOnDrop<'_> {
fn drop(&mut self) {
*self.0 = Box::new([0]);
}
}
let guard = SetToNulOnDrop(&mut target.inner);
let mut b = mem::take(guard.0).into_vec();
self.to_bytes_with_nul().clone_into(&mut b);
*guard.0 = b.into_boxed_slice();
mem::forget(guard);
}Edit: wait... I see a problem... no reason that Box::new([0]) should succeed....
uhhhh... I guess that necessitates an abort if Box::new([0]) fails? Still seems worth it to avoid the happy-path allocation.
There was a problem hiding this comment.
Possible fix:
fn clone_into(&self, target: &mut CString) {
struct SetToNulOnDrop<'a>(&'a mut Box<[u8]>);
impl Drop for SetToNulOnDrop<'_> {
fn drop(&mut self) {
if let Ok(nul) = Box::try_new([0]) {
*self.0 = nul;
} else {
rtabort!("failed to allocate 1-byte Box while restoring CString invariant");
}
}
}
let guard = SetToNulOnDrop(&mut target.inner);
let mut b = mem::take(guard.0).into_vec();
self.to_bytes_with_nul().clone_into(&mut b);
*guard.0 = b.into_boxed_slice();
mem::forget(guard);
}There was a problem hiding this comment.
Yeah basically any way of improving this gets really messy.
|
Is it legal for allocators to panic? They can abort, sure, but can they unwind? |
I haven't looked if the Allocator trait has the same guarantee but for CString I don't think it matters since it's not parameterized over an allocator anyway. So this doesn't seem like unsoundness after all? |
|
It does sound like we should get miri to detect allocator unwinding as the UB root cause rather than letting that spread... |
Strictly speaking, looking at the repro, it's not the allocator that panics; it's just that the allocator reports an error. It's
Unfortunately, looks like it is unsound, since the unwind is documented as intentionally possible: https://doc.rust-lang.org/std/alloc/fn.set_alloc_error_hook.html |
|
Ah so not in the allocator itself. This looks like another instance where a |
| } else { | ||
| // Reuse the existing allocation's capacity by converting to a Vec. | ||
| // We temporarily replace `target` with a valid dummy to remain panic-safe. | ||
| let mut b = mem::replace(&mut target.inner, Box::new([0])).into_vec(); |
There was a problem hiding this comment.
why not just directly realloc the target CString rather than trying to go through Vec? that way if the realloc fails, the CString is completely valid and you can just call the oom handler. if the realloc succeeds, you just need to copy the memory and your done.
That allows doing it without needing a temporary Box::new([0])
There was a problem hiding this comment.
a possible implementation (may need some adjusting):
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=80eda60df068a36c082182b69acc2ea4
pub struct CString(Box<[u8]>);
#[repr(transparent)]
pub struct CStr([u8]);
impl ToOwned for CStr {
...
fn clone_into(&self, target: &mut CString) {
if self.0.len() != target.0.len() {
unsafe {
let target = &raw mut target.0;
let old_layout = Layout::for_value(&**target);
let new = realloc(&raw mut **target as *mut u8, old_layout, self.0.len());
if new.is_null() {
handle_alloc_error(Layout::for_value(&self.0));
} else {
// don't drop the old box, we already deallocated it with the realloc above
target.write(Box::from_raw(ptr::slice_from_raw_parts_mut(
new,
self.0.len(),
)));
}
}
}
target.0.copy_from_slice(&self.0);
}
}There was a problem hiding this comment.
Yeah so I considered doing this and decided not to because I didn't really want to replace some broken code with a lot of unsafe.
I'm having a hard time figuring out a way to do this safely and simply. I think if the libs team prefers having this be manual, it's a good idea, but I want to give them the opportunity to ask for it.
There was a problem hiding this comment.
well, if you don't want to bother with realloc, it seems simpler to just do:
fn clone_into(&self, target: &mut CString) {
if self.count_bytes() == target.count_bytes() {
target.inner.copy_from_slice(self.to_bytes_with_nul());
} else {
*target = self.to_owned();
}
}There was a problem hiding this comment.
I didn't want to bother with manually calling realloc. My current solution preserves realloc behavior without adding tricky unsafe code. I'm happy to explore the more unsafe options if the libs team wants me to.
There was a problem hiding this comment.
I would agree with not manually calling realloc. To be clear, I would love to have a more general approach to handling fallible allocations but I don't think this is the time to introduce it and I would strongly prefer to give it a safe interface (such as the hypothetical FallibleVec I mentioned earlier) so we're not unnecessarily sprinkling unsafe about the place.
There was a problem hiding this comment.
maybe we should add a safe Box::<[T]>::realloc?
|
The Vec to boxed slice conversion must reallocate anyway (since boxed slice loses capacity information, which is necessary for freeing, |
That's what I was initially doing, but someone pointed out that |
On the test question - i'd look at vec_deque_alloc_error.rs as the model: same |
|
@bors r+ rollup I think we can merge this in and iterate in the future on alternatives. |
…uwer Rollup of 15 pull requests Successful merges: - #152995 (ACP Implementation of PermissionsExt for Windows ) - #153457 (prevent deref coercions in `pin!`) - #155250 (Windows: Cache the pipe filesystem handle) - #155574 (Move `std::io::RawOsError` to `core::io`) - #155757 (macro_metavar_expr_concat: explain why idents are invalid) - #155823 (miri subtree update) - #155693 (Suggest enclosing format string with `""` under special cases) - #155707 (Fix minor panic-unsoundness in CString::clone_into) - #155719 (Suggest `.iter()` for shared projections) - #155779 (ssa_range_prop: use `if let` guards) - #155789 (Cleanups to `AttributeExt`) - #155805 (Mention `DEPRECATED_LLVM_INTRINSIC` lint for internal use) - #155806 (Remove the incomplete marker from `impl` restrictions) - #155820 (Avoid improper spans when `...` or `..=` is recovered from non-ASCII) - #155822 (Add default field values to diagnostic FormatArgs)
Rollup merge of #155707 - Manishearth:cstring-vuln, r=Mark-Simulacrum Fix minor panic-unsoundness in CString::clone_into `CString` must always contain a null byte, calling `mem::take` on its inner allocation puts it in an invalid state (causing UB if e.g. it hits `CString::drop`) that can be observed if the allocator panics. Unfortunately, this solution allocates an intermediate 1-element `Box`. I'm not sure of a clean way to avoid that additional allocation; we could directly `realloc` if we want but it's tricky. Might be something we can do with `ManuallyDrop`. I do have a gnarly miri test for this that uses a panicky allocator, but I'm not sure where it would go. Happy to push it up if someone has a suggestion. Bug discovered by Rust Foundation Security using AI. I'm just helping with the patch as a member of wg-security-response. We do not believe this bug needs embargo, it is a soundness fix for hard-to-trigger unsoundness.
…uwer Rollup of 15 pull requests Successful merges: - rust-lang/rust#152995 (ACP Implementation of PermissionsExt for Windows ) - rust-lang/rust#153457 (prevent deref coercions in `pin!`) - rust-lang/rust#155250 (Windows: Cache the pipe filesystem handle) - rust-lang/rust#155574 (Move `std::io::RawOsError` to `core::io`) - rust-lang/rust#155757 (macro_metavar_expr_concat: explain why idents are invalid) - rust-lang/rust#155823 (miri subtree update) - rust-lang/rust#155693 (Suggest enclosing format string with `""` under special cases) - rust-lang/rust#155707 (Fix minor panic-unsoundness in CString::clone_into) - rust-lang/rust#155719 (Suggest `.iter()` for shared projections) - rust-lang/rust#155779 (ssa_range_prop: use `if let` guards) - rust-lang/rust#155789 (Cleanups to `AttributeExt`) - rust-lang/rust#155805 (Mention `DEPRECATED_LLVM_INTRINSIC` lint for internal use) - rust-lang/rust#155806 (Remove the incomplete marker from `impl` restrictions) - rust-lang/rust#155820 (Avoid improper spans when `...` or `..=` is recovered from non-ASCII) - rust-lang/rust#155822 (Add default field values to diagnostic FormatArgs)
View all comments
CStringmust always contain a null byte, callingmem::takeon its inner allocation puts it in an invalid state (causing UB if e.g. it hitsCString::drop) that can be observed if the allocator panics.Unfortunately, this solution allocates an intermediate 1-element
Box. I'm not sure of a clean way to avoid that additional allocation; we could directlyreallocif we want but it's tricky. Might be something we can do withManuallyDrop.I do have a gnarly miri test for this that uses a panicky allocator, but I'm not sure where it would go. Happy to push it up if someone has a suggestion.
Bug discovered by Rust Foundation Security using AI. I'm just helping with the patch as a member of wg-security-response. We do not believe this bug needs embargo, it is a soundness fix for hard-to-trigger unsoundness.