turn on alignment sanitizer

This sanitizer checks for overaligned reads and writes, or put another way, use of underaligned pointers. This usually happens when you cast, e.g. char* to int* without checking that the char* is 4-byte aligned. Each of the changes under src/ fixes something just like that. The unusual setup for tools/xsan.blacklist is there to force a rebuild whenever tools/xsan.blacklist changes. I spent a good few minutes debugging rebuilds not happening this morning, perhaps from some strange ccache interaction. Align SkTextBlobs as void* (today they're just 4-byte) so the SkTextBlob::RunRecords we put after them in SkTextBlobBuilder buffers are properly aligned (for the SkTypeface* inside). There's no obvious error in void SkRRect::computeType(), but one bot seems to have seen some sort of issue with SK_AT_SCOPE_EXIT(SkASSERT(this->isValid())); I can't reproduce it locally, so I'm just going to unroll it. Change-Id: I904d94f65f695e1b626b684c32216a4930b72b0c Reviewed-on: https://skia-review.googlesource.com/146104 Commit-Queue: Mike Klein <[email protected]> Reviewed-by: Florin Malita <[email protected]> Reviewed-by: Mike Reed <[email protected]> Reviewed-by: Ben Wagner <[email protected]>
rust-skia · Aug 9, 2018 · 475c5e9 · 475c5e9
1 parent f1b1464
commit 475c5e9
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 22 deletions.
diff --git a/gn/BUILD.gn b/gn/BUILD.gn
@@ -231,7 +231,7 @@ config("default") {
     fyi_sanitizers = fyi_sanitize
     if (sanitize == "ASAN" || sanitize == "UBSAN") {
       # ASAN implicitly runs all UBSAN checks also.
-      sanitizers = "bool,float-cast-overflow,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound"
+      sanitizers = "alignment,bool,float-cast-overflow,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound"
 
       if (fyi_sanitize == "" && !is_android) {
         fyi_sanitizers = "float-divide-by-zero"
@@ -266,9 +266,13 @@ config("default") {
       sanitizers = "memory"
     }
 
+    _blacklist = rebase_path("../tools/xsan.blacklist")
+
     cflags += [
       "-fsanitize=$sanitizers,$fyi_sanitizers",
       "-fno-sanitize-recover=$sanitizers",
+      "-fsanitize-blacklist=$_blacklist",
+      "-include$_blacklist",
     ]
     if (!is_win) {
       cflags += [ "-fno-omit-frame-pointer" ]

diff --git a/include/core/SkTextBlob.h b/include/core/SkTextBlob.h
@@ -21,7 +21,7 @@ struct SkDeserialProcs;
 
     SkTextBlob combines multiple text runs into an immutable, ref-counted structure.
 */
-class SK_API SkTextBlob final : public SkNVRefCnt<SkTextBlob> {
+class SK_API alignas(void*) SkTextBlob final : public SkNVRefCnt<SkTextBlob> {
 public:
     /**
      *  Returns a conservative blob bounding box.

diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
@@ -6,7 +6,6 @@
  */
 
 #include "SkRRectPriv.h"
-#include "SkScopeExit.h"
 #include "SkBuffer.h"
 #include "SkMalloc.h"
 #include "SkMatrix.h"
@@ -323,14 +322,13 @@ static bool radii_are_nine_patch(const SkVector radii[4]) {
 
 // There is a simplified version of this method in setRectXY
 void SkRRect::computeType() {
-    SK_AT_SCOPE_EXIT(SkASSERT(this->isValid()));
-
     if (fRect.isEmpty()) {
         SkASSERT(fRect.isSorted());
         for (size_t i = 0; i < SK_ARRAY_COUNT(fRadii); ++i) {
             SkASSERT((fRadii[i] == SkVector{0, 0}));
         }
         fType = kEmpty_Type;
+        SkASSERT(this->isValid());
         return;
     }
 
@@ -350,6 +348,7 @@ void SkRRect::computeType() {
 
     if (allCornersSquare) {
         fType = kRect_Type;
+        SkASSERT(this->isValid());
         return;
     }
 
@@ -360,6 +359,7 @@ void SkRRect::computeType() {
         } else {
             fType = kSimple_Type;
         }
+        SkASSERT(this->isValid());
         return;
     }
 
@@ -368,6 +368,7 @@ void SkRRect::computeType() {
     } else {
         fType = kComplex_Type;
     }
+    SkASSERT(this->isValid());
 }
 
 static bool matrix_only_scale_and_translate(const SkMatrix& matrix) {

diff --git a/src/core/SkWriter32.h b/src/core/SkWriter32.h
@@ -113,7 +113,9 @@ class SK_API SkWriter32 : SkNoncopyable {
     }
 
     void writePtr(void* value) {
-        *(void**)this->reserve(sizeof(value)) = value;
+        // this->reserve() only returns 4-byte aligned pointers,
+        // so this may be an under-aligned write if we were to do this like the others.
+        memcpy(this->reserve(sizeof(value)), &value, sizeof(value));
     }
 
     void writeScalar(SkScalar value) {

diff --git a/src/opts/Sk4px_SSE2.h b/src/opts/Sk4px_SSE2.h
@@ -65,7 +65,8 @@ inline Sk4px Sk4px::Wide::div255() const {
     }
 
     inline Sk4px Sk4px::Load4Alphas(const SkAlpha a[4]) {
-        uint32_t as = *(const uint32_t*)a;
+        uint32_t as;
+        memcpy(&as, a, 4);
         __m128i splat = _mm_set_epi8(3,3,3,3, 2,2,2,2, 1,1,1,1, 0,0,0,0);
         return Sk16b(_mm_shuffle_epi8(_mm_cvtsi32_si128(as), splat));
     }
@@ -80,16 +81,20 @@ inline Sk4px Sk4px::Wide::div255() const {
     }
 
     inline Sk4px Sk4px::Load4Alphas(const SkAlpha a[4]) {
-        __m128i as = _mm_cvtsi32_si128(*(const uint32_t*)a);  // ____ ____ ____ 3210
-        as = _mm_unpacklo_epi8 (as, as);                      // ____ ____ 3322 1100
-        as = _mm_unpacklo_epi16(as, as);                      // 3333 2222 1111 0000
+        __m128i as;
+        memcpy(&as, a, 4);                   // ____ ____ ____ 3210
+        as = _mm_unpacklo_epi8 (as, as);     // ____ ____ 3322 1100
+        as = _mm_unpacklo_epi16(as, as);     // 3333 2222 1111 0000
         return Sk16b(as);
     }
 #endif
 
 inline Sk4px Sk4px::Load2Alphas(const SkAlpha a[2]) {
-    uint32_t as = *(const uint16_t*)a;   // Aa -> Aa00
-    return Load4Alphas((const SkAlpha*)&as);
+    uint16_t alphas;
+    memcpy(&alphas, a, 2);
+    uint32_t alphas_and_two_zeros = alphas;   // Aa -> Aa00
+
+    return Load4Alphas((const SkAlpha*)&alphas_and_two_zeros);
 }
 
 inline Sk4px Sk4px::zeroColors() const {

diff --git a/src/sfnt/SkOTTable_name.cpp b/src/sfnt/SkOTTable_name.cpp
@@ -13,15 +13,22 @@
 #include "SkTemplates.h"
 #include "SkUtils.h"
 
-static SkUnichar SkUTF16BE_NextUnichar(const uint16_t** srcPtr) {
+static SkUnichar next_unichar_UTF16BE(const char** srcPtr) {
     SkASSERT(srcPtr && *srcPtr);
 
-    const uint16_t* src = *srcPtr;
-    SkUnichar c = SkEndian_SwapBE16(*src++);
+    const char* src = *srcPtr;
+    uint16_t lo;
+    memcpy(&lo, src, 2);
+    src += 2;
+
+    SkUnichar c = SkEndian_SwapBE16(lo);
 
     SkASSERT(!SkUTF16_IsLowSurrogate(c));
     if (SkUTF16_IsHighSurrogate(c)) {
-        unsigned c2 = SkEndian_SwapBE16(*src++);
+        uint16_t hi;
+        memcpy(&hi, src, 2);
+        src += 2;
+        unsigned c2 = SkEndian_SwapBE16(hi);
         SkASSERT(SkUTF16_IsLowSurrogate(c2));
 
         c = (c << 10) + c2 + (0x10000 - (0xD800 << 10) - 0xDC00);
@@ -30,14 +37,15 @@ static SkUnichar SkUTF16BE_NextUnichar(const uint16_t** srcPtr) {
     return c;
 }
 
-static void SkStringFromUTF16BE(const uint16_t* utf16be, size_t length, SkString& utf8) {
+static void SkString_from_UTF16BE(const char* utf16be, size_t length, SkString& utf8) {
+    // Note that utf16be may not be 2-byte aligned.
     SkASSERT(utf16be != nullptr);
 
     utf8.reset();
     size_t numberOf16BitValues = length / 2;
-    const uint16_t* end = utf16be + numberOf16BitValues;
+    const char* end = utf16be + numberOf16BitValues*2;
     while (utf16be < end) {
-        utf8.appendUnichar(SkUTF16BE_NextUnichar(&utf16be));
+        utf8.appendUnichar(next_unichar_UTF16BE(&utf16be));
     }
 }
 
@@ -475,7 +483,7 @@ bool SkOTTableName::Iterator::next(SkOTTableName::Iterator::Record& record) {
             }
         case SkOTTableName::Record::PlatformID::Unicode:
         case SkOTTableName::Record::PlatformID::ISO:
-            SkStringFromUTF16BE((const uint16_t*)nameString, nameLength, record.name);
+            SkString_from_UTF16BE(nameString, nameLength, record.name);
             break;
 
         case SkOTTableName::Record::PlatformID::Macintosh:
@@ -513,8 +521,8 @@ bool SkOTTableName::Iterator::next(SkOTTableName::Iterator::Record& record) {
 
             uint16_t offset = SkEndian_SwapBE16(languageTagRecord[languageTagRecordIndex].offset);
             uint16_t length = SkEndian_SwapBE16(languageTagRecord[languageTagRecordIndex].length);
-            const uint16_t* string = SkTAddOffset<const uint16_t>(stringTable, offset);
-            SkStringFromUTF16BE(string, length, record.language);
+            const char* string = SkTAddOffset<const char>(stringTable, offset);
+            SkString_from_UTF16BE(string, length, record.language);
             return true;
         }
     }

diff --git a/tools/xsan.blacklist b/tools/xsan.blacklist
@@ -0,0 +1,15 @@
+#if 0
+
+#  This file must be a no-op C #include header, and a valid *SAN blacklist file.
+#  Luckily, anything starting with # is a comment to *SAN blacklist files,
+#  and anything inside #if 0 is ignored by C.  Yippee!
+#
+#  If you want to type '*', type '.*' instead.  Don't make C comments!
+
+# libpng and zlib both dereference under-aligned pointers.
+# TODO: it'd be nice to tag these as [alignment] only but our Mac toolchain can't yet.
+# [alignment]
+src:.*third_party/externals/libpng/intel/filter_sse2_intrinsics.c
+src:.*third_party/externals/zlib/deflate.c
+
+#endif