diff options
author | mtklein <mtklein@chromium.org> | 2015-11-19 09:40:48 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-11-19 09:40:48 -0800 |
commit | 6c7b104b4c08ae2332a6ce3c8c906da4e8c51e5f (patch) | |
tree | 40fd55186fda91bcb743bfd9c63ef9913b0ff595 | |
parent | c11a527cd06a0a6c2cef0d986ea8f5ef0989505a (diff) |
Fix UB in SkDivBits
DIVBITS_ITER was shifting bits up into the sign bit, which is a no-no.
This turns numer into a uint32_t to make those defined, and adds a few notes.
x >= 0 is always true for unsigned x, so we needed a few small logic refactors.
BUG=skia:3562
Committed: https://skia.googlesource.com/skia/+/988adddd48322bfa3e3cb0c017cfce71fbbf1123
Review URL: https://codereview.chromium.org/1455163004
-rw-r--r-- | BUILD.public | 2 | ||||
-rw-r--r-- | src/core/SkMath.cpp | 46 | ||||
-rw-r--r-- | tools/dm_flags.json | 4 | ||||
-rwxr-xr-x | tools/dm_flags.py | 5 |
4 files changed, 33 insertions, 24 deletions
diff --git a/BUILD.public b/BUILD.public index a029a9fa02..e76d879bce 100644 --- a/BUILD.public +++ b/BUILD.public @@ -439,7 +439,7 @@ cc_test( "--nogpu", "--verbose", # TODO(mtklein): maybe investigate why these fail? - "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~Math ~RecordDraw_TextBounds", + "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~RecordDraw_TextBounds", "--resourcePath %s/resources" % BASE_DIR, "--images %s/resources" % BASE_DIR, ], diff --git a/src/core/SkMath.cpp b/src/core/SkMath.cpp index af93d7ecb2..9ca1569de6 100644 --- a/src/core/SkMath.cpp +++ b/src/core/SkMath.cpp @@ -45,21 +45,38 @@ int SkCLZ_portable(uint32_t x) { /////////////////////////////////////////////////////////////////////////////// -#define DIVBITS_ITER(n) \ - case n: \ - if ((numer = (numer << 1) - denom) >= 0) \ - result |= 1 << (n - 1); else numer += denom - -int32_t SkDivBits(int32_t numer, int32_t denom, int shift_bias) { - SkASSERT(denom != 0); - if (numer == 0) { + +#define DIVBITS_ITER(k) \ + case k: \ + if (numer*2 >= denom) { \ + numer = numer*2 - denom; \ + result |= 1 << (k-1); \ + } else { \ + numer *= 2; \ + } + +// NOTE: if you're thinking of editing this method, consider replacing it with +// a simple shift and divide. This legacy method predated reliable hardware division. +int32_t SkDivBits(int32_t n, int32_t d, int shift_bias) { + SkASSERT(d != 0); + if (n == 0) { return 0; } - // make numer and denom positive, and sign hold the resulting sign - int32_t sign = SkExtractSign(numer ^ denom); - numer = SkAbs32(numer); - denom = SkAbs32(denom); + // Make numer and denom positive, and sign hold the resulting sign + // We'll be left-shifting numer, so it's important it's a uint32_t. + // We put denom in a uint32_t just to keep things simple. + int32_t sign = SkExtractSign(n ^ d); +#if defined(SK_SUPPORT_LEGACY_DIVBITS_UB) + // Blink layout tests are baselined to Clang optimizing through the UB. + int32_t numer = SkAbs32(n); + int32_t denom = SkAbs32(d); +#else + uint32_t numer = SkAbs32(n); + uint32_t denom = SkAbs32(d); +#endif + + // It's probably a bug to use n or d below here. int nbits = SkCLZ(numer) - 1; int dbits = SkCLZ(denom) - 1; @@ -78,10 +95,9 @@ int32_t SkDivBits(int32_t numer, int32_t denom, int shift_bias) { SkFixed result = 0; // do the first one - if ((numer -= denom) >= 0) { + if (numer >= denom) { + numer -= denom; result = 1; - } else { - numer += denom; } // Now fall into our switch statement if there are more bits to compute diff --git a/tools/dm_flags.json b/tools/dm_flags.json index be6bc3d747..3c557865dc 100644 --- a/tools/dm_flags.json +++ b/tools/dm_flags.json @@ -1221,9 +1221,7 @@ "_", "image", "_", - "interlaced3.png", - "--match", - "~Math" + "interlaced3.png" ], "Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Valgrind": [ "--matrix", diff --git a/tools/dm_flags.py b/tools/dm_flags.py index 7c901891ca..5341b221da 100755 --- a/tools/dm_flags.py +++ b/tools/dm_flags.py @@ -169,11 +169,6 @@ def get_args(bot): if 'Valgrind' in bot: # skia:3021 match.append('~Threaded') - # skia:3562 - if ('TSAN' in bot or - 'Test-Mac10.8-Clang-MacMini4.1-CPU-SSE4-x86_64-Release' in bot): - match.append('~Math') - if 'GalaxyS3' in bot: # skia:1699 match.append('~WritePixels') |