crash at null [@DataAtOffset] in gfx/2d/DataSurfaceHelpers.cpp:91

RESOLVED FIXED in Firefox 52

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: tsmith, Assigned: vliu)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {crash, csectype-nullptr, testcase})

Trunk
mozilla53
crash, csectype-nullptr, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 affected, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8825246 [details]
log.txt

==20996==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fdcb9b10a73 bp 0x7ffef4c2c070 sp 0x7ffef4c2bb20 T0)
    #0 0x7fdcb9b10a72 in DataAtOffset /home/worker/workspace/build/src/gfx/2d/DataSurfaceHelpers.cpp:91:5
    #1 0x7fdcb9b10a72 in DoRender<float> /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3523
    #2 0x7fdcb9b10a72 in mozilla::gfx::FilterNodeLightingSoftware<mozilla::gfx::(anonymous namespace)::SpotLightSoftware, mozilla::gfx::(anonymous namespace)::SpecularLightingSoftware>::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3466
    #3 0x7fdcb9a9f29a in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:613:21
    #4 0x7fdcb9aa867e in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:714:17
    #5 0x7fdcb9ad75d4 in mozilla::gfx::FilterNodeCropSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3126:10
    #6 0x7fdcb9a9f29a in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:613:21
    #7 0x7fdcb9a699bb in mozilla::gfx::FilterNodeSoftware::Draw(mozilla::gfx::DrawTarget*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::DrawOptions const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:566:14
...
see log.txt
(Reporter)

Comment 1

a year ago
Created attachment 8825247 [details]
test_case.html
(Assignee)

Comment 2

a year ago
The crash hits by returning false in [1]. The thing was width got overflow.

(mozilla::gfx::IntSize) $1 = {
  mozilla::gfx::BaseSize<int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > = {
     = {
       = (width = 69527009, height = 1)
      components = ([0] = 69527009, [1] = 1)
    }
  }
}

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DataSurfaceHelpers.c
Since you've already dug a little bit, are you going to work on this bug? Thanks!
Flags: needinfo?(vliu)
Whiteboard: gfx-noted
(Assignee)

Comment 4

a year ago
(In reply to Mason Chang [:mchang] from comment #3)
> Since you've already dug a little bit, are you going to work on this bug?
> Thanks!

Sure, I will take it for the following tracking.
Assignee: nobody → vliu
Flags: needinfo?(vliu)
(Assignee)

Comment 5

a year ago
From SVG spec of view, we should properly handle a negative or zero value for kernelUnitLength.


[1]: https://www.w3.org/TR/SVG/filters.html#feConvolveMatrixElementKernelUnitLengthAttribute
(Assignee)

Comment 6

a year ago
Created attachment 8825786 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

Hi Bas,
Could you please have a review for the patch. Thanks
(Assignee)

Comment 7

a year ago
Created attachment 8825787 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

Hi Bas,
Could you please have a review for the patch. Thanks
Attachment #8825787 - Flags: review?(bas)
(Assignee)

Updated

a year ago
Attachment #8825786 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Comment on attachment 8825787 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

I will re-attach the patch for better solution.
Attachment #8825787 - Attachment is obsolete: true
Attachment #8825787 - Flags: review?(bas)

Comment 9

a year ago
if (aKernelUnitLengthX <= 0 && aKernelUnitLengthY <= 0) {

=>

if (aKernelUnitLengthX <= 0 || aKernelUnitLengthY <= 0) {
(Assignee)

Comment 10

a year ago
Created attachment 8826472 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

Hi :dholbert,
Can you please help me to review this patch? Thanks
Attachment #8826472 - Flags: review?(dholbert)
(Assignee)

Comment 11

a year ago
Created attachment 8826474 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

Hi :dholbert,
Can you please help me to review this patch? Thanks
Attachment #8826474 - Flags: review?(dholbert)
(Assignee)

Updated

a year ago
Attachment #8826472 - Attachment is obsolete: true
Attachment #8826472 - Flags: review?(dholbert)
Comment on attachment 8826474 [details] [diff] [review]
Error handling when KernelUnitLength is negative or zero.

Review of attachment 8826474 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fix! A few bits of feedback:

First, the commit message ("Error handling when KernelUnitLength is negative or zero.") doesn't quite give enough context (and also incorrectly capitalizes the leading "k").  This might be better:
 "In SVG filter lighting code, bail out if kernelUnitLength is negative or zero"

Second, this needs crashtests to be included. At least these 3:
 * a version of the original testcase here (probably fixed to be valid SVG, cribbing from another SVG file if you like. View-source higlights invalid stuff in red.)
 * a version with kernelUnitLength="-80 10" or something like that, with one negative and one positive value (which I expect wouldn't have been fixed by your original patch here -- this will ensure that we don't accidentally introduce the bug that cku pointed out in comment 9)
 * a version with feDiffuseLighting to exercise the other AddLightingAttributes callsite (which I expect is still buggy with this version of the patch, per my note below)
And you should make sure these crashtests each crash (when loaded) before the fix, and pass after the fix.

These should go in dom/svg/crashtests, and they probably could just be named 1329849-1.svg, 1329849-2.svg, 1329849-3.svg

::: dom/svg/SVGFESpecularLightingElement.cpp
@@ +88,5 @@
> +
> +  if (kernelUnitLength.width <= 0 || kernelUnitLength.height <= 0) {
> +    // According to spec, A negative or zero value is an error. See link below for details.
> +    // https://www.w3.org/TR/SVG/filters.html#feConvolveMatrixElementKernelUnitLengthAttribute
> +    return failureDescription;

Two issues here:

(1) The URI in your comment here has the wrong anchor -- you're linking to the description of this attribute on a *different element* (feConvolveMatrix -- note the feConvolveMatrix in the URI).  You really want to link to:
 https://www.w3.org/TR/SVG/filters.html#feSpecularLightingKernelUnitLengthAttribute
(note that this one has "feSpecularLighting" in its anchor)

(2) This whole new code check needs to move to a more specific place.  This code here doesn't seem like the best place for this check, because:
 * we don't actually use the variable (kernelUnitLength) that you're sanity-checking (so it's odd that we're adding code to look it up, sanity check it, and then throw it away)
 * The place where we *do* use kernelUnitLength (and independently look it up) is inside of AddLightingAttributes, and that function has one other caller (from SVGFEDiffuseLightingElement) and that other caller doesn't benefit from your added check here! So it's probably still crashy even with this patch.  That callsite is here, FWIW:
https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/dom/svg/SVGFEDiffuseLightingElement.cpp#65-74

So, let's insert this check in a more direct place, where the kernelUnitLength is actually used, in AddLightingAttributes:
https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/dom/svg/nsSVGFilters.cpp#518
Attachment #8826474 - Flags: review?(dholbert) → review-
(Assignee)

Comment 13

a year ago
Created attachment 8827466 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

Hi :dholbert,
Thanks for the review. The attached code was modified as your good suggestion. Can you please have a review? Thanks
Attachment #8827466 - Flags: review?(dholbert)
(Assignee)

Updated

a year ago
Attachment #8826474 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8827470 [details] [diff] [review]
Add crash tests.

Hi :dholbert,
This patch intends to add crash test cases. For the version of feDiffuseLighting, I created two test cases as 1329849-3.svg and 1329849-3.svg based on the condition 1 and 2.
All of them won't crash after the patch applied. Can you please also have a review? Thanks.
Attachment #8827470 - Flags: review?(dholbert)
Comment on attachment 8827470 [details] [diff] [review]
Add crash tests.

Review of attachment 8827470 [details] [diff] [review]:
-----------------------------------------------------------------

These files need to be added to the crashtest.list file for their directory (in the correct sorted spot).  Please include them there.

::: dom/svg/crashtests/1329849-1.svg
@@ +1,2 @@
> +<svg height='600'
> +	 xmlns="http://www.w3.org/2000/svg" 

Two whitespace issues with this line -- it's got:
 (1) a tab character at the beginning
 (2) a trailing space character at the end

This affects all 4 of the crashtest files here. Ideally it'd be nice to fix this.  (But also not a huge deal in testcases, so if you don't get to it, it's not the end of the world.)

::: dom/svg/crashtests/1329849-4.svg
@@ +2,5 @@
> +	 xmlns="http://www.w3.org/2000/svg" 
> +     xmlns:xlink="http://www.w3.org/1999/xlink">
> +
> +<filter id='f1'>
> +<feDiffuseLighting kernelUnitLength='-80 10'>

For robustness, could you swap the negative sign to the other value here, so it's "80 -10"?   (So then you'll have one test (#2) testing negative x, and one test (#4) testing negative y)
Comment on attachment 8827466 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

Review of attachment 8827466 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/svg/nsSVGFilters.cpp
@@ +504,5 @@
>  FilterPrimitiveDescription
>  nsSVGFELightingElement::AddLightingAttributes(FilterPrimitiveDescription aDescription,
>                                                nsSVGFilterInstance* aInstance)
>  {
> +  const FilterPrimitiveDescription failureDescription(PrimitiveType::Empty);

Let's just drop this local variable and do "return FilterPrimitiveDescription(PrimitiveType::Empty);" further down.

(This function already has an early-return which looks like that; and it's awkward to declare this variable and then only use it in one of the two failure-return-clauses.  Plus, we don't expect to use this variable most of the time, so it's kinda wasteful to declare/initialize it unconditionally.)
(Assignee)

Comment 17

a year ago
Created attachment 8827736 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

> Let's just drop this local variable and do "return
> FilterPrimitiveDescription(PrimitiveType::Empty);" further down.
> 
> (This function already has an early-return which looks like that; and it's
> awkward to declare this variable and then only use it in one of the two
> failure-return-clauses.  Plus, we don't expect to use this variable most of
> the time, so it's kinda wasteful to declare/initialize it unconditionally.)

Thanks for the great suggestion and let the code better.)
Attachment #8827736 - Flags: review?(dholbert)
(Assignee)

Updated

a year ago
Attachment #8827466 - Attachment is obsolete: true
Attachment #8827466 - Flags: review?(dholbert)
(Assignee)

Comment 18

a year ago
Created attachment 8827738 [details] [diff] [review]
Add crash tests.

For robustness, currently I added 6 test cases for different conditions. Can you please also have a review? Thanks
Attachment #8827738 - Flags: review?(dholbert)
(Assignee)

Updated

a year ago
Attachment #8827470 - Attachment is obsolete: true
Attachment #8827470 - Flags: review?(dholbert)
Comment on attachment 8827736 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

Review of attachment 8827736 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me
Attachment #8827736 - Flags: review?(dholbert) → review+
Comment on attachment 8827738 [details] [diff] [review]
Add crash tests.

Review of attachment 8827738 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8827738 - Flags: review?(dholbert) → review+

Comment 22

a year ago
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f65ce79824
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb7c631c0d6
Add crash tests. r=dholbert

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30f65ce79824
https://hg.mozilla.org/mozilla-central/rev/3bb7c631c0d6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Vincent, could you fill an uplift request to 52? (aurora currently) thanks
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
Flags: needinfo?(vliu)
Flags: in-testsuite+
(Assignee)

Comment 25

a year ago
Comment on attachment 8827736 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Crash happens when user use wrong parameter
[Is this code covered by automated tests?]: crash test
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Add a check to detect wrong parameter
[String changes made/needed]:
Flags: needinfo?(vliu)
Attachment #8827736 - Flags: approval-mozilla-beta?
(Assignee)

Comment 26

a year ago
Comment on attachment 8827738 [details] [diff] [review]
Add crash tests.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Crash happens when user use wrong parameter
[Is this code covered by automated tests?]: crash test
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Add a check to detect wrong parameter
[String changes made/needed]:
Attachment #8827738 - Flags: approval-mozilla-beta?
Blocks: 1332980
Comment on attachment 8827736 [details] [diff] [review]
In SVG filter lighting code, bail out if kernelUnitLength is negative or zero.

svg crash fix, beta52+, should be in b2
Attachment #8827736 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8827738 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Reporter)

Updated

a year ago
See Also: → bug 1358795
No longer blocks: 1332980
Depends on: 1332980
You need to log in before you can comment on or make changes to this bug.