From 7695359876b8f90225bc4be895c20f34fcdfaf2e Mon Sep 17 00:00:00 2001 From: Hal Canary Date: Thu, 16 Feb 2017 12:42:24 -0500 Subject: [PATCH] SkRegion deserialization more robust BUG=chromium:688987 Change-Id: Ide6d70330c8cd1fce814eb2c445da1fbff498ef6 Reviewed-on: https://skia-review.googlesource.com/8694 Reviewed-by: Kevin Lubick --- include/core/SkRegion.h | 2 ++ src/core/SkRegion.cpp | 84 +++++++++++++++++++++++++++++++------------------ src/core/SkRegionPriv.h | 18 ++++++++--- tests/RegionTest.cpp | 58 ++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 35 deletions(-) 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 @@ private: 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 @@ bool SkRegion::setRuns(RunType runs[], int count) { 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 @@ void SkRegion::translate(int dx, int dy, SkRegion* dst) const { } else { SkRegion tmp; tmp.allocateRuns(*fRunHead); + SkASSERT(tmp.isComplex()); tmp.fBounds = fBounds; dst->swap(tmp); } @@ -1133,6 +1135,9 @@ size_t SkRegion::readFromMemory(const void* storage, size_t length) { 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 @@ size_t SkRegion::readFromMemory(const void* storage, size_t length) { 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 @@ const SkRegion& SkRegion::GetEmptyRegion() { /////////////////////////////////////////////////////////////////////////////// -#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 @@ static const SkRegion::RunType* skip_intervals_slow(const SkRegion::RunType runs 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 @@ static void compute_bounds(const SkRegion::RunType runs[], 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 @@ static void compute_bounds(const SkRegion::RunType runs[], 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; + } + } + if (SkRegion::kRunTypeSentinel != *runs) { + return false; } - SkASSERT(SkRegion::kRunTypeSentinel == *runs); runs += 1; } while (SkRegion::kRunTypeSentinel != *runs); @@ -1226,38 +1243,43 @@ static void compute_bounds(const SkRegion::RunType runs[], 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); - + 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; - compute_bounds(run, &bounds, &ySpanCount, &intervalCount); - - SkASSERT(bounds == fBounds); - SkASSERT(ySpanCount > 0); - SkASSERT(fRunHead->getYSpanCount() == ySpanCount); - // SkASSERT(intervalCount > 1); - SkASSERT(fRunHead->getIntervalCount() == intervalCount); - } + 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 @@ public: //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 @@ public: } 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 @@ static void test_write(const SkRegion& region, skiatest::Reporter* r) { 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 @@ DEF_TEST(Region_writeToMemory, r) { 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\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)); +} -- 2.7.4