[core] harden Glyph PBF parsing

8 files changed, 230 insertions, 12 deletions

diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake
index 32ee236994..7eca71299f 100644
--- a/cmake/test-files.cmake
+++ b/cmake/test-files.cmake
@@ -88,6 +88,7 @@ set(MBGL_TEST_FILES
 
     # text
     test/text/glyph_atlas.test.cpp
+    test/text/glyph_pbf.test.cpp
     test/text/quads.test.cpp
 
     # tile
diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index d07fbdff21..2bf1448492 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -77,9 +77,13 @@ class Shaping {
 
 class SDFGlyph {
 public:
+    // We're using this value throughout the Mapbox GL ecosystem. If this is different, the glyphs
+    // also need to be reencoded.
+    static constexpr const uint8_t borderSize = 3;
+
     uint32_t id = 0;
 
-    // A signed distance field of the glyph with a border of 3 pixels.
+    // A signed distance field of the glyph with a border (see above).
     std::string bitmap;
 
     // Glyph metrics
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 5d30dacdce..17b3e7e482 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -107,9 +107,6 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
                                     const FontStack& fontStack,
                                     const SDFGlyph& glyph)
 {
-    // Use constant value for now.
-    const uint8_t buffer = 3;
-
     std::map<uint32_t, GlyphValue>& face = index[fontStack];
     auto it = face.find(glyph.id);
 
@@ -125,8 +122,8 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
         return Rect<uint16_t>{ 0, 0, 0, 0 };
     }
 
-    uint16_t buffered_width = glyph.metrics.width + buffer * 2;
-    uint16_t buffered_height = glyph.metrics.height + buffer * 2;
+    uint16_t buffered_width = glyph.metrics.width + SDFGlyph::borderSize * 2;
+    uint16_t buffered_height = glyph.metrics.height + SDFGlyph::borderSize * 2;
 
     // Add a 1px border around every image.
     const uint16_t padding = 1;
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 21edfe436c..cdeac57984 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -12,9 +12,11 @@
 
 #include <protozero/pbf_reader.hpp>
 
+namespace mbgl {
+
 namespace {
 
-void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) {
+void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::string& data) {
     protozero::pbf_reader glyphs_pbf(data);
 
     while (glyphs_pbf.next(1)) {
@@ -22,30 +24,39 @@ void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) {
         while (fontstack_pbf.next(3)) {
             auto glyph_pbf = fontstack_pbf.get_message();
 
-            mbgl::SDFGlyph glyph;
+            SDFGlyph glyph;
+
+            bool hasID = false, hasWidth = false, hasHeight = false, hasLeft = false,
+                 hasTop = false, hasAdvance = false;
 
             while (glyph_pbf.next()) {
                 switch (glyph_pbf.tag()) {
                 case 1: // id
                     glyph.id = glyph_pbf.get_uint32();
+                    hasID = true;
                     break;
                 case 2: // bitmap
                     glyph.bitmap = glyph_pbf.get_string();
                     break;
                 case 3: // width
                     glyph.metrics.width = glyph_pbf.get_uint32();
+                    hasWidth = true;
                     break;
                 case 4: // height
                     glyph.metrics.height = glyph_pbf.get_uint32();
+                    hasHeight = true;
                     break;
                 case 5: // left
                     glyph.metrics.left = glyph_pbf.get_sint32();
+                    hasLeft = true;
                     break;
                 case 6: // top
                     glyph.metrics.top = glyph_pbf.get_sint32();
+                    hasTop = true;
                     break;
                 case 7: // advance
                     glyph.metrics.advance = glyph_pbf.get_uint32();
+                    hasAdvance = true;
                     break;
                 default:
                     glyph_pbf.skip();
@@ -53,15 +64,32 @@ void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) {
                 }
             }
 
-            glyphSet.insert(glyph.id, std::move(glyph));
+            // If the area of width/height is non-zero, we need to adjust the expected size
+            // with the implicit border size, otherwise we expect there to be no bitmap at all.
+            const uint32_t expectedBitmapSize =
+                glyph.metrics.width && glyph.metrics.height
+                    ? (glyph.metrics.width + 2 * SDFGlyph::borderSize) *
+                          (glyph.metrics.height + 2 * SDFGlyph::borderSize)
+                    : 0;
+
+            // Only treat this glyph as a correct glyph if it has all required fields, and if
+            // the bitmap has the correct length. It also needs to satisfy a few metrics conditions
+            // that ensure that the glyph isn't bogus. All other glyphs are malformed.
+            // We're also discarding all glyphs that are outside the expected glyph range.
+            if (hasID && hasWidth && hasHeight && hasLeft && hasTop && hasAdvance &&
+                glyph.metrics.width < 256 && glyph.metrics.height < 256 &&
+                glyph.metrics.left >= -128 && glyph.metrics.left < 128 &&
+                glyph.metrics.top >= -128 && glyph.metrics.top < 128 &&
+                glyph.metrics.advance < 256 && glyph.bitmap.size() == expectedBitmapSize &&
+                glyph.id >= glyphRange.first && glyph.id <= glyphRange.second) {
+                glyphSet.insert(glyph.id, std::move(glyph));
+            }
         }
     }
 }
 
 } // namespace
 
-namespace mbgl {
-
 GlyphPBF::GlyphPBF(GlyphAtlas* atlas,
                    const FontStack& fontStack,
                    const GlyphRange& glyphRange,
@@ -79,7 +107,7 @@ GlyphPBF::GlyphPBF(GlyphAtlas* atlas,
             observer->onGlyphsLoaded(fontStack, glyphRange);
         } else {
             try {
-                parseGlyphPBF(**atlas->getGlyphSet(fontStack), *res.data);
+                parseGlyphPBF(**atlas->getGlyphSet(fontStack), glyphRange, *res.data);
             } catch (...) {
                 observer->onGlyphsError(fontStack, glyphRange, std::current_exception());
                 return;
diff --git a/test/fixtures/resources/fake_glyphs-0-255.pbf b/test/fixtures/resources/fake_glyphs-0-255.pbf
new file mode 100644
index 0000000000..8d7445d5c2
--- /dev/null
+++ b/test/fixtures/resources/fake_glyphs-0-255.pbf
diff --git a/test/fixtures/resources/fake_glyphs-0-255.pbf.txt b/test/fixtures/resources/fake_glyphs-0-255.pbf.txt
new file mode 100644
index 0000000000..6433af5d3d
--- /dev/null
+++ b/test/fixtures/resources/fake_glyphs-0-255.pbf.txt
@@ -0,0 +1,86 @@
+# Make changes below, then run this command to update the encoded PBF file:
+# protoc --encode glyphs test/fixtures/resources/fake_glyphs.proto < test/fixtures/resources/fake_glyphs-0-255.pbf.txt > test/fixtures/resources/fake_glyphs-0-255.pbf
+
+stacks {
+  # testing invalid metrics and bitmap too short
+  glyphs {
+    id: 65
+    bitmap: "fake"
+    width: -1
+    height: 65536
+    left: -10000000
+    top: 10000000
+    advance: -42
+  }
+
+  # testing missing bitmap => malformed
+  glyphs {
+    id: 66
+    width: 12
+    height: 17
+    left: 2
+    top: -11
+    advance: 15
+  }
+
+  # testing bitmap too long => malformed
+  glyphs {
+    id: 67
+    bitmap: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    width: 1
+    height: 1
+  }
+
+  # testing missing ID => malformed
+  glyphs {
+    width: 12
+    height: 17
+    left: 2
+    top: -11
+    advance: 15
+  }
+
+  # invalid metrics + bitmap => malformed
+  glyphs {
+    id: 65
+    bitmap: "fake"
+    width: -1
+    height: 65536
+    left: -10000000
+    top: 10000000
+    advance: -42
+  }
+
+  # invalid metrics => malformed
+  glyphs {
+    id: 68
+    bitmap: "x"
+    width: 1
+    height: 1
+    left: -10000000
+    top: 10000000
+    advance: -42
+  }
+
+  # a valid glyph
+  glyphs {
+    id: 69
+    bitmap: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    width: 1
+    height: 1
+    left: 20
+    top: 2
+    advance: 8
+  }
+
+  # a valid glyph that has an ID outside the range
+  glyphs {
+    id: 300
+    bitmap: ""
+    width: 0
+    height: 0
+    left: 0
+    top: 0
+    advance: 0
+  }
+}
diff --git a/test/fixtures/resources/fake_glyphs.proto b/test/fixtures/resources/fake_glyphs.proto
new file mode 100644
index 0000000000..d074541d69
--- /dev/null
+++ b/test/fixtures/resources/fake_glyphs.proto
@@ -0,0 +1,31 @@
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+// Stores a glyph with metrics and optional SDF bitmap information.
+message glyph {
+    optional int64 id = 1;
+
+    // A signed distance field of the glyph with a border of 3 pixels.
+    optional bytes bitmap = 2;
+
+    // Glyph metrics.
+    optional int64 width = 3;
+    optional int64 height = 4;
+    optional sint64 left = 5;
+    optional sint64 top = 6;
+    optional int64 advance = 7;
+}
+
+// Stores fontstack information and a list of faces.
+message fontstack {
+    optional string name = 1;
+    optional string range = 2;
+    repeated glyph glyphs = 3;
+}
+
+message glyphs {
+    repeated fontstack stacks = 1;
+
+    extensions 16 to 8191;
+}
diff --git a/test/text/glyph_pbf.test.cpp b/test/text/glyph_pbf.test.cpp
new file mode 100644
index 0000000000..1e28dfbc31
--- /dev/null
+++ b/test/text/glyph_pbf.test.cpp
@@ -0,0 +1,71 @@
+#include <mbgl/test/util.hpp>
+
+#include <mbgl/text/glyph_set.hpp>
+#include <mbgl/text/glyph_atlas_observer.hpp>
+#include <mbgl/text/glyph_atlas.hpp>
+#include <mbgl/text/glyph_pbf.hpp>
+#include <mbgl/storage/default_file_source.hpp>
+#include <mbgl/util/run_loop.hpp>
+#include <mbgl/util/string.hpp>
+
+#include <future>
+
+using namespace mbgl;
+
+using namespace std::string_literals;
+
+class MockGlyphAtlasObserver : public GlyphAtlasObserver {
+public:
+    std::function<void(const FontStack&, const GlyphRange&)> glyphsLoaded;
+    std::function<void(const FontStack&, const GlyphRange&, std::exception_ptr)> glyphsError;
+
+    void onGlyphsLoaded(const FontStack& fontStack, const GlyphRange& glyphRange) override {
+        if (glyphsLoaded) {
+            glyphsLoaded(fontStack, glyphRange);
+        }
+    }
+    void onGlyphsError(const FontStack& fontStack, const GlyphRange& glyphRange, std::exception_ptr error) override {
+        if (glyphsError) {
+            glyphsError(fontStack, glyphRange, error);
+        }
+    }
+};
+
+TEST(GlyphPBF, Parsing) {
+    util::RunLoop loop;
+    DefaultFileSource fileSource{ ":memory:", "." };
+    GlyphAtlas glyphAtlas{ { 1024, 1024 }, fileSource };
+    FontStack fontStack{ "fake_glyphs" };
+    GlyphRange glyphRange{ 0, 255 };
+
+    glyphAtlas.setURL("asset://test/fixtures/resources/{fontstack}-{range}.pbf");
+
+    MockGlyphAtlasObserver glyphAtlasObserver;
+    glyphAtlasObserver.glyphsLoaded = [&](const FontStack&, const GlyphRange&) {
+        loop.stop();
+
+        auto sdfs = glyphAtlas.getGlyphSet(fontStack)->getSDFs();
+
+        // The fake glyphs don't contain a glyph that has the ID 0; it only contains glyphs with
+        // undefined IDs, but the parser should remove them.
+
+        EXPECT_TRUE(sdfs.size() == 1);
+        EXPECT_TRUE(sdfs.find(69) != sdfs.end());
+        auto& sdf = sdfs[69];
+        EXPECT_EQ("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"s, sdf.bitmap);
+        EXPECT_EQ(1u, sdf.metrics.width);
+        EXPECT_EQ(1u, sdf.metrics.height);
+        EXPECT_EQ(20, sdf.metrics.left);
+        EXPECT_EQ(2, sdf.metrics.top);
+        EXPECT_EQ(8u, sdf.metrics.advance);
+    };
+
+    glyphAtlasObserver.glyphsError = [&](const FontStack&, const GlyphRange&, std::exception_ptr error) {
+        loop.stop();
+        FAIL() << util::toString(error);
+    };
+
+    GlyphPBF pbf(&glyphAtlas, fontStack, glyphRange, &glyphAtlasObserver, fileSource);
+
+    loop.run();
+}