Smithay / drm-rs

A low-level abstraction of the Direct Rendering Manager API
MIT License
77 stars 51 forks source link

Various valgrind errors related to uninitialized memory #196

Open ids1024 opened 4 months ago

ids1024 commented 4 months ago

When running a smithay compositor, or an example like list_modes in valgrind, various uninitialized memory errors are reported in drm-rs, like Syscall param ioctl(generic) points to uninitialised byte(s) and Conditional jump or move depends on uninitialised value(s).

I think Valgrind may be overly strict about arbitrary ioctls that it isn't familiar with, but it would be good to fix or suppress this in some way so that valgrind can be used for testing without dealing with a bunch of spurious messages.

It might make sense to change things like map_reserve! to zero initialize memory.

ids1024 commented 4 months ago

I guess to supress the first warning, padding bytes in structs also need to be initialized.

ids1024 commented 4 months ago

Things like mem::zeroed() and MaybeUninit::zeroed() don't seem to actually initialize padding bytes either. But ptr::write_bytes can be used to memset the bytes to 0. https://github.com/Smithay/drm-rs/pull/197 does this for the dynamic allocations, where it is fairly clean.

This is more awkward with the stack allocated ioctl parameters. This seems to work, but is not... ideal:

pub struct ZeroInit<T: Copy> {
    inner: Box<std::mem::MaybeUninit<T>>,
}

impl<T: Copy> ZeroInit<T> {
    pub unsafe fn new() -> Self {
        // Using `MaybeUninit::zeroed` doesn't initialize padding bytes
        let mut inner = Box::new(std::mem::MaybeUninit::uninit());
        (inner.as_mut_ptr() as *mut _ as *mut u8).write_bytes(0, std::mem::size_of::<T>());
        Self { inner }
    }
}

impl<T: Copy> ops::Deref for ZeroInit<T> {
    type Target = T;
    fn deref(&self) -> &T {
        unsafe { &*self.inner.as_ptr() }
    }
}

impl<T: Copy> ops::DerefMut for ZeroInit<T> {
    fn deref_mut(&mut self) -> &mut T {
        unsafe { &mut *self.inner.as_mut_ptr() }
    }
}

I think Box is necessary to do this and also abstract it, since copying won't initialize the padding. Otherwise I guess a macro could work...

With the change in https://github.com/Smithay/drm-rs/pull/197, there seems to only be the Syscall param ioctl(generic) points to uninitialised byte(s) errors. Not sure if valgrind has a way to supress those.

Well, at least I have something to test Smithay/cosmic-comp in valgrind. I'll see what else I find doing that.