Soundness issue with base::Error

[ammaraskar]

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that xcb exports the xcb::base::Error struct publicly:

https://github.com/rtbo/rust-xcb/blob/b44e52845e21621613d57bf5eb47c48c2ad521dd/src/base.rs#L130-L158

Since the struct and its fields are public, it can be created from safe Rust code which can potentially leading to use-after-frees and double-frees. Maybe this struct should have a hidden constructor or be marked unsafe?

Here's a proof-of-concept showing a use-after-free with this struct:

#![forbid(unsafe_code)]

use xcb::base::Error;

fn main() {
    let mut v1: Vec<i8> = vec![1, 2, 3, 0];
    let _ = Error {
        ptr: v1.as_mut_ptr(),
    };

    // use-after-free in v1
    // v1 and v2 are now backed by the same buffer.
    let v2: Vec<i8> = vec![4, 5, 6, 0];

    let measure1 = v2[0];
    v1[0] = 123;
    let measure2 = v2[0];

    assert_eq!(measure1, measure2);
}

This outputs:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `123`', src/main.rs:32:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
free(): double free detected in tcache 2

Terminated with signal 6 (SIGABRT)

[rtbo]

This is addressed in the new API. See v1.0.0-beta.0 release in the v1.0-dev branch.
In a nutshell, error management is now defined as follow:

// at crate level:

pub enum ProtocolError {
    X(x::Error),
    Xkb(xkb::Error),
    // other extensions errors 
}

pub enum Error {
    Conn(ConnError),
    Protocol(ProtocolError),
}

pub type ProtocolResult<T> = std::Result<T, ProtocolError>;
pub type Result<T> = std::Result<T, Error>;

impl Connection {
    pub fn wait_for_event(&self) -> Result<Event> { /* ... */ }
    pub fn check_request(&self, cookie: CheckedVoidCookie) -> ProtocolResult<()> { /* ... */ }
    pub fn wait_for_reply<C>(&self, cookie: C) -> Result<C::Reply> { /* ... */ }
}

pub trait BaseError {
    /// Build an error from a raw pointer
    ///
    /// # Safety
    /// `raw` must be a valid pointer to the error, and be allocated with `libc::malloc`
    unsafe fn from_raw(raw: *mut xcb_generic_error_t) -> Self;

    /// Convert the error into a raw pointer
    ///
    /// # Safety
    /// returned value should be freed with `libc::free` to avoid memory leak, or used to build a new error
    unsafe fn into_raw(self) -> *mut xcb_generic_error_t;

    /// Obtain the error as a raw pointer
    fn as_raw(&self) -> *mut xcb_generic_error_t;
}


pub mod x {
    // auto generated module for the core X protocol

    pub struct RequestError {
        raw: *mut xcb_generic_error_t,
    }

    impl BaseError for RequestError {
        unsafe fn from_raw(raw: *mut xcb_generic_error_t) -> Self {
            RequestError { raw }
        }

        unsafe fn into_raw(self) -> *mut xcb_generic_error_t {
            let raw = self.raw;
            std::mem::forget(self);
            raw
        }

        fn as_raw(&self) -> *mut xcb_generic_error_t {
            self.raw
        }
    }

    impl RequestError {
        pub fn response_type(&self) -> u8 { /* ... */ }
        pub fn error_code(&self) -> u8 { /* ... */ }
        pub fn bad_value(&self) -> u32 { /* ... */ }
    }

    pub struct ValueError {
        raw: *mut xcb_generic_error_t,
    }

    impl BaseError for ValueError { /* ... */ }

    pub type WindowError = ValueError;
    pub type PixmapError = ValueError;

    /// Unified error type for the X core protocol
    #[derive(Debug)]
    pub enum Error {
        Request(RequestError),
        Value(ValueError),
        Window(WindowError),
        Pixmap(PixmapError),
        // ...
    }
}

and would generally be used as follow:

fn main() -> xcb::Result<()> {
    // ...
    conn.check_request(conn.send_request_checked(&x::ChangeProperty {
        mode: x::PropMode::Replace,
        window,
        property: wm_protocols,
        r#type: x::ATOM_ATOM,
        data: &[wm_del_window],
    }))?;  // <-- note '?' here

    loop {
        match conn.wait_for_event()? {  // <-- note '?' here
            // ...
        }
    }
}

[rtbo]

v1.0.0 is released.