Closed Bug 681190 Opened 13 years ago Closed 13 years ago

crash nsPNGEncoder::ConvertHostARGBRow

Categories

(Core :: Graphics: ImageLib, defect)

All
Windows Vista
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: m_kato, Assigned: arno)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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>
<html>
<body>
<canvas id="crashtest" width="50000" height="50000" />
<script>
var file = document.getElementById("crashtest").mozGetAsFile("");
</script>
</body>
</html>
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
I'm interested in working on that bug.
Assignee: nobody → arno
Attached patch patch proposal (obsolete) — Splinter Review
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.
Attachment #555401 - Flags: review?(roc)
Here is try server result for the patch:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=64ff7c725a07
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, NS_ERROR_OUT_OF_MEMORY);
> +    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().
Attached patch patch v1.2Splinter Review
updated patch
Attachment #555401 - Attachment is obsolete: true
Attachment #555401 - Flags: review?(roc)
Attachment #555726 - Flags: review?(roc)
Keywords: checkin-needed
In my queue, pushing to inbound once confirmed everything builds locally.
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2da2cc9a885c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: