SDL: write access violation with OSD when updating screen

[criezy]

The issue was introduced with the recent changes related to OSD in SurfaceSdlGraphicsManager. Now when displaying OSD messages or icons dirty rects with coordinates in destination screen are created, but the issue is that dirty rects are assumed to be in source screen coordinates. So when using a 2X or 3X scaler we can get dirty rects that are outside of the screen. There is a sanity check on Y, but not on X, so in some cases we end up trying to write beyond the destination screen when applying the scaler.

Adding a sanity check on X would fix the crash, but this is not a proper fix and updates would be missing.

Here is the relevant part for the call stack of the crash:

0   Normal2x(unsigned char const*, unsigned int, unsigned char*, unsigned int, int, int) + 216 (scaler.cpp:218)
1   SurfaceSdlGraphicsManager::internUpdateScreen() + 1463 (surfacesdl-graphics.cpp:1130)
2   SurfaceSdlGraphicsManager::updateScreen() + 145 (surfacesdl-graphics.cpp:1006)
3   ModularBackend::updateScreen() + 50 (modular-backend.cpp:152)

I consider this a blocker because it for example causes a random crash when using a 2X or 3X scaler and switching between windowed and full screen mode as we get a OSD message and it triggers the bug.

With a 2X scaler when playing a 320x200 game, the OSD message when leaving full screen generates a dirty rect starting at 274x185 en ending at 366x214. The 214 gets clips to 200 because of the sanity check, but it still writes beyond the end of the screen surface because of the x=366 for the right border. The dirty rect should in this case have been 137x92->183x107.

[criezy]

Some notes, as I am confused by the code and not sure what it is supposed to do:

• SurfaceSdlGraphicsManager::internUpdateScreen() actually assumes dirty rects are either in virtual coordinates or in real coordinates depending if _overlayVisible is true.
• addDirtyRect() takes a bool to indicate if the coordinates are real ones. The default is false and before the recent change the only place where it was called with true was from drawMouse() where the mouse coordinate actually again depend on whether _overlayVisible is true or not.
• internUpdateScreen() calls SDL_UpdateRects() with the scaled surface (post-scaling, thus in real coordinates) and the dirty rect list. This is a bit strange to be missing real and virtual coordinate like that. SDL_UpdateRects() doesn't use the dirty rects though, so it doesn't matter.

From this it appears that the easier change might be to change the recently added dirty rects for OSD to check _overlayVisible and either pass virtual or real coordinates to addDirtyRect(). I am confused however in what the 'realCoordinates' flag is supposed to mean in the call to addDirtyRect().

Also I would suggest to remove the dirty rect list from the call to SDL_UpdateRects() since it is both unused and confusing (there is a risk we might decide to use it assuming it is in screen coordinates when it might actually be in virtual coordinates).

[bgK]

I switched the screen update behavior to back to full screen updates. It's not as good for performance, but it was not an issue before.