From 9bbff29d9ca38b7f044041d13752466326af30b5 Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Tue, 8 Feb 2022 12:28:11 -0500 Subject: [PATCH] Add flatten and MakeFromBuffer to SkDescriptor Create canonical flattening for SkDescriptor and unflattening for SkAutoDescriptor. Eventually Slug serialization and the remote glyphs cache will use this method for SkDescriptor serialization. Change-Id: Ia4b6be43058aeca19fbfdcf3c5cdd8d703935775 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/505681 Reviewed-by: Kevin Lubick Reviewed-by: Ben Wagner Commit-Queue: Herb Derby --- fuzz/oss_fuzz/FuzzSkDescriptorDeserialize.cpp | 12 ++-- .../chromium/SkChromeRemoteGlyphCache.h | 4 -- src/core/SkChromeRemoteGlyphCache.cpp | 6 -- src/core/SkDescriptor.cpp | 41 ++++++++++-- src/core/SkDescriptor.h | 8 ++- tests/DescriptorTest.cpp | 66 +++++++++++++++++++ 6 files changed, 114 insertions(+), 23 deletions(-) diff --git a/fuzz/oss_fuzz/FuzzSkDescriptorDeserialize.cpp b/fuzz/oss_fuzz/FuzzSkDescriptorDeserialize.cpp index b3be063ea7..681cbf32e5 100644 --- a/fuzz/oss_fuzz/FuzzSkDescriptorDeserialize.cpp +++ b/fuzz/oss_fuzz/FuzzSkDescriptorDeserialize.cpp @@ -5,17 +5,17 @@ * found in the LICENSE file. */ -#include "include/private/chromium/SkChromeRemoteGlyphCache.h" #include "src/core/SkDescriptor.h" +#include "src/core/SkReadBuffer.h" void FuzzSkDescriptorDeserialize(sk_sp bytes) { - SkAutoDescriptor aDesc; - bool ok = SkFuzzDeserializeSkDescriptor(bytes, &aDesc); - if (!ok) { - return; + SkReadBuffer buffer{bytes->data(), bytes->size()}; + auto sut = SkAutoDescriptor::MakeFromBuffer(buffer); + if (!sut.has_value()) { + return; } - auto desc = aDesc.getDesc(); + auto desc = sut->getDesc(); desc->computeChecksum(); desc->isValid(); diff --git a/include/private/chromium/SkChromeRemoteGlyphCache.h b/include/private/chromium/SkChromeRemoteGlyphCache.h index 033b03fe6a..4673b39408 100644 --- a/include/private/chromium/SkChromeRemoteGlyphCache.h +++ b/include/private/chromium/SkChromeRemoteGlyphCache.h @@ -136,8 +136,4 @@ public: private: std::unique_ptr fImpl; }; - -// For exposure to fuzzing only. -bool SkFuzzDeserializeSkDescriptor(sk_sp bytes, SkAutoDescriptor* ad); - #endif // SkChromeRemoteGlyphCache_DEFINED diff --git a/src/core/SkChromeRemoteGlyphCache.cpp b/src/core/SkChromeRemoteGlyphCache.cpp index 88f41a08cd..830707ac8e 100644 --- a/src/core/SkChromeRemoteGlyphCache.cpp +++ b/src/core/SkChromeRemoteGlyphCache.cpp @@ -1186,9 +1186,3 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi sk_sp SkStrikeClient::deserializeTypeface(const void* buf, size_t len) { return fImpl->deserializeTypeface(buf, len); } - -// ------------------------------------------------------------------------------------------------- -bool SkFuzzDeserializeSkDescriptor(sk_sp bytes, SkAutoDescriptor* ad) { - auto d = Deserializer(reinterpret_cast(bytes->data()), bytes->size()); - return d.readDescriptor(ad); -} diff --git a/src/core/SkDescriptor.cpp b/src/core/SkDescriptor.cpp index 787fb29655..bac40a880b 100644 --- a/src/core/SkDescriptor.cpp +++ b/src/core/SkDescriptor.cpp @@ -6,16 +6,21 @@ */ #include "src/core/SkDescriptor.h" +#include #include #include "include/core/SkTypes.h" #include "include/private/SkTo.h" +#include "include/private/chromium/SkChromeRemoteGlyphCache.h" #include "src/core/SkOpts.h" +#include "src/core/SkReadBuffer.h" +#include "src/core/SkWriteBuffer.h" +#include "src/gpu/GrResourceProvider.h" std::unique_ptr SkDescriptor::Alloc(size_t length) { - SkASSERT(SkAlign4(length) == length); - void* allocation = ::operator new (length); + SkASSERT(length >= sizeof(SkDescriptor) && SkAlign4(length) == length); + void* allocation = ::operator new(length); return std::unique_ptr(new (allocation) SkDescriptor{}); } @@ -24,6 +29,10 @@ void* SkDescriptor::operator new(size_t) { SK_ABORT("Descriptors are created with placement new."); } +void SkDescriptor::flatten(SkWriteBuffer& buffer) { + buffer.writePad32(static_cast(this), this->fLength); +} + void* SkDescriptor::addEntry(uint32_t tag, size_t length, const void* data) { SkASSERT(tag); SkASSERT(SkAlign4(length) == length); @@ -38,7 +47,7 @@ void* SkDescriptor::addEntry(uint32_t tag, size_t length, const void* data) { fCount += 1; fLength = SkToU32(fLength + sizeof(Entry) + length); - return (entry + 1); // return its data + return (entry + 1); // return its data } void SkDescriptor::computeChecksum() { @@ -47,7 +56,7 @@ void SkDescriptor::computeChecksum() { const void* SkDescriptor::findEntry(uint32_t tag, uint32_t* length) const { const Entry* entry = (const Entry*)(this + 1); - int count = fCount; + int count = fCount; while (--count >= 0) { if (entry->fTag == tag) { @@ -68,7 +77,6 @@ std::unique_ptr SkDescriptor::copy() const { } bool SkDescriptor::operator==(const SkDescriptor& other) const { - // the first value we should look at is the checksum, so this loop // should terminate early if they descriptors are different. // NOTE: if we wrote a sentinel value at the end of each, we could @@ -96,7 +104,7 @@ SkString SkDescriptor::dumpRec() const { } uint32_t SkDescriptor::ComputeChecksum(const SkDescriptor* desc) { - const uint32_t* ptr = (const uint32_t*)desc + 1; // skip the checksum field + const uint32_t* ptr = (const uint32_t*)desc + 1; // skip the checksum field size_t len = desc->fLength - sizeof(uint32_t); return SkOpts::hash(ptr, len); } @@ -165,6 +173,27 @@ SkAutoDescriptor& SkAutoDescriptor::operator=(SkAutoDescriptor&& that) { SkAutoDescriptor::~SkAutoDescriptor() { this->free(); } +std::optional SkAutoDescriptor::MakeFromBuffer(SkReadBuffer& buffer) { + SkDescriptor descriptorHeader; + if (!buffer.readPad32(&descriptorHeader, sizeof(SkDescriptor))) { return {}; } + + // Basic bounds check on header length to make sure that bodyLength calculation does not + // underflow. + if (descriptorHeader.getLength() < sizeof(SkDescriptor)) { return {}; } + uint32_t bodyLength = descriptorHeader.getLength() - sizeof(SkDescriptor); + + SkAutoDescriptor ad{descriptorHeader.getLength()}; + memcpy(ad.fDesc, &descriptorHeader, sizeof(SkDescriptor)); + if (!buffer.readPad32(SkTAddOffset(ad.fDesc, sizeof(SkDescriptor)), bodyLength)) { + return {}; + } + + if (SkDescriptor::ComputeChecksum(ad.getDesc()) != ad.getDesc()->fChecksum) { return {}; } + if (!ad.getDesc()->isValid()) { return {}; } + + return {ad}; +} + void SkAutoDescriptor::reset(size_t size) { this->free(); if (size <= sizeof(fStorage)) { diff --git a/src/core/SkDescriptor.h b/src/core/SkDescriptor.h index 3142908791..de1b98fc86 100644 --- a/src/core/SkDescriptor.h +++ b/src/core/SkDescriptor.h @@ -13,6 +13,8 @@ #include "include/private/SkMacros.h" #include "include/private/SkNoncopyable.h" +#include "src/core/SkBuffer.h" +#include "src/core/SkFontPriv.h" #include "src/core/SkScalerContext.h" class SkDescriptor : SkNoncopyable { @@ -30,6 +32,8 @@ public: void* operator new(size_t); void* operator new(size_t, void* p) { return p; } + void flatten(SkWriteBuffer& buffer); + uint32_t getLength() const { return fLength; } void* addEntry(uint32_t tag, size_t length, const void* data = nullptr); void computeChecksum(); @@ -86,9 +90,11 @@ public: SkAutoDescriptor& operator=(const SkAutoDescriptor&); SkAutoDescriptor(SkAutoDescriptor&&); SkAutoDescriptor& operator=(SkAutoDescriptor&&); - ~SkAutoDescriptor(); + // Returns no value if there is an error. + static std::optional MakeFromBuffer(SkReadBuffer& buffer); + void reset(size_t size); void reset(const SkDescriptor& desc); SkDescriptor* getDesc() const { SkASSERT(fDesc); return fDesc; } diff --git a/tests/DescriptorTest.cpp b/tests/DescriptorTest.cpp index 53bf67b0ad..3f234b7d4a 100644 --- a/tests/DescriptorTest.cpp +++ b/tests/DescriptorTest.cpp @@ -6,8 +6,13 @@ */ #include "include/core/SkTypes.h" +#include "include/private/chromium/SkChromeRemoteGlyphCache.h" #include "src/core/SkDescriptor.h" +#include "src/core/SkFontPriv.h" +#include "src/core/SkReadBuffer.h" #include "src/core/SkScalerContext.h" +#include "src/core/SkWriteBuffer.h" +#include "src/gpu/GrResourceProvider.h" #include "tests/Test.h" #include @@ -128,3 +133,64 @@ DEF_TEST(Descriptor_entry_over_end, r) { SkDescriptorTestHelper::SetCount(desc.get(), 2); REPORTER_ASSERT(r, !desc->isValid()); } + +DEF_TEST(Descriptor_flatten_unflatten, r) { + { + SkBinaryWriteBuffer writer; + auto desc = SkDescriptor::Alloc(sizeof(SkDescriptor)); + desc->computeChecksum(); + desc->flatten(writer); + auto data = writer.snapshotAsData(); + SkReadBuffer reader{data->data(), data->size()}; + auto ad = SkAutoDescriptor::MakeFromBuffer(reader); + REPORTER_ASSERT(r, ad.has_value()); + REPORTER_ASSERT(r, ad->getDesc()->isValid()); + } + + { // broken header + SkBinaryWriteBuffer writer; + writer.writeInt(0); // fChecksum + auto data = writer.snapshotAsData(); + SkReadBuffer reader{data->data(), data->size()}; + auto ad = SkAutoDescriptor::MakeFromBuffer(reader); + REPORTER_ASSERT(r, !ad.has_value()); + } + + { // length too big + SkBinaryWriteBuffer writer; + // Simulate a broken header + writer.writeInt(0); // fChecksum + writer.writeInt(4000); // fLength + writer.writeInt(0); // fCount + auto data = writer.snapshotAsData(); + SkReadBuffer reader{data->data(), data->size()}; + auto ad = SkAutoDescriptor::MakeFromBuffer(reader); + REPORTER_ASSERT(r, !ad.has_value()); + } + + { // length too small + SkBinaryWriteBuffer writer; + // Simulate a broken header + writer.writeInt(0); // fChecksum + writer.writeInt(3); // fLength + writer.writeInt(0); // fCount + auto data = writer.snapshotAsData(); + SkReadBuffer reader{data->data(), data->size()}; + auto ad = SkAutoDescriptor::MakeFromBuffer(reader); + REPORTER_ASSERT(r, !ad.has_value()); + } + + { // garbage in count + SkBinaryWriteBuffer writer; + // Simulate a broken header + writer.writeInt(0); // fChecksum + writer.writeInt(20); // fLength + writer.writeInt(10); // fCount + writer.writeInt(0); + writer.writeInt(0); + auto data = writer.snapshotAsData(); + SkReadBuffer reader{data->data(), data->size()}; + auto ad = SkAutoDescriptor::MakeFromBuffer(reader); + REPORTER_ASSERT(r, !ad.has_value()); + } +}