remote fonts: Validate format on deserialized glyph.

R=mtklein@google.com, herb@google.com

Bug: 949000
Change-Id: Ib17635ea34db80987a1f2b3c0e6fcc33ab2b7a6b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/206173
Commit-Queue: Khushal Sagar <khushalsagar@chromium.org>
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Auto-Submit: Khushal Sagar <khushalsagar@chromium.org>

src/core/SkMask.h

@@ -40,6 +40,8 @@ struct SkMask {
     uint32_t    fRowBytes;
     Format      fFormat;
 
+    static bool IsValidFormat(uint8_t format) { return format < kCountMaskFormats; }
+
     /** Returns true if the mask is empty: i.e. it has an empty bounds.
      */
     bool isEmpty() const { return fBounds.isEmpty(); }

src/core/SkRemoteGlyphCache.cpp

@@ -174,6 +174,10 @@ bool read_path(Deserializer* deserializer, SkGlyph* glyph, SkStrike* cache) {
     auto* path = deserializer->read(pathSize, kPathAlignment);
     if (!path) return false;
 
+    // Don't overwrite the path if we already have one. We could have used a fallback if the
+    // glyph was missing earlier.
+    if (glyph->fPathData != nullptr) return true;
+
     return cache->initializePath(glyph, path, pathSize);
 }
 
@@ -486,8 +490,9 @@ void SkStrikeServer::SkGlyphCacheState::writePendingGlyphs(Serializer* serialize
     for (const auto& glyphID : fPendingGlyphImages) {
         SkGlyph glyph{glyphID};
         fContext->getMetrics(&glyph);
-        writeGlyph(&glyph, serializer);
+        SkASSERT(SkMask::IsValidFormat(glyph.fMaskFormat));
 
+        writeGlyph(&glyph, serializer);
         auto imageSize = glyph.computeImageSize();
         if (imageSize == 0u) continue;
 
@@ -503,6 +508,8 @@ void SkStrikeServer::SkGlyphCacheState::writePendingGlyphs(Serializer* serialize
     for (const auto& glyphID : fPendingGlyphPaths) {
         SkGlyph glyph{glyphID};
         fContext->getMetrics(&glyph);
+        SkASSERT(SkMask::IsValidFormat(glyph.fMaskFormat));
+
         writeGlyph(&glyph, serializer);
         writeGlyphPath(glyphID, serializer);
     }
@@ -680,6 +687,8 @@ static bool readGlyph(SkTLazy<SkGlyph>& glyph, Deserializer* deserializer) {
     if (!deserializer->read<int16_t>(&glyph->fLeft)) return false;
     if (!deserializer->read<int8_t>(&glyph->fForceBW)) return false;
     if (!deserializer->read<uint8_t>(&glyph->fMaskFormat)) return false;
+    if (!SkMask::IsValidFormat(glyph->fMaskFormat)) return false;
+
     return true;
 }
 
@@ -764,9 +773,14 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi
             auto imageSize = glyph->computeImageSize();
             if (imageSize == 0u) continue;
 
-            auto* image = deserializer.read(imageSize, allocatedGlyph->formatAlignment());
+            auto* image = deserializer.read(imageSize, glyph->formatAlignment());
             if (!image) READ_FAILURE
-            strike->initializeImage(image, imageSize, allocatedGlyph);
+
+            // Don't overwrite the image if we already have one. We could have used a fallback if
+            // the glyph was missing earlier.
+            if (allocatedGlyph->fImage == nullptr) {
+                strike->initializeImage(image, imageSize, allocatedGlyph);
+            }
         }
 
         uint64_t glyphPathsCount = 0u;

src/core/SkStrike.cpp

@@ -142,9 +142,7 @@ const void* SkStrike::findImage(const SkGlyph& glyph) {
 }
 
 void SkStrike::initializeImage(const volatile void* data, size_t size, SkGlyph* glyph) {
-    // Don't overwrite the image if we already have one. We could have used a fallback if the
+    SkASSERT(!glyph->fImage);
-    // glyph was missing earlier.
-    if (glyph->fImage) return;
 
     if (glyph->fWidth > 0 && glyph->fWidth < kMaxGlyphWidth) {
         size_t allocSize = glyph->allocImage(&fAlloc);
@@ -180,9 +178,7 @@ const SkPath* SkStrike::findPath(const SkGlyph& glyph) {
 }
 
 bool SkStrike::initializePath(SkGlyph* glyph, const volatile void* data, size_t size) {
-    // Don't overwrite the path if we already have one. We could have used a fallback if the
+    SkASSERT(!glyph->fPathData);
-    // glyph was missing earlier.
-    if (glyph->fPathData) return true;
 
     if (glyph->fWidth) {
         SkGlyph::PathData* pathData = fAlloc.make<SkGlyph::PathData>();

tests/SkRemoteGlyphCacheTest.cpp

@@ -950,12 +950,6 @@ DEF_TEST(SkRemoteGlyphCache_ReWriteGlyph, reporter) {
         REPORTER_ASSERT(reporter, fallbackCache.get() != nullptr);
         auto glyph = fallbackCache->getRawGlyphByID(lostGlyphID);
         REPORTER_ASSERT(reporter, glyph->fMaskFormat == fakeMask);
-
-        // Try overriding the image, it should stay the same.
-        REPORTER_ASSERT(reporter,
-                        memcmp(glyph->fImage, glyphImage, glyph->computeImageSize()) == 0);
-        const uint8_t newGlyphImage[] = {0, 0};
-        fallbackCache->initializeImage(newGlyphImage, glyph->computeImageSize(), glyph);
         REPORTER_ASSERT(reporter,
                         memcmp(glyph->fImage, glyphImage, glyph->computeImageSize()) == 0);
     }