1332980 - Assertion failure: aKernelUnitLengthX > 0 (aKernelUnitLengthX can not be a negative or zero value)

Status: REOPENED

(Core :: Graphics, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Assertion failure: aKernelUnitLengthX > 0 (aKernelUnitLengthX can be a negative or zero value), at /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3485

==28655==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f570af1fc79 bp 0x7fff27eea530 sp 0x7fff27eea100 T0)
    #0 0x7f570af1fc78 in already_AddRefed<mozilla::gfx::DataSourceSurface> mozilla::gfx::FilterNodeLightingSoftware<mozilla::gfx::(anonymous namespace)::DistantLightSoftware, mozilla::gfx::(anonymous namespace)::SpecularLightingSoftware>::DoRender<int>(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, int, int) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3489:18
    #1 0x7f570af1ed5f in mozilla::gfx::FilterNodeLightingSoftware<mozilla::gfx::(anonymous namespace)::DistantLightSoftware, mozilla::gfx::(anonymous namespace)::SpecularLightingSoftware>::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3464:12
    #2 0x7f570aec7495 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:613:21
    #3 0x7f570aec8cd0 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
    #4 0x7f570aedf67f in mozilla::gfx::FilterNodeCropSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:3126:10
    #5 0x7f570aec7495 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/gfx/2d/FilterNodeSoftware.cpp:613:21
    #6 0x7f570aea222c 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
    #7 0x7f570af82330 in mozilla::gfx::FilterSupport::RenderFilterDescription
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

We added this assert in bug 1329849

Comment 3

[Vincent Liu[:vliu]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> We added this assert in bug 1329849

The test case is different situation compared with bug 1329849 that kernelUnitLength is a quite large value we didn't considered. I will look into this.

<feSpecularLighting ' kernelUnitLength='2123255551'>

Comment 4

[Vincent Liu[:vliu]]

Correct the title first for the wrong assert message. I will fix it in the following patch.

Comment 5

[Vincent Liu[:vliu]]

Hi Daniel,

After the fix in bug 1329849, a negative or zero value of kernelUnitLength won't cause crash. But in this bug, kernelUnitLength in test case is a quite large value. The cause Gecko see it as positive large value  in [1] but it becomes a negative value in [2]. 


[1]: https://dxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGFilters.cpp#520
[2]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#3485

From looked into the spec, I didn't see any maximum value was defined. Do you have any idea for this case?

Comment 6

[Daniel Holbert [:dholbert]]

Do you know how/where/why it goes from positive to negative?

(Do we scale or multiply the value between [1] and [2] or something, and it wraps around, or what?)

It looks like maybe we could convert FilterNodeSoftware::DoRender() asserts to an error-check-and-return-null, I suppose.  (The function already has an error case that returns null, it seems, so hopefully the caller can handle that gracefully).

But I want to understand slightly better why the negative check at your [1] isn't sufficient, and how we end up wrapping into negative territory later on.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

We do a ceil() call on the float into an int32 and wrap into negative value.

Comment 8

[Daniel Holbert [:dholbert]]

I don't fully understand comment 7 -- but if this is simply integer-overflow from huge-float-to-int conversion, then we should just clamp where we do that conversion.

We should use a known-good helper function to do that clamping -- perhaps NSFloatPixelsToAppUnits or something similar. (which uses NSToCoordRoundWithClamp under the hood, and the "WithClamp" is the key thing that we need here IIUC). (Maybe there's something similar for clamped float-->int conversions in gfx code?)

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Right, sorry, should have been explicit. Code like this [1] (and also in other places):

      Size kernelUnitLength = atts.GetSize(eLightingKernelUnitLength);
      int32_t dx = ceil(kernelUnitLength.width);
      int32_t dy = ceil(kernelUnitLength.height);
      return aInputChangeRegions[0].Inflated(nsIntMargin(dy, dx, dy, dx));


[1] https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#1465

Markus, do we have an existing pattern we use in filters for things like this?

I suppose, overall, we have a few options.  Adding max to the spec is probably not a valid one as numbers get bigger :).  We can just ignore it, not return an error, but do nothing, as we would for negative values.  Or, we can treat these as +info, and investigate each filter in turn, and see if there is a valid operation to be done with +inf.  I don't know if there are any examples for this last one (e.g., places where we use 1/value, where +inf works.)

Comment 10

[Vincent Liu[:vliu]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

> But I want to understand slightly better why the negative check at your [1]
> isn't sufficient, and how we end up wrapping into negative territory later
> on.

As comment 9 said.

Is it possible we treat this like other PrimitiveType in [1]? The range could be 1(ignore it) to a max value.

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#1699

Comment 11

[Vincent Liu[:vliu]]

An example code like this.

   int32_t dx = clamped(int32_t(ceil(kernelUnitLength.width)), 1, MaxValue);
   int32_t dy = clamped(int32_t(ceil(kernelUnitLength.height)), 1, MaxValue);

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

That looks reasonable to me, and I imagine it solves the problem.

It feels like we should have a int32_t FloatToIntWithClamping() inline function/macro somewhere, if this is a common things we'd doing.  Perhaps there are other places where this could be hit.

Comment 13

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1332980-Add-FloatToInt32WithClamping-to-avoid-ov.patch

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Do you know how/where/why it goes from positive to negative?
> 

The place it goes from positive to negative is in [1] where we convert to int32_t.

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#3464

Based on above, I created an inline function FloatToInt32WithClamping() to clamp this conversion. Can you please have a review? Really thanks.

Comment 14

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1332980-Add-crash-test.-r-milan-dholbert.patch

Add crash test.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8831436 [details] [diff] [review]
0001-Bug-1332980-Add-FloatToInt32WithClamping-to-avoid-ov.patch

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

We could argue about the naming, given that we clamp to positive Int32 only, but this is likely fine.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8831437 [details] [diff] [review]
0001-Bug-1332980-Add-crash-test.-r-milan-dholbert.patch

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

I'm assuming we already have test that attempt to use negative numbers.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8831436 [details] [diff] [review]
0001-Bug-1332980-Add-FloatToInt32WithClamping-to-avoid-ov.patch

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

Commit message nit: please add "in SVG filter code".

(Otherwise it's unclear what part of the codebase this is modifying, and that makes it harder to skim hg pushlogs to guess about which changesets might be involved in regressions.)

::: gfx/2d/FilterProcessing.h
@@ +136,5 @@
>  }
>  
> +static inline int32_t FloatToInt32WithClamping(float a)
> +{
> +  return clamped(int32_t(ceil(a)), 1, INT32_MAX);

Three things:

(1) I agree with Milan's naming concern here, and I really think we should address it before landing.  Given that this returns *signed* int32_t, it's extremely likely that people will *incorrectly* assume this utility function preserves negative numbers.  And given that you're exposing this function in a header, people may add new calls to the function all over the place which are broken due to their misinterpretation. So: we really need to give this "Pos" or "Positive" in the name somewhere.

Here are some suggested alternative names which include "Pos" and aren't any longer than your current name:
 FloatToPosInt32WithClamp(...) [same as your current name, but with s/ing/Pos/]
 ClampFloatToPosInt32(...) [a bit shorter, with a more active verb]

(2) Since you're exposing this function in a header file, please add a brief comment (even just a one-liner) above it to explain what it does.

(3) Please add an #include for nsAlgorithm.h to the top of this header file, to provide the clamped() function that you're using here.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8831437 [details] [diff] [review]
0001-Bug-1332980-Add-crash-test.-r-milan-dholbert.patch

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

Just one commit message issue on the crashtests patch -- right now, the commit message is:
>  [PATCH] Bug 1332980 - Add crash test. r=milan, dholbert

Please drop the [PATCH] prefix there, before landing or requesting checkin-needed.

(I'm guessing one of your tools is adding the [PATCH] thing automatically?  If so, you might want to see if there's a way to suppress it.)

Comment 19

[Markus Stange [:mstange]]

(In reply to Milan Sreckovic [:milan] from comment #9)
> Right, sorry, should have been explicit. Code like this [1] (and also in
> other places):
> 
>       Size kernelUnitLength = atts.GetSize(eLightingKernelUnitLength);
>       int32_t dx = ceil(kernelUnitLength.width);
>       int32_t dy = ceil(kernelUnitLength.height);
>       return aInputChangeRegions[0].Inflated(nsIntMargin(dy, dx, dy, dx));
> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#1465
> 
> Markus, do we have an existing pattern we use in filters for things like
> this?

Not that I know of.
For this case however, applying a reasonable maximum (e.g. 1000) shouldn't cause problems. It would be similar to the cap we put on blur radii in various places - we'd only try to handle as much as we can handle and hope that nobody is going to notice for large values.

The use case of specifying kernelUnitLength is to get consistent results across different display densities (because kernelUnitLength defaults to one device pixel, if unspecified), and sometimes to speed up rendering. It's usually one or two CSS pixels, which then gets converted to device pixels, so usually between 1 and 4. Our old filter code (and filter code in other browsers) implemented kernelUnitLength by scaling the input down by that number, applying the filter, and scaling the result up again. This makes the filter faster to compute but gives a slightly blurry result (depending on the number that was chosen). In the new filter implementation I opted for an implementation that always gives sharp outputs, but now I think that was probably the wrong choice.

Comment 20

[Tyson Smith [:tsmith]]

Attachment: test_case_crash.html

I found a variant that triggers a null deref.

Comment 21

[Tyson Smith [:tsmith]]

Attachment: null_crash_log.txt

Comment 22

[Tyson Smith [:tsmith]]

Err sorry I see this was already linked to bug 1329849.

Comment 23

[Vincent Liu[:vliu]]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Comment on attachment 8831436 [details] [diff] [review]
> 0001-Bug-1332980-Add-FloatToInt32WithClamping-to-avoid-ov.patch
> 
> Review of attachment 8831436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We could argue about the naming, given that we clamp to positive Int32 only,
> but this is likely fine.

Thanks, I will correct the naming into the suitable one.

(In reply to Milan Sreckovic [:milan] from comment #16)
> Comment on attachment 8831437 [details] [diff] [review]
> 0001-Bug-1332980-Add-crash-test.-r-milan-dholbert.patch
> 
> Review of attachment 8831437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming we already have test that attempt to use negative numbers.

Bug 1329849 already have test for negative numbers.

Comment 24

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1332980-Add-ClampFloatToPosInt32-to-avoid-overfl.patch

Hi Daniel, Milan,

I'd changed the naming of this utility function, plus adding a brief comment for this function. Can you please have a review to see if it is clear to explain what it does? Thanks.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8832726 [details] [diff] [review]
0001-Bug-1332980-Add-ClampFloatToPosInt32-to-avoid-overfl.patch

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

::: gfx/2d/FilterProcessing.h
@@ +139,5 @@
> +// Clamp a Float-to-Positive-int32 number to between 1 and INT32_MAX.
> +static inline int32_t
> +ClampFloatToPosInt32(float a)
> +{
> +  return clamped(int32_t(ceil(a)), 1, INT32_MAX);

I'm going to r- while we sort out exactly what kind of result we want here.  On a large float, ceil will return float, int32_t cast will wrap to negative value, at which point clamp will return 1.  So, as written, ClampFloatToPosInt32(2.e9) will return INT32_MAX (good), and ClampFloatToPosInt32(3.e9) will return 1 (probably not good.)

We may not need to worry about the behaviour of very large values, as long as we avoid the negative value, but something like a generic clamp function will probably want to have very large values clamped to the maximum, rather than minimum value.  So, let's be explicit here.

In this particular case, I think it's OK to clamp to large values, though not necessarily allow us to go all the way to INT32_MAX.  We probably do not want any filter operations with 2e.9.

While something like this may look like it would work (get the ceiling, clamp, then cast to int32):

int32_t(clamped(ceil(a), 1.f, float(INT32_MAX)));

it doesn't actually, because float(INT32_MAX) > INT32_MAX because of the rounding rules :)

Going to doubles should take care of it:

int32_t(clamped(ceil((double)a), 1.0, double(INT32_MAX)))

Either way, this is deserving of some unit tests.  Whatever we come up with as the solution, run it with negative floating point values, but also with very large values, say 2.e9, 3.e9, 4.e9, 5.e9, and make sure that we don't get 1 as the answer (which we do with the proposed solution.)

Comment 26

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1332980-Add-SurfaceContainsPoint-to-check-whethe.patch

(In reply to Milan Sreckovic [:milan] from comment #25)
> 
> int32_t(clamped(ceil((double)a), 1.0, double(INT32_MAX)))
> 

It is better if clamping in floating and than convert to int32_t to keep as large as positive value. But this large value would cause offset in
[1] goes negative and hit crash in [2]. 1332980-3.svg can reproduce this case actually.

Rethink the call flow, it is weird that we convert float-to-int32 in [2] but we still convert back to float to Inflate for srcRect. 
If we have overflow concern, is it posible we don't convert it before calling DoRender()? Also, adding SurfaceContainsPoint() to check if sourceSurface contains points after KernelUnitLength scaling?


[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#3540
[2]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#3487
[3]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#3513

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8834281 [details] [diff] [review]
0001-Bug-1332980-Add-SurfaceContainsPoint-to-check-whethe.patch

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

I don't feel like I know this code well enough to feel comfortable r+'ing these changes to it.  I think Markus has more history/context here --> punting my review request to him.

While I'm here, though, one commit-message nit:
> Subject: Bug 1332980 - Add SurfaceContainsPoint to check whether overflow or
>  not. r=milan, dholbert

Please don't add any linebreaks in the commit message. (At least, not in the main part.)  And while you're fixing that, remember to replace "dholbert" with "mstange". :D

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8834281 [details] [diff] [review]
0001-Bug-1332980-Add-SurfaceContainsPoint-to-check-whethe.patch

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

Beyond my expertise, leaving the review to Markus.

We should investigate other places where we do the cast/ceil operation, to make sure we don't have the similar problem though.

Comment 29

[Vincent Liu[:vliu]]

Hi :mstang,

Do you think of any suggestion to approach the issue to be fixed? Really thanks.

Comment 30

[Markus Stange [:mstange]]

Sorry, I thought I had already submitted a review comment, but I must have lost it. Let me write it again.

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8834281 [details] [diff] [review]
0001-Bug-1332980-Add-SurfaceContainsPoint-to-check-whethe.patch

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

::: gfx/2d/FilterNodeSoftware.cpp
@@ -3483,5 @@
>  FilterNodeLightingSoftware<LightType, LightingType>::Render(const IntRect& aRect)
>  {
> -  if (mKernelUnitLength.width == floor(mKernelUnitLength.width) &&
> -      mKernelUnitLength.height == floor(mKernelUnitLength.height)) {
> -    return DoRender(aRect, (int32_t)mKernelUnitLength.width, (int32_t)mKernelUnitLength.height);

So the point of this cast-to-int32_t operation was that we'll call a different template specialization of the DoRender function: CoordType would be int32_t in that call, instead of Float. DoRender then will call ConvolvePixel with CoordType=int32_t, and that will call ColorComponentAtPoint, which has an int32_t and a Float overload. The int32_t overload is much faster because it doesn't need to do interpolation.

So I'd like to keep this branch. It would probably be good to add a comment.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Vincent, are you still working on this?

Comment 34

[Vincent Liu[:vliu]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> Vincent, are you still working on this?

No. Set the owner to Nobody.

Comment 35

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Please don't close bugs with test-cases.

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 62/63 but we can still potentially take a patch for 64.

Comment 39

[Tyson Smith [:tsmith]]

The test case with crash in the name DOS'd my browser to the point I had to terminate the process via Task Manager (Windows).