From d87bd7cfd1dae55fbe4331586aacac3468d59a77 Mon Sep 17 00:00:00 2001 From: fmalita Date: Thu, 6 Oct 2016 12:09:50 -0700 Subject: Harden SkPicturePlayback::handleOp() skips SkValidatingReadBuffer::skip() may return null - tread more carefully around it. BUG=skia:5828 R=reed@google.com,mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2399043002 Review-Url: https://codereview.chromium.org/2399043002 --- src/core/SkPicturePlayback.cpp | 149 ++++++++++++++++++++++++++++++++++------- 1 file changed, 126 insertions(+), 23 deletions(-) (limited to 'src') diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp index a246a306e6..bede60111e 100644 --- a/src/core/SkPicturePlayback.cpp +++ b/src/core/SkPicturePlayback.cpp @@ -126,6 +126,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, uint32_t size, SkCanvas* canvas, const SkMatrix& initialMatrix) { +#define BREAK_ON_READ_ERROR(r) if (!r->isValid()) { break; } + switch (op) { case NOOP: { SkASSERT(size >= 4); @@ -137,6 +139,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkCanvas::ClipOp clipOp = ClipParams_unpackRegionOp(packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipPath(path, clipOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { @@ -149,6 +153,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, uint32_t packed = reader->readInt(); SkCanvas::ClipOp clipOp = ClipParams_unpackRegionOp(packed); size_t offsetToRestore = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRegion(region, clipOp); if (canvas->isClipEmpty() && offsetToRestore) { @@ -162,6 +168,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkCanvas::ClipOp clipOp = ClipParams_unpackRegionOp(packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRect(rect, clipOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { @@ -175,6 +183,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkCanvas::ClipOp clipOp = ClipParams_unpackRegionOp(packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + SkASSERT(!offsetToRestore || offsetToRestore >= reader->offset()); canvas->clipRRect(rrect, clipOp, doAA); if (canvas->isClipEmpty() && offsetToRestore) { @@ -186,6 +196,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, case CONCAT: { SkMatrix matrix; reader->readMatrix(&matrix); + BREAK_ON_READ_ERROR(reader); + canvas->concat(matrix); break; } @@ -194,7 +206,10 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, reader->readRect(&rect); SkString key; reader->readString(&key); - canvas->drawAnnotation(rect, key.c_str(), reader->readByteArrayAsData().get()); + sk_sp data = reader->readByteArrayAsData(); + BREAK_ON_READ_ERROR(reader); + + canvas->drawAnnotation(rect, key.c_str(), data.get()); } break; case DRAW_ARC: { const SkPaint* paint = fPictureData->getPaint(reader); @@ -203,6 +218,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkScalar startAngle = reader->readScalar(); SkScalar sweepAngle = reader->readScalar(); int useCenter = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawArc(rect, startAngle, sweepAngle, SkToBool(useCenter), *paint); } @@ -224,6 +241,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, if (flags & DRAW_ATLAS_HAS_CULL) { cull = (const SkRect*)reader->skip(sizeof(SkRect)); } + BREAK_ON_READ_ERROR(reader); + canvas->drawAtlas(atlas, xform, tex, colors, count, mode, cull, paint); } break; case DRAW_BITMAP: { @@ -231,6 +250,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkImage* image = fPictureData->getBitmapAsImage(reader); SkPoint loc; reader->readPoint(&loc); + BREAK_ON_READ_ERROR(reader); + canvas->drawImage(image, loc.fX, loc.fY, paint); } break; case DRAW_BITMAP_RECT: { @@ -241,6 +262,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkRect dst; reader->readRect(&dst); // required SkCanvas::SrcRectConstraint constraint = (SkCanvas::SrcRectConstraint)reader->readInt(); + BREAK_ON_READ_ERROR(reader); + if (src) { canvas->drawImageRect(image, *src, dst, paint, constraint); } else { @@ -252,6 +275,7 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkImage* image = fPictureData->getBitmapAsImage(reader); SkMatrix matrix; reader->readMatrix(&matrix); + BREAK_ON_READ_ERROR(reader); SkAutoCanvasRestore acr(canvas, true); canvas->concat(matrix); @@ -264,24 +288,34 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, reader->readIRect(&src); SkRect dst; reader->readRect(&dst); + BREAK_ON_READ_ERROR(reader); + canvas->drawImageNine(image, src, dst, paint); } break; - case DRAW_CLEAR: - canvas->clear(reader->readInt()); - break; + case DRAW_CLEAR: { + auto c = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + + canvas->clear(c); + } break; case DRAW_DATA: { // This opcode is now dead, just need to skip it for backwards compatibility size_t length = reader->readInt(); (void)reader->skip(length); // skip handles padding the read out to a multiple of 4 } break; - case DRAW_DRAWABLE: - canvas->drawDrawable(fPictureData->getDrawable(reader)); - break; + case DRAW_DRAWABLE: { + auto* d = fPictureData->getDrawable(reader); + BREAK_ON_READ_ERROR(reader); + + canvas->drawDrawable(d); + } break; case DRAW_DRAWABLE_MATRIX: { SkMatrix matrix; reader->readMatrix(&matrix); SkDrawable* drawable = fPictureData->getDrawable(reader); + BREAK_ON_READ_ERROR(reader); + canvas->drawDrawable(drawable, &matrix); } break; case DRAW_DRRECT: { @@ -289,6 +323,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkRRect outer, inner; reader->readRRect(&outer); reader->readRRect(&inner); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawDRRect(outer, inner, *paint); } @@ -314,6 +350,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkImage* image = fPictureData->getImage(reader); SkPoint loc; reader->readPoint(&loc); + BREAK_ON_READ_ERROR(reader); + canvas->drawImage(image, loc.fX, loc.fY, paint); } break; case DRAW_IMAGE_LATTICE: { @@ -332,6 +370,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, lattice.fBounds = &src; SkRect dst; reader->readRect(&dst); + BREAK_ON_READ_ERROR(reader); + canvas->drawImageLattice(image, lattice, dst, paint); } break; case DRAW_IMAGE_NINE: { @@ -341,6 +381,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, reader->readIRect(¢er); SkRect dst; reader->readRect(&dst); + BREAK_ON_READ_ERROR(reader); + canvas->drawImageNine(image, center, dst, paint); } break; case DRAW_IMAGE_RECT_STRICT: @@ -357,18 +399,24 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, // newer op-code stores the constraint explicitly constraint = (SkCanvas::SrcRectConstraint)reader->readInt(); } + BREAK_ON_READ_ERROR(reader); + canvas->legacy_drawImageRect(image, src, dst, paint, constraint); } break; case DRAW_OVAL: { const SkPaint* paint = fPictureData->getPaint(reader); SkRect rect; reader->readRect(&rect); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawOval(rect, *paint); } } break; case DRAW_PAINT: { const SkPaint* paint = fPictureData->getPaint(reader); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawPaint(*paint); } @@ -396,24 +444,34 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, } xfer = SkXfermode::Make((SkXfermode::Mode)mode); } + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawPatch(cubics, colors, texCoords, std::move(xfer), *paint); } } break; case DRAW_PATH: { const SkPaint* paint = fPictureData->getPaint(reader); + const auto& path = fPictureData->getPath(reader); + BREAK_ON_READ_ERROR(reader); + if (paint) { - canvas->drawPath(fPictureData->getPath(reader), *paint); + canvas->drawPath(path, *paint); } } break; - case DRAW_PICTURE: - canvas->drawPicture(fPictureData->getPicture(reader)); - break; + case DRAW_PICTURE: { + const auto* pic = fPictureData->getPicture(reader); + BREAK_ON_READ_ERROR(reader); + + canvas->drawPicture(pic); + } break; case DRAW_PICTURE_MATRIX_PAINT: { const SkPaint* paint = fPictureData->getPaint(reader); SkMatrix matrix; reader->readMatrix(&matrix); const SkPicture* pic = fPictureData->getPicture(reader); + BREAK_ON_READ_ERROR(reader); + canvas->drawPicture(pic, &matrix, paint); } break; case DRAW_POINTS: { @@ -421,6 +479,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkCanvas::PointMode mode = (SkCanvas::PointMode)reader->readInt(); size_t count = reader->readInt(); const SkPoint* pts = (const SkPoint*)reader->skip(sizeof(SkPoint)* count); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawPoints(mode, count, pts, *paint); } @@ -431,6 +491,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, get_text(reader, &text); size_t points = reader->readInt(); const SkPoint* pos = (const SkPoint*)reader->skip(points * sizeof(SkPoint)); + BREAK_ON_READ_ERROR(reader); + if (paint && text.text()) { canvas->drawPosText(text.text(), text.length(), pos, *paint); } @@ -443,6 +505,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkPoint* pos = (const SkPoint*)reader->skip(points * sizeof(SkPoint)); const SkScalar top = reader->readScalar(); const SkScalar bottom = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + SkRect clip; canvas->getClipBounds(&clip); if (top < clip.fBottom && bottom > clip.fTop && paint && text.text()) { @@ -456,6 +520,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, size_t xCount = reader->readInt(); const SkScalar constY = reader->readScalar(); const SkScalar* xpos = (const SkScalar*)reader->skip(xCount * sizeof(SkScalar)); + BREAK_ON_READ_ERROR(reader); + if (paint && text.text()) { canvas->drawPosTextH(text.text(), text.length(), xpos, constY, *paint); } @@ -466,6 +532,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, get_text(reader, &text); size_t xCount = reader->readInt(); const SkScalar* xpos = (const SkScalar*)reader->skip((3 + xCount) * sizeof(SkScalar)); + BREAK_ON_READ_ERROR(reader); + const SkScalar top = *xpos++; const SkScalar bottom = *xpos++; const SkScalar constY = *xpos++; @@ -479,6 +547,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkPaint* paint = fPictureData->getPaint(reader); SkRect rect; reader->readRect(&rect); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawRect(rect, *paint); } @@ -487,6 +557,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkPaint* paint = fPictureData->getPaint(reader); SkRegion region; reader->readRegion(®ion); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawRegion(region, *paint); } @@ -495,6 +567,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkPaint* paint = fPictureData->getPaint(reader); SkRRect rrect; reader->readRRect(&rrect); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawRRect(rrect, *paint); } @@ -512,6 +586,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, get_text(reader, &text); SkScalar x = reader->readScalar(); SkScalar y = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + if (paint && text.text()) { canvas->drawText(text.text(), text.length(), x, y, *paint); } @@ -521,6 +597,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkTextBlob* blob = fPictureData->getTextBlob(reader); SkScalar x = reader->readScalar(); SkScalar y = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawTextBlob(blob, x, y, *paint); } @@ -530,6 +608,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, TextContainer text; get_text(reader, &text); const SkScalar* ptr = (const SkScalar*)reader->skip(4 * sizeof(SkScalar)); + BREAK_ON_READ_ERROR(reader); + // ptr[0] == x // ptr[1] == y // ptr[2] == top @@ -549,6 +629,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkPath& path = fPictureData->getPath(reader); SkMatrix matrix; reader->readMatrix(&matrix); + BREAK_ON_READ_ERROR(reader); + if (paint && text.text()) { canvas->drawTextOnPath(text.text(), text.length(), path, &matrix, *paint); } @@ -564,6 +646,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, if (flags & DRAW_TEXT_RSXFORM_HAS_CULL) { cull = (const SkRect*)reader->skip(sizeof(SkRect)); } + BREAK_ON_READ_ERROR(reader); + if (text.text()) { canvas->drawTextRSXform(text.text(), text.length(), xform, cull, *paint); } @@ -596,6 +680,8 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, } xfer = SkXfermode::Make((SkXfermode::Mode)mode); } + BREAK_ON_READ_ERROR(reader); + if (paint) { canvas->drawVertices(vmode, vCount, verts, texs, colors, xfer, indices, iCount, *paint); @@ -604,16 +690,13 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, case RESTORE: canvas->restore(); break; - case ROTATE: - canvas->rotate(reader->readScalar()); - break; + case ROTATE: { + auto deg = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + + canvas->rotate(deg); + } break; case SAVE: - // SKPs with version < 29 also store a SaveFlags param. - if (size > 4) { - if (reader->validate(8 == size)) { - reader->readInt(); - } - } canvas->save(); break; case SAVE_LAYER_SAVEFLAGS_DEPRECATED: { @@ -621,13 +704,18 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, const SkRect* boundsPtr = get_rect_ptr(reader, &storage); const SkPaint* paint = fPictureData->getPaint(reader); auto flags = SkCanvas::LegacySaveFlagsToSaveLayerFlags(reader->readInt()); + BREAK_ON_READ_ERROR(reader); + canvas->saveLayer(SkCanvas::SaveLayerRec(boundsPtr, paint, flags)); } break; case SAVE_LAYER_SAVELAYERFLAGS_DEPRECATED_JAN_2016: { SkRect storage; const SkRect* boundsPtr = get_rect_ptr(reader, &storage); const SkPaint* paint = fPictureData->getPaint(reader); - canvas->saveLayer(SkCanvas::SaveLayerRec(boundsPtr, paint, reader->readInt())); + auto flags = reader->readInt(); + BREAK_ON_READ_ERROR(reader); + + canvas->saveLayer(SkCanvas::SaveLayerRec(boundsPtr, paint, flags)); } break; case SAVE_LAYER_SAVELAYERREC: { SkCanvas::SaveLayerRec rec(nullptr, nullptr, nullptr, 0); @@ -641,42 +729,57 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, rec.fPaint = fPictureData->getPaint(reader); } if (flatFlags & SAVELAYERREC_HAS_BACKDROP) { - const SkPaint* paint = fPictureData->getPaint(reader); - rec.fBackdrop = paint->getImageFilter(); + if (const auto* paint = fPictureData->getPaint(reader)) { + rec.fBackdrop = paint->getImageFilter(); + } } if (flatFlags & SAVELAYERREC_HAS_FLAGS) { rec.fSaveLayerFlags = reader->readInt(); } + BREAK_ON_READ_ERROR(reader); + canvas->saveLayer(rec); } break; case SCALE: { SkScalar sx = reader->readScalar(); SkScalar sy = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + canvas->scale(sx, sy); } break; case SET_MATRIX: { SkMatrix matrix; reader->readMatrix(&matrix); + BREAK_ON_READ_ERROR(reader); + matrix.postConcat(initialMatrix); canvas->setMatrix(matrix); } break; case SKEW: { SkScalar sx = reader->readScalar(); SkScalar sy = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + canvas->skew(sx, sy); } break; case TRANSLATE: { SkScalar dx = reader->readScalar(); SkScalar dy = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + canvas->translate(dx, dy); } break; case TRANSLATE_Z: { #ifdef SK_EXPERIMENTAL_SHADOWING SkScalar dz = reader->readScalar(); + BREAK_ON_READ_ERROR(reader); + canvas->translateZ(dz); #endif } break; default: SkASSERTF(false, "Unknown draw type: %d", op); } + +#undef BREAK_ON_READ_ERROR } -- cgit v1.2.3