Skip to content

double drop can happen upon panic within user provided impl of `Endian` trait

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

If a user-provided implementation of Endian panics, a double-drop error happens since the ownership of an object is unsafely duplicated by ptr::read(elt).

https://gitlab.com/myrrlyn/endian_trait/-/blob/master/src/slices.rs#L7-33

/// Traverse a slice, performing the `Endian` method on each item in place.
impl<'a, T: Endian> Endian for &'a mut [T] {
	fn from_be(self) -> Self {
		for elt in self.iter_mut() { unsafe {
			ptr::write(elt, ptr::read(elt).from_be());
		} }
		self
	}
	fn from_le(self) -> Self {
		for elt in self.iter_mut() { unsafe {
			ptr::write(elt, ptr::read(elt).from_le());
		} }
		self
	}
	fn to_be(self) -> Self {
		for elt in self.iter_mut() { unsafe {
			ptr::write(elt, ptr::read(elt).to_be());
		} }
		self
	}
	fn to_le(self) -> Self {
		for elt in self.iter_mut() { unsafe {
			ptr::write(elt, ptr::read(elt).to_le());
		} }
		self
	}
}

Proof of Concept

Below example program exhibits heap corruption upon a panic within the user-provided Endian implementation.

use endian_trait::Endian;

#[derive(Debug)]
struct Foo(Box<Option<i32>>);

impl Endian for Foo {
    fn to_be(self) -> Self {
        println!(
            "PANIC BY MISTAKE {}",
            self.0.as_ref().as_ref().unwrap()
        );
        self
    }
    fn to_le(self) -> Self { self }
    fn from_be(self) -> Self { self }
    fn from_le(self) -> Self { self }
}

fn main() {
    let mut foo = [Foo(Box::new(None))];
    let x = (&mut foo).to_be();
    dbg!(x);
}

When the below program is executed on Windows 10, the output was as below

error: process didn't exit successfully: `target\debug\garbage.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)

Suggested Fix

The fact that the APIs defined on Endian trait take owned self as input makes it quite difficult to come up with a neat code fix for this issue.

A solution I can think of is as below.

  1. Make the Endian trait an unsafe trait.
  2. Add a warning to the documentation about implementing the Endian trait: users are responsible to make sure no panics happen within the implementation of Endian.

Thank you for checking out this issue 👍