Avoid SkFixed overflow in decal bitmap procs

The check for decal mode can overflow in SkFixed.  Promote to
64bit (48.16) instead.

Also update can_truncate_to_fixed_for_decal() to take SkFixed params and
used it in ClampX_ClampY_filter_scale_SSE2().

BUG=chromium:675444
R=reed@google.com

CQ_INCLUDE_TRYBOTS=skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD

Change-Id: I759464cdaa5c005159e38e32167fb1937e2a1d28
Reviewed-on: https://skia-review.googlesource.com/6538
Reviewed-by: Cary Clark <caryclark@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>

diff --git a/src/core/SkBitmapProcState_matrix.h b/src/core/SkBitmapProcState_matrix.h
index e0180c6..ea784c6 100644 (file)
--- a/src/core/SkBitmapProcState_matrix.h
+++ b/src/core/SkBitmapProcState_matrix.h
@@ -70,9 +70,10 @@ void SCALE_FILTER_NAME(const SkBitmapProcState& s,
     }
 
 #ifdef CHECK_FOR_DECAL
-    if (can_truncate_to_fixed_for_decal(fx, dx, count, maxX)) {
-        decal_filter_scale(xy, SkFractionalIntToFixed(fx),
-                           SkFractionalIntToFixed(dx), count);
+    const SkFixed fixedFx = SkFractionalIntToFixed(fx);
+    const SkFixed fixedDx = SkFractionalIntToFixed(dx);
+    if (can_truncate_to_fixed_for_decal(fixedFx, fixedDx, count, maxX)) {
+        decal_filter_scale(xy, fixedFx, fixedDx, count);
     } else
 #endif
     {

diff --git a/src/core/SkBitmapProcState_matrix_template.h b/src/core/SkBitmapProcState_matrix_template.h
index 0c93718..c38610a 100644 (file)
--- a/src/core/SkBitmapProcState_matrix_template.h
+++ b/src/core/SkBitmapProcState_matrix_template.h
@@ -36,32 +36,37 @@ void NoFilterProc_Scale(const SkBitmapProcState& s, uint32_t xy[],
 
     const SkFractionalInt dx = s.fInvSxFractionalInt;
 
-    if (tryDecal && can_truncate_to_fixed_for_decal(fx, dx, count, maxX)) {
-        decal_nofilter_scale(xy, SkFractionalIntToFixed(fx),
-                             SkFractionalIntToFixed(dx), count);
-    } else {
-        int i;
-        for (i = (count >> 2); i > 0; --i) {
-            unsigned a, b;
-            a = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
-            b = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
+    if (tryDecal) {
+        const SkFixed fixedFx = SkFractionalIntToFixed(fx);
+        const SkFixed fixedDx = SkFractionalIntToFixed(dx);
+
+        if (can_truncate_to_fixed_for_decal(fixedFx, fixedDx, count, maxX)) {
+            decal_nofilter_scale(xy, fixedFx, fixedDx, count);
+            return;
+        }
+    }
+
+    int i;
+    for (i = (count >> 2); i > 0; --i) {
+        unsigned a, b;
+        a = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
+        b = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
 #ifdef SK_CPU_BENDIAN
-            *xy++ = (a << 16) | b;
+        *xy++ = (a << 16) | b;
 #else
-            *xy++ = (b << 16) | a;
+        *xy++ = (b << 16) | a;
 #endif
-            a = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
-            b = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
+        a = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
+        b = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
 #ifdef SK_CPU_BENDIAN
-            *xy++ = (a << 16) | b;
+        *xy++ = (a << 16) | b;
 #else
-            *xy++ = (b << 16) | a;
+        *xy++ = (b << 16) | a;
 #endif
-        }
-        uint16_t* xx = (uint16_t*)xy;
-        for (i = (count & 3); i > 0; --i) {
-            *xx++ = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
-        }
+    }
+    uint16_t* xx = (uint16_t*)xy;
+    for (i = (count & 3); i > 0; --i) {
+        *xx++ = TileProc::X(s, SkFractionalIntToFixed(fx), maxX); fx += dx;
     }
 }
 

diff --git a/src/core/SkBitmapProcState_utils.h b/src/core/SkBitmapProcState_utils.h
index 3c4c1fa..4609ff3 100644 (file)
--- a/src/core/SkBitmapProcState_utils.h
+++ b/src/core/SkBitmapProcState_utils.h
@@ -1,10 +1,17 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
 #ifndef SkBitmapProcState_utils_DEFINED
 #define SkBitmapProcState_utils_DEFINED
 
 // Helper to ensure that when we shift down, we do it w/o sign-extension
 // so the caller doesn't have to manually mask off the top 16 bits
 //
-static unsigned SK_USHIFT16(unsigned x) {
+static inline unsigned SK_USHIFT16(unsigned x) {
     return x >> 16;
 }
 
@@ -18,10 +25,10 @@ static unsigned SK_USHIFT16(unsigned x) {
  *  the decal_ function just operates on SkFixed. If that were changed, we could
  *  skip the very_small test here.
  */
-static inline bool can_truncate_to_fixed_for_decal(SkFractionalInt frX,
-                                                   SkFractionalInt frDx,
+static inline bool can_truncate_to_fixed_for_decal(SkFixed fx,
+                                                   SkFixed dx,
                                                    int count, unsigned max) {
-    SkFixed dx = SkFractionalIntToFixed(frDx);
+    SkASSERT(count > 0);
 
     // if decal_ kept SkFractionalInt precision, this would just be dx <= 0
     // I just made up the 1/256. Just don't want to perceive accumulated error
@@ -30,11 +37,20 @@ static inline bool can_truncate_to_fixed_for_decal(SkFractionalInt frX,
         return false;
     }
 
+    // Note: it seems the test should be (fx <= max && lastFx <= max); but
+    // historically it's been a strict inequality check, and changing produces
+    // unexpected diffs.  Further investigation is needed.
+
     // We cast to unsigned so we don't have to check for negative values, which
     // will now appear as very large positive values, and thus fail our test!
-    SkFixed fx = SkFractionalIntToFixed(frX);
-    return (unsigned)SkFixedFloorToInt(fx) <= max &&
-           (unsigned)SkFixedFloorToInt(fx + dx * (count - 1)) < max;
+    if ((unsigned)SkFixedFloorToInt(fx) >= max) {
+        return false;
+    }
+
+    // Promote to 64bit (48.16) to avoid overflow.
+    const uint64_t lastFx = fx + sk_64_mul(dx, count - 1);
+
+    return sk_64_isS32(lastFx) && (unsigned)SkFixedFloorToInt(sk_64_asS32(lastFx)) < max;
 }
 
 #endif /* #ifndef SkBitmapProcState_utils_DEFINED */

diff --git a/src/opts/SkBitmapProcState_matrix_neon.h b/src/opts/SkBitmapProcState_matrix_neon.h
index a7f4753..fb91547 100644 (file)
--- a/src/opts/SkBitmapProcState_matrix_neon.h
+++ b/src/opts/SkBitmapProcState_matrix_neon.h
@@ -54,9 +54,10 @@ static void SCALE_NOFILTER_NAME(const SkBitmapProcState& s,
 
 #ifdef CHECK_FOR_DECAL
     // test if we don't need to apply the tile proc
-    if (can_truncate_to_fixed_for_decal(fx, dx, count, maxX)) {
-        decal_nofilter_scale_neon(xy, SkFractionalIntToFixed(fx),
-                             SkFractionalIntToFixed(dx), count);
+    const SkFixed fixedFx = SkFractionalIntToFixed(fx);
+    const SkFixed fixedDx = SkFractionalIntToFixed(dx);
+    if (can_truncate_to_fixed_for_decal(fixedFx, fixedDx, count, maxX)) {
+        decal_nofilter_scale_neon(xy, fixedFx, fixedDx, count);
         return;
     }
 #endif
@@ -309,9 +310,10 @@ static void SCALE_FILTER_NAME(const SkBitmapProcState& s,
 
 #ifdef CHECK_FOR_DECAL
     // test if we don't need to apply the tile proc
-    if (can_truncate_to_fixed_for_decal(fx, dx, count, maxX)) {
-        decal_filter_scale_neon(xy, SkFractionalIntToFixed(fx),
-                             SkFractionalIntToFixed(dx), count);
+    const SkFixed fixedFx = SkFractionalIntToFixed(fx);
+    const SkFixed fixedDx = SkFractionalIntToFixed(dx);
+    if (can_truncate_to_fixed_for_decal(fixedFx, fixedDx, count, maxX)) {
+        decal_filter_scale_neon(xy, fixedFx, fixedDx, count);
         return;
     }
 #endif

diff --git a/src/opts/SkBitmapProcState_opts_SSE2.cpp b/src/opts/SkBitmapProcState_opts_SSE2.cpp
index e45c4ba..fa1e042 100644 (file)
--- a/src/opts/SkBitmapProcState_opts_SSE2.cpp
+++ b/src/opts/SkBitmapProcState_opts_SSE2.cpp
@@ -7,6 +7,7 @@
 
 #include <emmintrin.h>
 #include "SkBitmapProcState_opts_SSE2.h"
+#include "SkBitmapProcState_utils.h"
 #include "SkColorPriv.h"
 #include "SkPaint.h"
 #include "SkUtils.h"
@@ -262,8 +263,7 @@ void ClampX_ClampY_filter_scale_SSE2(const SkBitmapProcState& s, uint32_t xy[],
     SkFixed fx = mapper.fixedX();
 
     // test if we don't need to apply the tile proc
-    if (dx > 0 && (unsigned)(fx >> 16) <= maxX &&
-        (unsigned)((fx + dx * (count - 1)) >> 16) < maxX) {
+    if (can_truncate_to_fixed_for_decal(fx, dx, count, maxX)) {
         if (count >= 4) {
             // SSE version of decal_filter_scale
             while ((size_t(xy) & 0x0F) != 0) {