From 9953737bcf885a52c08ade6c503f2202e4dd9aa5 Mon Sep 17 00:00:00 2001 From: Florin Malita Date: Wed, 4 Jan 2017 13:01:55 -0500 Subject: [PATCH] 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 Commit-Queue: Florin Malita --- src/core/SkBitmapProcState_matrix.h | 7 +++-- src/core/SkBitmapProcState_matrix_template.h | 45 +++++++++++++++------------- src/core/SkBitmapProcState_utils.h | 30 ++++++++++++++----- src/opts/SkBitmapProcState_matrix_neon.h | 14 +++++---- src/opts/SkBitmapProcState_opts_SSE2.cpp | 4 +-- 5 files changed, 62 insertions(+), 38 deletions(-) diff --git a/src/core/SkBitmapProcState_matrix.h b/src/core/SkBitmapProcState_matrix.h index e0180c6..ea784c6 100644 --- 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 --- 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 --- 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 --- 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 --- a/src/opts/SkBitmapProcState_opts_SSE2.cpp +++ b/src/opts/SkBitmapProcState_opts_SSE2.cpp @@ -7,6 +7,7 @@ #include #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) { -- 2.7.4