Skip to content

Commit 351b8d8

Browse files
johnstiles-googleSkia Commit-Bot
authored andcommitted
Poison unallocated block memory in GrBlockAllocator.
This will allow ASAN to detect use-after-free errors in pooled memory, enabling our fuzzers to catch errors sooner. Testing with oss-fuzz:26942 : http://screen/C5TEbu3CJvHzRqA Change-Id: Ic47d6b043998e5069525490cd25b2390cad94360 Bug: skia:10885 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331482 Reviewed-by: Michael Ludwig <[email protected]> Commit-Queue: John Stiles <[email protected]> Auto-Submit: John Stiles <[email protected]>
1 parent 52991f8 commit 351b8d8

4 files changed

Lines changed: 83 additions & 2 deletions

File tree

gn/core.gni

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ skia_core_sources = [
105105
"$_src/c/sk_types_priv.h",
106106
"$_src/core/Sk4px.h",
107107
"$_src/core/SkAAClip.cpp",
108+
"$_src/core/SkASAN.h",
108109
"$_src/core/SkATrace.cpp",
109110
"$_src/core/SkATrace.h",
110111
"$_src/core/SkAdvancedTypefaceMetrics.h",
@@ -247,6 +248,7 @@ skia_core_sources = [
247248
"$_src/core/SkM44.cpp",
248249
"$_src/core/SkMD5.cpp",
249250
"$_src/core/SkMD5.h",
251+
"$_src/core/SkMSAN.h",
250252
"$_src/core/SkMalloc.cpp",
251253
"$_src/core/SkMallocPixelRef.cpp",
252254
"$_src/core/SkMarkerStack.cpp",

src/core/SkASAN.h

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/*
2+
* Copyright 2020 Google LLC
3+
*
4+
* Use of this source code is governed by a BSD-style license that can be
5+
* found in the LICENSE file.
6+
*/
7+
8+
#ifndef SkASAN_DEFINED
9+
#define SkASAN_DEFINED
10+
11+
#include "include/core/SkTypes.h"
12+
13+
#include <string.h>
14+
15+
#ifdef __SANITIZE_ADDRESS__
16+
#define SK_SANITIZE_ADDRESS 1
17+
#endif
18+
#if !defined(SK_SANITIZE_ADDRESS) && defined(__has_feature)
19+
#if __has_feature(address_sanitizer)
20+
#define SK_SANITIZE_ADDRESS 1
21+
#endif
22+
#endif
23+
24+
// Typically declared in LLVM's asan_interface.h.
25+
#if SK_SANITIZE_ADDRESS
26+
extern "C" {
27+
void __asan_poison_memory_region(void const volatile *addr, size_t size);
28+
void __asan_unpoison_memory_region(void const volatile *addr, size_t size);
29+
}
30+
#endif
31+
32+
// Code that implements bespoke allocation arenas can poison the entire arena on creation, then
33+
// unpoison chunks of arena memory as they are parceled out. Consider leaving gaps between blocks
34+
// to detect buffer overrun.
35+
static inline void sk_asan_poison_memory_region(void const volatile *addr, size_t size) {
36+
#if SK_SANITIZE_ADDRESS
37+
__asan_poison_memory_region(addr, size);
38+
#endif
39+
}
40+
41+
static inline void sk_asan_unpoison_memory_region(void const volatile *addr, size_t size) {
42+
#if SK_SANITIZE_ADDRESS
43+
__asan_unpoison_memory_region(addr, size);
44+
#endif
45+
}
46+
47+
#endif // SkASAN_DEFINED

src/gpu/GrBlockAllocator.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ GrBlockAllocator::GrBlockAllocator(GrowthPolicy policy, size_t blockIncrementByt
2323
, fN1(1)
2424
// The head block always fills remaining space from GrBlockAllocator's size, because it's
2525
// inline, but can take over the specified number of bytes immediately after it.
26-
, fHead(nullptr, additionalPreallocBytes + BaseHeadBlockSize()) {
26+
, fHead(/*prev=*/nullptr, additionalPreallocBytes + BaseHeadBlockSize()) {
2727
SkASSERT(fBlockIncrement >= 1);
2828
SkASSERT(additionalPreallocBytes <= kMaxAllocationSize);
2929
}
@@ -37,9 +37,13 @@ GrBlockAllocator::Block::Block(Block* prev, int allocationSize)
3737
, fAllocatorMetadata(0) {
3838
SkASSERT(allocationSize >= (int) sizeof(Block));
3939
SkDEBUGCODE(fSentinel = kAssignedMarker;)
40+
41+
this->poisonRange(kDataStart, fSize);
4042
}
4143

4244
GrBlockAllocator::Block::~Block() {
45+
this->unpoisonRange(kDataStart, fSize);
46+
4347
SkASSERT(fSentinel == kAssignedMarker);
4448
SkDEBUGCODE(fSentinel = kFreedMarker;) // FWIW
4549
}
@@ -94,6 +98,7 @@ void GrBlockAllocator::releaseBlock(Block* block) {
9498
// Reset the cursor of the head block so that it can be reused if it becomes the new tail
9599
block->fCursor = kDataStart;
96100
block->fMetadata = 0;
101+
block->poisonRange(kDataStart, block->fSize);
97102
// Unlike in reset(), we don't set the head's next block to null because there are
98103
// potentially heap-allocated blocks that are still connected to it.
99104
} else {
@@ -168,6 +173,7 @@ void GrBlockAllocator::reset() {
168173
// For reset(), but NOT releaseBlock(), the head allocatorMetadata and scratch block
169174
// are reset/destroyed.
170175
b->fAllocatorMetadata = 0;
176+
b->poisonRange(kDataStart, b->fSize);
171177
this->resetScratchSpace();
172178
} else {
173179
delete b;

src/gpu/GrBlockAllocator.h

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "include/private/GrTypesPriv.h"
1212
#include "include/private/SkNoncopyable.h"
13+
#include "src/core/SkASAN.h"
1314

1415
#include <memory> // std::unique_ptr
1516
#include <cstddef> // max_align_t
@@ -125,6 +126,14 @@ class GrBlockAllocator final : SkNoncopyable {
125126

126127
Block(Block* prev, int allocationSize);
127128

129+
// We poison the unallocated space in a Block to allow ASAN to catch invalid writes.
130+
void poisonRange(int start, int end) {
131+
sk_asan_poison_memory_region(reinterpret_cast<char*>(this) + start, end - start);
132+
}
133+
void unpoisonRange(int start, int end) {
134+
sk_asan_unpoison_memory_region(reinterpret_cast<char*>(this) + start, end - start);
135+
}
136+
128137
// Get fCursor, but aligned such that ptr(rval) satisfies Align.
129138
template <size_t Align, size_t Padding>
130139
int cursor() const { return this->alignedOffset<Align, Padding>(fCursor); }
@@ -133,7 +142,10 @@ class GrBlockAllocator final : SkNoncopyable {
133142
int alignedOffset(int offset) const;
134143

135144
bool isScratch() const { return fCursor < 0; }
136-
void markAsScratch() { fCursor = -1; }
145+
void markAsScratch() {
146+
fCursor = -1;
147+
this->poisonRange(kDataStart, fSize);
148+
}
137149

138150
SkDEBUGCODE(int fSentinel;) // known value to check for bad back pointers to blocks
139151

@@ -577,6 +589,9 @@ GrBlockAllocator::ByteRange GrBlockAllocator::allocate(size_t size) {
577589

578590
int start = fTail->fCursor;
579591
fTail->fCursor = end;
592+
593+
fTail->unpoisonRange(offset - Padding, end);
594+
580595
return {fTail, start, offset, end};
581596
}
582597

@@ -634,6 +649,14 @@ bool GrBlockAllocator::Block::resize(int start, int end, int deltaBytes) {
634649
SkASSERT(nextCursor >= start);
635650
// We still check nextCursor >= start for release builds that wouldn't assert.
636651
if (nextCursor <= fSize && nextCursor >= start) {
652+
if (nextCursor < fCursor) {
653+
// The allocation got smaller; poison the space that can no longer be used.
654+
this->poisonRange(nextCursor + 1, end);
655+
} else {
656+
// The allocation got larger; unpoison the space that can now be used.
657+
this->unpoisonRange(end, nextCursor);
658+
}
659+
637660
fCursor = nextCursor;
638661
return true;
639662
}
@@ -647,6 +670,9 @@ bool GrBlockAllocator::Block::resize(int start, int end, int deltaBytes) {
647670
bool GrBlockAllocator::Block::release(int start, int end) {
648671
SkASSERT(fSentinel == kAssignedMarker);
649672
SkASSERT(start >= kDataStart && end <= fSize && start < end);
673+
674+
this->poisonRange(start, end);
675+
650676
if (fCursor == end) {
651677
fCursor = start;
652678
return true;

0 commit comments

Comments
 (0)