Closed
Bug 770122
Opened 12 years ago
Closed 12 years ago
Crash on websites with ico/favicon containing EXIF data in its bitmaps
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: s.lange, Assigned: bbondy)
References
()
Details
(4 keywords)
Crash Data
Attachments
(2 files)
28.41 KB,
application/octet-stream
|
Details | |
1.16 KB,
patch
|
joe
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
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•12 years ago
|
||
I can reproduce on Nightly: bp-34153b3b-af5c-45bd-b0eb-eaca22120702.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::image::Decoder::HasError()]
Component: Untriaged → ImageLib
Ever confirmed: true
Keywords: crash,
reproducible
Product: Firefox → Core
QA Contact: untriaged → imagelib
Version: 13 Branch → Trunk
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → netzen
Assignee | ||
Comment 3•12 years ago
|
||
This bug was introduced in bug 682677, it is due to not saving the jump buffer before a call that can longjump.
Attachment #638573 -
Flags: review?(joe)
Assignee | ||
Comment 4•12 years ago
|
||
After review and landing I'll be requesting that this goes on Aurora and Beta.
Updated•12 years ago
|
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Keywords: regression
Version: Trunk → 10 Branch
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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 PixelFormat32bppARGB). There is no BITMAPINFOHEADER 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.
Assignee | ||
Comment 8•12 years ago
|
||
> 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•12 years ago
|
||
(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
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
This is passing try tests on all platforms by the way: https://tbpl.mozilla.org/?tree=Try&rev=2f6ae534c7a4
Assignee | ||
Updated•12 years ago
|
status-firefox16:
--- → affected
Assignee | ||
Comment 12•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #638573 -
Flags: review?(joe) → review+
Comment 13•12 years ago
|
||
It wasn't that I didn't have time, it was that I was on vacation :)
Assignee | ||
Comment 14•12 years ago
|
||
Then by definition you had no time! :P Thanks for the review.
Assignee | ||
Comment 15•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/91f65d802d58 Moving to mozilla17 because I *think* that's where it will land.
Target Milestone: --- → mozilla17
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91f65d802d58
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•12 years ago
|
||
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
Attachment #638573 -
Flags: approval-mozilla-beta?
Attachment #638573 -
Flags: approval-mozilla-aurora?
Comment 18•12 years ago
|
||
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.
Attachment #638573 -
Flags: approval-mozilla-beta?
Attachment #638573 -
Flags: approval-mozilla-beta+
Attachment #638573 -
Flags: approval-mozilla-aurora?
Attachment #638573 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/e8531445dd68 http://hg.mozilla.org/releases/mozilla-aurora/rev/1387ad6209b1
Comment 20•12 years ago
|
||
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.
Updated•12 years ago
|
QA Contact: ioana.budnar
Comment 21•12 years ago
|
||
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•12 years ago
|
||
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
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•