403363 - crash loading image [@ imgContainer::DrawFrameTo]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Brian Polidoro]

Attachment: crashing image

Loading the attached image crashes.  

20071107 works
20071108 crash

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-11-07+04%3A00%3A00&maxdate=2007-11-08+05%3A00%3A00&cvsroot=%2Fcvsroot

Breakpad reports have come for Windows and Mac.

Comment 1

[Alfred Kayser]

The problem is in the ASSERTION in imgContainer.cpp line 1122:
    // dstRect must fully fit within destination image 
    NS_ASSERTION((aDstRect.x >= 0) && (aDstRect.y >= 0) &&
                 (aDstRect.x + aDstRect.width <= dstRect.width) &&
                 (aDstRect.y + aDstRect.height <= dstRect.height),
                "imgContainer::DrawFrameTo: Invalid aDstRect");
    // dstRect size may be smaller than source, but not larger
    NS_ASSERTION((aDstRect.width <= srcRect.width) &&
                 (aDstRect.height <= srcRect.height),
                 "imgContainer::DrawFrameTo: source and dest size must be equal");

The image in question does have a bad y offset (397) while the total image is only 224x244 big.

Instead of only asserting on these values, we need to really catch them (and clip where possible, or abort frame composition).

Comment 2

[Alfred Kayser]

Attachment: V1: Fix frames with out-of-bound offsets and widths

Comment 3

[Alfred Kayser]

Attachment: Another testcase (this one is really screwed)

Open this one with IrfanView, and you can see how screwed the animation is...
With my patch, gecko doesn't crash on this image anymore.

Comment 4

[(not currently active) Ted Mielczarek]

I hit this on a myspace page, it probably has a similarly screwed up image:
http://profile.myspace.com/index.cfm?fuseaction=user.viewprofile&friendid=215157965

Comment 8

[Samuel Sidler (old account; do not CC)]

This is currently the #2 topcrash on trunk.

http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0b2pre
http://tinyurl.com/37nwbd

Comment 9

[Alfred Kayser]

Given the straightforward patch, and the clear testcases, this could be reviewed and applied quite quickly.

Comment 11

[Reed Loden [:reed]]

Checking in modules/libpr0n/src/imgContainer.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v  <--  imgContainer.cpp
new revision: 1.61; previous revision: 1.60
done

Comment 12

[Alfred Kayser]

bug 403985, bug 403757, bug 403580 and the link in comment #4, and the two test images don't crash anymore.

But, the link in bug 403578 still crashes... (the offending gif image seems to have a tpixel outside the colormap (globa_colormap_depth=3 (so 8 colors), and tpixel =15).