Closed Bug 403578 Opened 12 years ago Closed 12 years ago

Memory corruption in GIF decoder due to tpixel outside of colormap

Categories

(Core Graveyard :: Image: Painting, defect, P2, critical)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bc, Assigned: alfredkayser)

References

()

Details

(Keywords: crash, regression, Whiteboard: [sg:critical?] post 1.8-branch)

Attachments

(4 files)

blaming thebes based on the first part of the backtrace.

======= Backtrace: =========
/lib/libc.so.6[0xe5c583]
/lib/libc.so.6(__libc_malloc+0x7e)[0xe5debe]
/usr/lib/libstdc++.so.6(_Znwj+0x27)[0x4cb0547]
/work/mozilla/builds/1.9.0/mozilla/firefox-debug/dist/bin/components/libgkgfxthebes.so[0x1a10113]
Flags: blocking1.9?
Severity: normal → critical
Keywords: crash
Igor, I found this looking at crash-reports for js related stacks. maybe this is something you could look at?
AFAIK, this just means that a malloc() done by thebes triggered a heap check which discovered the corruption.  Doesn't mean that thebes is responsible for it.  If it's reproducible with the given testcase, should be easy to figure out the actual problem using valgrind/purify/etc.
Attached file stack
I'm thinking ImageLib here. Maybe the same issue as bug 403580
Attached file valgrind log
over to imagegfx.
Component: GFX: Thebes → Image: GFX
QA Contact: thebes → image.gfx
bug 143046 looks like the culprit.
Bug 4033363 has covered this 'imgContainer:DrawFrameTo' problem.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 403363
And confirmed that with my patch in 403363 the URL above (http://omnimaga.org/index.php?showtopic=1548) doesn't crash anymore.
Actually, the crash is indeed somewhere in nsGIFDecoder2.cpp, on the line with:
        mColormap[mGIFStruct.tpixel] = 0;

The offending gif image seems to have a tpixel outside the colormap (globa_colormap_depth=3 (so 8 colors), and tpixel =15.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → ASSIGNED
Assignee: nobody → alfredkayser
Status: ASSIGNED → NEW
Attached image The crashing image
The image that causes the crash of http://img249.imageshack.us/img249/7711/ptiani1fm3.gif
Content induced crash - so marking blocking and making a p2.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
The crashing image has tpixel 15 and depth 3, and other image viewers (such as IrfanView and Opera) report this image has 4 bpp. So, for this image we need to change the depth to accomodiate for the tpixel value.

This didn't crash before as we always used colormaps with 256 entries, but with the patch from bug 143046, palettes are sized to the actual depth.

So, to fix this issue, we increase the depth value till tpixel also fits in the colormap.

(note: the other solution would be to ignore tpixel values outside the depth range, but then this image still doesn't display correctly)
Attachment #289291 - Flags: review?
Attachment #289291 - Flags: review? → review?(pavlov)
Attachment #289291 - Flags: superreview?(tor)
Attachment #289291 - Flags: superreview?(tor) → superreview+
Attachment #289291 - Flags: review?(pavlov) → review+
Attachment #289291 - Flags: approval1.9?
Comment on attachment 289291 [details] [diff] [review]
Patch to fix depth for images with tpixel outside the depth range

(clearing a?, it's a blocker, doesn't need approval, please land)
Attachment #289291 - Flags: approval1.9?
Keywords: checkin-needed
Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDec
oder2.cpp
new revision: 1.88; previous revision: 1.87
done

Checked in.

This regressed between 2007-11-07 and 2007-11-08, when bug 143046 was fixed, so a regression from that bug.
So since this is a recent regression, can this bug be opened up?
Blocks: slowGIF
Status: NEW → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 408310
Summary: glibc detected firefox-bin: malloc(): memory corruption → Memory corruption in GIF decoder due to tpixel outside of colormap
Followup bug to address the regression is bug 408310.
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] post 1.8-branch
Group: security
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.