| Summary: | Crash/undefined behaviour in SDL_ConvertSurface with RLE surfaces on Windows | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Dan Lawrence <danintheshed> |
| Component: | video | Assignee: | Sylvain <sylvain.becker> |
| Status: | ASSIGNED --- | QA Contact: | Sam Lantinga <slouken> |
| Severity: | major | ||
| Priority: | P2 | CC: | sezeroz, sylvain.becker |
| Version: | 2.0.13 | ||
| Hardware: | x86 | ||
| OS: | Windows 10 | ||
| URL: | https://github.com/pygame/pygame/issues/2195 | ||
It seems to me we should keep the RLE information before doing the conversion: https://hg.libsdl.org/SDL/rev/114969db664a Btw, thanks for the testcase.
It's not only an issue on windows, but all os. There was an invalid memory access.
I wonder whether there could other places where the RLE info could be removed.
You may want to activate some warnings by adding this in your RLE code to check:
In SDL_UnRLESurface():
--- a/src/video/SDL_RLEaccel.c Sat Oct 17 21:47:05 2020 +0200
+++ b/src/video/SDL_RLEaccel.c Sun Oct 18 08:34:15 2020 +0200
@@ -1550,6 +1550,11 @@
surface->flags &= ~SDL_RLEACCEL;
if (recode && !(surface->flags & SDL_PREALLOC)) {
+
+ if (surface->map->info.flags & (SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY)) {
+ SDL_Log("Missing RLE infos in map->info.flags");
+ }
+
if (surface->map->info.flags & SDL_COPY_RLE_COLORKEY) {
SDL_Rect full;
|
This is the C++ reproduction code: #define SDL_MAIN_HANDLED #include "SDL/SDL.h" #include <stdio.h> #include <iostream> int main(int argc, char* argv[]) { SDL_SetMainReady(); SDL_Init(SDL_INIT_VIDEO); int width = 256; int height = 256; Uint32 rmask, gmask, bmask, amask; #if SDL_BYTEORDER == SDL_BIG_ENDIAN rmask = 0xff000000; gmask = 0x00ff0000; bmask = 0x0000ff00; amask = 0x000000ff; #else rmask = 0x000000ff; gmask = 0x0000ff00; bmask = 0x00ff0000; amask = 0xff000000; #endif SDL_Surface* dst = SDL_CreateRGBSurface(0, width, height, 32, rmask, gmask, bmask, 0); SDL_FillRect(dst, NULL, SDL_MapRGB(dst->format, 0, 0, 0)); SDL_Surface* src = SDL_CreateRGBSurface(0, width, height, 32, rmask, gmask, bmask, 0); Uint32 color = SDL_MapRGB(src->format, 0, 0, 0); SDL_FillRect(src, NULL, color); SDL_SetSurfaceRLE(src, SDL_TRUE); SDL_SetColorKey(src, SDL_TRUE, color); SDL_BlitSurface(src, NULL, dst, NULL); std::cout << "Got this far!\n"; SDL_ConvertSurface(src, src->format, src->flags); std::cout << "Got to the end!\n"; // Clean up SDL_Quit(); return 0; } And was discovered in pygame via several segfaults that kept cropping up in code using RLE. Here is a discussion on GitHub: https://github.com/pygame/pygame/issues/2195 The short version is that when you encode a surface with run length encoding there are two possible paths to go down - ColorKey & Alpha. In theory these paths both set a flag on the surface via surface->map->info.flags so that the surface can be correctly decoded from RLE at a later point. However, the UnRLE functions can be (and are) called while those flags no longer exist on the surface. This then results in un predictable behaviour where a surface that was originally encoded via the ColorKey RLE path gets decoded via the Alpha RLE path without the proper surface format (in my tests the surface format was full of garbage data). Anyway, I'm not sure what the best solution is but you can change the line that tests the unreliable flag in SDL_UnRLESurface() for a more reliable check. Where it currently says: if (surface->map->info.flags & SDL_COPY_RLE_COLORKEY) { Instead do: if (!surface->format->Amask) { Which mirrors part of the check when setting up the two RLE pathways. Or even better combine the two: if ((!surface->format->Amask) || (surface->map->info.flags & SDL_COPY_RLE_COLORKEY)) { This seems to make all the segfaults I've found disappear. I guess this might miss some RLE surfaces that don't have an alpha mask - but do have an alpha blendmode but that doesn't seem a very likely combo to me?