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

Maybe unsound in ffi::BitmapBgra pixels field #655

Open
lwz23 opened this issue Dec 24, 2024 · 0 comments
Open

Maybe unsound in ffi::BitmapBgra pixels field #655

lwz23 opened this issue Dec 24, 2024 · 0 comments

Comments

@lwz23
Copy link

lwz23 commented Dec 24, 2024

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub struct BitmapBgra {
    ..................
    /// pointer to pixel 0,0; should be of length > h * stride
    pub pixels: *mut u8,
...................
}

pub fn short_hash_pixels(&self) -> u64{
        use std::hash::Hasher;

        let width_bytes = self.w as usize *  self.fmt.bytes();

        let mut hash = ::twox_hash::XxHash::with_seed(0x8ed1_2ad9_483d_28a0);
        for h in 0isize..(self.h as isize){
            let row_slice = unsafe{ slice::from_raw_parts(self.pixels.offset(h * self.stride as isize), width_bytes) };
            hash.write(row_slice)
        }
        hash.finish()
    }

Considering that pub mod ffi, BitmapBgra is a pub struct and pixels is a pub field and short_hash_pixels is also a pub function. I assume that users can directly call this function. This potential situation could result in unsafe{ slice::from_raw_parts(self.pixels.offset(h * self.stride as isize), width_bytes) } being operating on a invalid pointer, I suppose it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
I suggest Several possible fixes:

  1. If there is no external usage for BitmapBgra, it should not marked as pub at least its pixels field should not be pub.
  2. short_hash_pixels method should add additional check for valid pointer.
  3. mark short_hash_pixels method as unsafe and proper doc to let users know that they should provide valid Pointers.
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