Address stack overflow in SkPicturePriv::MakeFromBuffer An SkPicture can contain an SkReadBuffer which can contain an SkPicture which contains an SkPicture which can contain an SkPicture which contains an SkPicture which can contain ... https://review.skia.org/592736 added support for avoiding deeply nested skps, but missed the case where a ReadBuffer got tossed in the mix. I had a test case for this in PS2, but removed it because it was effectively just a huge opaque binary checked in and I didn't want it to get stale eventually. Bug: 496175229 Fixed: 496175229 Change-Id: I6183a78ed5a69499cb457d16d7e0fc430bff2b1f Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1226696 Reviewed-by: Florin Malita <fmalita@google.com> Commit-Queue: Kaylee Lubick <kjlubick@google.com>
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp index 94ae980..4610658 100644 --- a/src/core/SkPicture.cpp +++ b/src/core/SkPicture.cpp
@@ -142,10 +142,8 @@ return r.finishRecordingAsPicture(); } -static const int kNestedSKPLimit = 100; // Arbitrarily set - sk_sp<SkPicture> SkPicture::MakeFromStream(SkStream* stream, const SkDeserialProcs* procs) { - return MakeFromStreamPriv(stream, procs, nullptr, kNestedSKPLimit); + return MakeFromStreamPriv(stream, procs, nullptr, SkPicturePriv::kDefaultRecursionLimit); } sk_sp<SkPicture> SkPicture::MakeFromData(const void* data, size_t size, @@ -154,7 +152,7 @@ return nullptr; } SkMemoryStream stream(data, size); - return MakeFromStreamPriv(&stream, procs, nullptr, kNestedSKPLimit); + return MakeFromStreamPriv(&stream, procs, nullptr, SkPicturePriv::kDefaultRecursionLimit); } sk_sp<SkPicture> SkPicture::MakeFromData(const SkData* data, const SkDeserialProcs* procs) { @@ -162,7 +160,7 @@ return nullptr; } SkMemoryStream stream(data->data(), data->size()); - return MakeFromStreamPriv(&stream, procs, nullptr, kNestedSKPLimit); + return MakeFromStreamPriv(&stream, procs, nullptr, SkPicturePriv::kDefaultRecursionLimit); } sk_sp<SkPicture> SkPicture::MakeFromStreamPriv(SkStream* stream, const SkDeserialProcs* procsPtr, @@ -211,6 +209,9 @@ } sk_sp<SkPicture> SkPicturePriv::MakeFromBuffer(SkReadBuffer& buffer) { + if (!buffer.isValid()) { + return nullptr; + } SkPictInfo info; if (!SkPicture::BufferIsSKP(&buffer, &info)) { return nullptr; @@ -229,7 +230,7 @@ // 1 is the magic 'size' that means SkPictureData follows return nullptr; } - std::unique_ptr<SkPictureData> data(SkPictureData::CreateFromBuffer(buffer, info)); + std::unique_ptr<SkPictureData> data(SkPictureData::CreateFromBuffer(buffer, info)); return SkPicture::Forwardport(info, data.get(), &buffer); }
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp index 1d8d8f4..3382e63 100644 --- a/src/core/SkPictureData.cpp +++ b/src/core/SkPictureData.cpp
@@ -387,6 +387,10 @@ SkReadBuffer buffer(storage.get(), size); buffer.setVersion(fInfo.getVersion()); + // A Picture stream can contain a ReadBuffer which can, in turn, contain more + // pictures (but not more streams or buffers), so we need to remove 1 from the + // current recursion limit to count *this* picture before passing it on. + buffer.setRecursionLimit(recursionLimit - 1); if (!fFactoryPlayback) { return false; @@ -412,7 +416,7 @@ } } break; } - return true; // success + return true; // success } static sk_sp<SkImage> create_image_from_buffer(SkReadBuffer& buffer) { @@ -423,10 +427,13 @@ return sk_sp<SkDrawable>((SkDrawable*)buffer.readFlattenable(SkFlattenable::kSkDrawable_Type)); } +template <typename U> using FactoryFn = sk_sp<U> (*)(SkReadBuffer&); // We need two types 'cause SkDrawable is const-variant. template <typename T, typename U> -bool new_array_from_buffer(SkReadBuffer& buffer, uint32_t inCount, - TArray<sk_sp<T>>& array, sk_sp<U> (*factory)(SkReadBuffer&)) { +bool new_array_from_buffer(SkReadBuffer& buffer, + uint32_t inCount, + TArray<sk_sp<T>>& array, + FactoryFn<U> factory) { if (!buffer.validate(array.empty() && SkTFitsIn<int>(inCount))) { return false; } @@ -506,10 +513,14 @@ fOpData = std::move(data); } break; case SK_PICT_PICTURE_TAG: + buffer.downLevel(); new_array_from_buffer(buffer, size, fPictures, SkPicturePriv::MakeFromBuffer); + buffer.upLevel(); break; case SK_PICT_DRAWABLE_TAG: + buffer.downLevel(); // Drawables could contain pictures in theory new_array_from_buffer(buffer, size, fDrawables, create_drawable_from_buffer); + buffer.upLevel(); break; default: buffer.validate(false); // The tag was invalid. @@ -522,6 +533,9 @@ const SkDeserialProcs& procs, SkTypefacePlayback* topLevelTFPlayback, int recursionLimit) { + if (recursionLimit <= 0) { + return nullptr; + } std::unique_ptr<SkPictureData> data(new SkPictureData(info)); if (!topLevelTFPlayback) { topLevelTFPlayback = &data->fTFPlayback; @@ -535,6 +549,9 @@ SkPictureData* SkPictureData::CreateFromBuffer(SkReadBuffer& buffer, const SkPictInfo& info) { + if (!buffer.isValid()) { + return nullptr; + } std::unique_ptr<SkPictureData> data(new SkPictureData(info)); buffer.setVersion(info.getVersion());
diff --git a/src/core/SkPicturePriv.h b/src/core/SkPicturePriv.h index aae89b4..844fa08 100644 --- a/src/core/SkPicturePriv.h +++ b/src/core/SkPicturePriv.h
@@ -31,6 +31,7 @@ * @return A new SkPicture representing the serialized data, or NULL if the buffer is * invalid. */ + static constexpr int kDefaultRecursionLimit = 100; static sk_sp<SkPicture> MakeFromBuffer(SkReadBuffer& buffer); /**
diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp index 819d2a6..aecb07c 100644 --- a/src/core/SkReadBuffer.cpp +++ b/src/core/SkReadBuffer.cpp
@@ -470,6 +470,9 @@ } SkFlattenable* SkReadBuffer::readRawFlattenable() { + if (!this->isValid()) { + return nullptr; + } SkFlattenable::Factory factory = nullptr; if (fFactoryCount > 0) {
diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h index 6578908..7d2c92e 100644 --- a/src/core/SkReadBuffer.h +++ b/src/core/SkReadBuffer.h
@@ -227,6 +227,21 @@ SkSamplingOptions readSampling(); + // SKPs can contain other SKPs and ImageFilters can contain other ImageFilters, so this + // can be used to limit how deeply nested things are. + void downLevel() { + fRecursionLimit -= 1; + this->validate(fRecursionLimit > 0); + } + void upLevel() { + fRecursionLimit += 1; + // If the readbuffer hit the limit, we'll maintain the error state + } + void setRecursionLimit(int newLimit) { + fRecursionLimit = newLimit; + this->validate(fRecursionLimit > 0); + } + private: const char* readString(size_t* length); @@ -256,6 +271,8 @@ return SkIsAlign4((uintptr_t)ptr); } + int fRecursionLimit = 100; + bool fAllowSkSL = true; bool fError = false; };
diff --git a/src/core/SkRecordedDrawable.cpp b/src/core/SkRecordedDrawable.cpp index 1c309b2..7a1cb56 100644 --- a/src/core/SkRecordedDrawable.cpp +++ b/src/core/SkRecordedDrawable.cpp
@@ -86,6 +86,9 @@ } sk_sp<SkFlattenable> SkRecordedDrawable::CreateProc(SkReadBuffer& buffer) { + if (!buffer.isValid()) { + return nullptr; + } // Read the bounds. SkRect bounds; buffer.readRect(&bounds);