Does this leak memory or is there a better way to do this?

code review
1

Hello,

I'm building a small project where I'm using opencv-rs and manually link to raylib (for fun and learning rust).

It happens to be that one can copy the pointer to the data of an opencv mat to a raylib image and if the format is correct it works.

OpenCV mat is reference counted matrix and if no copy exists it deallocates the memory which I suspect is quite involved since many (image) formats are supported.

Raylib provides an UnloadImage function which accepts the raylib image struct, which holds a void pointer to the image data. The deallocation is also quite complex, for the same reasons as for the OpenCV mat.

Therefore I don't want to implement the deallocations myself, just organize the deallocation in a way no memory is leaked or pointer is double freed

I'v created the following structs, Image which wraps the FFI save Raylib Image (RImage)

pub struct Image {
    image: RImage,
    mat: Option<Mat>,
}

impl From<Mat> for Image {
    fn from(mat: Mat) -> Self {
        Self {
            image: (&mat).into(),
            mat: Some(mat),
        }
    }
}

impl Clone for Image {
    fn clone(&self) -> Self {
        Self {
            image: self.image.clone(),
            mat: None,
        }
    }
}

unsafe impl Send for Image {}

impl Drop for Image {
    fn drop(&mut self) {
        if let Some(_mat) = &mut self.mat {
            // following values won't be leaked
            let _ = self.image.data;
            let _ = self.image.width;
            let _ = self.image.height;
            let _ = self.image.mipmaps;
            let _ = self.image.format;
            println!("OpenCV mat unloaded")
        } else {
            unsafe {
                UnloadImage(RImage {
                    data: self.image.data,
                    width: self.image.width,
                    height: self.image.height,
                    mipmaps: self.image.mipmaps,
                    format: self.image.format,
                })
            };
            println!("Raylib image unloaded")
        }
    }
}

#[repr(C)]
pub struct RImage {
    pub data: *mut c_void,
    pub width: c_int,        // Image base width
    pub height: c_int,       // Image base height
    pub mipmaps: c_int,      // Mipmap levels, 1 by default
    pub format: PixelFormat, // Data format (PixelFormat type)
}

unsafe impl Send for RImage {}

impl Clone for RImage {
    fn clone(&self) -> Self {
        // does a deep clone/copy of the image data
        unsafe {
            ImageCopy(RImage {
                data: self.data,
                width: self.width,
                height: self.height,
                mipmaps: self.mipmaps,
                format: self.format,
            })
        }
    }
}

impl From<&Mat> for RImage {
    fn from(m: &Mat) -> Self {
        let f = match m.typ() {
            CV_8UC3 => PixelFormat::PIXELFORMAT_UNCOMPRESSED_R8G8B8,
            _ => panic!("Matrix Type not supported\n {:#?}", m),
        };

        let size = m.size().unwrap();

        // evil pointer copy
        let data = m.data() as *mut c_void;

        Self {
            data,
            width: size.width,
            height: size.height,
            mipmaps: 1,
            format: f,
        }
    }
}

From what I understand, which can be wrong, the drop function is called when a variable goes out of scope of the owner. Since my Image wrapper takes ownership of the OpenCV mat (if there is one) and how I implemented the drop trait, it should call drop on the OpenCV mat, correct?

I pull the mat out of the Option and then do nothing with it which should call the OpenCV drop function when it goes out of scope, which internally calls the c++ bindings. OpenCV then internally manages all the reference counting stuff. The UnloadImage function is not called.

If there is none only the UnloadImage function ist called. Thus not resulting in a double free.

English isn't my first language, so if I can clarify something just ask!

Thank you

2

You can treat Rust's invocations of Drop as always correct and foolproof. All kinds of scopes, moves, variable swaps, field overrides, no matter how complicated, will all be handled reliably for you. You can't forget to Drop anything by accident, and safe code can't be the cause of a double-free in any case [1].

So, when you assign to an initialized field, the previous value will always be Dropped. If an initialized variable goes out of scope, whether due to a move, return, or a panic[2], it will also always be Dropped. Rust also tracks when variables or fields are uninitialized, and will never try to call Drop on something that it shouldn't.

But keep in mind that objects live independently, and can be dropped in any order.

There are situations where safe Rust can leak memory, but these don't happen by accident. You need to explicitly ask to leak memory via things like ManuallyDrop or Box::leak. It's also possible to cause leaks due to reference count cycles, but Rust doesn't have implicit reference counting (you would have to wrap types in an Rc/Arc-like type, and use interior mutability, and make them recursive types, and cause a cycle, and then the leak won't be due to Rust not calling Drop when it should, but due to a correctly-called Drop not being able to break the cycle).

  1. bugs in the compiler or unsafe could cause UAF, but it's not something that safe code is responsible for ↩︎

  2. if it doesn't abort the process ↩︎

4 Likes
3

However, ideally each object should be responsible for freeing its own memory, and having Drop on Image looks suspicious. If you ever allow RImage to be moved out of Image, this could lead to use-after-free. Note that any &mut in Rust allows swapping two objects (and this doesn't need to call Drop), so if your Image ever exposes &mut RImage, it will be unsound.

You should have Drop on RImage itself.

It's not clear to me how these two libraries handle ownership/reference counting. If there can be many RImage instances that need one Mat to live, they should either handle that on the C site by increasing its internal reference count when created, and decrementing it after being dropped (independently of what refcounting you do for your Mat instance), or all RImage instances should have a private field with an Arc<Mat> instance to keep the shared copy alive on the Rust side.

2 Likes
4

RImage is just for FFI and is not used directly. I implemented drop on Image according to this: Foreign Function Interface - Secure Rust Guidelines (the part after A simple example of Rust wrapping over an external opaque type:)

Thus the "user" is just using Image.

It's not clear to me how these two libraries handle ownership/reference counting. If there can be many RImage instances that need one Mat to live, they should either handle that on the C site by increasing its internal reference count when created, and decrementing it after being dropped, or all RImage instances should have a field with an Arc<Mat> instance.

I know OpenCV Reference counts the Mat on the c++ side, because it is documented in the OpenCV Reference and the rust bindings just call the corresponding c++ functions. I know when the c side of raylib calls the Unload function, because i Implemented the call.

RImage doens't always need mat to live, just when i copy the data pointer from a mat.

Thank you!

5

If Mat has a destructor, and RImage doesn't increment Mat's C++ count by itself (which "evil pointer copy" suggests it doesn't), then you can have an UAF bug:

let image1 = Image::with_mat_and_rimage();
let image2 = image1.clone(); // skips `Mat`
drop(image1); // invokes `Drop` on `Mat`
image2 // has RImage that uses dropped `Mat`

so in this case the Option<Mat> has to be Arc<Mat> and get cloned with Image/RImage.

6

I think it doesn't use mat since

impl Clone for RImage {
    fn clone(&self) -> Self {
        // does a deep clone/copy of the image data
        unsafe {
            ImageCopy(RImage {
                data: self.data,
                width: self.width,
                height: self.height,
                mipmaps: self.mipmaps,
                format: self.format,
            })
        }
    }
}

deeply clones the RImage (I know that because the Function ImageCopy does that, just on the c side) and the mat is not copied over.

What I try to achive is that Image takes complete ownership over the mat and when Image is dropped, it just drops it with the "mat" drop and not with the Raylib UnloadImage. If the Image was created by Raylib and thus no mat is involved the UnloadImage should be called.

7

I've missed the deep copy thing. In that case, this seems okay.

2 Likes
8

Ah okay, thank you! I'm still a beginner in rust, so I didn't tried to be impolite I was just curious.

One last question: Are the RImage Struct member dropped after this? I assume because they're just integers, an enum (with #[repr(C)]) and a void pointer.

9

repr(C) and simplicity of the data doesn't affect drops. You can even have zero-sized types that don't even exist in memory, and Rust will still call Drop on them everywhere it should. Of course only the Rust side of the code will know to call Drop.

All struct fields are dropped individually themselves, whether you implement Drop on the struct or not. The fields are still dropped after your struct's fn drop runs.

2 Likes
10

Thank you!