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 724 - SDL_bmp.c crashes on headers with negative height values
Summary: SDL_bmp.c crashes on headers with negative height values
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: 1.2.13
Hardware: All All
: P2 critical
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL: http://www.assembla.com/spaces/SDL_Cl...
Keywords: target-1.2.14
Depends on:
Blocks:
 
Reported: 2009-04-07 08:07 UTC by Eric Wing
Modified: 2009-09-26 04:44 UTC (History)
3 users (show)

See Also:

Attachments
negative height/width in bmp loading. (14.32 KB, patch)
2009-09-17 05:03 UTC, Rene Dudfield 		Details | Diff
fixes to SDL_bmp.c for negative height (14.06 KB, text/plain)
2009-09-17 11:14 UTC, Eric wing 		Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Wing 2009-04-07 08:07:11 UTC 
SDL_bmp.c does not handle BMP headers with negative height values. I discovered this problem while working on a copy & paste API for SDL. I discovered that Apple tends to create BMP headers that have negative values for height to represent that an image is inverted. This negative value messes up a bunch of calculations in SDL and usually causes a crash in memory allocation.

This is a problem for reading BMP files written by OS X and also reading BMPs from memory that were generated within OS X (e.g. pasteboard).


I tried to track down more information about the use of negative values in BMP headers. All I found was the following from the Cocoa mailing list responding to somebody who also ran into this problem with ImageIO:

<quote>
John Stiles <JStiles@blizzard.com> 	Fri, Jan 11, 2008 at 2:34 AM
Subject: BMP bug in ImageIO?
Cc: cocoa-dev@lists.apple.com
A negative height in the BMP header means that the scanlines are stored from top-to-bottom (rather than bottom-to-top, which is what you get if you put a positive height). I don't believe that this is a bug.
</quote>


The attached file is my modified version from the SDL 1.2.13 official source release. I assume SDL 1.3 also needs a similar fix.

My fix looks for negative height values and flips the sign. I also invert the image. I also recommend that a public API to invert an image be added to SDL 1.3 as it is really handy for this kind of stuff and OpenGL stuff. (And with a public API, writing optimizations might finally make sense, e.g. using Apple's vImage on OS X.)


If you need a reproducible test case to crash on, you can use my current work for the SDL clipboard at:
http://www.assembla.com/spaces/SDL_Clipboard
When building/running on a Mac, pasting an image from the pasteboard will result in a crash (click the bottom-right quadrant).
Comment 1 Ryan C. Gordon 2009-09-13 16:33:14 UTC 
Tagging this bug with "target-1.2.14" so we can try to resolve it for SDL 1.2.14.

Please note that we may choose to resolve it as WONTFIX. This tag is largely so we have a comprehensive wishlist of bugs to examine for 1.2.14 (and so we can close bugs that we'll never fix, rather than have them live forever in Bugzilla).

--ryan.
Comment 2 Rene Dudfield 2009-09-17 05:03:23 UTC 
Created attachment 359 [details]
negative height/width in bmp loading.

ah, this explains my issues with SDL and OSX bmp files.  This is an important fix for pygame too.

There's no patch attached to this bug.  I assume this was forgotten to be attached?

I've attached an SDL_bmp2.c that 'Kenneth Bull' sent to the mailing list.


"""
Subject: Re: [SDL] SDL_Loadbmp fail

Try the attached file (not thoroughly tested, but compiles without warnings
with gcc -c SDL_bmp2.c `pkg-config sdl --cflags`).

I renamed SDL_LoadBMP_RW and SDL_SaveBMP_RW so you can test it without
recompiling SDL.  This is for 1.2, not 1.3.

This should also work better for BITMAPV4HEADER and BITMAPV5HEADER bitmaps.
"""
Comment 3 Rene Dudfield 2009-09-17 05:04:53 UTC 
Updated to critical, as this is also a security vulnerability.
Comment 4 Eric wing 2009-09-17 11:14:21 UTC 
Created attachment 360 [details]
fixes to SDL_bmp.c for negative height
Comment 5 Eric wing 2009-09-17 11:15:53 UTC 
Sorry, I forgot to explicitly state where my fix was located. It is in my aforementioned repository under the FixesToSDL subdirectory.

http://trac-hg.assembla.com/SDL_Clipboard/browser/FixesToSDL/SDL_bmp.c

I have attached my file here for convenience.
Comment 6 Sam Lantinga 2009-09-26 04:44:07 UTC 
This is fixed for SDL 1.2, 1.3, and SDL_image.

Thanks!