libsdl-org / SDL_image

Image decoding for many popular formats for Simple Directmedia Layer.
zlib License
513 stars 174 forks source link

[2.8.x] Double-free if IMG_SaveJPG_RW_jpeglib() fails and IMG_SaveJPG_RW_tinyjpeg() is tried #428

Closed smcv closed 4 months ago

smcv commented 4 months ago

To reproduce:

Expected result: Saving with libjpeg fails. SDL_image tries to fall back to tinyjpeg. No crash.

Actual result: First SDL_image calls IMG_SaveJPG_RW_jpeglib(., ., freedst=1, .), and then it calls IMG_SaveJPG_RW_tinyjpeg(., ., freedst=1, .). The first call closes (frees) the destination RWops object (file). The second call is a use-after-free and then a double-free, which in my case is detected by glibc, causing an abort().

Suggested solution: Probably the entire implementation of freedst should be in IMG_SaveJPG_RW(), and the two backend-specific implementations IMG_SaveJPG_RW_jpeglib() and IMG_SaveJPG_RW_tinyjpeg() should never free the RWops object.

slouken commented 4 months ago

That sounds like the right solution.

smcv commented 4 months ago

I'll provide a PR when I've figured out what is even going on in my scout build...

smcv commented 4 months ago

It looks as though d2c7e0905 might have accidentally fixed this as well (it was intentionally fixing a missing close/free, but seems to have unintentionally fixed this double-free at the same time).

smcv commented 4 months ago

I think #430 resolves this, but I want to get to the bottom of what is happening in my scout build to have better confidence about that solution before undrafting it.