From 9ca06c4b00bfb9bb1a7f352efd264185e5a95fbc Mon Sep 17 00:00:00 2001 From: robertphillips Date: Wed, 20 Apr 2016 11:43:33 -0700 Subject: Fix ImageFilter fuzzer issue What appears to be happening in this fuzz is that a paint index inside the picture of an SkPictureImageFilter is getting changed to be out of range. BUG=skia:5192 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1893423002 Review URL: https://codereview.chromium.org/1893423002 --- src/core/SkPictureData.h | 40 +++++++------- src/core/SkPicturePlayback.cpp | 102 ++++++++++++++++++++++-------------- src/core/SkPicturePlayback.h | 4 +- src/core/SkReadBuffer.cpp | 4 ++ src/core/SkReadBuffer.h | 7 ++- src/core/SkValidatingReadBuffer.cpp | 7 +++ src/core/SkValidatingReadBuffer.h | 1 + 7 files changed, 103 insertions(+), 62 deletions(-) (limited to 'src/core') diff --git a/src/core/SkPictureData.h b/src/core/SkPictureData.h index 3acaf579bc..dbb117ef14 100644 --- a/src/core/SkPictureData.h +++ b/src/core/SkPictureData.h @@ -87,39 +87,34 @@ protected: bool parseBuffer(SkReadBuffer& buffer); public: - const SkBitmap& getBitmap(SkReader32* reader) const { + const SkBitmap& getBitmap(SkReadBuffer* reader) const { const int index = reader->readInt(); - return fBitmaps[index]; + return reader->validateIndex(index, fBitmaps.count()) ? fBitmaps[index] : fEmptyBitmap; } - const SkImage* getImage(SkReader32* reader) const { + const SkImage* getImage(SkReadBuffer* reader) const { const int index = reader->readInt(); - return fImageRefs[index]; + return reader->validateIndex(index, fImageCount) ? fImageRefs[index] : nullptr; } - const SkPath& getPath(SkReader32* reader) const { - int index = reader->readInt() - 1; - return fPaths[index]; + const SkPath& getPath(SkReadBuffer* reader) const { + const int index = reader->readInt() - 1; + return reader->validateIndex(index, fPaths.count()) ? fPaths[index] : fEmptyPath; } - const SkPicture* getPicture(SkReader32* reader) const { - int index = reader->readInt(); - SkASSERT(index > 0 && index <= fPictureCount); - return fPictureRefs[index - 1]; + const SkPicture* getPicture(SkReadBuffer* reader) const { + const int index = reader->readInt() - 1; + return reader->validateIndex(index, fPictureCount) ? fPictureRefs[index] : nullptr; } - const SkPaint* getPaint(SkReader32* reader) const { - int index = reader->readInt(); - if (index == 0) { - return nullptr; - } - return &fPaints[index - 1]; + const SkPaint* getPaint(SkReadBuffer* reader) const { + const int index = reader->readInt() - 1; + return reader->validateIndex(index, fPaints.count()) ? &fPaints[index] : nullptr; } - const SkTextBlob* getTextBlob(SkReader32* reader) const { - int index = reader->readInt(); - SkASSERT(index > 0 && index <= fTextBlobCount); - return fTextBlobRefs[index - 1]; + const SkTextBlob* getTextBlob(SkReadBuffer* reader) const { + const int index = reader->readInt() - 1; + return reader->validateIndex(index, fTextBlobCount) ? fTextBlobRefs[index] : nullptr; } #if SK_SUPPORT_GPU @@ -160,6 +155,9 @@ private: sk_sp fOpData; // opcodes and parameters + const SkPath fEmptyPath; + const SkBitmap fEmptyBitmap; + const SkPicture** fPictureRefs; int fPictureCount; const SkTextBlob** fTextBlobRefs; diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp index 774c2c065a..2c0a164f22 100644 --- a/src/core/SkPicturePlayback.cpp +++ b/src/core/SkPicturePlayback.cpp @@ -10,7 +10,7 @@ #include "SkPictureData.h" #include "SkPicturePlayback.h" #include "SkPictureRecord.h" -#include "SkReader32.h" +#include "SkReadBuffer.h" #include "SkRSXform.h" #include "SkTextBlob.h" #include "SkTDArray.h" @@ -41,7 +41,7 @@ SkCanvas::SaveLayerFlags SkCanvas::LegacySaveFlagsToSaveLayerFlags(uint32_t flag * to the next chunk's op code. This also means that the size of a chunk * with no arguments (just an opcode) will be 4. */ -DrawType SkPicturePlayback::ReadOpAndSize(SkReader32* reader, uint32_t* size) { +DrawType SkPicturePlayback::ReadOpAndSize(SkReadBuffer* reader, uint32_t* size) { uint32_t temp = reader->readInt(); uint32_t op; if (((uint8_t)temp) == temp) { @@ -58,9 +58,10 @@ DrawType SkPicturePlayback::ReadOpAndSize(SkReader32* reader, uint32_t* size) { } -static const SkRect* get_rect_ptr(SkReader32* reader) { +static const SkRect* get_rect_ptr(SkReadBuffer* reader, SkRect* storage) { if (reader->readBool()) { - return &reader->skipT(); + reader->readRect(storage); + return storage; } else { return nullptr; } @@ -74,7 +75,7 @@ public: const char* fText; }; -void get_text(SkReader32* reader, TextContainer* text) { +void get_text(SkReadBuffer* reader, TextContainer* text) { size_t length = text->fByteLength = reader->readInt(); text->fText = (const char*)reader->skip(length); } @@ -88,7 +89,7 @@ void SkPicturePlayback::draw(SkCanvas* canvas, SkPicture::AbortCallback* callbac AutoResetOpID aroi(this); SkASSERT(0 == fCurOffset); - SkReader32 reader(fPictureData->opData()->bytes(), fPictureData->opData()->size()); + SkReadBuffer reader(fPictureData->opData()->bytes(), fPictureData->opData()->size()); // Record this, so we can concat w/ it if we encounter a setMatrix() SkMatrix initialMatrix = canvas->getTotalMatrix(); @@ -108,7 +109,7 @@ void SkPicturePlayback::draw(SkCanvas* canvas, SkPicture::AbortCallback* callbac } } -void SkPicturePlayback::handleOp(SkReader32* reader, +void SkPicturePlayback::handleOp(SkReadBuffer* reader, DrawType op, uint32_t size, SkCanvas* canvas, @@ -127,7 +128,7 @@ void SkPicturePlayback::handleOp(SkReader32* reader, SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipPath(path, regionOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { - reader->setOffset(offsetToRestore); + reader->skip(offsetToRestore - reader->offset()); } } break; case CLIP_REGION: { @@ -139,11 +140,12 @@ void SkPicturePlayback::handleOp(SkReader32* reader, SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRegion(region, regionOp); if (canvas->isClipEmpty() && offsetToRestore) { - reader->setOffset(offsetToRestore); + reader->skip(offsetToRestore - reader->offset()); } } break; case CLIP_RECT: { - const SkRect& rect = reader->skipT(); + SkRect rect; + reader->readRect(&rect); uint32_t packed = reader->readInt(); SkRegion::Op regionOp = ClipParams_unpackRegionOp(packed); bool doAA = ClipParams_unpackDoAA(packed); @@ -151,7 +153,7 @@ void SkPicturePlayback::handleOp(SkReader32* reader, SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRect(rect, regionOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { - reader->setOffset(offsetToRestore); + reader->skip(offsetToRestore - reader->offset()); } } break; case CLIP_RRECT: { @@ -164,7 +166,7 @@ void SkPicturePlayback::handleOp(SkReader32* reader, SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRRect(rrect, regionOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { - reader->setOffset(offsetToRestore); + reader->skip(offsetToRestore - reader->offset()); } } break; case PUSH_CULL: break; // Deprecated, safe to ignore both push and pop. @@ -176,22 +178,24 @@ void SkPicturePlayback::handleOp(SkReader32* reader, break; } case DRAW_ANNOTATION: { - const SkRect& rect = reader->skipT(); - const char* key = reader->readString(); - canvas->drawAnnotation(rect, key, reader->readData().get()); + SkRect rect; + reader->readRect(&rect); + SkString key; + reader->readString(&key); + canvas->drawAnnotation(rect, key.c_str(), reader->readByteArrayAsData().get()); } break; case DRAW_ATLAS: { const SkPaint* paint = fPictureData->getPaint(reader); const SkImage* atlas = fPictureData->getImage(reader); - const uint32_t flags = reader->readU32(); - const int count = reader->readU32(); + const uint32_t flags = reader->readUInt(); + const int count = reader->readUInt(); const SkRSXform* xform = (const SkRSXform*)reader->skip(count * sizeof(SkRSXform)); const SkRect* tex = (const SkRect*)reader->skip(count * sizeof(SkRect)); const SkColor* colors = nullptr; SkXfermode::Mode mode = SkXfermode::kDst_Mode; if (flags & DRAW_ATLAS_HAS_COLORS) { colors = (const SkColor*)reader->skip(count * sizeof(SkColor)); - mode = (SkXfermode::Mode)reader->readU32(); + mode = (SkXfermode::Mode)reader->readUInt(); } const SkRect* cull = nullptr; if (flags & DRAW_ATLAS_HAS_CULL) { @@ -202,14 +206,17 @@ void SkPicturePlayback::handleOp(SkReader32* reader, case DRAW_BITMAP: { const SkPaint* paint = fPictureData->getPaint(reader); const SkBitmap bitmap = shallow_copy(fPictureData->getBitmap(reader)); - const SkPoint& loc = reader->skipT(); + SkPoint loc; + reader->readPoint(&loc); canvas->drawBitmap(bitmap, loc.fX, loc.fY, paint); } break; case DRAW_BITMAP_RECT: { const SkPaint* paint = fPictureData->getPaint(reader); const SkBitmap bitmap = shallow_copy(fPictureData->getBitmap(reader)); - const SkRect* src = get_rect_ptr(reader); // may be null - const SkRect& dst = reader->skipT(); // required + SkRect storage; + const SkRect* src = get_rect_ptr(reader, &storage); // may be null + SkRect dst; + reader->readRect(&dst); // required SkCanvas::SrcRectConstraint constraint = (SkCanvas::SrcRectConstraint)reader->readInt(); canvas->legacy_drawBitmapRect(bitmap, src, dst, paint, constraint); } break; @@ -226,8 +233,10 @@ void SkPicturePlayback::handleOp(SkReader32* reader, case DRAW_BITMAP_NINE: { const SkPaint* paint = fPictureData->getPaint(reader); const SkBitmap bitmap = shallow_copy(fPictureData->getBitmap(reader)); - const SkIRect& src = reader->skipT(); - const SkRect& dst = reader->skipT(); + SkIRect src; + reader->readIRect(&src); + SkRect dst; + reader->readRect(&dst); canvas->drawBitmapNine(bitmap, src, dst, paint); } break; case DRAW_CLEAR: @@ -246,37 +255,46 @@ void SkPicturePlayback::handleOp(SkReader32* reader, reader->readRRect(&inner); canvas->drawDRRect(outer, inner, paint); } break; - case BEGIN_COMMENT_GROUP: - reader->readString(); + case BEGIN_COMMENT_GROUP: { + SkString tmp; + reader->readString(&tmp); // deprecated (M44) break; - case COMMENT: - reader->readString(); - reader->readString(); + } + case COMMENT: { + SkString tmp; + reader->readString(&tmp); + reader->readString(&tmp); // deprecated (M44) break; + } case END_COMMENT_GROUP: // deprecated (M44) break; case DRAW_IMAGE: { const SkPaint* paint = fPictureData->getPaint(reader); const SkImage* image = fPictureData->getImage(reader); - const SkPoint& loc = reader->skipT(); + SkPoint loc; + reader->readPoint(&loc); canvas->drawImage(image, loc.fX, loc.fY, paint); } break; case DRAW_IMAGE_NINE: { const SkPaint* paint = fPictureData->getPaint(reader); const SkImage* image = fPictureData->getImage(reader); - const SkIRect& center = reader->skipT(); - const SkRect& dst = reader->skipT(); + SkIRect center; + reader->readIRect(¢er); + SkRect dst; + reader->readRect(&dst); canvas->drawImageNine(image, center, dst, paint); } break; case DRAW_IMAGE_RECT_STRICT: case DRAW_IMAGE_RECT: { const SkPaint* paint = fPictureData->getPaint(reader); const SkImage* image = fPictureData->getImage(reader); - const SkRect* src = get_rect_ptr(reader); // may be null - const SkRect& dst = reader->skipT(); // required + SkRect storage; + const SkRect* src = get_rect_ptr(reader, &storage); // may be null + SkRect dst; + reader->readRect(&dst); // required // DRAW_IMAGE_RECT_STRICT assumes this constraint, and doesn't store it SkCanvas::SrcRectConstraint constraint = SkCanvas::kStrict_SrcRectConstraint; if (DRAW_IMAGE_RECT == op) { @@ -287,7 +305,9 @@ void SkPicturePlayback::handleOp(SkReader32* reader, } break; case DRAW_OVAL: { const SkPaint& paint = *fPictureData->getPaint(reader); - canvas->drawOval(reader->skipT(), paint); + SkRect rect; + reader->readRect(&rect); + canvas->drawOval(rect, paint); } break; case DRAW_PAINT: canvas->drawPaint(*fPictureData->getPaint(reader)); @@ -382,7 +402,9 @@ void SkPicturePlayback::handleOp(SkReader32* reader, } break; case DRAW_RECT: { const SkPaint& paint = *fPictureData->getPaint(reader); - canvas->drawRect(reader->skipT(), paint); + SkRect rect; + reader->readRect(&rect); + canvas->drawRect(rect, paint); } break; case DRAW_RRECT: { const SkPaint& paint = *fPictureData->getPaint(reader); @@ -479,21 +501,25 @@ void SkPicturePlayback::handleOp(SkReader32* reader, canvas->save(); break; case SAVE_LAYER_SAVEFLAGS_DEPRECATED: { - const SkRect* boundsPtr = get_rect_ptr(reader); + SkRect storage; + const SkRect* boundsPtr = get_rect_ptr(reader, &storage); const SkPaint* paint = fPictureData->getPaint(reader); auto flags = SkCanvas::LegacySaveFlagsToSaveLayerFlags(reader->readInt()); canvas->saveLayer(SkCanvas::SaveLayerRec(boundsPtr, paint, flags)); } break; case SAVE_LAYER_SAVELAYERFLAGS_DEPRECATED_JAN_2016: { - const SkRect* boundsPtr = get_rect_ptr(reader); + SkRect storage; + const SkRect* boundsPtr = get_rect_ptr(reader, &storage); const SkPaint* paint = fPictureData->getPaint(reader); canvas->saveLayer(SkCanvas::SaveLayerRec(boundsPtr, paint, reader->readInt())); } break; case SAVE_LAYER_SAVELAYERREC: { SkCanvas::SaveLayerRec rec(nullptr, nullptr, nullptr, 0); const uint32_t flatFlags = reader->readInt(); + SkRect bounds; if (flatFlags & SAVELAYERREC_HAS_BOUNDS) { - rec.fBounds = &reader->skipT(); + reader->readRect(&bounds); + rec.fBounds = &bounds; } if (flatFlags & SAVELAYERREC_HAS_PAINT) { rec.fPaint = fPictureData->getPaint(reader); diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h index ca6ad1ade4..7cedaeb499 100644 --- a/src/core/SkPicturePlayback.h +++ b/src/core/SkPicturePlayback.h @@ -38,13 +38,13 @@ protected: // The offset of the current operation when within the draw method size_t fCurOffset; - void handleOp(SkReader32* reader, + void handleOp(SkReadBuffer* reader, DrawType op, uint32_t size, SkCanvas* canvas, const SkMatrix& initialMatrix); - static DrawType ReadOpAndSize(SkReader32* reader, uint32_t* size); + static DrawType ReadOpAndSize(SkReadBuffer* reader, uint32_t* size); class AutoResetOpID { public: diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp index abd46312f7..bafcbc2b91 100644 --- a/src/core/SkReadBuffer.cpp +++ b/src/core/SkReadBuffer.cpp @@ -138,6 +138,10 @@ void SkReadBuffer::readRect(SkRect* rect) { memcpy(rect, fReader.skip(sizeof(SkRect)), sizeof(SkRect)); } +void SkReadBuffer::readRRect(SkRRect* rrect) { + fReader.readRRect(rrect); +} + void SkReadBuffer::readRegion(SkRegion* region) { fReader.readRegion(region); } diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h index faf853aef5..52758d05ec 100644 --- a/src/core/SkReadBuffer.h +++ b/src/core/SkReadBuffer.h @@ -101,6 +101,7 @@ public: size_t offset() { return fReader.offset(); } bool eof() { return fReader.eof(); } virtual const void* skip(size_t size) { return fReader.skip(size); } + void* readFunctionPtr() { return fReader.readPtr(); } // primitives @@ -121,6 +122,7 @@ public: virtual void readMatrix(SkMatrix* matrix); virtual void readIRect(SkIRect* rect); virtual void readRect(SkRect* rect); + virtual void readRRect(SkRRect* rrect); virtual void readRegion(SkRegion* region); virtual void readPath(SkPath* path); @@ -203,9 +205,12 @@ public: } // Default impelementations don't check anything. - virtual bool validate(bool isValid) { return true; } + virtual bool validate(bool isValid) { return isValid; } virtual bool isValid() const { return true; } virtual bool validateAvailable(size_t size) { return true; } + bool validateIndex(int index, int count) { + return this->validate(index >= 0 && index < count); + } protected: SkReader32 fReader; diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp index 52006f23ad..b85e532d0c 100644 --- a/src/core/SkValidatingReadBuffer.cpp +++ b/src/core/SkValidatingReadBuffer.cpp @@ -145,6 +145,13 @@ void SkValidatingReadBuffer::readRect(SkRect* rect) { } } +void SkValidatingReadBuffer::readRRect(SkRRect* rrect) { + const void* ptr = this->skip(sizeof(SkRRect)); + if (!fError) { + memcpy(rrect, ptr, sizeof(SkRRect)); + } +} + void SkValidatingReadBuffer::readRegion(SkRegion* region) { size_t size = 0; if (!fError) { diff --git a/src/core/SkValidatingReadBuffer.h b/src/core/SkValidatingReadBuffer.h index 7fb203b45e..28d8c34e0e 100644 --- a/src/core/SkValidatingReadBuffer.h +++ b/src/core/SkValidatingReadBuffer.h @@ -44,6 +44,7 @@ public: void readMatrix(SkMatrix* matrix) override; void readIRect(SkIRect* rect) override; void readRect(SkRect* rect) override; + void readRRect(SkRRect* rrect) override; void readRegion(SkRegion* region) override; void readPath(SkPath* path) override; -- cgit v1.2.3