Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Panic safety issue leading to double-drop in insert_slice_clone #3

Closed
ammaraskar opened this issue Feb 3, 2021 · 2 comments
Closed
Labels

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that in insert_slice_clone, it pushes elements over for insertion using ptr::copy here:

unsafe {
{
let mut p = self.as_mut_ptr().add(index);
ptr::copy(p, p.add(slen), vlen - index);
for v in slice {
ptr::write(p,v.clone());
p = p.offset(1);
}
}
self.set_len(dlen);

This can duplicate items in the Vec, after which it calls v.clone(), which can potentially panic. This means that during this panic the copied elements can be dropped twice. Here's a quick example of this issue:

#![forbid(unsafe_code)]

use qwutils::*;

// Type with a Clone() implementation that panics.
struct DropDetector(u32);

impl Drop for DropDetector {
    fn drop(&mut self) {
        println!("Dropping {}", self.0);
    }
}

impl Clone for DropDetector {
    fn clone(&self) -> Self {
        panic!("DropDetector {} panic on clone()", self.0);
    }
}


fn main() {
    let mut a = vec![DropDetector(1), DropDetector(2)];
    let b = [DropDetector(3)];

    a.insert_slice_clone(0, &b);
}

This outputs:

thread 'main' panicked at 'DropDetector 3 panic on clone()', src/main.rs:29:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Dropping 3
Dropping 1
Dropping 1

As you can see the first element is being dropped twice.

We'd recommend wrapping the vector in ManuallyDrop while you're operating on it to prevent this.

qwertz19281 added a commit that referenced this issue Feb 4, 2021
- Vec::insert_slice_clone: fix unsound Vec after T::clone panic (#3) (20cb73d)
- arc_slice: fix is_unsliced (0107a92)
- arc_slice: optimization for unwrap (465e87f)
@qwertz19281
Copy link
Owner

Fix implemented in 0.3.1 (20cb73d)

Props to @ammaraskar and @sslab-gatech for their efforts in scanning and security

@ammaraskar
Copy link
Author

Thank you for the quick fix, it looks good to me :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants