Misbehaving `HandleLike` implementation can lead to memory safety violation

[qwaz]

crayon/src/utils/handle.rs

Lines 90 to 94 in 48d4e87

	pub trait HandleLike: Debug + Copy + Hash + PartialEq + Eq + Send + Sync {
	fn new(index: HandleIndex, version: HandleIndex) -> Self;
	fn index(&self) -> HandleIndex;
	fn version(&self) -> HandleIndex;
	}

crayon/src/utils/object_pool.rs

Lines 48 to 66 in 48d4e87

	/// Returns mutable reference to internal value with name `Handle`.
	#[inline]
	pub fn get_mut(&mut self, handle: H) -> Option<&mut T> {
	if self.handles.contains(handle) {
	unsafe { Some(self.entries.get_unchecked_mut(handle.index() as usize)) }
	} else {
	None
	}
	}
	/// Returns immutable reference to internal value with name `Handle`.
	#[inline]
	pub fn get(&self, handle: H) -> Option<&T> {
	if self.handles.contains(handle) {
	unsafe { Some(self.entries.get_unchecked(handle.index() as usize)) }
	} else {
	None
	}
	}

crayon/src/utils/object_pool.rs

Lines 164 to 174 in 48d4e87

	impl<H: HandleLike, T: Sized> Drop for ObjectPool<H, T> {
	fn drop(&mut self) {
	unsafe {
	for v in &self.handles {
	::std::ptr::drop_in_place(&mut self.entries[v.index() as usize]);
	}
	self.entries.set_len(0);
	}
	}
	}

Description

Unsafe code in ObjectPool has time-of-check to time-of-use (TOCTOU) bug that can eventually lead to a memory safety violation. ObjectPool and HandlePool implicitly assumes that HandleLike trait methods are pure, i.e., they always return the same value. However, this assumption is unsound since HandleLike is a safe, public trait that allows a custom implementation.

Demonstration

• Crate: crayon
• Version: 0.7.1
• OS: Ubuntu 18.04.5 LTS
• Rust: rustc 1.46.0 (04488afe3 2020-08-24)

#![forbid(unsafe_code)]

use crayon::utils::handle::{HandleIndex, HandleLike};
use crayon::utils::object_pool::ObjectPool;
use std::sync::atomic::{AtomicBool, Ordering};

#[derive(Debug)]
struct DropDetector(u32);

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

static FLAG: AtomicBool = AtomicBool::new(false);

#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
struct MyHandle {
    indices: [HandleIndex; 2],
    version: HandleIndex,
}

impl HandleLike for MyHandle {
    fn new(index: HandleIndex, version: HandleIndex) -> Self {
        MyHandle {
            indices: [index, index],
            version,
        }
    }

    fn index(&self) -> HandleIndex {
        if dbg!(FLAG.fetch_xor(true, Ordering::Relaxed)) {
            self.indices[1]
        } else {
            self.indices[0]
        }
    }

    fn version(&self) -> HandleIndex {
        self.version
    }
}

impl MyHandle {
    fn with_indices(indices: [HandleIndex; 2], version: HandleIndex) -> Self {
        MyHandle { indices, version }
    }
}

fn main() {
    let mut pool = ObjectPool::new();
    let real_handle: MyHandle = pool.create(123);
    let fake_handle =
        MyHandle::with_indices([real_handle.index(), 12345678], real_handle.version());

    // Segfault with OOB, accessing`pool.entries[12345678]` without boundary checking
    dbg!(pool.get(fake_handle));

    // The bug can be similarly triggered in all other methods of `ObjectPool`
    // that call `handle.index()` in an unsafe block.
}

Output:

[src/main.rs:57] FLAG.fetch_xor(true, Ordering::Relaxed) = false
[src/main.rs:57] FLAG.fetch_xor(true, Ordering::Relaxed) = true
[src/main.rs:57] FLAG.fetch_xor(true, Ordering::Relaxed) = false
[src/main.rs:57] FLAG.fetch_xor(true, Ordering::Relaxed) = true
[src/main.rs:82] pool.get(fake_handle) = Some(

Return Code: -11 (SIGSEGV)

[qwaz]

crayon/src/utils/object_pool.rs

Lines 75 to 87 in 48d4e87

	/// Recycles the value with name `Handle`.
	#[inline]
	pub fn free(&mut self, handle: H) -> Option<T> {
	if self.handles.free(handle) {
	unsafe {
	let mut v = ::std::mem::uninitialized();
	::std::mem::swap(&mut v, &mut self.entries[handle.index() as usize]);
	Some(v)
	}
	} else {
	None
	}
	}

Also, storing uninitialized T to Vec<T> is an undefined behavior even if it is not accessed AFAIK.