Return nullptr when ReadBuffer becomes invalid

This especially helps in SkDrawLooper because we can bail out early
instead of looping for a potentially long time, e.g. when fuzzed
input says count is a large number.

This also cleans up validate in a few spots, and adds validateCanReadN
as a helper function.

Bug: skia:7937
Change-Id: Ic5eff357c8cadc91eeafc6e39c78c570ba74df2f
Reviewed-on: https://skia-review.googlesource.com/128847
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>

11 files changed, 31 insertions, 16 deletions

diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index f6b86c5aa9..5362c1aca7 100644
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -261,11 +261,10 @@ private:
 
 sk_sp<SkFlattenable> SkSRGBGammaColorFilter::CreateProc(SkReadBuffer& buffer) {
     uint32_t dir = buffer.read32();
-    if (dir <= 1) {
-        return sk_sp<SkFlattenable>(new SkSRGBGammaColorFilter(static_cast<Direction>(dir)));
+    if (!buffer.validate(dir <= 1)) {
+        return nullptr;
     }
-    buffer.validate(false);
-    return nullptr;
+    return sk_sp<SkFlattenable>(new SkSRGBGammaColorFilter(static_cast<Direction>(dir)));
 }
 
 void SkSRGBGammaColorFilter::toString(SkString* str) const {
diff --git a/src/core/SkColorTable.cpp b/src/core/SkColorTable.cpp
index fab3c3dc1c..022b9f8ae3 100644
--- a/src/core/SkColorTable.cpp
+++ b/src/core/SkColorTable.cpp
@@ -25,9 +25,7 @@ SkColorTable::~SkColorTable() {
 
 void SkColorTable::Skip(SkReadBuffer& buffer) {
     const int count = buffer.getArrayCount();
-    if (count < 0 || count > 256) {
-        buffer.validate(false);
-    } else {
+    if (buffer.validate(count >= 0 && count <= 256)) {
         buffer.skip(count * sizeof(SkPMColor));
     }
 }
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index 571c6d7b35..efd41837d9 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -427,7 +427,7 @@ void SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t
         case SK_PICT_READER_TAG: {
             // Preflight check that we can initialize all data from the buffer
             // before allocating it.
-            if (!buffer.validate(size <= buffer.available())) {
+            if (!buffer.validateCanReadN<uint8_t>(size)) {
                 return;
             }
             auto data(SkData::MakeUninitialized(size));
diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp
index d41f2902f2..0c2edf622d 100644
--- a/src/core/SkReadBuffer.cpp
+++ b/src/core/SkReadBuffer.cpp
@@ -317,7 +317,7 @@ sk_sp<SkImage> SkReadBuffer::readImage() {
     // Preflight check to make sure there's enough stuff in the buffer before
     // we allocate the memory. This helps the fuzzer avoid OOM when it creates
     // bad/corrupt input.
-    if (!this->validate(((size_t)size) <= this->available())) {
+    if (!this->validateCanReadN<uint8_t>(size)) {
         return nullptr;
     }
 
diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h
index fd6f5d279a..48a7aedf89 100644
--- a/src/core/SkReadBuffer.h
+++ b/src/core/SkReadBuffer.h
@@ -219,6 +219,17 @@ public:
         }
         return !fError;
     }
+
+    /**
+     * Helper function to do a preflight check before a large allocation or read.
+     * Returns true if there is enough bytes in the buffer to read n elements of T.
+     * If not, the buffer will be "invalid" and false will be returned.
+     */
+    template <typename T>
+    bool validateCanReadN(size_t n) {
+        return this->validate(n <= (fReader.available() / sizeof(T)));
+    }
+
     bool isValid() const { return !fError; }
     bool validateIndex(int index, int count) {
         return this->validate(index >= 0 && index < count);
diff --git a/src/effects/SkArithmeticImageFilter.cpp b/src/effects/SkArithmeticImageFilter.cpp
index 24c95c82ce..984ed80a19 100644
--- a/src/effects/SkArithmeticImageFilter.cpp
+++ b/src/effects/SkArithmeticImageFilter.cpp
@@ -86,6 +86,9 @@ sk_sp<SkFlattenable> ArithmeticImageFilterImpl::CreateProc(SkReadBuffer& buffer)
         k[i] = buffer.readScalar();
     }
     const bool enforcePMColor = buffer.readBool();
+    if (!buffer.isValid()) {
+        return nullptr;
+    }
     return SkArithmeticImageFilter::Make(k[0], k[1], k[2], k[3], enforcePMColor, common.getInput(0),
                                          common.getInput(1), &common.cropRect());
 }
diff --git a/src/effects/SkDashPathEffect.cpp b/src/effects/SkDashPathEffect.cpp
index 4cb98b3ad8..cdadcf907d 100644
--- a/src/effects/SkDashPathEffect.cpp
+++ b/src/effects/SkDashPathEffect.cpp
@@ -369,7 +369,7 @@ sk_sp<SkFlattenable> SkDashImpl::CreateProc(SkReadBuffer& buffer) {
     uint32_t count = buffer.getArrayCount();
 
     // Don't allocate gigantic buffers if there's not data for them.
-    if (count > buffer.size() / sizeof(SkScalar)) {
+    if (!buffer.validateCanReadN<SkScalar>(count)) {
         return nullptr;
     }
 
diff --git a/src/effects/SkLayerDrawLooper.cpp b/src/effects/SkLayerDrawLooper.cpp
index 6a8254d894..db61e08c4f 100644
--- a/src/effects/SkLayerDrawLooper.cpp
+++ b/src/effects/SkLayerDrawLooper.cpp
@@ -272,6 +272,9 @@ sk_sp<SkFlattenable> SkLayerDrawLooper::CreateProc(SkReadBuffer& buffer) {
         buffer.readPoint(&info.fOffset);
         info.fPostTranslate = buffer.readBool();
         buffer.readPaint(builder.addLayerOnTop(info));
+        if (!buffer.isValid()) {
+            return nullptr;
+        }
     }
     return builder.detach();
 }
diff --git a/src/effects/SkXfermodeImageFilter.cpp b/src/effects/SkXfermodeImageFilter.cpp
index 1c670fbe0b..587784a391 100644
--- a/src/effects/SkXfermodeImageFilter.cpp
+++ b/src/effects/SkXfermodeImageFilter.cpp
@@ -375,6 +375,9 @@ sk_sp<SkFlattenable> SkXfermodeImageFilter_Base::LegacyArithmeticCreateProc(SkRe
         k[i] = buffer.readScalar();
     }
     const bool enforcePMColor = buffer.readBool();
+    if (!buffer.isValid()) {
+        return nullptr;
+    }
     return SkArithmeticImageFilter::Make(k[0], k[1], k[2], k[3], enforcePMColor, common.getInput(0),
                                          common.getInput(1), &common.cropRect());
 }
diff --git a/src/shaders/gradients/SkGradientShader.cpp b/src/shaders/gradients/SkGradientShader.cpp
index 60e9f2adbf..78f514c1c6 100644
--- a/src/shaders/gradients/SkGradientShader.cpp
+++ b/src/shaders/gradients/SkGradientShader.cpp
@@ -17,7 +17,6 @@
 #include "SkMallocPixelRef.h"
 #include "SkRadialGradient.h"
 #include "SkReadBuffer.h"
-#include "SkSafeMath.h"
 #include "SkSweepGradient.h"
 #include "SkTwoPointConicalGradient.h"
 #include "SkWriteBuffer.h"
@@ -74,10 +73,7 @@ void SkGradientShaderBase::Descriptor::flatten(SkWriteBuffer& buffer) const {
 
 template <int N, typename T, bool MEM_MOVE>
 static bool validate_array(SkReadBuffer& buffer, size_t count, SkSTArray<N, T, MEM_MOVE>* array) {
-    SkSafeMath safe;
-    const auto expectedSize = safe.mul(sizeof(T), count);
-
-    if (!buffer.validate(safe && expectedSize <= buffer.available())) {
+    if (!buffer.validateCanReadN<T>(count)) {
         return false;
     }
 
diff --git a/src/shaders/gradients/SkTwoPointConicalGradient.cpp b/src/shaders/gradients/SkTwoPointConicalGradient.cpp
index d98f4bce20..73ec3f213b 100644
--- a/src/shaders/gradients/SkTwoPointConicalGradient.cpp
+++ b/src/shaders/gradients/SkTwoPointConicalGradient.cpp
@@ -156,7 +156,9 @@ sk_sp<SkFlattenable> SkTwoPointConicalGradient::CreateProc(SkReadBuffer& buffer)
             }
         }
     }
-
+    if (!buffer.isValid()) {
+        return nullptr;
+    }
     return SkGradientShader::MakeTwoPointConical(c1, r1, c2, r2, desc.fColors,
                                                  std::move(desc.fColorSpace), desc.fPos,
                                                  desc.fCount, desc.fTileMode, desc.fGradFlags,