SkRegion deserialization more robust
BUG=chromium:688987
Change-Id: Ide6d70330c8cd1fce814eb2c445da1fbff498ef6
Reviewed-on: https://skia-review.googlesource.com/8694
Reviewed-by: Kevin Lubick <kjlubick@google.com>
diff --git a/include/core/SkRegion.h b/include/core/SkRegion.h
index a0f0e4a..49e5a19 100644
--- a/include/core/SkRegion.h
+++ b/include/core/SkRegion.h
@@ -436,6 +436,8 @@
int count_runtype_values(int* itop, int* ibot) const;
+ bool isValid() const;
+
static void BuildRectRuns(const SkIRect& bounds,
RunType runs[kRectRegionRuns]);
diff --git a/src/core/SkRegion.cpp b/src/core/SkRegion.cpp
index 1123cf0..dca9b9d 100644
--- a/src/core/SkRegion.cpp
+++ b/src/core/SkRegion.cpp
@@ -284,6 +284,7 @@
if (!this->isComplex() || fRunHead->fRunCount != count) {
this->freeRuns();
this->allocateRuns(count);
+ SkASSERT(this->isComplex());
}
// must call this before we can write directly into runs()
@@ -547,6 +548,7 @@
} else {
SkRegion tmp;
tmp.allocateRuns(*fRunHead);
+ SkASSERT(tmp.isComplex());
tmp.fBounds = fBounds;
dst->swap(tmp);
}
@@ -1133,6 +1135,9 @@
int32_t count;
if (buffer.readS32(&count) && (count >= 0) && buffer.read(&tmp.fBounds, sizeof(tmp.fBounds))) {
+ if (tmp.fBounds.isEmpty()) {
+ return 0; // bad bounds for non-empty region; report failure
+ }
if (count == 0) {
tmp.fRunHead = SkRegion_gRectRunHeadPtr;
} else {
@@ -1140,12 +1145,17 @@
if (buffer.readS32(&ySpanCount) && buffer.readS32(&intervalCount) &&
intervalCount > 1) {
tmp.allocateRuns(count, ySpanCount, intervalCount);
+ if (!tmp.isComplex()) {
+ return 0; // report failure
+ }
buffer.read(tmp.fRunHead->writable_runs(), count * sizeof(RunType));
+ } else {
+ return 0; // report failure;
}
}
}
size_t sizeRead = 0;
- if (buffer.isValid()) {
+ if (buffer.isValid() && tmp.isValid()) {
this->swap(tmp);
sizeRead = buffer.pos();
}
@@ -1161,8 +1171,6 @@
///////////////////////////////////////////////////////////////////////////////
-#ifdef SK_DEBUG
-
// Starts with first X-interval, and returns a ptr to the X-sentinel
static const SkRegion::RunType* skip_intervals_slow(const SkRegion::RunType runs[]) {
// want to track that our intevals are all disjoint, such that
@@ -1172,19 +1180,22 @@
SkRegion::RunType prevR = -SkRegion::kRunTypeSentinel;
while (runs[0] < SkRegion::kRunTypeSentinel) {
- SkASSERT(prevR < runs[0]);
- SkASSERT(runs[0] < runs[1]);
- SkASSERT(runs[1] < SkRegion::kRunTypeSentinel);
+ if (prevR >= runs[0]
+ || runs[0] >= runs[1]
+ || runs[1] >= SkRegion::kRunTypeSentinel) {
+ return nullptr;
+ }
prevR = runs[1];
runs += 2;
}
return runs;
}
-static void compute_bounds(const SkRegion::RunType runs[],
+static bool compute_bounds(const SkRegion::RunType runs[],
SkIRect* bounds, int* ySpanCountPtr,
int* intervalCountPtr) {
- assert_sentinel(runs[0], false); // top
+ SkASSERT(bounds && ySpanCountPtr && intervalCountPtr);
+ if (SkRegionValueIsSentinel(runs[0])) { return false; }
int left = SK_MaxS32;
int rite = SK_MinS32;
@@ -1192,10 +1203,11 @@
int ySpanCount = 0;
int intervalCount = 0;
+ if (!runs) { return false; }
bounds->fTop = *runs++;
do {
bot = *runs++;
- SkASSERT(SkRegion::kRunTypeSentinel > bot);
+ if (SkRegion::kRunTypeSentinel <= bot) { return false; }
ySpanCount += 1;
@@ -1207,17 +1219,22 @@
const SkRegion::RunType* prev = runs;
runs = skip_intervals_slow(runs);
+ if (!runs) { return false; }
int intervals = SkToInt((runs - prev) >> 1);
- SkASSERT(prev[-1] == intervals);
+ if (prev[-1] != intervals) { return false; }
intervalCount += intervals;
if (rite < runs[-1]) {
rite = runs[-1];
}
- } else {
- SkASSERT(0 == runs[-1]); // no intervals
+ } else { // no intervals
+ if (0 != runs[-1]) {
+ return false;
+ }
}
- SkASSERT(SkRegion::kRunTypeSentinel == *runs);
+ if (SkRegion::kRunTypeSentinel != *runs) {
+ return false;
+ }
runs += 1;
} while (SkRegion::kRunTypeSentinel != *runs);
@@ -1226,38 +1243,43 @@
bounds->fBottom = bot;
*ySpanCountPtr = ySpanCount;
*intervalCountPtr = intervalCount;
+ return true;
}
-void SkRegion::validate() const {
+bool SkRegion::isValid() const {
if (this->isEmpty()) {
// check for explicit empty (the zero rect), so we can compare rects to know when
// two regions are equal (i.e. emptyRectA == emptyRectB)
// this is stricter than just asserting fBounds.isEmpty()
- SkASSERT(fBounds.fLeft == 0 && fBounds.fTop == 0 && fBounds.fRight == 0 && fBounds.fBottom == 0);
+ return fBounds == SkIRect{0, 0, 0, 0};
} else {
- SkASSERT(!fBounds.isEmpty());
+ if (fBounds.isEmpty()) {
+ return false;
+ }
if (!this->isRect()) {
- SkASSERT(fRunHead->fRefCnt >= 1);
- SkASSERT(fRunHead->fRunCount > kRectRegionRuns);
-
- const RunType* run = fRunHead->readonly_runs();
-
- // check that our bounds match our runs
- {
- SkIRect bounds;
- int ySpanCount, intervalCount;
- compute_bounds(run, &bounds, &ySpanCount, &intervalCount);
-
- SkASSERT(bounds == fBounds);
- SkASSERT(ySpanCount > 0);
- SkASSERT(fRunHead->getYSpanCount() == ySpanCount);
- // SkASSERT(intervalCount > 1);
- SkASSERT(fRunHead->getIntervalCount() == intervalCount);
+ if (!fRunHead
+ || fRunHead->fRefCnt < 1
+ || fRunHead->fRunCount <= kRectRegionRuns) {
+ return false;
}
+ const RunType* run = fRunHead->readonly_runs();
+ // check that our bounds match our runs
+ SkIRect bounds;
+ int ySpanCount, intervalCount;
+ return compute_bounds(run, &bounds, &ySpanCount, &intervalCount)
+ && bounds == fBounds
+ && ySpanCount > 0
+ && fRunHead->getYSpanCount() == ySpanCount
+ && fRunHead->getIntervalCount() == intervalCount;
+ } else {
+ return true;
}
}
}
+#ifdef SK_DEBUG
+void SkRegion::validate() const { SkASSERT(this->isValid()); }
+
void SkRegion::dump() const {
if (this->isEmpty()) {
SkDebugf(" rgn: empty\n");
diff --git a/src/core/SkRegionPriv.h b/src/core/SkRegionPriv.h
index a4cf77b..ef36914 100644
--- a/src/core/SkRegionPriv.h
+++ b/src/core/SkRegionPriv.h
@@ -12,8 +12,12 @@
#include "SkRegion.h"
#include "SkAtomics.h"
+inline bool SkRegionValueIsSentinel(int32_t value) {
+ return value == (int32_t)SkRegion::kRunTypeSentinel;
+}
+
#define assert_sentinel(value, isSentinel) \
- SkASSERT(((value) == SkRegion::kRunTypeSentinel) == isSentinel)
+ SkASSERT(SkRegionValueIsSentinel(value) == isSentinel)
//SkDEBUGCODE(extern int32_t gRgnAllocCounter;)
@@ -62,7 +66,9 @@
//SkDEBUGCODE(sk_atomic_inc(&gRgnAllocCounter);)
//SkDEBUGF(("************** gRgnAllocCounter::alloc %d\n", gRgnAllocCounter));
- SkASSERT(count >= SkRegion::kRectRegionRuns);
+ if (count < SkRegion::kRectRegionRuns) {
+ return nullptr;
+ }
const int64_t size = sk_64_mul(count, sizeof(RunType)) + sizeof(RunHead);
if (count < 0 || !sk_64_isS32(size)) { SK_ABORT("Invalid Size"); }
@@ -77,10 +83,14 @@
}
static RunHead* Alloc(int count, int yspancount, int intervalCount) {
- SkASSERT(yspancount > 0);
- SkASSERT(intervalCount > 1);
+ if (yspancount <= 0 || intervalCount <= 1) {
+ return nullptr;
+ }
RunHead* head = Alloc(count);
+ if (!head) {
+ return nullptr;
+ }
head->fYSpanCount = yspancount;
head->fIntervalCount = intervalCount;
return head;
diff --git a/tests/RegionTest.cpp b/tests/RegionTest.cpp
index 1720822..f2a3d02 100644
--- a/tests/RegionTest.cpp
+++ b/tests/RegionTest.cpp
@@ -271,6 +271,11 @@
SkAutoMalloc storage(bytesNeeded);
const size_t bytesWritten = region.writeToMemory(storage.get());
REPORTER_ASSERT(r, bytesWritten == bytesNeeded);
+
+ // Also check that the bytes are meaningful.
+ SkRegion copy;
+ REPORTER_ASSERT(r, copy.readFromMemory(storage.get(), bytesNeeded));
+ REPORTER_ASSERT(r, region == copy);
}
DEF_TEST(Region_writeToMemory, r) {
@@ -291,3 +296,56 @@
REPORTER_ASSERT(r, region.isComplex());
test_write(region, r);
}
+
+DEF_TEST(Region_readFromMemory_bad, r) {
+ // These assume what our binary format is: conceivably we could change it
+ // and might need to remove or change some of these tests.
+ SkRegion region;
+
+ static const char data0[] =
+ "\2\0\0\0\277]\345\222\\\2G\252\0\177'\10\203\236\211>\377\340@\351"
+ "!\370y\3\31\232r\353\343\336Ja\177\377\377\377\244\301\362:Q\\\0\0"
+ "\1\200\263\214\374\276\336P\225^\230\20UH N\265\357\177\240\0\306\377"
+ "\177\346\222S \0\375\0\332\247 \302I\240H\374\200lk\r`\0375\324W\215"
+ "\270tE^,\224n\310fy\377\231AH\16\235A\371\315\347\360\265\372r\232"
+ "\301\216\35\227:\265]\32\20W\263yc\207\246\270tE^,\224n\310sy\2\0A"
+ "\14\241SQ\\\303\364\0\0\1\200\0\0\374k\r`\0375\324Wp\270\267\313\313"
+ "\313\313\313@\277\365b\341\343\336Ja\357~\263\0\2\333\260\220\\\303"
+ "\364\265\332\267\242\325nlX\367\27I4444;\266\256\37/M\207";
+ size_t data0length = 221;
+ REPORTER_ASSERT(r, 0 == region.readFromMemory(data0, data0length));
+
+ static const char data1[] =
+ "\2\0\0\0\\\2G\252\0\177'\10\247 \302I\240H\374\200lk\r`\0375\324Wr"
+ "\232\301\216\35\227:\265]\32\20W\263yc\207\246\270tE^,\224n\310sy\2"
+ "\0A\14\241SQ\\\303\364\0\0\1\200\0\0\374k\r`\0375\324Wp\270\267\313"
+ "\313\313\313\313@\277\365b\341\343\336Ja\357~\263\0\2\333\260\220\\"
+ "\303\364\265\332\267\242\325nlX\367\27I4444;\266\256\37/M\207";
+ size_t data1length = 129;
+ REPORTER_ASSERT(r, 0 == region.readFromMemory(data1, data1length));
+
+ static const char data2[] =
+ " \0\0\0`\6\363\234AH\26\235\0\0\0\0\251\217\27I\27C\361,\320u\3171"
+ "\10.\206\277]\345\222\334\2C\252\242a'\10\251\31\326\372\334A\277\30"
+ "\240M\275v\201\271\3527\215{)S\3771{\345Z\250\23\213\331\23j@\13\220"
+ "\200Z^-\20\212=;\355\314\36\260c\224M\16\271Szy\373\204M\21\177\251"
+ "\275\r\274M\370\201\243^@\343\236JaS\204\3212\244\301\327\22\352KI"
+ "\207\350z\300\250\372\26\14\2\233K\330\16\251\230\223\r\"\243\271\17"
+ ")\260\262\2[a.*.4\14\344\307\350\3\0\0-\350G!\31\300\205\205\205\205"
+ "\205\205\205\205\205\205\205\205\205\205\205\205\305m\311<Q\347\30"
+ "\324\203f\2614\3115\206\214@:\346n\254\37\225\263\214\374\276\336\23"
+ "\270\304\262\25\24_\342\223\253\351L\30\372\373\243\240g\0367V\336"
+ "P\7-1{\345Z\250\23\213P\225^\230\27UH\206N\265\357\177\262\302\306"
+ "kk\7\233\234N\32@\355H\327\34\337\0V\30 \225\35\225\233\253\0144>\310"
+ "\352\346L\232\215\270t[^,\224l\312f\2025?}\1ZL\217wf8C\346\222S\240"
+ "\203\375\374\332\247 \302I\271H\0\0lk\22`\0375\324W\374\265\342\243"
+ "yL\211\215\270tE^,\224l\312f\2025?}\1ZL\217wf8C\333\370_.\277A\277"
+ "^\\\313!\342\340\213\210\244\272\33\275\360\301\347\315\377\6a\272"
+ "kyi:W\332\366\5\312F\217c\243\20,\"\240\347o\375\277\317}HEji\367\374"
+ "\331\214\314\242x\356\340\350\362r$\222\266\325\201\234\267P\243N\361"
+ "++++++++\370+@++\205!8B\255L\3\3416\335$\\\r\265W[F\326\316w{.\306"
+ ">f2i\244\242=Y\236\364\302\357xR:Q\\\303\364\265\332\200\242\325nl"
+ "X\373\307\5<-";
+ size_t data2length = 512;
+ REPORTER_ASSERT(r, 0 == region.readFromMemory(data2, data2length));
+}
