403578 - Memory corruption in GIF decoder due to tpixel outside of colormap

Status: VERIFIED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

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]

Comment 1

[Bob Clary [:bc] (inactive)]

Igor, I found this looking at crash-reports for js related stacks. maybe this is something you could look at?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: stack

I'm thinking ImageLib here. Maybe the same issue as bug 403580

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: valgrind log

Comment 5

[Bob Clary [:bc] (inactive)]

over to imagegfx.

Comment 6

[Bob Clary [:bc] (inactive)]

bug 143046 looks like the culprit.

Comment 7

[Alfred Kayser]

Bug 4033363 has covered this 'imgContainer:DrawFrameTo' problem.

Comment 8

[Alfred Kayser]

And confirmed that with my patch in 403363 the URL above (http://omnimaga.org/index.php?showtopic=1548) doesn't crash anymore.

Comment 9

[Alfred Kayser]

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.

Comment 10

[Alfred Kayser]

Attachment: The crashing image

The image that causes the crash of http://img249.imageshack.us/img249/7711/ptiani1fm3.gif

Comment 11

[Mike Schroepfer]

Content induced crash - so marking blocking and making a p2.

Comment 12

[Alfred Kayser]

Attachment: Patch to fix depth for images with tpixel outside the depth range

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)

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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)

Comment 14

[Martijn Wargers (dead)]

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?

Comment 15

[Alfred Kayser]

Followup bug to address the regression is bug 408310.