We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 2868

Summary: Definite bug in SDL_FillRect
Product: SDL Reporter: skaller <skaller>
Component: videoAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: critical    
Priority: P2 CC: dll, icculus, skaller
Version: 2.0.3Keywords: triage-2.0.4
Hardware: All   
OS: All   
Attachments: Exhibits bug in SDL_FillRect.
fix for bug 2868 (SDL_FillRect bug)
simplified test program for SDL_FillRect() crash
fix for bug 2868 (SDL_FillRect bug)

Description skaller 2015-02-05 17:05:31 UTC 
I have code which is doing some drawing using at least SDL_RenderDrawLine on the surface of a window. If the window is resized so it it is too small, the program crashes, but not every time.

On thinking about this it would seem to be a serious design bug in SDL.
SDL deletes the surface of a window when the window is resized.

It appears this happens asynchronously, and can delete the surface whilst
it is in use. SDL_MUSTLOCK is NOT being set (so I'm not locking it).

This does not seem to bother bliting or filling, only line drawing,
and only when the line ends up being drawn off the edge of the window.

This happened with 2.0.0 so I upgraded to 2.0.3 and it seems to happen less 
often but it still happens.
Comment 1 skaller 2015-02-22 23:44:46 UTC 
Created attachment 2041 [details]
Exhibits bug in SDL_FillRect.

Definite bug in SDL_FillRect.c exhibited by this attachment.
Comment 2 skaller 2015-02-22 23:49:57 UTC 
Here is the fix:

AFTER Line 255 of SDL_FillRect.c add this line:

if(SDL_RectEmpty(rect)) return 1;

This prevents negative heights and widths or out of bounds starting
pixel for fill crashing the subsequent calculations.

Please also examine ALL similar code: blits, line drawing
etc. Sorry, I cannot build SDL from source (OSX 10.6.8).

This patch has been tested on Linux and prevents the test
code from crashing.

I would really like a fixed up SDL2.dmg for OSX since I cannot
compile it myself.
Comment 3 David Ludwig 2015-02-23 01:37:48 UTC 
Created attachment 2042 [details]
fix for bug 2868 (SDL_FillRect bug)
Comment 4 David Ludwig 2015-02-23 01:38:31 UTC 
Created attachment 2043 [details]
simplified test program for SDL_FillRect() crash
Comment 5 David Ludwig 2015-02-23 01:38:43 UTC 
Hi Sam, Ryan, etc.

I'm able to reproduce this bug on iOS + Win32.  My guess, from looking at SDL's code, is that it occurs on other platforms as well.

In summary, I think the bug can be condensed to the following: SDL_FillRect() can crash if the surface's clip-rect is completely out-of-bounds

To reproduce:

1. set an SDL_Surface's clip-rect to something completely outside the bounds of the surface (via SDL_SetClipRect()).
2. call SDL_FillRect() on the surface, telling it to fill the entire rect (by passing in NULL as the 'rect/2nd-param).
3. crash!


John's suggestion for a patch looks good.  I've tested it, and encoded it into a patch.  It takes the following statement from SDL_SetClipRect's documentation:

'If the clip rectangle doesn't intersect the surface, the function will return SDL_FALSE and blits will be completely clipped.'

... and applies it to SDL_FillRect().  I.e. if and when a surface's clip-rect is out-of-bounds, 'SDL_FillRect(surface, NULL, color)' does nothing.

If you all like, I'd be happy to push this fix out to Mercurial.

In addition to the patch, I've attached a simplified test case, which should reproduce on any platform.

Cheers!
-- David L.
Comment 6 David Ludwig 2015-02-23 03:08:41 UTC 
Created attachment 2044 [details]
fix for bug 2868 (SDL_FillRect bug)

Doh, I posted the simplified test program twice.  Here's the patch!
Comment 7 Ryan C. Gordon 2015-02-24 04:33:13 UTC 
Looks good. David, go ahead and push this and resolve the bug.

Thanks!

--ryan.
Comment 8 David Ludwig 2015-02-24 04:44:46 UTC 
Fixed via https://hg.libsdl.org/SDL/rev/b577c4753421