From 9d91eb313623dca16bc988bad2e8b01712e4c578 Mon Sep 17 00:00:00 2001 From: reed Date: Wed, 28 Jan 2015 11:44:48 -0800 Subject: [PATCH] add more checks for computing clamp counts, remove dead code BUG=448299 Review URL: https://codereview.chromium.org/886473003 --- src/effects/gradients/SkClampRange.cpp | 48 +++++++++++++++++----- src/effects/gradients/SkClampRange.h | 9 ++++ src/effects/gradients/SkLinearGradient.cpp | 2 + tests/GradientTest.cpp | 25 +++++++++++ 4 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/effects/gradients/SkClampRange.cpp b/src/effects/gradients/SkClampRange.cpp index 4f8611281a..0bf30f14ea 100644 --- a/src/effects/gradients/SkClampRange.cpp +++ b/src/effects/gradients/SkClampRange.cpp @@ -1,4 +1,3 @@ - /* * Copyright 2011 Google Inc. * @@ -6,8 +5,33 @@ * found in the LICENSE file. */ - #include "SkClampRange.h" +#include "SkMath.h" + +static int SkCLZ64(uint64_t value) { + int count = 0; + if (value >> 32) { + value >>= 32; + } else { + count += 32; + } + return count + SkCLZ(SkToU32(value)); +} + +static bool sk_64_smul_check(int64_t a, int64_t b, int64_t* result) { + // Do it the slow way until we have some assembly. + int64_t ua = SkTAbs(a); + int64_t ub = SkTAbs(b); + int zeros = SkCLZ64(ua) + SkCLZ64(ub); + // this is a conservative check: it may return false when in fact it would not have overflowed. + // Hackers Delight uses 34 as its convervative check, but that is for 32x32 multiplies. + // Since we are looking at 64x64 muls, we add 32 to the check. + if (zeros < (32 + 34)) { + return false; + } + *result = a * b; + return true; +} /* * returns [0..count] for the number of steps (<= count) for which x0 <= edge @@ -58,6 +82,14 @@ void SkClampRange::init(SkGradFixed fx0, SkGradFixed dx0, int count, int v0, int int64_t dx = dx0; // start with ex equal to the last computed value + int64_t count_times_dx; + if (!sk_64_smul_check(count - 1, dx, &count_times_dx)) { + // we can't represent the computed end in 32.32, so just draw something (first color) + fCount1 = fCount2 = 0; + fCount0 = count; + return; + } + int64_t ex = fx + (count - 1) * dx; if ((uint64_t)(fx | ex) <= kFracMax_SkGradFixed) { @@ -77,8 +109,6 @@ void SkClampRange::init(SkGradFixed fx0, SkGradFixed dx0, int count, int v0, int return; } - int extraCount = 0; - // now make ex be 1 past the last computed value ex += dx; @@ -93,11 +123,15 @@ void SkClampRange::init(SkGradFixed fx0, SkGradFixed dx0, int count, int v0, int fCount0 = chop(fx, 0, ex, dx, count); + SkASSERT(fCount0 >= 0); + SkASSERT(fCount0 <= count); count -= fCount0; fx += fCount0 * dx; SkASSERT(fx >= 0); SkASSERT(fCount0 == 0 || (fx - dx) < 0); fCount1 = chop(fx, kFracMax_SkGradFixed, ex, dx, count); + SkASSERT(fCount1 >= 0); + SkASSERT(fCount1 <= count); count -= fCount1; fCount2 = count; @@ -121,10 +155,4 @@ void SkClampRange::init(SkGradFixed fx0, SkGradFixed dx0, int count, int v0, int if (fCount1 > 0) { fFx1 = fx0 + fCount0 * dx; } - - if (dx > 0) { - fCount2 += extraCount; - } else { - fCount0 += extraCount; - } } diff --git a/src/effects/gradients/SkClampRange.h b/src/effects/gradients/SkClampRange.h index e970d4319f..945f9a7ff1 100644 --- a/src/effects/gradients/SkClampRange.h +++ b/src/effects/gradients/SkClampRange.h @@ -35,6 +35,15 @@ struct SkClampRange { void init(SkGradFixed fx, SkGradFixed dx, int count, int v0, int v1); + void validate(int count) const { +#ifdef SK_DEBUG + SkASSERT(fCount0 >= 0); + SkASSERT(fCount1 >= 0); + SkASSERT(fCount2 >= 0); + SkASSERT(fCount0 + fCount1 + fCount2 == count); +#endif + } + private: void initFor1(SkGradFixed fx); }; diff --git a/src/effects/gradients/SkLinearGradient.cpp b/src/effects/gradients/SkLinearGradient.cpp index b34c7964ee..c7d845a188 100644 --- a/src/effects/gradients/SkLinearGradient.cpp +++ b/src/effects/gradients/SkLinearGradient.cpp @@ -149,6 +149,7 @@ void shadeSpan_linear_clamp(TileProc proc, SkGradFixed dx, SkGradFixed fx, int toggle, int count) { SkClampRange range; range.init(fx, dx, count, 0, SkGradientShaderBase::kCache32Count - 1); + range.validate(count); if ((count = range.fCount0) > 0) { sk_memset32_dither(dstC, @@ -332,6 +333,7 @@ void shadeSpan16_linear_clamp(TileProc proc, SkGradFixed dx, SkGradFixed fx, int toggle, int count) { SkClampRange range; range.init(fx, dx, count, 0, SkGradientShaderBase::kCache32Count - 1); + range.validate(count); if ((count = range.fCount0) > 0) { dither_memset16(dstC, diff --git a/tests/GradientTest.cpp b/tests/GradientTest.cpp index e6fd7b99fd..f530b3e263 100644 --- a/tests/GradientTest.cpp +++ b/tests/GradientTest.cpp @@ -12,6 +12,30 @@ #include "SkTemplates.h" #include "Test.h" +// https://code.google.com/p/chromium/issues/detail?id=448299 +// Giant (inverse) matrix causes overflow when converting/computing using 32.32 +// Before the fix, we would assert (and then crash). +static void test_big_grad(skiatest::Reporter* reporter) { + const SkColor colors[] = { SK_ColorRED, SK_ColorBLUE }; + const SkPoint pts[] = {{ 15, 14.7112684f }, { 0.709064007f, 12.6108112f }}; + SkShader* s = SkGradientShader::CreateLinear(pts, colors, NULL, 2, SkShader::kClamp_TileMode); + SkPaint paint; + paint.setShader(s)->unref(); + + SkBitmap bm; + bm.allocN32Pixels(2000, 1); + SkCanvas c(bm); + + const SkScalar affine[] = { + 1.06608627e-06f, 4.26434525e-07f, 6.2855f, 2.6611f, 273.4393f, 244.0046f + }; + SkMatrix matrix; + matrix.setAffine(affine); + c.concat(matrix); + + c.drawPaint(paint); +} + struct GradRec { int fColorCount; const SkColor* fColors; @@ -192,4 +216,5 @@ static void TestGradientShaders(skiatest::Reporter* reporter) { DEF_TEST(Gradient, reporter) { TestGradientShaders(reporter); TestConstantGradient(reporter); + test_big_grad(reporter); }