From 98624d249d279f68127c76754d542ab5cd0f8eab Mon Sep 17 00:00:00 2001 From: Robert Phillips Date: Mon, 19 Dec 2016 11:37:37 -0500 Subject: [PATCH] "Fix" some ImageFilter fuzzer issues SkClipOp.h & SkPictureFlat.h Invalid SkClipOps were getting through - the question here is where (for a class enum) is a good place to put the k*Mask definition. SkPath1DPathEffect NaNs were getting past. SkBlurMaskFilter Assert wasn't necessary since we whacked the flag on the next line. Change-Id: I87f95ad39f4760284f881d7c4500eb82fcdba282 Reviewed-on: https://skia-review.googlesource.com/6194 Commit-Queue: Robert Phillips Reviewed-by: Herb Derby --- gm/drawfilter.cpp | 4 ++-- include/core/SkClipOp.h | 2 ++ include/effects/SkBlurMaskFilter.h | 4 ++-- src/core/SkPictureFlat.h | 13 +++++++++++-- src/core/SkPicturePlayback.cpp | 8 ++++---- src/effects/Sk1DPathEffect.cpp | 2 +- src/effects/SkBlurMaskFilter.cpp | 15 +++++++++------ tests/BlurTest.cpp | 2 +- 8 files changed, 32 insertions(+), 18 deletions(-) diff --git a/gm/drawfilter.cpp b/gm/drawfilter.cpp index d989c4c..b283c13 100644 --- a/gm/drawfilter.cpp +++ b/gm/drawfilter.cpp @@ -46,8 +46,8 @@ protected: void onOnceBeforeDraw() override { fBlur = SkBlurMaskFilter::Make(kNormal_SkBlurStyle, - SkBlurMask::ConvertRadiusToSigma(10.0f), - kLow_SkBlurQuality); + SkBlurMask::ConvertRadiusToSigma(10.0f), + kLow_SkBlurQuality); } void onDraw(SkCanvas* canvas) override { diff --git a/include/core/SkClipOp.h b/include/core/SkClipOp.h index 1c4b3b7..3035565 100644 --- a/include/core/SkClipOp.h +++ b/include/core/SkClipOp.h @@ -61,6 +61,8 @@ enum class SkClipOp { kXOR_private_internal_do_not_use = 3, kReverseDifference_private_internal_do_not_use = 4, kReplace_private_internal_do_not_use = 5, + + kMax_EnumValue = kReplace_private_internal_do_not_use }; #endif diff --git a/include/effects/SkBlurMaskFilter.h b/include/effects/SkBlurMaskFilter.h index 4106b01..8b032e5 100644 --- a/include/effects/SkBlurMaskFilter.h +++ b/include/effects/SkBlurMaskFilter.h @@ -22,13 +22,13 @@ public: static SkScalar ConvertRadiusToSigma(SkScalar radius); enum BlurFlags { - kNone_BlurFlag = 0x00, + kNone_BlurFlag = 0x00, /** The blur layer's radius is not affected by transforms */ kIgnoreTransform_BlurFlag = 0x01, /** Use a smother, higher qulity blur algorithm */ kHighQuality_BlurFlag = 0x02, /** mask for all blur flags */ - kAll_BlurFlag = 0x03 + kAll_BlurFlag = 0x03 }; /** Create a blur maskfilter. diff --git a/src/core/SkPictureFlat.h b/src/core/SkPictureFlat.h index 1ac91b7..3489043 100644 --- a/src/core/SkPictureFlat.h +++ b/src/core/SkPictureFlat.h @@ -133,8 +133,17 @@ static inline uint32_t ClipParams_pack(SkClipOp op, bool doAA) { return (doAABit << 4) | static_cast(op); } -static inline SkClipOp ClipParams_unpackRegionOp(uint32_t packed) { - return (SkClipOp)(packed & 0xF); +template T asValidEnum(SkReadBuffer* buffer, uint32_t candidate) { + + if (buffer->validate(candidate <= static_cast(T::kMax_EnumValue))) { + return static_cast(candidate); + } + + return T::kMax_EnumValue; +} + +static inline SkClipOp ClipParams_unpackRegionOp(SkReadBuffer* buffer, uint32_t packed) { + return asValidEnum(buffer, packed & 0xF); } static inline bool ClipParams_unpackDoAA(uint32_t packed) { diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp index 85e5c03..c52233a 100644 --- a/src/core/SkPicturePlayback.cpp +++ b/src/core/SkPicturePlayback.cpp @@ -136,7 +136,7 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, case CLIP_PATH: { const SkPath& path = fPictureData->getPath(reader); uint32_t packed = reader->readInt(); - SkClipOp clipOp = ClipParams_unpackRegionOp(packed); + SkClipOp clipOp = ClipParams_unpackRegionOp(reader, packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); BREAK_ON_READ_ERROR(reader); @@ -151,7 +151,7 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkRegion region; reader->readRegion(®ion); uint32_t packed = reader->readInt(); - SkClipOp clipOp = ClipParams_unpackRegionOp(packed); + SkClipOp clipOp = ClipParams_unpackRegionOp(reader, packed); size_t offsetToRestore = reader->readInt(); BREAK_ON_READ_ERROR(reader); @@ -165,7 +165,7 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkRect rect; reader->readRect(&rect); uint32_t packed = reader->readInt(); - SkClipOp clipOp = ClipParams_unpackRegionOp(packed); + SkClipOp clipOp = ClipParams_unpackRegionOp(reader, packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); BREAK_ON_READ_ERROR(reader); @@ -180,7 +180,7 @@ void SkPicturePlayback::handleOp(SkReadBuffer* reader, SkRRect rrect; reader->readRRect(&rrect); uint32_t packed = reader->readInt(); - SkClipOp clipOp = ClipParams_unpackRegionOp(packed); + SkClipOp clipOp = ClipParams_unpackRegionOp(reader, packed); bool doAA = ClipParams_unpackDoAA(packed); size_t offsetToRestore = reader->readInt(); BREAK_ON_READ_ERROR(reader); diff --git a/src/effects/Sk1DPathEffect.cpp b/src/effects/Sk1DPathEffect.cpp index 26cd046..2247a79 100644 --- a/src/effects/Sk1DPathEffect.cpp +++ b/src/effects/Sk1DPathEffect.cpp @@ -205,7 +205,7 @@ void SkPath1DPathEffect::toString(SkString* str) const { sk_sp SkPath1DPathEffect::Make(const SkPath& path, SkScalar advance, SkScalar phase, Style style) { - if (advance <= 0 || path.isEmpty()) { + if (advance <= 0 || !SkScalarIsFinite(advance) || path.isEmpty()) { return nullptr; } return sk_sp(new SkPath1DPathEffect(path, advance, phase, style)); diff --git a/src/effects/SkBlurMaskFilter.cpp b/src/effects/SkBlurMaskFilter.cpp index 84d9e18..f56c273 100644 --- a/src/effects/SkBlurMaskFilter.cpp +++ b/src/effects/SkBlurMaskFilter.cpp @@ -128,14 +128,12 @@ const SkScalar SkBlurMaskFilterImpl::kMAX_BLUR_SIGMA = SkIntToScalar(128); sk_sp SkBlurMaskFilter::Make(SkBlurStyle style, SkScalar sigma, const SkRect& occluder, uint32_t flags) { + SkASSERT(!(flags & ~SkBlurMaskFilter::kAll_BlurFlag)); + SkASSERT(style <= kLastEnum_SkBlurStyle); + if (!SkScalarIsFinite(sigma) || sigma <= 0) { return nullptr; } - if ((unsigned)style > (unsigned)kLastEnum_SkBlurStyle) { - return nullptr; - } - SkASSERT(flags <= SkBlurMaskFilter::kAll_BlurFlag); - flags &= SkBlurMaskFilter::kAll_BlurFlag; return sk_sp(new SkBlurMaskFilterImpl(sigma, style, occluder, flags)); } @@ -735,7 +733,12 @@ void SkBlurMaskFilterImpl::computeFastBounds(const SkRect& src, sk_sp SkBlurMaskFilterImpl::CreateProc(SkReadBuffer& buffer) { const SkScalar sigma = buffer.readScalar(); const unsigned style = buffer.readUInt(); - const unsigned flags = buffer.readUInt(); + unsigned flags = buffer.readUInt(); + + buffer.validate(style <= kLastEnum_SkBlurStyle); + buffer.validate(!(flags & ~SkBlurMaskFilter::kAll_BlurFlag)); + + flags &= SkBlurMaskFilter::kAll_BlurFlag; SkRect occluder; if (buffer.isVersionLT(SkReadBuffer::kBlurMaskFilterWritesOccluder)) { diff --git a/tests/BlurTest.cpp b/tests/BlurTest.cpp index 2893a0c..f2c35d7 100644 --- a/tests/BlurTest.cpp +++ b/tests/BlurTest.cpp @@ -507,7 +507,7 @@ DEF_TEST(BlurAsABlur, reporter) { // Test asABlur for SkBlurMaskFilter // for (size_t i = 0; i < SK_ARRAY_COUNT(styles); ++i) { - const SkBlurStyle style = (SkBlurStyle)styles[i]; + const SkBlurStyle style = styles[i]; for (size_t j = 0; j < SK_ARRAY_COUNT(sigmas); ++j) { const SkScalar sigma = sigmas[j]; for (int flags = 0; flags <= SkBlurMaskFilter::kAll_BlurFlag; ++flags) { -- 2.7.4