Last Comment Bug 681190 - crash nsPNGEncoder::ConvertHostARGBRow
: crash nsPNGEncoder::ConvertHostARGBRow
: crash
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All Windows Vista
-- critical (vote)
: mozilla9
Assigned To: arno renevier
: Milan Sreckovic [:milan]
: 688854 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-08-22 22:36 PDT by Makoto Kato [:m_kato]
Modified: 2011-09-24 08:08 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

crash test (195 bytes, text/html)
2011-08-22 22:36 PDT, Makoto Kato [:m_kato]
no flags Details
patch proposal (4.26 KB, patch)
2011-08-24 08:01 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v1.2 (4.22 KB, patch)
2011-08-25 07:25 PDT, arno renevier
roc: review+
Details | Diff | Splinter Review

Description User image Makoto Kato [:m_kato] 2011-08-22 22:36:43 PDT
Created attachment 555031 [details]
crash test

This bug was filed from the Socorro interface and is 
report bp-a066da9e-5773-4da0-b807-07ba32110822 .

This is crash sample.  Repro rate is 100%

<!DOCTYPE html>
<canvas id="crashtest" width="50000" height="50000" />
var file = document.getElementById("crashtest").mozGetAsFile("");
Comment 1 User image arno renevier 2011-08-23 14:16:04 PDT
I'm interested in working on that bug.
Comment 2 User image arno renevier 2011-08-24 08:01:06 PDT
Created attachment 555401 [details] [diff] [review]
patch proposal

Crash happens, when emptyCanvas is created in nsHTMLCanvasElement::ExtractData, it's too big so it's data is leaved to null. Then, in nsPNGEncoder::ConvertHostARGBRow, 
     const PRUint32& pixelIn = ((const PRUint32*)(aSrc))[x];
segfaults (aSrc is emptyCanvas data).
Here is a patch proposal. It does two things to ensure crash does not happen:
First, return early from nsHTMLCanvasElement::ExtractData in case emptyCanvas data is null.
Also, it returns early from nsJPEGEncoder::InitFromData and nsPNGEncoder::InitFromData in case they receive a null data argument. That should prevent some similar crashes in the future.
Comment 3 User image arno renevier 2011-08-24 08:01:32 PDT
Here is try server result for the patch:
Comment 4 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-24 18:03:11 PDT
Comment on attachment 555401 [details] [diff] [review]
patch proposal

Review of attachment 555401 [details] [diff] [review]:

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +228,5 @@
>    nsIntSize size = GetWidthHeight();
>    if (!mCurrentContext) {
>      emptyCanvas = new gfxImageSurface(gfxIntSize(size.width, size.height), gfxASurface::ImageFormatARGB32);
> +    NS_ENSURE_TRUE(emptyCanvas->Data(), NS_ERROR_INVALID_ARG);

emptyCanvas can't be null because the new is infallible.

instead of checking emptyCanvas->Data(), check emptyCanvas->CairoStatus().
Comment 5 User image arno renevier 2011-08-25 07:25:59 PDT
Created attachment 555726 [details] [diff] [review]
patch v1.2

updated patch
Comment 6 User image Ed Morley [:emorley] 2011-08-26 10:08:03 PDT
In my queue, pushing to inbound once confirmed everything builds locally.
Comment 8 User image Marco Bonardo [::mak] 2011-08-27 01:49:51 PDT
Comment 9 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-24 08:08:27 PDT
*** Bug 688854 has been marked as a duplicate of this bug. ***

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