crash nsPNGEncoder::ConvertHostARGBRow

RESOLVED FIXED in mozilla9

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: m_kato, Assigned: arno renevier)

Tracking

({crash})

Trunk
mozilla9
All
Windows Vista
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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>
<html>
<body>
<canvas id="crashtest" width="50000" height="50000" />
<script>
var file = document.getElementById("crashtest").mozGetAsFile("");
</script>
</body>
</html>
(Reporter)

Updated

6 years ago
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
(Assignee)

Comment 1

6 years ago
I'm interested in working on that bug.
Assignee: nobody → arno
(Assignee)

Comment 2

6 years ago
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.
Attachment #555401 - Flags: review?(roc)
(Assignee)

Comment 3

6 years ago
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().
(Assignee)

Comment 5

6 years ago
Created attachment 555726 [details] [diff] [review]
patch v1.2

updated patch
Attachment #555401 - Attachment is obsolete: true
Attachment #555401 - Flags: review?(roc)
Attachment #555726 - Flags: review?(roc)
Attachment #555726 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 6

6 years ago
In my queue, pushing to inbound once confirmed everything builds locally.
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 7

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/2da2cc9a885c
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/2da2cc9a885c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 688854
You need to log in before you can comment on or make changes to this bug.