cogciprocate / ocl

OpenCL for Rust
Other
721 stars 75 forks source link

double drop may happen upon panic in `EventList as std::convert::From<[E; n]>>::from` #194

Closed JOE1994 closed 5 months ago

JOE1994 commented 3 years ago

Hello :crab: , 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

We found 2 cases (below) where a double drop of an objects can happen if a panic occurs within the user-provided Into<Event> implementation.

https://github.com/cogciprocate/ocl/blob/03086863e95e43033ae67f1530801cb57032e1a3/ocl/src/standard/event.rs#L1000-L1014

https://github.com/cogciprocate/ocl/blob/03086863e95e43033ae67f1530801cb57032e1a3/ocl/src/standard/event.rs#L1037-L1050

Proof of Concept

The example program below exhibits a double-drop.

use fil_ocl::{Event, EventList};
use std::convert::Into;

struct Foo(Option<i32>);

impl Into<Event> for Foo {
    fn into(self) -> Event {
        /*
        According to the docs, `Into<T>` implementations shouldn't panic.
        However rustc doesn't check whether panics can happen in the Into implementation,
        so it's possible for a user-provided `into()` to panic..
        */
        println!("LOUSY PANIC : {}", self.0.unwrap());

        Event::empty()
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("I'm dropping");
    }
}

fn main() {
    let eventlist: EventList = [Foo(None)].into();
    dbg!(eventlist);
}

Suggested Fix

In this case, using ManuallyDrop can help guard against the potential panic within into(). I'll submit a PR with the suggested fix right away.

Thank you for checking out this issue :+1:

eslerm commented 1 year ago

this issue was assigned CVE-2021-25908 (aka GHSA-x3v2-fgr6-3wmm and RUSTSEC-2021-0011)