xproto::GetPropertyReply::value() is unsound

[psychon]

The below is pure unsoundness. The caller can specify any type they like. How about bool or an enum? Or something with a pointer like Vec or String or &[u8] for extra memory-fun? Edit: To explain this some more: A bool is either 0 or 1. If a variable of type bool contains any other value, that's undefined behaviour aka "that must not happen".

impl GetPropertyReply {
[...]
    pub fn value<T>(&self) -> &[T] {
        unsafe {
            let field = self.ptr;
            let len = xcb_get_property_value_length(field) as usize;
            let data = xcb_get_property_value(field);
            debug_assert_eq!(len % std::mem::size_of::<T>(), 0);
            std::slice::from_raw_parts(data as *const T, len / std::mem::size_of::<T>())
        }
    }
}

[Shnatsel]

While the Rust standard library does not yet provide the necessary traits for marking values that can be safely created from arbitrary byte sequences, this functionality is provided by a crate: https://github.com/Lokathor/bytemuck

Anything that implements bytemuck::Pod is safe to convert to/from bytes. If the data needs to be converted into types that are not Pod, the underlying data must be checked for validity.

[rtbo]

In the new v1.0 API:

/// Trait for element in a property list
///
/// In events (e.g. `GetProperty::value`), it allows to assert that the format
/// correspond to the type cast and therefore to do the cast safely at runtime.
///
/// In request (e.g. `ChangeProperty::data`), it allows to infer the format value
/// from the type of passed data.
pub trait PropEl {
    const FORMAT: u8;
}

impl PropEl for u8 {
    const FORMAT: u8 = 8;
}

impl PropEl for u16 {
    const FORMAT: u8 = 16;
}

impl PropEl for u32 {
    const FORMAT: u8 = 32;
}

impl PropEl for Atom {
    const FORMAT: u8 = 32;
}

impl GetPropertyReply {
    pub fn value<P: PropEl>(&self) -> &[P] {
        assert_eq!(
            self.format(),
            P::FORMAT,
            "mismatched format of xproto::GetPropertyReply::value"
        );
        unsafe {
            let offset = 32usize;
            let len = ((self.value_len() as usize) * ((self.format() as usize) / 8usize));
            let len = len / std::mem::size_of::<P>();
            let ptr = self.wire_ptr().add(offset) as *const P;
            std::slice::from_raw_parts(ptr, len)
        }
    }
}

[rtbo]

v1.0.0 is released.

[Shnatsel]

Thank you for all the fixes and the v1.0 release! 🎉

I've opened a PR to mark all the soundness issues tracked by RustSec as fixed: rustsec/advisory-db#1206