[core] abort early when placing a glyph in GlyphAtlas if the bitmap is invalid

2 files changed, 52 insertions, 2 deletions

diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 17b3e7e482..dc1fe2b1d9 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -117,14 +117,22 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
         return value.rect;
     }
 
-    // The glyph bitmap has zero width.
-    if (glyph.bitmap.empty()) {
+    // Guard against glyphs that are too large, or that we don't need to place into the atlas since
+    // they don't have any pixels.
+    if (glyph.metrics.width == 0 || glyph.metrics.width >= 256 ||
+        glyph.metrics.height == 0 || glyph.metrics.height >= 256) {
         return Rect<uint16_t>{ 0, 0, 0, 0 };
     }
 
     uint16_t buffered_width = glyph.metrics.width + SDFGlyph::borderSize * 2;
     uint16_t buffered_height = glyph.metrics.height + SDFGlyph::borderSize * 2;
 
+    // Guard against mismatches between the glyph bitmap size and the size mandated by
+    // its metrics.
+    if (size_t(buffered_width * buffered_height) != glyph.bitmap.size()) {
+        return Rect<uint16_t>{ 0, 0, 0, 0 };
+    }
+
     // Add a 1px border around every image.
     const uint16_t padding = 1;
     uint16_t pack_width = buffered_width + 2 * padding;
@@ -142,6 +150,8 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
         return rect;
     }
 
+    // Verify that binpack didn't produce a rect that goes beyond the size of the image.
+    // This should never happen.
     assert(rect.x + rect.w <= image.size.width);
     assert(rect.y + rect.h <= image.size.height);
 
diff --git a/test/text/glyph_atlas.test.cpp b/test/text/glyph_atlas.test.cpp
index 020a7140cb..e229cd117b 100644
--- a/test/text/glyph_atlas.test.cpp
+++ b/test/text/glyph_atlas.test.cpp
@@ -142,3 +142,43 @@ TEST(GlyphAtlas, LoadingCancel) {
         {{"Test Stack"}},
         {{0, 255}});
 }
+
+TEST(GlyphAtlas, InvalidSDFGlyph) {
+    GlyphSet glyphSet;
+    glyphSet.insert(65, SDFGlyph{ 65 /* ASCII 'A' */,
+                                  "x" /* bitmap is too short */,
+                                  { 1 /* width */, 1 /* height */, 0 /* left */, 0 /* top */,
+                                    0 /* advance */ } });
+    glyphSet.insert(66, SDFGlyph{ 66 /* ASCII 'B' */,
+                                  std::string(7 * 7, 'x'), /* correct */
+                                  { 1 /* width */, 1 /* height */, 0 /* left */, 0 /* top */,
+                                    0 /* advance */ } });
+    glyphSet.insert(67, SDFGlyph{ 67 /* ASCII 'C' */,
+                                  std::string(518 * 8, 'x'), /* correct */
+                                  { 512 /* width */, 2 /* height */, 0 /* left */, 0 /* top */,
+                                    0 /* advance */ } });
+
+
+    const FontStack fontStack{ "Mock Font" };
+
+    GlyphAtlasTest test;
+    GlyphPositions positions;
+    test.glyphAtlas.addGlyphs(1, std::u16string{u"ABC"}, fontStack, glyphSet, positions);
+
+    ASSERT_EQ(3u, positions.size());
+
+    // 'A' was not placed because the bitmap size is invalid.
+    ASSERT_NE(positions.end(), positions.find(65));
+    ASSERT_EQ((Rect<uint16_t>{ 0, 0, 0, 0 }), positions[65].rect);
+
+    // 'B' was placed at the top left.
+    ASSERT_NE(positions.end(), positions.find(66));
+    // Width is 12 because actual dimensions are 1+6 pixels, with 1px border added, rounded up to
+    // the next multiple of 4.
+    ASSERT_EQ((Rect<uint16_t>{ 0, 0, 12, 12 }), positions[66].rect);
+
+    // 'C' was not placed because the width is too big.
+    ASSERT_NE(positions.end(), positions.find(67));
+    ASSERT_EQ((Rect<uint16_t>{ 0, 0, 0, 0 }), positions[67].rect);
+
+}