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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: 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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Vincent Liu[:vliu]]

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

Comment 3

[Mason Chang [Inactive] [:mchang]]

Since you've already dug a little bit, are you going to work on this bug? Thanks!

Comment 4

[Vincent Liu[:vliu]]

(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.

Comment 5

[Vincent Liu[:vliu]]

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

Comment 6

[Vincent Liu[:vliu]]

Attachment: Error handling when KernelUnitLength is negative or zero.

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

Comment 7

[Vincent Liu[:vliu]]

Attachment: Error handling when KernelUnitLength is negative or zero.

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

Comment 8

[Vincent Liu[:vliu]]

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

I will re-attach the patch for better solution.

Comment 9

[u459114]

if (aKernelUnitLengthX <= 0 && aKernelUnitLengthY <= 0) {

=>

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

Comment 10

[Vincent Liu[:vliu]]

Attachment: Error handling when KernelUnitLength is negative or zero.

Hi :dholbert,
Can you please help me to review this patch? Thanks

Comment 11

[Vincent Liu[:vliu]]

Attachment: Error handling when KernelUnitLength is negative or zero.

Hi :dholbert,
Can you please help me to review this patch? Thanks

Comment 12

[Daniel Holbert [: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

Comment 13

[Vincent Liu[:vliu]]

Attachment: 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

Comment 14

[Vincent Liu[:vliu]]

Attachment: 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.

Comment 15

[Daniel Holbert [: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 16

[Daniel Holbert [:dholbert]]

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.)

Comment 17

[Vincent Liu[:vliu]]

Attachment: 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.)

Comment 18

[Vincent Liu[:vliu]]

Attachment: Add crash tests.

For robustness, currently I added 6 test cases for different conditions. Can you please also have a review? Thanks

Comment 19

[Daniel Holbert [: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

Comment 20

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 21

[Vincent Liu[:vliu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bfef248612ec06bbab9398153a1cd8f19429ae9

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30f65ce79824
https://hg.mozilla.org/mozilla-central/rev/3bb7c631c0d6

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Vincent, could you fill an uplift request to 52? (aurora currently) thanks

Comment 25

[Vincent Liu[:vliu]]

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]:

Comment 26

[Vincent Liu[:vliu]]

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]:

Comment 27

[Julien Cristau [:jcristau]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a10f4ba73b3e
https://hg.mozilla.org/releases/mozilla-beta/rev/f6822b27c3e9