double free of T upon panic in two functions

[JOE1994]

Hello,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

Public functions through::through() and through::through_and() are not panic-safe.
Both take a user-provided closure as a parameter, and a double free of T will happen
if the user-provided closure panics.

Reproduction

Below is an example program that exhibits undefined behavior using safe APIs of through.

Show Detail

#![forbid(unsafe_code)]
use through::through;

fn main() {
    let mut hello = String::from("Hello");
    let object = through(&mut hello, |mut s| {
        s.push_str(" World!");
        panic!("Unexpected panic");
        s
    });
    dbg!(object);
}

Output:

thread 'main' panicked at 'Unexpected panic', src/main.rs:22:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
free(): double free detected in tcache 2

Terminated with signal 6 (SIGABRT)

Tested Environment

• Crate: through
• Version: 0.1.0
• OS: Ubuntu 18.04.5 LTS
• Rustc version: rustc 1.50.0 (cb75ad5db 2021-02-10)

[Shnatsel]

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

[gretchenfrage]

Thank you for bringing this to my attention, even though I didn't see this until recently. I am simply yanking this crate as I don't see a way to fix it without defeating the point of it (the point being that it's convenient).

Rejected conceivable fixes:

• make through unsafe (require user code to not panic)
• catch panics, somehow handle them but just not returning (abort process? sleep forever?)

Recommended alternatives:

• the Rust compiler may allow you to do a *foo = bar(*foo) in some situations
• wrap your type in an Option, and do foo = Some(bar(foo.take().unwrap()))
• more generally, if your type implements Default in a way that is trivially cheap, eg with Vec or String, do foo = bar(std::mem::take(&mut foo))
• if none of the above are acceptable for whatever extreme performance reason or something, just use raw pointers, and make sure your code doesn't panic