Skip to content

Fix minor panic-unsoundness in CString::clone_into#155707

Merged
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Manishearth:cstring-vuln
Apr 26, 2026
Merged

Fix minor panic-unsoundness in CString::clone_into#155707
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Manishearth:cstring-vuln

Conversation

@Manishearth
Copy link
Copy Markdown
Member

@Manishearth Manishearth commented Apr 24, 2026

View all comments

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.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 24, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 24, 2026

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 6 candidates
  • Random selection from Mark-Simulacrum, jhpratt

@Manishearth
Copy link
Copy Markdown
Member Author

Manishearth commented Apr 24, 2026

Here is the AI-generated report for this

Details

CString invariant broken during clone_into across unwind

Details

<CStr as ToOwned>::clone_into calls mem::take(&mut target.inner), which replaces the CString's backing Box<[u8]> with Default::default() — an empty boxed slice. This temporarily violates CString's documented invariant that inner always has length >= 1 with a trailing NUL. The function then performs allocating operations (<[u8]>::clone_into may grow the Vec; into_boxed_slice may shrink it). If either allocation fails and the allocation-error handler unwinds rather than aborts, target is dropped during unwinding while inner is still the empty box. impl Drop for CString (line 706) then executes *self.inner.get_unchecked_mut(0) = 0, an unchecked write to index 0 of a zero-length slice — undefined behavior (a store through NonNull::dangling()). On stable Rust this is unreachable because handle_alloc_error always aborts; it is reachable only under the unstable nightly -Zoom=panic configuration (or any future stable unwinding-OOM mode).

Location

https://github.com/rust-lang/rust/blob/main/library/alloc/src/ffi/c_str.rs#L1080

Impact

Out-of-bounds write through dangling pointer during unwinding

Reproduction steps

  1. A program is compiled with nightly Rust and -Zoom=panic. It calls source.clone_into(&mut dest) where source: &CStr is larger than dest's current capacity. An attacker drives the process to memory exhaustion so the internal Vec::reserve fails. handle_alloc_error panics; while unwinding, dest is dropped with a zero-length inner, and Drop performs an out-of-bounds write through a dangling pointer. This is undefined behavior; in practice it dereferences address 1 and crashes, but formally the compiler may miscompile surrounding code arbitrarily.

Recommended fix

The 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();

@rust-log-analyzer

This comment has been minimized.

}

fn clone_into(&self, target: &mut CString) {
let mut b = mem::take(&mut target.inner).into_vec();
Copy link
Copy Markdown
Contributor

@asder8215 asder8215 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could. I wasn't sure if that actually would be cleaner, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>.

Copy link
Copy Markdown

@robofinch robofinch Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah basically any way of improving this gets really messy.

@ChrisDenton
Copy link
Copy Markdown
Member

Is it legal for allocators to panic? They can abort, sure, but can they unwind?

@Mark-Simulacrum
Copy link
Copy Markdown
Member

It’s undefined behavior if global allocators unwind. This restriction may be lifted in the future, but currently a panic from any of these functions may lead to memory unsafety.

https://doc.rust-lang.org/nightly/std/alloc/trait.GlobalAlloc.html#safety

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?

@Mark-Simulacrum
Copy link
Copy Markdown
Member

It does sound like we should get miri to detect allocator unwinding as the UB root cause rather than letting that spread...

@robofinch
Copy link
Copy Markdown

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.

Strictly speaking, looking at the repro, it's not the allocator that panics; it's just that the allocator reports an error. It's handle_alloc_error which unwinds.

So this doesn't seem like unsoundness after all?

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

@ChrisDenton
Copy link
Copy Markdown
Member

Ah so not in the allocator itself. This looks like another instance where a FallibleVec type would be useful so that we can be explicit about handling alloc errors. Even if we ultimately do panic, it'd be good to be able to control where that panic happens. But that's well beyond the scope of this PR.

} 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();
Copy link
Copy Markdown
Member

@programmerjake programmerjake Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add a safe Box::<[T]>::realloc?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe!

@haileys
Copy link
Copy Markdown

haileys commented Apr 24, 2026

The Vec to boxed slice conversion must reallocate anyway (since boxed slice loses capacity information, which is necessary for freeing, into_boxed_slice calls shrink_to_fit) so if the lengths aren't equal why not fall back to the unspecialised path? It results in the same number of allocations and the logic is easier to understand

@Manishearth
Copy link
Copy Markdown
Member Author

The Vec to boxed slice conversion must reallocate anyway (since boxed slice loses capacity information, which is necessary for freeing, into_boxed_slice calls shrink_to_fit) so if the lengths aren't equal why not fall back to the unspecialised path? It results in the same number of allocations and the logic is easier to understand

That's what I was initially doing, but someone pointed out that realloc() is a thing. So many times it will actually not be reallocating.

@Vastargazing
Copy link
Copy Markdown
Contributor

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.

On the test question - i'd look at vec_deque_alloc_error.rs as the model: same set_alloc_error_hook + catch_unwind(AssertUnwindSafe(...)) + cfg_attr(not(panic = "unwind"), ignore) structure.
Since CString has no allocator param you'll need a #[global_allocator] wrapper (like in your PoC) rather than allocator_api.
Probably library/alloctests/tests/c_str_alloc_error.rs?

@Mark-Simulacrum
Copy link
Copy Markdown
Member

@bors r+ rollup

I think we can merge this in and iterate in the future on alternatives.

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Apr 26, 2026

📌 Commit b4e013b has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2026
rust-bors Bot pushed a commit that referenced this pull request Apr 26, 2026
…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)
@rust-bors rust-bors Bot merged commit 27aa1c5 into rust-lang:main Apr 26, 2026
11 checks passed
@rustbot rustbot added this to the 1.97.0 milestone Apr 26, 2026
rust-timer added a commit that referenced this pull request Apr 26, 2026
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.
pull Bot pushed a commit to LeeeeeeM/miri that referenced this pull request Apr 30, 2026
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants