Last Comment Bug 770122 - Crash on websites with ico/favicon containing EXIF data in its bitmaps
: Crash on websites with ico/favicon containing EXIF data in its bitmaps
Status: RESOLVED FIXED
: crash, icon, regression, reproducible
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: 10 Branch
: All All
: -- critical (vote)
: mozilla17
Assigned To: Brian R. Bondy [:bbondy]
: Ioana (away)
: Milan Sreckovic [:milan]
Mentors:
http://www.ratgeberebooks.eu/WebRoot/...
Depends on:
Blocks: 682677
  Show dependency treegraph
 
Reported: 2012-07-02 04:09 PDT by s.lange
Modified: 2012-10-12 06:59 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
verified
verified
verified


Attachments
ratgeberebookseu.zip => Logo PNG image from which the favicon was generated + the favicon that leads to the firefox crash (28.41 KB, application/octet-stream)
2012-07-02 04:09 PDT, s.lange
no flags Details
Patch v1 (1.16 KB, patch)
2012-07-02 19:30 PDT, Brian R. Bondy [:bbondy]
joe: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description s.lange 2012-07-02 04:09:55 PDT
Created attachment 638300 [details]
ratgeberebookseu.zip => Logo PNG image from which the favicon was generated + the favicon that leads to the firefox crash

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

Open Website http://www.ratgeberebooks.eu or only the favicon
http://www.ratgeberebooks.eu/WebRoot/Store18/Shops/63630130/favicon.ico


Actual results:

Firefox crashes with bug report window.
Comment 1 s.lange 2012-07-02 04:16:05 PDT
Crash report is: https://crash-stats.mozilla.com/report/index/35196c16-5fcd-472f-ae1a-bf8c02120702

Reproduceable under Windows 7 Prof. 64bit, Fedora Linux 17.

---

Notice that the favicon was generated from the Logo PNG image file with image magick. I think the issue might be some remaining EXIF data in the bitmap inside the ico. We had to call ImageMagick's $Image->Strip(); method to get rid of the metadata, then it works. Though firefox shoulnd't crash.
Comment 2 Scoobidiver (away) 2012-07-02 04:55:10 PDT
I can reproduce on Nightly: bp-34153b3b-af5c-45bd-b0eb-eaca22120702.
Comment 3 Brian R. Bondy [:bbondy] 2012-07-02 19:30:38 PDT
Created attachment 638573 [details] [diff] [review]
Patch v1

This bug was introduced in bug 682677, it is due to not saving the jump buffer before a call that can longjump.
Comment 4 Brian R. Bondy [:bbondy] 2012-07-02 19:35:14 PDT
After review and landing I'll be requesting that this goes on Aurora and Beta.
Comment 5 Glenn Randers-Pehrson 2012-07-03 05:23:22 PDT
I think that the existing code and the v1 patch will leak the
mPNG struct; it probably needs png_destroy_read_struct() both inside
the setjmp block and in the "return false" block.
Also, IsValidICO should return false if png_width > 256
or png_height > 256.
Comment 6 Glenn Randers-Pehrson 2012-07-03 05:55:07 PDT
The sample PNG is colortype 6 and has 10 text chunks but no EXIF data.
The converted favicon.ico has three PNG files (16x16, 32x32, and 64x64,
all colortype 3 (indexed)).  So the issue is apparently simply that
they are indexed PNGs, which are rejected by IsValidICO().
Comment 7 Glenn Randers-Pehrson 2012-07-03 06:37:58 PDT
The ICO definition presented at wikipedia says nothing about
limiting the PNG to 8 bit, colortype 6.

The blog referenced in a comment in nsPNGDecoder.h says

"The format of a PNG-compressed image consists simply of a PNG image, starting with the PNG file signature. The image must be in 32bpp ARGB format (known to GDI+ as Pixel­Format­32bpp­ARGB). There is no BITMAP­INFO­HEADER prefix, and no monochrome mask is present."

I interpret that to mean that the PNG decoder must return a 32-bit ARGB image.  Which ours always does via png_set_expand and GFX_PACKED_PIXEL calls within nsPNGDecoder.cpp, no matter what the actual PNG colortype and bit depth.

IsValidICO() is looking at the PNG header data before these conversions have happened.
Comment 8 Brian R. Bondy [:bbondy] 2012-07-03 07:36:09 PDT
> I think that the existing code and the v1 patch will leak the
> mPNG struct; it probably needs png_destroy_read_struct() both 
> inside the setjmp block and in the "return false" block.

No a call to png_destroy_read_struct will already happen when the mContainedDecoder is freed inside ~nsPNGDecoder.

> Also, IsValidICO should return false if png_width > 256
> or png_height > 256.

Although I agree with this assertion, it is not related to this bug.  Please post a followup bug and I can work on it there.  There are checks for the size inside the ICO decoder by the way already which is the only place this is used. 

> The sample PNG is colortype 6 and has 10 text chunks but no EXIF data.
> The converted favicon.ico has three PNG files (16x16, 32x32, and 64x64,
> all colortype 3 (indexed)).  So the issue is apparently simply that
> they are indexed PNGs, which are rejected by IsValidICO().

The issue is that there is an error in the call to png_check_IHDR (width == 0 && height==0).  Note that all other browsers also throw an error for this ICO.

> IsValidICO() is looking at the PNG header data before these conversions 
> have happened.

If you disagree with the logic in IsValidICO please post a follow up bug.  This bug is related to fixing a crash, and will be prioritized differently.  I.e. it will likely be moving up to different branches, but other changes would not.

Thanks for the feedback by the way.
Leaving the patch as is for Joe to review.
Comment 9 Glenn Randers-Pehrson 2012-07-03 08:27:42 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #8)

> Although I agree with this assertion, it is not related to this bug.  Please
> post a followup bug and I can work on it there.

OK

> The issue is that there is an error in the call to png_check_IHDR (width ==
> 0 && height==0).  Note that all other browsers also throw an error for this
> ICO.

ImageMagick sees valid dimensions:

identify favicon.ico
favicon.ico[0] ICO 16x16 16x16+0+0 8-bit DirectClass 8.83KB 0.000u 0:00.000
favicon.ico[1] ICO 32x32 32x32+0+0 8-bit PseudoClass 252c 8.83KB 0.010u 0:00.000
favicon.ico[2] ICO 64x64 64x64+0+0 8-bit PseudoClass 256c 8.83KB 0.010u 0:00.000

but colortype 3 (indexed):
identify -verbose favicon.ico
    [image 0]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 16, 16
    [image 1]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 32, 32
    [image 2]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 64, 64
Comment 10 Brian R. Bondy [:bbondy] 2012-07-03 08:30:46 PDT
libpng sees width, height as 0 from that function call and throws an error. I'm not sure why exactly but in any case it now throws an error instead of crashing. We are now working consistently with every other browser.
Comment 11 Brian R. Bondy [:bbondy] 2012-07-04 16:45:45 PDT
This is passing try tests on all platforms by the way:
https://tbpl.mozilla.org/?tree=Try&rev=2f6ae534c7a4
Comment 12 Brian R. Bondy [:bbondy] 2012-07-16 01:55:13 PDT
This fixes a crash which can be triggered from page loads, so was wondering if there's someone else I can push the review to with more time? I think the fix is pretty straightforward.  

This won't make mozilla16, but I'd like to request it up to Aurora/Beta after the migration.
Comment 13 Joe Drew (not getting mail) 2012-07-16 12:38:48 PDT
It wasn't that I didn't have time, it was that I was on vacation :)
Comment 14 Brian R. Bondy [:bbondy] 2012-07-16 12:43:49 PDT
Then by definition you had no time! :P Thanks for the review.
Comment 15 Brian R. Bondy [:bbondy] 2012-07-16 16:12:25 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/91f65d802d58
Moving to mozilla17 because I *think* that's where it will land.
Comment 16 Ed Morley [:emorley] 2012-07-17 02:11:54 PDT
https://hg.mozilla.org/mozilla-central/rev/91f65d802d58
Comment 17 Brian R. Bondy [:bbondy] 2012-07-17 07:24:22 PDT
Comment on attachment 638573 [details] [diff] [review]
Patch v1

Request for Aurora 16 and Beta 15:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 682677
User impact if declined: A webpage can be loaded which contains a crafted ICO that causes a browser crash
Testing completed (on m-c, etc.): Tested locally, landed on m-c already as well
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: none
Comment 18 Alex Keybl [:akeybl] 2012-07-18 16:40:58 PDT
Comment on attachment 638573 [details] [diff] [review]
Patch v1

[Triage Comment]
This has had a day of bake time now without regression, and we're very early in the cycle. Approving for both Aurora 16 and Beta 15.
Comment 20 Ioana (away) 2012-08-13 05:06:25 PDT
Verified as fixed on Firefox 15.0 beta 4 (20120808131812) with the ico file attached by the reporter. On the latest beta, the user receives an error message that the opened file cannot be displayed because it contains errors. Firefox does not crash anymore.
Comment 21 Ioana (away) 2012-09-28 05:32:19 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20100101 Firefox/16.0
BuildID: 20120925201946
Comment 22 Ioana (away) 2012-10-12 06:59:53 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0
BuildID: 20121010150351

Note You need to log in before you can comment on or make changes to this bug.