[core] Make Image safer

Provide Image::copy, which handles copying rectangles from a source to a destination, with thorough bounds checking.

Also fixes an indexing error in SpriteAtlas, where the top row of pixels in a wrapped image was copied from the wrong source row.

4 files changed, 38 insertions, 49 deletions

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index c89d045dfc..78ea613cd3 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -3,6 +3,7 @@
 #include <mbgl/text/glyph_range.hpp>
 #include <mbgl/util/rect.hpp>
 #include <mbgl/util/traits.hpp>
+#include <mbgl/util/image.hpp>
 
 #include <cstdint>
 #include <vector>
@@ -89,7 +90,7 @@ public:
     uint32_t id = 0;
 
     // A signed distance field of the glyph with a border (see above).
-    std::string bitmap;
+    AlphaImage bitmap;
 
     // Glyph metrics
     GlyphMetrics metrics;
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index dc1fe2b1d9..88b319366d 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -124,48 +124,26 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
         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;
-    uint16_t pack_height = buffered_height + 2 * padding;
+    const uint32_t padding = 1;
+    uint16_t width = glyph.bitmap.size.width + 2 * padding;
+    uint16_t height = glyph.bitmap.size.height + 2 * padding;
 
     // Increase to next number divisible by 4, but at least 1.
     // This is so we can scale down the texture coordinates and pack them
     // into 2 bytes rather than 4 bytes.
-    pack_width += (4 - pack_width % 4);
-    pack_height += (4 - pack_height % 4);
+    width += (4 - width % 4);
+    height += (4 - height % 4);
 
-    Rect<uint16_t> rect = bin.allocate(pack_width, pack_height);
+    Rect<uint16_t> rect = bin.allocate(width, height);
     if (rect.w == 0) {
         Log::Error(Event::OpenGL, "glyph bitmap overflow");
         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);
-
     face.emplace(glyph.id, GlyphValue { rect, tileUID });
 
-    // Copy the bitmap
-    const uint8_t* source = reinterpret_cast<const uint8_t*>(glyph.bitmap.data());
-    for (uint32_t y = 0; y < buffered_height; y++) {
-        uint32_t y1 = image.size.width * (rect.y + y + padding) + rect.x + padding;
-        uint32_t y2 = buffered_width * y;
-        for (uint32_t x = 0; x < buffered_width; x++) {
-            image.data[y1 + x] = source[y2 + x];
-        }
-    }
+    AlphaImage::copy(glyph.bitmap, image, { 0, 0 }, { rect.x + padding, rect.y + padding }, glyph.bitmap.size);
 
     dirty = true;
 
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index af14aace5b..84f29aeb73 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -101,7 +101,7 @@ private:
     std::mutex mtx;
     BinPack<uint16_t> bin;
     std::unordered_map<FontStack, std::map<uint32_t, GlyphValue>, FontStackHash> index;
-    const AlphaImage image;
+    AlphaImage image;
     std::atomic<bool> dirty;
     mbgl::optional<gl::Texture> texture;
 };
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index cdeac57984..5c57d278db 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -25,6 +25,7 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
             auto glyph_pbf = fontstack_pbf.get_message();
 
             SDFGlyph glyph;
+            protozero::data_view glyphData;
 
             bool hasID = false, hasWidth = false, hasHeight = false, hasLeft = false,
                  hasTop = false, hasAdvance = false;
@@ -36,7 +37,7 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
                     hasID = true;
                     break;
                 case 2: // bitmap
-                    glyph.bitmap = glyph_pbf.get_string();
+                    glyphData = glyph_pbf.get_view();
                     break;
                 case 3: // width
                     glyph.metrics.width = glyph_pbf.get_uint32();
@@ -64,26 +65,35 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
                 }
             }
 
+            // Only treat this glyph as a correct glyph if it has all required fields. 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.id < glyphRange.first || glyph.id > glyphRange.second) {
+                continue;
+            }
+
             // 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));
+            if (glyph.metrics.width && glyph.metrics.height) {
+                const Size size {
+                    glyph.metrics.width + 2 * SDFGlyph::borderSize,
+                    glyph.metrics.height + 2 * SDFGlyph::borderSize
+                };
+
+                if (size.area() != glyphData.size()) {
+                    continue;
+                }
+
+                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
             }
+
+            glyphSet.insert(glyph.id, std::move(glyph));
         }
     }
 }