307622 - crash with feGaussianBlur when stdDeviation="0"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Holger Will]

firfox crashes if one or both oft the parameters passed in by stdDeviation are 0.

1. <feGaussianBlur in="SourceGraphic" stdDeviation="0" result="blur"/>

this is what the spec says about zero as parameter:
A value of zero disables the effect of the given filter primitive (i.e., the
result is a transparent black image)

Comment 1

[Holger Will]

Attachment: testcase

crash

Comment 2

[Alex Menzies]

Attachment: Zero Gaussian Fix

Patch fixes the case when stdDeviation is 0

Comment 3

[tor]

Check should be done at a higher level, and shortcut going through the filtering.

Comment 4

[Alex Menzies]

Attachment: Updated Patch

Comment 5

[tor]

At least with my interpretation of the subregion language of the specification, we need to run fixup on the target.

Would be nice if the WG would ever answer this:

  http://lists.w3.org/Archives/Public/www-svg/2006Jan/0432.html

Comment 6

[Alex Menzies]

Attachment: Fixed style issues

Patch with corrected style issues

Comment 7

[tor]

You should also check for negative deviations in Gaussian::Filter, and return an error in that case.

Comment 8

[Alex Menzies]

Attachment: Patch with negative error check

Added error checking for negative stdDeviation

Comment 9

[tor]

Comment on attachment 226362 [details] [diff] [review]
Patch with negative error check

>+static nsresult
> gaussianBlur(PRUint8 *aInput, PRUint8 *aOutput,
>              PRUint32 aLength, PRInt32 aStride, nsRect aRegion,
>              float aStdX, float aStdY)
> {
>   PRUint32 dX, dY;
>   dX = (PRUint32) floor(aStdX * 3*sqrt(2*M_PI)/4 + 0.5);
>   dY = (PRUint32) floor(aStdY * 3*sqrt(2*M_PI)/4 + 0.5);
> 
>   PRUint8 *tmp = new PRUint8[aLength];
> 
>+  if (aStdX < 0 || aStdY < 0)
>+    return NS_ERROR_FAILURE;
>+  if (aStdX == 0 || aStdY == 0) {
>+    memset(aOutput, 0, aLength);
>+    return NS_OK;
>+  }

It's only by happy coincidence that bad things don't happen if aStdX or aStdY is a very small positive number (say 0.001), as dX or dY will be zero.  The following code will then pass in 0xffffffff as one of the blur lobe sizes, but wrapping in the callees will cause boxSize to be zero.  We'll end up with a black image - not sure if that's what should happen.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 226362 [details] [diff] [review]
Patch with negative error check

>-static void
>+static nsresult
> gaussianBlur(PRUint8 *aInput, PRUint8 *aOutput,
>              PRUint32 aLength, PRInt32 aStride, nsRect aRegion,
>              float aStdX, float aStdY)
> {
>   PRUint32 dX, dY;
>   dX = (PRUint32) floor(aStdX * 3*sqrt(2*M_PI)/4 + 0.5);
>   dY = (PRUint32) floor(aStdY * 3*sqrt(2*M_PI)/4 + 0.5);
> 
>   PRUint8 *tmp = new PRUint8[aLength];
> 
>+  if (aStdX < 0 || aStdY < 0)
>+    return NS_ERROR_FAILURE;
>+  if (aStdX == 0 || aStdY == 0) {
>+    memset(aOutput, 0, aLength);
>+    return NS_OK;
>+  }

That will leak tmp. Just put the checks at the very begining of the function body as early returns.

Comment 11

[Jonathan Watt [:jwatt]]

>+  if (aStdX < 0 || aStdY < 0)
>+    return NS_ERROR_FAILURE;

And please either put a blank line after this and/or wrap it in curly brackets. Probably both or at least the latter if we're conforming to the general gecko coding style.

Comment 12

[Alex Menzies]

Attachment: Patch fixes low stdDev and memory leaks

This version of the patch fixes the style of the “if” statements and moves them up to prevent memory leakage.  An additional check has been added for the case when aStdX/aStdY are very small positive values.  In this case dX/dY will be zero and the boxBlur routine cannot be used.  Currently, the affect of such a small blur is assumed to be negligible and the source image is copied to the output without being blurred.  This is a reasonable thing to do because scaling a filtered object will also scale the aStdX/aStdY variables proportionally before the filter is applied.  

It is important to note that the boxBlur method used to approximate a Gaussian blur looses accuracy when aStdX/aStdY are less than 2.  However, this error is also assumed to be negligible for the reason  above.  A value of 2 results in a very small blur and any errors in this blur will be hard to see.  If the object is enlarged, the standard deviation being used will be scaled up and the box blur Gaussian approximation will become more accurate.

Comment 13

[tor]

Comment on attachment 227468 [details] [diff] [review]
Patch fixes low stdDev and memory leaks

File a bug about doing the correct thing for small stdX/Y.

Comment 14

[Alex Menzies]

Attachment: Version for checkin

Comment 15

[tor]

Checked in for Alex.