Bug 141596 – FEGaussianBlur::calculateUnscaledKernelSize does unspeakable things with signed and unsigned values

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

The 'calculateUnscaledKernelSize' method improperly treats signed and unsigned values as interchangeable. This creates problems when we have extremely large stdDevication values (such as in the test case "css3/filters/huge-blur-value.html"). When the standard deviation supplied to the routine overflows a int value, we pass 

In this pathological case, the Std Deviation used for the blur is the float 2.14748365E+9. When this is cast as an unsigned, then assigned to a signed integer, we end up with -257759744.

The minimum of -257759744 and 500 turns out to be -257759744.

    IntSize kernelSize;

    // Limit the kernel size to 500. A bigger radius won't make a big difference for the result image but
    // inflates the absolute paint rect too much. This is compatible with Firefox' behavior.
    if (stdDeviation.x()) {
        int size = std::max<unsigned>(2, static_cast<unsigned>(floorf(stdDeviation.x() * gaussianKernelFactor() + 0.5f)));
        kernelSize.setWidth(std::min(size, gMaxKernelSize));
    }

Later, this kernelSize is cast back to an unsigned, bypassing this ceiling, and subjecting other code to values far in excess of 500.

[Radar WebKit Bug Importer]

<rdar://problem/19837103>

[Brent Fulgham]

Created attachment 246582 [details]
Patch

[zalan]

Comment on attachment 246582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479
> +        kernelSize.setWidth(static_cast<int>(clampedSize));

I'd rather use clampTo and not static casting.

[Brent Fulgham]

Comment on attachment 246582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review

>> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479
>> +        kernelSize.setWidth(static_cast<int>(clampedSize));
> 
> I'd rather use clampTo and not static casting.

OK!

[Brent Fulgham]

Created attachment 246591 [details]
Patch

[Brent Fulgham]

Created attachment 246593 [details]
Patch

[zalan]

Comment on attachment 246593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:469
> +static int clampedKernelSize(float value)

clampedToKernelSize() might be a better name.

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:475
> +    unsigned clampedSize = std::min(size, static_cast<unsigned>(gMaxKernelSize));
> +    return clampTo<int>(clampedSize);

the clampedSize variable is a bit redundant, i'd just go with clampTo<int>(std::min(size, static_cast<unsigned>(gMaxKernelSize)));

[Brent Fulgham]

Committed r180147: <http://trac.webkit.org/changeset/180147>

[Simon Fraser (smfr)]

Comment on attachment 246593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:473
> +    unsigned size = std::max<unsigned>(2, static_cast<unsigned>(floorf(value * gaussianKernelFactor() + 0.5f)));

Why not just do this math as floats, then clamp to int directly without going via unsigned?