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>
diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index f6b86c5..5362c1a 100644
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -261,11 +261,10 @@
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 fab3c3d..022b9f8 100644
--- a/src/core/SkColorTable.cpp
+++ b/src/core/SkColorTable.cpp
@@ -25,9 +25,7 @@
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 571c6d7..efd4183 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -427,7 +427,7 @@
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 d41f290..0c2edf6 100644
--- a/src/core/SkReadBuffer.cpp
+++ b/src/core/SkReadBuffer.cpp
@@ -317,7 +317,7 @@
// 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 fd6f5d2..48a7aed 100644
--- a/src/core/SkReadBuffer.h
+++ b/src/core/SkReadBuffer.h
@@ -219,6 +219,17 @@
}
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 24c95c8..984ed80 100644
--- a/src/effects/SkArithmeticImageFilter.cpp
+++ b/src/effects/SkArithmeticImageFilter.cpp
@@ -86,6 +86,9 @@
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 4cb98b3..cdadcf9 100644
--- a/src/effects/SkDashPathEffect.cpp
+++ b/src/effects/SkDashPathEffect.cpp
@@ -369,7 +369,7 @@
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 6a8254d..db61e08 100644
--- a/src/effects/SkLayerDrawLooper.cpp
+++ b/src/effects/SkLayerDrawLooper.cpp
@@ -272,6 +272,9 @@
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 1c670fb..587784a 100644
--- a/src/effects/SkXfermodeImageFilter.cpp
+++ b/src/effects/SkXfermodeImageFilter.cpp
@@ -375,6 +375,9 @@
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 60e9f2a..78f514c 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 @@
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 d98f4bc..73ec3f2 100644
--- a/src/shaders/gradients/SkTwoPointConicalGradient.cpp
+++ b/src/shaders/gradients/SkTwoPointConicalGradient.cpp
@@ -156,7 +156,9 @@
}
}
}
-
+ 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,