From e202cf3ef8e18c0f47f1d7098fb96efbdc1953c1 Mon Sep 17 00:00:00 2001
From: Kaylee Lubick <[email protected]>
Date: Fri, 29 May 2026 15:16:50 -0400
Subject: [PATCH] Address potential MSAN issue in SkScalerContext

If malformed data was passed to SkStrikeClient, an unexpected
mask would pass through asserts in a release build and lead
to a mask being allocated that was 4x bigger than what
was written to.

To defend against this problem, we 1) reject masks that
aren't of the three formats SkScalerContext::GenerateImageFromPath
expects; 2) zero out the whole mask, regardless of how big
it is instead of relying on how big an A8 mask would be.

Additionally, I noticed that in the intermediateDst case
(e.g. for LCD text when we draw into an A8 and then later unpack
it to be LCD16) we weren't zeroing that intermediate buffer
which could be a problem if the glyph itself was small (but
the bounds were corrupted to be big). Thus, we zero that
intermediate A8 buffer too.

Change-Id: Ib7080fc45eb77ea19b3e733570da33cd339a51f8
Bug: 513780208
Fixed: 513780208
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1248536
Commit-Queue: Kaylee Lubick <[email protected]>
Auto-Submit: Kaylee Lubick <[email protected]>
Reviewed-by: Florin Malita <[email protected]>
---

diff --git a/src/core/SkGlyph.cpp b/src/core/SkGlyph.cpp
index 26c8004..7f440a4 100644
--- a/src/core/SkGlyph.cpp
+++ b/src/core/SkGlyph.cpp
@@ -406,6 +406,12 @@
         const bool pathIsHairline = buffer.readBool();
         const bool pathIsModified = buffer.readBool();
         if (auto path = buffer.readPath()) {
+            if (fMaskFormat != SkMask::kBW_Format &&
+                fMaskFormat != SkMask::kA8_Format &&
+                fMaskFormat != SkMask::kLCD16_Format) {
+                buffer.validate(false);
+                return 0;
+            }
             if (this->setPath(alloc, &path.value(), pathIsHairline, pathIsModified)) {
                 memoryIncrease += path->approximateBytesUsed();
             }
diff --git a/src/core/SkScalerContext.cpp b/src/core/SkScalerContext.cpp
index 85edc26..2d68c14 100644
--- a/src/core/SkScalerContext.cpp
+++ b/src/core/SkScalerContext.cpp
@@ -572,10 +572,11 @@
             sk_bzero(dstMask.image(), dstMask.computeImageSize());
             return;
         }
+        sk_bzero(dst.writable_addr(), dst.computeByteSize());
     } else {
         dst.reset(info, dstMask.image(), dstMask.fRowBytes);
     }
-    sk_bzero(dst.writable_addr(), dst.computeByteSize());
+    sk_bzero(dstMask.image(), dstMask.computeImageSize());
 
     skcpu::Draw draw;
     draw.fBlitterChooser = SkA8Blitter_Choose;
@@ -600,6 +601,7 @@
             pack4xHToMask(dst, dstMask, maskPreBlend, doBGR, verticalLCD);
             break;
         default:
+            SkUNREACHABLE;
             break;
     }
 }
diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp
index 2b437a2..f357fce 100644
--- a/tests/SkRemoteGlyphCacheTest.cpp
+++ b/tests/SkRemoteGlyphCacheTest.cpp
@@ -13,6 +13,7 @@
 #include "include/core/SkColorType.h"
 #include "include/core/SkData.h"
 #include "include/core/SkFont.h"
+#include "include/core/SkFontMetrics.h"
 #include "include/core/SkFontStyle.h"
 #include "include/core/SkFontTypes.h"
 #include "include/core/SkGraphics.h"
@@ -33,9 +34,12 @@
 #include "include/private/base/SkMutex.h"
 #include "include/private/chromium/SkChromeRemoteGlyphCache.h"
 #include "include/private/chromium/Slug.h"
+#include "src/core/SkFontMetricsPriv.h"
 #include "src/core/SkFontPriv.h"
 #include "src/core/SkGlyph.h"
 #include "src/core/SkReadBuffer.h"
+#include "src/core/SkScalerContext.h"
+#include "src/core/SkStrikeCache.h"
 #include "src/core/SkStrikeSpec.h"
 #include "src/core/SkTHash.h"
 #include "src/core/SkTextBlobPriv.h"
@@ -1406,3 +1410,85 @@
 
     SkGraphics::SetTypefaceCacheCountLimit(prev1);  // restore orig
 }
+
+DEF_TEST(SkRemoteGlyphCache_b513780208, reporter) {
+    // Legitimate path glyphs should only use kBW_Format, kA8_Format, or kLCD16_Format.
+    // If a malicious strike provided a path with kARGB32_Format, it could trigger an
+    // uninitialized memory issue in SkScalerContext::GenerateImageFromPath.
+    //
+    // SkGlyph::addPathFromBuffer now rejects any path whose format is not an allowed type.
+
+    constexpr int kW = 200;
+    constexpr int kH = 4;
+    constexpr SkTypefaceID kServerTypefaceID = 0x65000000u;  // arbitrary
+    constexpr uint32_t kPackedGlyphID = 0x42u;               // arbitrary
+
+    auto BuildMaliciousStrikeData = [&]() {
+        SkBinaryWriteBuffer buffer{nullptr, 0, {}};
+
+        // --- typefaces ---
+        buffer.writeInt(1);  // typefaceCount
+        buffer.writeUInt(kServerTypefaceID);
+        buffer.writeInt(256);
+        buffer.write32(0);
+        buffer.writeBool(false);
+        buffer.writeBool(false);
+
+        // --- strikes ---
+        buffer.writeInt(1);                   // strikeCount
+        buffer.writeUInt(kServerTypefaceID);  // serverTypefaceID
+        buffer.writeUInt(1);                  // discardableHandleID
+
+        // SkDescriptor: Craft a Rec that triggers GenerateImageFromPath (fFrameWidth >= 0).
+        {
+            SkScalerContextRec rec;
+            rec.fTypefaceID = kServerTypefaceID;
+            rec.fTextSize = 16.0f;
+            rec.fPreScaleX = 1.0f;
+            rec.fPost2x2[0][0] = 1.0f;
+            rec.fPost2x2[1][1] = 1.0f;
+            rec.fFrameWidth = 0.0f;  // triggers fGenerateImageFromPath
+            rec.fMaskFormat = SkMask::kA8_Format;
+            SkAutoDescriptor ad{SkDescriptor::ComputeOverhead(1) + sizeof(rec)};
+            SkDescriptor* desc = ad.getDesc();
+            desc->addEntry(kRec_SkDescriptorTag, sizeof(rec), &rec);
+            desc->computeChecksum();
+            desc->flatten(buffer);
+        }
+
+        buffer.writeBool(false);  // fontMetricsInitialized == false
+        {
+            SkFontMetrics fm;
+            SkFontMetricsPriv::Flatten(buffer, fm);
+        }
+
+        buffer.writeInt(0);  // imagesCount
+
+        // Craft a path glyph with an invalid mask format (kARGB32_Format).
+        buffer.writeInt(1);  // pathsCount
+        buffer.writeUInt(kPackedGlyphID);
+        buffer.writePoint(SkPoint::Make(static_cast<float>(kW), 0.0f));
+        buffer.writeUInt((static_cast<uint32_t>(kW) << 16) | kH);
+        buffer.writeUInt(0);
+        buffer.writeUInt(static_cast<uint32_t>(SkMask::kARGB32_Format));
+        buffer.writeBool(true);  // hasPath
+        buffer.writeBool(false);
+        buffer.writeBool(false);
+        buffer.writePath(SkPath::Rect(SkRect::MakeXYWH(0, 0, 1, 1)));
+
+        buffer.writeInt(0);  // drawablesCount
+
+        return buffer.snapshotAsData();
+    };
+
+    sk_sp<SkData> blob = BuildMaliciousStrikeData();
+    REPORTER_ASSERT(reporter, blob);
+
+    SkStrikeCache cache;
+    auto discardableManager = sk_make_sp<DiscardableManager>();
+    SkStrikeClient client(discardableManager, /*isLogging=*/false, &cache);
+
+    // This should fail to read the strike data because SkGlyph::addPathFromBuffer
+    // now validates that path glyphs have an expected mask format.
+    REPORTER_ASSERT(reporter, !client.readStrikeData(blob->data(), blob->size()));
+}