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

Various valgrind errors related to uninitialized memory #196

Open
ids1024 opened this issue Apr 30, 2024 · 2 comments
Open

Various valgrind errors related to uninitialized memory #196

ids1024 opened this issue Apr 30, 2024 · 2 comments

Comments

@ids1024
Copy link
Member

ids1024 commented Apr 30, 2024

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
Copy link
Member Author

ids1024 commented Apr 30, 2024

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

@ids1024
Copy link
Member Author

ids1024 commented May 6, 2024

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. #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 #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.

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

No branches or pull requests

1 participant