Closed Bug 399630 Opened 12 years ago Closed 12 years ago

Don't decode PNG iCCP/cHRM chunks when color management disabled

Categories

(Core :: ImageLib, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: vlad, Assigned: glennrp+bmo)

References

Details

Attachments

(1 file)

I crash on startup with the 20071012 nightly in the PNG decoder; stack looks like:

 	ntdll.dll!774a18c3() 	
 	msvcr80.dll!free(void * pBlock=0x042f13b8)  Line 110	C
 	xul.dll!MOZ_PNG_free(png_struct_def * png_ptr=0x042d6a08, void * ptr=0x042f13b8)  Line 525 + 0xa bytes	C
 	xul.dll!MOZ_PNG_free_data(png_struct_def * png_ptr=0x042d6a08, png_info_struct * info_ptr=0x041c1438, unsigned long mask=0x00007fff, int num=0xffffffff)  Line 448	C
 	xul.dll!MOZ_PNG_info_dest(png_struct_def * png_ptr=0x042d6a08, png_info_struct * info_ptr=0x041c1438)  Line 611	C
 	xul.dll!MOZ_PNG_read_dest(png_struct_def * png_ptr=0x042d6a08, png_info_struct * info_ptr=0x041c1438, png_info_struct * end_info_ptr=0x00000000)  Line 1320 + 0x9 bytes	C
 	xul.dll!MOZ_PNG_dest_read_str(png_struct_def * * png_ptr_ptr=0x040a4a90, png_info_struct * * info_ptr_ptr=0x040a4a94, png_info_struct * * end_info_ptr_ptr=0x00000000)  Line 1261 + 0xa bytes	C
 	xul.dll!nsPNGDecoder::Close()  Line 219 + 0x14 bytes	C++
>	xul.dll!imgRequest::OnStopRequest(nsIRequest * aRequest=0x041870c0, nsISupports * ctxt=0x00000000, unsigned int status=0x00000000)  Line 706	C++
 	xul.dll!ProxyListener::OnStopRequest(nsIRequest * aRequest=0x041870c0, nsISupports * ctxt=0x00000000, unsigned int status=0x00000000)  Line 867	C++
 	xul.dll!nsJARChannel::OnStopRequest(nsIRequest * req=0x04184f98, nsISupports * ctx=0x00000000, unsigned int status=0x00000000)  Line 753	C++
 	xul.dll!nsInputStreamPump::OnStateStop()  Line 577	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x02855050)  Line 402	C++
 	xul.dll!nsOutputStreamReadyEvent::Run()  Line 112	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fbe8)  Line 491	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000001, int mayWait=0x00000001)  Line 227 + 0xd bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 445 + 0xa bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000006, unsigned int methodIndex=0x00000000, unsigned int paramCount=0x00000000, nsXPTCVariant * params=0x0032f418)  Line 102	C++
 	xul.dll!nsProxyObjectCallInfo::Run()  Line 181 + 0x19 bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000006, int * result=0x00000000)  Line 491	C++
 	xul.dll!nsProxyObjectCallInfo::Run()  Line 181 + 0x19 bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fc68)  Line 491	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000001, int mayWait=0x00000001)  Line 227 + 0xd bytes	C++
 	xul.dll!nsBaseAppShell::Run()  Line 154 + 0x8 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 171	C++
 	xul.dll!XRE_main(int argc=0x00000003, char * * argv=0x0032b198, const nsXREAppData * aAppData=0x0032b458)  Line 3144	C++
 	firefox.exe!main(int argc=0x00000003, char * * argv=0x0032b198)  Line 154	C++
 	firefox.exe!WinMain(HINSTANCE__ * __formal=0x00400000, HINSTANCE__ * __formal=0x00400000, char * args=0x00253fc8, HINSTANCE__ * __formal=0x00400000)  Line 166 + 0x13 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 589 + 0x1d bytes	C

(I can't give a breakpad trace because it still can't submit reports under Vista.)

The URI that it's trying to load varies; I've seen crashes with chrome://browser/skin/endcap-bkgnd.png and chrome://global/skin/icons/autocomplete-dropmark-bkgnd-mid-bottom.png .

In MOZ_PNG_free_data, it's always trying to free info_ptr->iccp_profile when the crash happens.  iccp_profile is not NULL, and iccp_name tends to have been "Photoshop".

The crash is intermittent, but frequent -- I can do two startups with a clean profile before crashing on the third and fourth.  May well be vista-only, otherwise the tinderboxes would be screaming, but maybe they're not hitting it for some reason.
Flags: blocking1.9+
This is probably the iCCP problem that is fixed in libpng-1.2.22
Please try applying the "diff.txt" file that is supplied with libpng-1.2.22rc1.
Please try the patch in bug #399546, which updates libpng to version 1.2.22rc1.
Status: NEW → ASSIGNED
Assignee: nobody → glennrp
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
The large number of crashes reported indicates that the startup crashes are occurring whether gfx.color_management is enabled or not.  That makes sense; the iCCP chunk is decoded by libpng whether or not it is going to be used later.  We ought to add iCCP and cHRM to the unused PNG chunk list in libpr0n/decoders/png/nsPNGDecoder.cpp at run time, when color_management is not enabled.  This would save a little CPU time and reduce vulnerability to problems in those chunk decoders.
This patch remedies the situation I described in comment #4. The PNG iCCP and cHRM chunks will only be decoded when gfx.color_management is enabled, saving some CPU time and reducing the vulnerability surface.
Comment on attachment 284769 [details] [diff] [review]
Avoid decoding iCCP and cHRM when color_management is disabled (checked in)

r? Note that there will still be crashes when gfx.color_management is enabled; that is addressed by updating to libpng-1.2.22.
Attachment #284769 - Flags: review?(vladimir)
Attachment #284769 - Flags: review?(vladimir) → review+
Attachment #284769 - Flags: superreview?(pavlov)
Attachment #284769 - Flags: superreview?(pavlov) → superreview+
Priority: -- → P1
is the patch in this bug included in the 1.2.22 patch as well?  can we get rid of this bug?
Priority: P1 → P3
The bug with freeing the profile is fixed, but the patch is useful nontheless because you avoid decoding iCCP when you are not going to use it.  So as far as priorities go, it's an enhancement, not a bugfix, once libpng-1.2.22 is checked in.  Sometimes iCCP chunks are fairly large and take a lot of CPU time to decompress.
Attachment #284769 - Flags: approval1.9?
Target Milestone: --- → mozilla1.9 M10
This is no longer a "blocker"; that was only when libpng-1.2.21 was checked in.
Neither the existing libpng-1.2.16 nor the future 1.2.22 has the vulnerability.  Both can benefit from this patch, however, as mentioned above.  Also it is good for all platforms and OS.  Revising flags accordingly.
No longer blocks: 386585
Severity: blocker → enhancement
OS: Windows Vista → All
Hardware: PC → All
Summary: Startup crash with 20071012 nightly in libpng decode → Don't decode PNG iCCP/cHRM chunks when color management disabled
Comment on attachment 284769 [details] [diff] [review]
Avoid decoding iCCP and cHRM when color_management is disabled (checked in)

a=drivers for after M9 freeze
Attachment #284769 - Flags: approval1.9? → approval1.9+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #284769 - Attachment description: Avoid decoding iCCP and cHRM when color_management is disabled → Avoid decoding iCCP and cHRM when color_management is disabled (checked in)
You need to log in before you can comment on or make changes to this bug.