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
This commit is contained in:
mtklein 2015-12-08 11:55:17 -08:00 committed by Commit bot
parent 290f00cd75
commit cc881dafcb
5 changed files with 30 additions and 6 deletions

View File

@ -436,6 +436,7 @@
[ 'skia_sanitizer', {
'cflags': [
'-fsanitize=<(skia_sanitizer)',
'-fno-sanitize-recover=<(skia_sanitizer)',
],
'ldflags': [
'-fsanitize=<(skia_sanitizer)',

View File

@ -23,11 +23,11 @@ inline void copy(SkTArray<T, true>* self, int dst, int src) {
}
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>

View File

@ -45,7 +45,7 @@ public:
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;
}
}

View File

@ -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
*/

View File

@ -72,7 +72,8 @@ void SkPathRef::CreateTransformedCopy(SkAutoTUnref<SkPathRef>* dst,
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 @@ void SkPathRef::copy(const SkPathRef& ref,
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) {