Suspiciosly sound safe functions. Break them!

Basically the functions are showed below. I couldn't come up with a legit counter-example to cause some sort of UB. Initially i came up with these functions, but with Q in-bounds of T check. I call these "backup" functions, bc i think i have proof in mind.

pub fn partial_replace<T: 'static, Q: 'static, F>(t: &mut T, f: F, x: T) -> Result<Q, T>
where
    F: FnOnce(&mut T) -> Option<&mut Q>,
{
    partial_replace_with(t, f, || x).map_err(|e| e())
}

pub fn partial_replace_with<T: 'static, Q: 'static, F, G>(t: &mut T, f: F, x: G) -> Result<Q, G>
where
    F: FnOnce(&mut T) -> Option<&mut Q>,
    G: FnOnce() -> T,
{
    let t = t as *mut T;
    let q = match f(unsafe { &mut *t }) {
        Some(q) => q,
        None => return Err(x),
    };

    let x = x();
    let q = unsafe { (q as *mut Q).read() };
    // leak previous but now invalid value
    unsafe { t.write(x) };

    Ok(q)
}

Here's also a link to playground with an example and these "backup" functions:

EDIT: moved description to the top
EDIT: fixed as proposed by @steffahn, top doc comment in the last version playground has a link to previous versions

BTW maybe someone seen something like this somewhere, please let me know.

fn main() {
    let mut x = String::from("hello world");
    // call with `T`=`&mut String`
    let s: String = partial_replace(&mut &mut x, |x| Some(*x), &mut String::new()).unwrap();
    println!("{s}");
    drop(x);
    println!("{s}"); // use-after-free
}
8 Likes

It would be helpful (at least me) if you would say that the *x is actually &mut **x. The seemingly move confused me at first.

1 Like

I left that as an exercise for the reader :wink: , I agree that *x’s special support for &mut T is somewhat magical :stars:

1 Like

I’m wondering… perhaps a function &'static mut T -> T is sound, in which case, one could implement something similar to partial_replace_with as follows:

use replace_with::replace_with_or_abort_and_return;
use std::ptr;

pub fn read_static<T>(r: &'static mut T) -> T {
    unsafe { ptr::read(r) }
}

pub fn partial_replace_with<T: 'static, Q: 'static, F, G>(t: &mut T, f: F, x: G) -> Result<Q, G>
where
    F: FnOnce(&mut T) -> Option<&mut Q>,
    G: FnOnce() -> T,
{
    replace_with_or_abort_and_return(t, |t| {
        let t = Box::leak(Box::new(t));
        match f(t) {
            Some(q) => (Ok(read_static(q)), x()),
            None => (Err(x), read_static(t)),
        }
    })
}

which compiles with polonius:

cargo +nightly rustc -- -Zpolonius

This leaks memory of course (from the Box), and it doesn’t handle panicking very nicely; the implementation is supposed to be a proof-of-concept / i.e. a sanity check for whether the type signature can be safely implemented.

This serves as an indication that your function (the version without bounds checks) might actually be sound, if you add T: 'static and Q: 'static bounds, which might still cover your intended use-case.

2 Likes

I'm on my way and too tired to understand what the function is doing, but using unsafe to circumvent the borrow checker almost always means that you are doing something you shouldn't be doing, and the result is UB with overwhelming probability.

So I'll ask instead: what are you trying to achieve? There is certainly a better (and safe) way of doing it.

4 Likes

Thank you very much for sharing these thoughts and a break solution (uh, i was almost there). What would you say about backup functions?

So i believe fixed functions and backup ones are pretty much sound. Perhaps i will publish a crate and share it there.

This problem was formulated by a different person, as he would have liked to eliminate unreachable!() in his code. You can look at it inside of the playground. I commented out the initial approach with double pattern matching with unreachable!(), and added proposed solution via partial_replace_with.

I am not trying to do anything except to push the limits of safe rust there.

I think you're referring to the version with the in-bounds check, right? I haven't found a way to break those yet, so those might be solid.

1 Like

Could you provide a link to the playground with the original code that you wanted to simplify? The mishmash of old and new code is a bit confusing.

Top doc comment in the last version playground has a link to previous versions.

It's not that different. Do I understand correctly that the problematic code is just the snippet with a match in the comments? I don't really understand what problem you are solving, that snippet looks ok. If you want to use unsafe, you can turn unreachable!() into std::hint::unreachable_unchecked(). It will result in obviously correct code and no extra branches.

he would have liked to eliminate unreachable!() in his code

Can everyone please not assume some underlying problem? You are not helpful. I only asked about a couple of functions i wrote, if they are sound or not. I am not trying to do anything except to write these safe functions properly.