commit ab43a068fc0d416393fa840f033e8f636048dee0
author Herb Derby <herb@google.com> Wed Mar 22 11:04:11 2023 -0400
committer SkCQ <skcq-be@skia-corp.google.com.iam.gserviceaccount.com> Wed Mar 22 16:04:07 2023 +0000
tree 19afecd6c9439afdc8724cb5c62dbd0db1538154
parent ac8e04309d67c41503e5d728137995221d6a7b97

For path, validate flow control variable hasPath

If hasPath is false on invalid data, then the empty path
would be set. This situation should leave the path unset.

Bug: skia:14163
Change-Id: Ib5bbea1f12738adbc26bc51addca8269b15b2339
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/660216
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

src/core/SkGlyph.cpp

diff --git a/src/core/SkGlyph.cpp b/src/core/SkGlyph.cpp
index 06ac812..56f481e 100644
--- a/src/core/SkGlyph.cpp
+++ b/src/core/SkGlyph.cpp
@@ -338,6 +338,10 @@
 
     size_t memoryIncrease = 0;
     const bool hasPath = buffer.readBool();
+    // Check if the buffer is invalid, so as to not make a logical decision on invalid data.
+    if (!buffer.isValid()) {
+        return 0;
+    }
     if (hasPath) {
         const bool pathIsHairline = buffer.readBool();
         SkPath path;

tests/SkGlyphTest.cpp

diff --git a/tests/SkGlyphTest.cpp b/tests/SkGlyphTest.cpp
index 70dd0eb..d3e2403 100644
--- a/tests/SkGlyphTest.cpp
+++ b/tests/SkGlyphTest.cpp
@@ -216,25 +216,46 @@
     const SkPath* dstPath = dstGlyph->path();
     REPORTER_ASSERT(reporter, *dstPath == srcPath);
 
-    // Add good metrics, but mess up path data
-    SkBinaryWriteBuffer badWriteBuffer;
-    srcGlyph.flattenMetrics(badWriteBuffer);
-    badWriteBuffer.writeInt(7);
-    badWriteBuffer.writeInt(8);
+    {
+        // Add good metrics, but mess up path data
+        SkBinaryWriteBuffer badWriteBuffer;
+        srcGlyph.flattenMetrics(badWriteBuffer);
+        // Force a false value to be read in addPathFromBuffer for hasPath.
+        badWriteBuffer.writeInt(8);
+        badWriteBuffer.writeInt(9);
 
-    data = badWriteBuffer.snapshotAsData();
+        data = badWriteBuffer.snapshotAsData();
 
-    SkReadBuffer badReadBuffer{data->data(), data->size()};
-    dstGlyph = SkGlyph::MakeFromBuffer(badReadBuffer);
-    REPORTER_ASSERT(reporter, badReadBuffer.isValid());  // Reading glyph metrics is okay.
-    REPORTER_ASSERT(reporter, dstGlyph.has_value());
-    REPORTER_ASSERT(reporter, srcGlyph.advanceVector() == dstGlyph->advanceVector());
-    REPORTER_ASSERT(reporter, srcGlyph.rect() == dstGlyph->rect());
-    REPORTER_ASSERT(reporter, srcGlyph.maskFormat() == dstGlyph->maskFormat());
+        SkReadBuffer badReadBuffer{data->data(), data->size()};
+        dstGlyph = SkGlyph::MakeFromBuffer(badReadBuffer);
+        REPORTER_ASSERT(reporter, dstGlyph.has_value());
+        REPORTER_ASSERT(reporter, srcGlyph.advanceVector() == dstGlyph->advanceVector());
+        REPORTER_ASSERT(reporter, srcGlyph.rect() == dstGlyph->rect());
+        REPORTER_ASSERT(reporter, srcGlyph.maskFormat() == dstGlyph->maskFormat());
 
-    dstGlyph->addPathFromBuffer(badReadBuffer, &alloc);
-    REPORTER_ASSERT(reporter, !badReadBuffer.isValid());
-    REPORTER_ASSERT(reporter, !dstGlyph->setPathHasBeenCalled());
+        dstGlyph->addPathFromBuffer(badReadBuffer, &alloc);
+        REPORTER_ASSERT(reporter, !badReadBuffer.isValid());
+        REPORTER_ASSERT(reporter, !dstGlyph->setPathHasBeenCalled());
+    }
+    {
+        // Add good metrics, but no path data.
+        SkBinaryWriteBuffer badWriteBuffer;
+        srcGlyph.flattenMetrics(badWriteBuffer);
+
+        data = badWriteBuffer.snapshotAsData();
+
+        SkReadBuffer badReadBuffer{data->data(), data->size()};
+        dstGlyph = SkGlyph::MakeFromBuffer(badReadBuffer);
+        REPORTER_ASSERT(reporter, badReadBuffer.isValid());  // Reading glyph metrics is okay.
+        REPORTER_ASSERT(reporter, dstGlyph.has_value());
+        REPORTER_ASSERT(reporter, srcGlyph.advanceVector() == dstGlyph->advanceVector());
+        REPORTER_ASSERT(reporter, srcGlyph.rect() == dstGlyph->rect());
+        REPORTER_ASSERT(reporter, srcGlyph.maskFormat() == dstGlyph->maskFormat());
+
+        dstGlyph->addPathFromBuffer(badReadBuffer, &alloc);
+        REPORTER_ASSERT(reporter, !badReadBuffer.isValid());
+        REPORTER_ASSERT(reporter, !dstGlyph->setPathHasBeenCalled());
+    }
 }
 
 DEF_TEST(SkGlyph_SendWithDrawable, reporter) {