Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined behaviour in xproto::change_property #94

Closed
nat-rix opened this issue Dec 21, 2020 · 2 comments
Closed

Undefined behaviour in xproto::change_property #94

nat-rix opened this issue Dec 21, 2020 · 2 comments

Comments

@nat-rix
Copy link

nat-rix commented Dec 21, 2020

xproto::change_property<T> takes the arguments format: u8 and a data: &[T].
The currently generated code

pub fn change_property<'a, T>(c       : &'a base::Connection,
                              mode    : u8,
                              window  : Window,
                              property: Atom,
                              type_   : Atom,
                              format  : u8,
                              data    : &[T])
        -> base::VoidCookie<'a> {
    unsafe {
        let data_len = data.len();
        let data_ptr = data.as_ptr();
        let cookie = xcb_change_property(c.get_raw_conn(),
                                         mode as u8,  // 0
                                         window as xcb_window_t,  // 1
                                         property as xcb_atom_t,  // 2
                                         type_ as xcb_atom_t,  // 3
                                         format as u8,  // 4
                                         data_len as u32,  // 5
                                         data_ptr as *const c_void);  // 6
        base::VoidCookie {
            cookie:  cookie,
            conn:    c,
            checked: false
        }
    }
}

does not check for the case core::mem::size_of::<T>() * 8 != format, so a user could do something like this:

xcb::xproto::change_property(con, mode, win, property, type, 32, [0u8; 4]).request_check();

xcb_change_property will assume to get 128 bits of data in data_ptr, but instead gets only 32 bits.
Reading the last 12 bytes from data_ptr would cause UB.
Also it is not desirable to get types, that are bigger than format.
We could make xproto::change_property unsafe or panic! in the case of core::mem::size_of::<T>() * 8 != format.
Another option would be to only allow u8, u16 and u32, since other formats are not allowed by xcb_change_property.

mod sealed { pub trait Num { fn bits() -> u8; } }
impl sealed::Num for u8 { fn bits() -> u8 { 8 } }
impl sealed::Num for u16 { fn bits() -> u8 { 16 } }
impl sealed::Num for u32 {  fn bits() -> u8 { 32 } }
pub fn change_property<'a, T: sealed::Num>(..., type_: Atom, data: &[T]) -> ... {
    let format = T::bits();
    unsafe { ... }
}
@rtbo
Copy link
Collaborator

rtbo commented Nov 7, 2021

This will be fixed with #105.
The new API is different but takes an approach similar to the proposed by this issue:

    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;
    }

#[derive(Clone, Debug)]
pub struct ChangeProperty<'a, P: PropEl> {
    pub mode: PropMode,
    pub window: Window,
    pub property: Atom,
    pub r#type: Atom,
    pub data: &'a [P],
}

@rtbo
Copy link
Collaborator

rtbo commented Mar 6, 2022

v1.0.0 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants