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 <mtklein@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Mike Reed <reed@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
diff --git a/gn/BUILD.gn b/gn/BUILD.gn
index ea6c819..7edff66 100644
--- a/gn/BUILD.gn
+++ b/gn/BUILD.gn
@@ -231,7 +231,7 @@
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 @@
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
index 0276f32..9253780 100644
--- a/include/core/SkTextBlob.h
+++ b/include/core/SkTextBlob.h
@@ -21,7 +21,7 @@
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
index 6a6b24b..669dbd2 100644
--- 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 @@
// 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 @@
if (allCornersSquare) {
fType = kRect_Type;
+ SkASSERT(this->isValid());
return;
}
@@ -360,6 +359,7 @@
} else {
fType = kSimple_Type;
}
+ SkASSERT(this->isValid());
return;
}
@@ -368,6 +368,7 @@
} 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
index de33c5c..a29fba0 100644
--- a/src/core/SkWriter32.h
+++ b/src/core/SkWriter32.h
@@ -113,7 +113,9 @@
}
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
index dc0c8ac..624fc22 100644
--- a/src/opts/Sk4px_SSE2.h
+++ b/src/opts/Sk4px_SSE2.h
@@ -65,7 +65,8 @@
}
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::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
index 20d97b6..05230f4 100644
--- 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 @@
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 @@
}
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 @@
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
new file mode 100644
index 0000000..df2b384
--- /dev/null
+++ 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