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 <kjlubick@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Herb Derby <herb@google.com>

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<SkData> 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();

include/private/chromium/SkChromeRemoteGlyphCache.h

@@ -136,8 +136,4 @@ public:
 private:
     std::unique_ptr<SkStrikeClientImpl> fImpl;
 };
-
-// For exposure to fuzzing only.
-bool SkFuzzDeserializeSkDescriptor(sk_sp<SkData> bytes, SkAutoDescriptor* ad);
-
 #endif  // SkChromeRemoteGlyphCache_DEFINED

src/core/SkChromeRemoteGlyphCache.cpp

@@ -1186,9 +1186,3 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi
 sk_sp<SkTypeface> SkStrikeClient::deserializeTypeface(const void* buf, size_t len) {
     return fImpl->deserializeTypeface(buf, len);
 }
-
-// -------------------------------------------------------------------------------------------------
-bool SkFuzzDeserializeSkDescriptor(sk_sp<SkData> bytes, SkAutoDescriptor* ad) {
-    auto d = Deserializer(reinterpret_cast<const volatile char*>(bytes->data()), bytes->size());
-    return d.readDescriptor(ad);
-}

src/core/SkDescriptor.cpp

@@ -6,16 +6,21 @@
  */
 
 #include "src/core/SkDescriptor.h"
+#include <string.h>
 
 #include <new>
 
 #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> 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<SkDescriptor>(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<void*>(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> 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> 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<void>(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)) {

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<SkAutoDescriptor> MakeFromBuffer(SkReadBuffer& buffer);
+
     void reset(size_t size);
     void reset(const SkDescriptor& desc);
     SkDescriptor* getDesc() const { SkASSERT(fDesc); return fDesc; }

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 <memory>
@@ -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());
+    }
+}