Unsoundness: Use-After-Free on `PixelFormat` possible

[t1mlange]

Hi,

PixelFormat is owned by Surface but is missing a lifetime specifier (or any other reference to it's owner). Therefore, it's possible to write a use-after-free in purely safe Rust:

use anyhow::anyhow;

fn main() -> anyhow::Result<()> {
    sdl2::init().map_err(|e| anyhow!(e))?;

    let pfmt;
    {
        let surface =
            sdl2::surface::Surface::new(1024, 1024, sdl2::pixels::PixelFormatEnum::RGB888)
                .map_err(|e| anyhow!(e))?;
        pfmt = surface.pixel_format();
    }
    let penum = sdl2::pixels::PixelFormatEnum::from(pfmt);
    println!("Pixel Format: {:#?}", penum);

    Ok(())
}

My suggested fix would be to add a phantom lifetime to PixelFormat:

pub struct PixelFormat<'a> {
    raw: *mut sys::SDL_PixelFormat,
    marker: PhantomData<&'a Surface<'a>>
}

Kind Regards
Tim

[renski-dev]

If we're changing PixelFormat, we should probably also fix owned instances, which simply leak the SDL_PixelFormat without ever calling SDL_FreeFormat.

fn main() {
    sdl2::init().unwrap();

    // The following leaks, since there is no destructor that could call `SDL_FreeFormat`, 
    // as shown by the failing assert that checks for drop glue.
    sdl2::pixels::PixelFormat::try_from(sdl2::pixels::PixelFormatEnum::BGR24).unwrap();
   
    assert!(std::mem::needs_drop::<sdl2::pixels::PixelFormat>());
}

Maybe we can make it work like Cow to support both, which would probably break existing code the least?

[Cobrand]

That is indeed an issue, and unfortunately it requires a breaking change to fix.

I think there is a definite need for documentation around it as well, because from the look of it nothing mentions in the rust doc that it's tied to a Surface.

[t1mlange]

Maybe we can make it work like Cow to support both, which would probably break existing code the least?

IIRC some other crates do it like this:

pub struct PixelFormat {
    raw: *mut sys::SDL_PixelFormat,
}

pub struct PixelFormatRef<'a> {
    raw: *mut sys::SDL_PixelFormat,
    marker: PhantomData<&'a Surface<'a>>
} 

Adding a lifetime might break existing code anyways, because mismatched_lifetime_syntaxes introduces a warning if the lifetime is completly omitted in the signature (possibly making CI pipelines fail). Also, if someone wraps PixelFormat in a struct, they will get a compilation error.

[renski-dev]

Yeah, the change is breaking anyway, even with a Cow, it's just a question of ergonomics then I guess.

What do the function signatures of existing APIs look like if we do two separate structs?

Also, this existing implementation probably has to be an associated function of PixelFormatEnum or something, no matter what we do:

impl From<PixelFormat> for PixelFormatEnum {
    fn from(pf: PixelFormat) -> PixelFormatEnum {
        unsafe {
            let sdl_pf = *pf.raw;
            match PixelFormatEnum::try_from(sdl_pf.format) {
                Ok(pfe) => pfe,
                Err(()) => panic!("Unknown pixel format: {:?}", sdl_pf.format),
            }
        }
    }
}

[antonilol]

This could be patched by using SDL's refcount field on SDL_PixelFormat, I don't know if this is public API but before a breaking change it can be used to prevent unsoundness. All assuming this is used by SDL to count references, like the name suggests.

[renski-dev]

It looks to me like "owned" instances of SDL_PixelFormat are protected by an atomic lock and modifying their refcount is not intended:
https://github.com/libsdl-org/SDL/blob/release-2.32.x/src/video/SDL_pixels.c#L620

I haven't looked too deeply into what the ownership semantics are for a SDL_PixelFormat retrieved from a surface, but SDL_AllocFormat and SDL_FreeFormat use a global linked list as a cache, which is where refcount is used.

Although this can probably be used in some way and is unlikely to ever change, I think it's still relying on private implementation details.

[antonilol]

It looks to me like "owned" instances of SDL_PixelFormat are protected by an atomic lock and modifying their refcount is not intended

I see, the lock is private so we can't even safely access it.

On SDL_Surface however, refcount is documented and is not protected with a lock, so can we safely assume that it is part of the public API?
https://wiki.libsdl.org/SDL2/SDL_Surface

An owned PixelFormat can keep the surface alive, without requiring a lifetime, but this keeps the memory of the surface live while only needing the PixelFormat, so I see this only as a temporary solution before adding the lifetime to PixelFormat.