Multiple panic safety issues

[ammaraskar]

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed a few panic safety issues in this library.

---

clone_from double-frees if T::clone panics

id-map/src/lib.rs

Lines 370 to 380 in a2fa8d4

	unsafe { self.drop_values() };
	self.ids.clone_from(&other.ids);
	let cap = other.values.capacity();
	self.values.reserve(cap);
	unsafe {
	for id in &self.ids {
	ptr::write(self.values.get_unchecked_mut(id),
	other.values.get_unchecked(id).clone());
	}
	}

The current values in the map are dropped and the ids are updated up front. This means that if other.values.get_unchecked(id).clone() panics, it can cause the previously dropped values to drop again.

---

get_or_insert double frees if insertion function f panics

id-map/src/lib.rs

Lines 169 to 180 in a2fa8d4

	// val was not previously in the map.
	if id == self.space {
	self.find_space();
	}
	if self.values.capacity() < id + 1 {
	self.values.reserve(id + 1);
	}
	unsafe {
	let space = self.values.get_unchecked_mut(id);
	ptr::write(space, f());
	space
	}

Since this reserves space for the value before calling ptr::write(space, f());, if f panics here, it can drop an already freed value.

---

remove_set double frees if drop panics

id-map/src/lib.rs

Lines 192 to 203 in a2fa8d4

	if let Some(first) = iter.next() {
	// Set iterators are increasing so we only need to change start once.
	self.space = cmp::min(self.space, first);
	unsafe {
	ptr::drop_in_place(self.values.get_unchecked_mut(first))
	}
	for id in iter {
	unsafe {
	ptr::drop_in_place(self.values.get_unchecked_mut(id))
	}
	}
	}

This code goes over to the ids to remove and calls drop_in_place on them. However if the drop function for the type panics, the element gets dropped again when the IdMap is dropped.

---

Code to recrate these problems is here:

#![forbid(unsafe_code)]

use id_map::IdMap;
use id_set::IdSet;

struct DropDetector(u32);

impl Drop for DropDetector {
    fn drop(&mut self) {
        println!("Dropping {}", self.0);
    }
}
impl Clone for DropDetector {
    fn clone(&self) -> Self { panic!("Panic on clone!"); }
}

fn main() {
    //clone_from_panic_will_drop_invalid_memory();
    //get_or_insert_with_leaves_state_inconsistent();
    remove_set_leaves_state_inconsistent_if_drop_panics();
}

fn clone_from_panic_will_drop_invalid_memory() {
    let mut map = IdMap::new();
    map.insert(DropDetector(1));

    let mut map_2 = IdMap::new();
    map_2.insert(DropDetector(2));
    map_2.clone_from(&map);
}

fn get_or_insert_with_leaves_state_inconsistent() {
    let mut map : IdMap<DropDetector> = IdMap::with_capacity(0);
    map.get_or_insert_with(0, || panic!("Panic in insertion function!"));
}

struct PanicsOnDrop(u32, bool);

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

        if (self.1) {
            self.1 = false;
            panic!("Panicking on drop");
        }
    }
}

fn remove_set_leaves_state_inconsistent_if_drop_panics() {
    let mut map = IdMap::new();
    map.insert(PanicsOnDrop(1, true));

    map.remove_set(&IdSet::new_filled(1));
}

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