Incorrect pointer handling in unsafe blocks causes rendering problems with non-zero opt-level

[ghost]

Hi! I'm using your crate in my rewrite of an old game engine and currently rendering everything with SDL_Renderer (e.g. Canvas) instead of raw graphics APIs.

When compiling with non-zero opt-level, most graphics don't render, and I can't see relevant draw calls in RenderDoc.

When calling Canvas<T>::copy without specifying src and dst, it works fine. However, specifying one or both causes the objects to disappear.

It seems there's a mistake in the unsafe blocks where you're taking references to temporary objects. The compiler optimizes these out, leading to invalid pointers being passed to SDL_RenderTexture. I don’t know how this should actually behave in Rust, but I'm seeing this issue.

I tried to fix it by modifying code like this and it actually works:

// Before

let ret = unsafe {
    sdl3::sys::render::SDL_RenderTexture(
        self.raw(),
        texture.raw(),
        match src.into() {
            Some(ref rect) => &rect.to_ll(),
            None => null(),
        },
        match dst.into() {
            Some(ref rect) => &rect.to_ll(),
            None => null(),
        },
    )
};

// After

let src = match src.into() {
    Some(ref rect) => &rect.to_ll(),
    None => null(),
};

let dst = match dst.into() {
    Some(ref rect) => &rect.to_ll(),
    None => null(),
};

let ret =
    unsafe { sdl3::sys::render::SDL_RenderTexture(self.raw(), texture.raw(), src, dst) };

I don't have much time and resources to investigate this further and create a PR with fix so just leaving this issue here

[WaffleLapkin]

The current version is quite clearly UB:

        match src.into() {
            //                 vvvvvvvvvvvv ---- this creates a temporary object
            Some(ref rect) => &rect.to_ll(), // <-- temporary is dropped here
            //                ^ -- this creates a reference that is immediately coerced to a pointer
            None => null(),
        },
        // here the pointer is invalid, since the temporary has been "freed"

I found 4 cases of this:

https://github.com/revmischa/sdl3-rs/blob/08e59fdee3d959db50fd50f613d00e76d003976a/src/sdl3/render.rs#L1342-L1349
https://github.com/revmischa/sdl3-rs/blob/08e59fdee3d959db50fd50f613d00e76d003976a/src/sdl3/render.rs#L1405-L1417
https://github.com/revmischa/sdl3-rs/blob/08e59fdee3d959db50fd50f613d00e76d003976a/src/sdl3/render.rs#L1295
https://github.com/revmischa/sdl3-rs/blob/08e59fdee3d959db50fd50f613d00e76d003976a/src/sdl3/video.rs#L1639-L1642

Although it does make you think how many similarly subtle UB is also in here.

The change made by @miabaka is not UB, but subtly so: since rust 1.79.0 match "forwards" lifetime extension from let, so the temporary from &rect.to_ll() lives for the whole function if it's in a match in a let.

Depending on lifetime extension for unsafe code is completely bonkers™ -- lifetime extension rules are quite subtle and even the most experienced rust users can't always tell how the code will behave wrt them. Lifetime extension rules were developed specifically for safe code/references, where a mistake leads simply to a compilation error.

The correct way of fixing all of these UB cases is to assign .to_ll() value to a local variable first, and then use that to get a reference/pointer. Something like this (example for the copy function, but similar code can be used in all of the linked code):

        let src = src.into().as_ref().map(FRect::to_ll);
        let dst = src.into().as_ref().map(FRect::to_ll);

        let ret = unsafe {
            sys::render::SDL_RenderTexture(
                self.context.raw,
                texture.raw,
                src.as_ref().map_or(ptr::null(), ptr::from_ref),
                dst.as_ref().map_or(ptr::null(), ptr::from_ref),
            )
        };

[revmischa]

Thanks for the detailed info! Want to open a PR with the fixes?

[WaffleLapkin]

I don't (never used sdl myself; researched just out of curiosity)

[ghost]

I'll probably fix this and open a PR in the next couple of days

[Maykeye]

Although it does make you think how many similarly subtle UB is also in here.

Good news is it it's possible to detect this exact bug by analyzing MIR: in mir code it such operations create two sequential operations; take a pointer to a value, then drop the value. Bad news, it makes use of MIR, aka "// WARNING: This output format is intended for human consumers only and is subject to change without notice. Knock yourself out." which you need to build and it will change between releases and who knows if there's code that take pointer to something that is not dropped first. Recipe for how to detect it

$ cd ~/src/sdl3-rs
$ git checkout v0.12.0
$ rm -rf target/*
$ cargo rustc -r -- --emit=mir
$ grep -Pn "= &raw (const|mut) (_\d+);\n\s+StorageDead\(\2\)"  target/release/deps/sdl3-51251c1be18b1678.mir 
118453:        _0 = &raw const _3;
118454|        StorageDead(_3);
118792:        _8 = &raw const _12;
118793|        StorageDead(_12);
118830:        _13 = &raw const _18;
118831|        StorageDead(_18);
119069:        _13 = &raw const _17;
119070|        StorageDead(_17);
119107:        _18 = &raw const _23;
119108|        StorageDead(_23);
119139:        _24 = &raw const _29;
119140|        StorageDead(_29);
151644:        _5 = &raw const _9;
151645|        StorageDead(_9);
$ git checkout b5c8e8b
$ rm -rf target/*
$ cargo rustc -r -- --emit=mir
$ grep -Pn "= &raw (const|mut) (_\d+);\n\s+StorageDead\(\2\)"  target/release/deps/sdl3-1ab2ad5658abea49.mir
(nothing found)

If you look for a string fn before like this, you'll get name of functions where it happens but grep is simple enough