Add sk_careful_memcpy to catch undefined behavior in memcpy.
It's undefined behavior to pass null as src or dst to memcpy, even if len is 0.
This currently triggers -fsanitize=attribute-nonnull warnings, but also can
lead to very unexpected code generation with GCC.
sk_careful_memcpy() checks len first before calling memcpy(),
which prevents that weird undefined situation.
This allows me to mark all sanitizers as no-recover, i.e. make-the-bots-red fatal.
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot
BUG=skia:4641
NOTREECHECKS=true
Review URL: https://codereview.chromium.org/1510683002
diff --git a/gyp/common_conditions.gypi b/gyp/common_conditions.gypi
index 7cc96a7..7a03f64 100644
--- a/gyp/common_conditions.gypi
+++ b/gyp/common_conditions.gypi
@@ -436,6 +436,7 @@
[ 'skia_sanitizer', {
'cflags': [
'-fsanitize=<(skia_sanitizer)',
+ '-fno-sanitize-recover=<(skia_sanitizer)',
],
'ldflags': [
'-fsanitize=<(skia_sanitizer)',
diff --git a/include/core/SkTArray.h b/include/core/SkTArray.h
index 401f708..9f1bfa1 100644
--- a/include/core/SkTArray.h
+++ b/include/core/SkTArray.h
@@ -23,11 +23,11 @@
}
template<typename T>
inline void copy(SkTArray<T, true>* self, const T* array) {
- memcpy(self->fMemArray, array, self->fCount * sizeof(T));
+ sk_careful_memcpy(self->fMemArray, array, self->fCount * sizeof(T));
}
template<typename T>
inline void copyAndDelete(SkTArray<T, true>* self, char* newMemArray) {
- memcpy(newMemArray, self->fMemArray, self->fCount * sizeof(T));
+ sk_careful_memcpy(newMemArray, self->fMemArray, self->fCount * sizeof(T));
}
template<typename T>
diff --git a/include/core/SkTDArray.h b/include/core/SkTDArray.h
index 26c6c7f..b5332a2 100644
--- a/include/core/SkTDArray.h
+++ b/include/core/SkTDArray.h
@@ -45,7 +45,7 @@
SkTDArray<T> tmp(src.fArray, src.fCount);
this->swap(tmp);
} else {
- memcpy(fArray, src.fArray, sizeof(T) * src.fCount);
+ sk_careful_memcpy(fArray, src.fArray, sizeof(T) * src.fCount);
fCount = src.fCount;
}
}
diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h
index 5720c30..6c2e636 100644
--- a/include/core/SkTypes.h
+++ b/include/core/SkTypes.h
@@ -24,6 +24,28 @@
#include <string.h>
+/**
+ * sk_careful_memcpy() is just like memcpy(), but guards against undefined behavior.
+ *
+ * It is undefined behavior to call memcpy() with null dst or src, even if len is 0.
+ * If an optimizer is "smart" enough, it can exploit this to do unexpected things.
+ * memcpy(dst, src, 0);
+ * if (src) {
+ * printf("%x\n", *src);
+ * }
+ * In this code the compiler can assume src is not null and omit the if (src) {...} check,
+ * unconditionally running the printf, crashing the program if src really is null.
+ * Of the compilers we pay attention to only GCC performs this optimization in practice.
+ */
+static inline void* sk_careful_memcpy(void* dst, const void* src, size_t len) {
+ // When we pass >0 len we had better already be passing valid pointers.
+ // So we just need to skip calling memcpy when len == 0.
+ if (len) {
+ memcpy(dst,src,len);
+ }
+ return dst;
+}
+
/** \file SkTypes.h
*/
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index 12429ae..e054496 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -72,7 +72,8 @@
if (*dst != &src) {
(*dst)->resetToSize(src.fVerbCnt, src.fPointCnt, src.fConicWeights.count());
- memcpy((*dst)->verbsMemWritable(), src.verbsMemBegin(), src.fVerbCnt * sizeof(uint8_t));
+ sk_careful_memcpy((*dst)->verbsMemWritable(), src.verbsMemBegin(),
+ src.fVerbCnt * sizeof(uint8_t));
(*dst)->fConicWeights = src.fConicWeights;
}
@@ -275,8 +276,8 @@
SkDEBUGCODE(this->validate();)
this->resetToSize(ref.fVerbCnt, ref.fPointCnt, ref.fConicWeights.count(),
additionalReserveVerbs, additionalReservePoints);
- memcpy(this->verbsMemWritable(), ref.verbsMemBegin(), ref.fVerbCnt * sizeof(uint8_t));
- memcpy(this->fPoints, ref.fPoints, ref.fPointCnt * sizeof(SkPoint));
+ sk_careful_memcpy(this->verbsMemWritable(), ref.verbsMemBegin(), ref.fVerbCnt*sizeof(uint8_t));
+ sk_careful_memcpy(this->fPoints, ref.fPoints, ref.fPointCnt * sizeof(SkPoint));
fConicWeights = ref.fConicWeights;
fBoundsIsDirty = ref.fBoundsIsDirty;
if (!fBoundsIsDirty) {