Fixed issues found by fuzzer
Last week, the fuzzer found a few numerical issue with filters and I had written some fixes for them. Here are the fixes with some unit tests.
For senorblanco : So I figured out what was asserting when we'd get a 0 width "result" in SkBicubicImageFilter::onFilterImage(). Basically, if the "result" SkBitmap object calls SkBitmap::setConfig() with "width" and/or "height" set to 0, then the SkBitmap object will call SkBitmap::reset(), making the SkBitmap object's config invalid. At this point, calling SkBitmap::getAddr32() will assert, even without attempting to dereference the data pointer, because the SkBitmap's config is invalid. If height is valid, but width is 0, then this call to SkBitmap::getAddr32() happens directly in SkBicubicImageFilter::onFilterImage() a few lines lower and asserts right away.
BUG=
R=senorblanco@google.com, senorblanco@chromium.org, bsalomon@google.com
Author: sugoi@chromium.org
Review URL: https://chromiumcodereview.appspot.com/23533042
git-svn-id: http://skia.googlecode.com/svn/trunk@11249 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/include/effects/SkLightingImageFilter.h b/include/effects/SkLightingImageFilter.h
index 07f713b..9c90cd9 100644
--- a/include/effects/SkLightingImageFilter.h
+++ b/include/effects/SkLightingImageFilter.h
@@ -26,7 +26,8 @@
return fX > fY ? (fX > fZ ? fX : fZ) : (fY > fZ ? fY : fZ);
}
void normalize() {
- SkScalar scale = SkScalarInvert(SkScalarSqrt(dot(*this)));
+ // Small epsilon is added to prevent division by 0.
+ SkScalar scale = SkScalarInvert(SkScalarSqrt(dot(*this)) + SK_ScalarNearlyZero);
fX = SkScalarMul(fX, scale);
fY = SkScalarMul(fY, scale);
fZ = SkScalarMul(fZ, scale);
diff --git a/src/effects/SkBicubicImageFilter.cpp b/src/effects/SkBicubicImageFilter.cpp
index b7dffb8..778df3f 100644
--- a/src/effects/SkBicubicImageFilter.cpp
+++ b/src/effects/SkBicubicImageFilter.cpp
@@ -102,6 +102,9 @@
SkScalarMul(SkIntToScalar(src.height()), fScale.fHeight));
SkIRect dstIRect;
dstRect.roundOut(&dstIRect);
+ if (dstIRect.isEmpty()) {
+ return false;
+ }
result->setConfig(src.config(), dstIRect.width(), dstIRect.height());
result->allocPixels();
if (!result->getPixels()) {
diff --git a/src/effects/SkLightingImageFilter.cpp b/src/effects/SkLightingImageFilter.cpp
index 5460559..3141e1d 100644
--- a/src/effects/SkLightingImageFilter.cpp
+++ b/src/effects/SkLightingImageFilter.cpp
@@ -64,9 +64,9 @@
colorScale = SkScalarClampMax(colorScale, SK_Scalar1);
SkPoint3 color(lightColor * colorScale);
return SkPackARGB32(255,
- SkScalarFloorToInt(color.fX),
- SkScalarFloorToInt(color.fY),
- SkScalarFloorToInt(color.fZ));
+ SkClampMax(SkScalarFloorToInt(color.fX), 255),
+ SkClampMax(SkScalarFloorToInt(color.fY), 255),
+ SkClampMax(SkScalarFloorToInt(color.fZ), 255));
}
private:
SkScalar fKD;
@@ -84,10 +84,10 @@
SkScalarPow(normal.dot(halfDir), fShininess));
colorScale = SkScalarClampMax(colorScale, SK_Scalar1);
SkPoint3 color(lightColor * colorScale);
- return SkPackARGB32(SkScalarFloorToInt(color.maxComponent()),
- SkScalarFloorToInt(color.fX),
- SkScalarFloorToInt(color.fY),
- SkScalarFloorToInt(color.fZ));
+ return SkPackARGB32(SkClampMax(SkScalarFloorToInt(color.maxComponent()), 255),
+ SkClampMax(SkScalarFloorToInt(color.fX), 255),
+ SkClampMax(SkScalarFloorToInt(color.fY), 255),
+ SkClampMax(SkScalarFloorToInt(color.fZ), 255));
}
private:
SkScalar fKS;
@@ -676,7 +676,7 @@
: INHERITED(color),
fLocation(location),
fTarget(target),
- fSpecularExponent(specularExponent)
+ fSpecularExponent(SkScalarPin(specularExponent, kSpecularExponentMin, kSpecularExponentMax))
{
fS = target - location;
fS.normalize();
@@ -785,6 +785,9 @@
}
private:
+ static const SkScalar kSpecularExponentMin;
+ static const SkScalar kSpecularExponentMax;
+
typedef SkLight INHERITED;
SkPoint3 fLocation;
SkPoint3 fTarget;
@@ -795,6 +798,11 @@
SkPoint3 fS;
};
+// According to the spec, the specular term should be in the range [1, 128] :
+// http://www.w3.org/TR/SVG/filters.html#feSpecularLightingSpecularExponentAttribute
+const SkScalar SkSpotLight::kSpecularExponentMin = SkFloatToScalar(1.0f);
+const SkScalar SkSpotLight::kSpecularExponentMax = SkFloatToScalar(128.0f);
+
///////////////////////////////////////////////////////////////////////////////
SkLightingImageFilter::SkLightingImageFilter(SkLight* light, SkScalar surfaceScale, SkImageFilter* input, const SkIRect* cropRect)
diff --git a/tests/ImageFilterTest.cpp b/tests/ImageFilterTest.cpp
index da1a719..0032c7f 100644
--- a/tests/ImageFilterTest.cpp
+++ b/tests/ImageFilterTest.cpp
@@ -7,12 +7,44 @@
*/
#include "Test.h"
+#include "SkBicubicImageFilter.h"
+#include "SkBitmap.h"
+#include "SkBitmapDevice.h"
+#include "SkBitmapSource.h"
+#include "SkCanvas.h"
#include "SkColorMatrixFilter.h"
#include "SkColorFilterImageFilter.h"
+#include "SkDeviceImageFilterProxy.h"
+#include "SkLightingImageFilter.h"
#include "SkRect.h"
class ImageFilterTest {
public:
+ static const int kBitmapSize = 4;
+
+ static void make_small_bitmap(SkBitmap& bitmap) {
+ bitmap.setConfig(SkBitmap::kARGB_8888_Config, kBitmapSize, kBitmapSize);
+ bitmap.allocPixels();
+ SkBitmapDevice device(bitmap);
+ SkCanvas canvas(&device);
+ canvas.clear(0x00000000);
+ SkPaint darkPaint;
+ darkPaint.setColor(0xFF804020);
+ SkPaint lightPaint;
+ lightPaint.setColor(0xFF244484);
+ const int i = kBitmapSize / 4;
+ for (int y = 0; y < kBitmapSize; y += i) {
+ for (int x = 0; x < kBitmapSize; x += i) {
+ canvas.save();
+ canvas.translate(SkIntToScalar(x), SkIntToScalar(y));
+ canvas.drawRect(SkRect::MakeXYWH(0, 0, i, i), darkPaint);
+ canvas.drawRect(SkRect::MakeXYWH(i, 0, i, i), lightPaint);
+ canvas.drawRect(SkRect::MakeXYWH(0, i, i, i), lightPaint);
+ canvas.drawRect(SkRect::MakeXYWH(i, i, i, i), darkPaint);
+ canvas.restore();
+ }
+ }
+ }
static SkImageFilter* make_scale(float amount, SkImageFilter* input = NULL) {
SkScalar s = SkFloatToScalar(amount);
@@ -70,6 +102,47 @@
SkAutoTUnref<SkImageFilter> grayWithCrop(make_grayscale(NULL, &cropRect));
REPORTER_ASSERT(reporter, false == grayWithCrop->asColorFilter(NULL));
}
+
+ {
+ // Tests pass by not asserting
+ SkBitmap bitmap, result;
+ make_small_bitmap(bitmap);
+ result.setConfig(SkBitmap::kARGB_8888_Config, kBitmapSize, kBitmapSize);
+ result.allocPixels();
+
+ {
+ // This tests for :
+ // 1 ) location at (0,0,1)
+ SkPoint3 location(0, 0, SK_Scalar1);
+ // 2 ) location and target at same value
+ SkPoint3 target(location.fX, location.fY, location.fZ);
+ // 3 ) large negative specular exponent value
+ SkScalar specularExponent = SkFloatToScalar(-1000);
+
+ SkPaint paint;
+ paint.setImageFilter(SkLightingImageFilter::CreateSpotLitSpecular(
+ location, target, specularExponent, SkFloatToScalar(180),
+ 0xFFFFFFFF, SK_Scalar1, SK_Scalar1, SK_Scalar1,
+ new SkBitmapSource(bitmap)))->unref();
+ SkCanvas canvas(result);
+ SkRect r = SkRect::MakeWH(kBitmapSize, kBitmapSize);
+ canvas.drawRect(r, paint);
+ }
+
+ {
+ // This tests for scale bringing width to 0
+ SkSize scale = SkSize::Make(SkFloatToScalar(-0.001), SK_Scalar1);
+ SkAutoTUnref<SkBicubicImageFilter> bicubic(
+ SkBicubicImageFilter::CreateMitchell(
+ scale, new SkBitmapSource(bitmap)));
+ SkBitmapDevice device(bitmap);
+ SkDeviceImageFilterProxy proxy(&device);
+ SkIPoint loc = SkIPoint::Make(0, 0);
+ // An empty input should early return and return false
+ REPORTER_ASSERT(reporter,
+ !bicubic->filterImage(&proxy, bitmap, SkMatrix::I(), &result, &loc));
+ }
+ }
}
};