Merge pull request #1484 from mapbox/fix-line-atlas-overflow

Fix potential integer overflow in line_atlas

src/mbgl/geometry/glyph_atlas.cpp

@@ -4,6 +4,8 @@
 #include <mbgl/platform/log.hpp>
 #include <mbgl/platform/platform.hpp>
 
+#include <mbgl/util/std.hpp>
+
 #include <cassert>
 #include <algorithm>
 
@@ -14,7 +16,7 @@ GlyphAtlas::GlyphAtlas(uint16_t width_, uint16_t height_)
     : width(width_),
       height(height_),
       bin(width_, height_),
-      data(new char[width_ *height_]),
+      data(util::make_unique<uint8_t[]>(width_ * height_)),
       dirty(true) {
 }
 
@@ -88,13 +90,12 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
     face.emplace(glyph.id, GlyphValue { rect, tileUID });
 
     // Copy the bitmap
-    char *target = data.get();
-    const char *source = glyph.bitmap.data();
+    const uint8_t* source = reinterpret_cast<const uint8_t*>(glyph.bitmap.data());
     for (uint32_t y = 0; y < buffered_height; y++) {
         uint32_t y1 = width * (rect.y + y) + rect.x;
         uint32_t y2 = buffered_width * y;
         for (uint32_t x = 0; x < buffered_width; x++) {
-            target[y1 + x] = source[y2 + x];
+            data[y1 + x] = source[y2 + x];
         }
     }
 
@@ -116,7 +117,7 @@ void GlyphAtlas::removeGlyphs(uintptr_t tileUID) {
                 const Rect<uint16_t>& rect = value.rect;
 
                 // Clear out the bitmap.
-                char *target = data.get();
+                uint8_t *target = data.get();
                 for (uint32_t y = 0; y < rect.h; y++) {
                     uint32_t y1 = width * (rect.y + y) + rect.x;
                     for (uint32_t x = 0; x < rect.w; x++) {

src/mbgl/geometry/glyph_atlas.hpp

@@ -49,7 +49,7 @@ private:
     std::mutex mtx;
     BinPack<uint16_t> bin;
     std::map<std::string, std::map<uint32_t, GlyphValue>> index;
-    std::unique_ptr<char[]> data;
+    const std::unique_ptr<uint8_t[]> data;
     std::atomic<bool> dirty;
     uint32_t texture = 0;
 };

src/mbgl/geometry/line_atlas.cpp

@@ -3,6 +3,7 @@
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/platform/log.hpp>
 #include <mbgl/platform/platform.hpp>
+#include <mbgl/util/std.hpp>
 
 #include <boost/functional/hash.hpp>
 
@@ -14,7 +15,7 @@ using namespace mbgl;
 LineAtlas::LineAtlas(uint16_t w, uint16_t h)
     : width(w),
       height(h),
-      data(new char[w * h]),
+      data(util::make_unique<uint8_t[]>(w * h)),
       dirty(true) {
 }
 
@@ -23,8 +24,6 @@ LineAtlas::~LineAtlas() {
 
     Environment::Get().abandonTexture(texture);
     texture = 0;
-
-    delete[] data;
 }
 
 LinePatternPos LineAtlas::getDashPosition(const std::vector<float> &dasharray, bool round) {
@@ -162,7 +161,7 @@ void LineAtlas::bind() {
                 0, // GLint border
                 GL_ALPHA, // GLenum format
                 GL_UNSIGNED_BYTE, // GLenum type
-                data // const GLvoid * data
+                data.get() // const GLvoid * data
             ));
         } else {
             MBGL_CHECK_ERROR(glTexSubImage2D(
@@ -174,7 +173,7 @@ void LineAtlas::bind() {
                 height, // GLsizei height
                 GL_ALPHA, // GLenum format
                 GL_UNSIGNED_BYTE, // GLenum type
-                data // const GLvoid *pixels
+                data.get() // const GLvoid *pixels
             ));
         }
 

src/mbgl/geometry/line_atlas.hpp

@@ -34,7 +34,7 @@ public:
 
 private:
     std::recursive_mutex mtx;
-    char *const data = nullptr;
+    const std::unique_ptr<uint8_t[]> data;
     std::atomic<bool> dirty;
     uint32_t texture = 0;
     int nextRow = 0;

src/mbgl/geometry/sprite_atlas.cpp

@@ -33,9 +33,7 @@ bool SpriteAtlas::resize(const float newRatio) {
     pixelRatio = newRatio;
 
     if (data) {
-        uint32_t *old_data = data;
-
-        data = nullptr;
+        const auto oldData = std::move(data);
         allocate();
 
         const int old_w = width * oldRatio;
@@ -44,19 +42,15 @@ bool SpriteAtlas::resize(const float newRatio) {
         const int new_h = height * newRatio;
 
         // Basic image scaling. TODO: Replace this with better image scaling.
-        uint32_t *img_new = reinterpret_cast<uint32_t *>(data);
-        const uint32_t *img_old = reinterpret_cast<const uint32_t *>(old_data);
-
         for (int y = 0; y < new_h; y++) {
             const int old_yoffset = ((y * old_h) / new_h) * old_w;
             const int new_yoffset = y * new_w;
             for (int x = 0; x < new_w; x++) {
                 const int old_x = (x * old_w) / new_w;
-                img_new[new_yoffset + x] = img_old[old_yoffset + old_x];
+                data[new_yoffset + x] = oldData[old_yoffset + old_x];
             }
         }
 
-        ::operator delete(old_data);
         dirty = true;
         fullUploadRequired = true;
 
@@ -144,8 +138,8 @@ void SpriteAtlas::allocate() {
     if (!data) {
         dimension w = static_cast<dimension>(width * pixelRatio);
         dimension h = static_cast<dimension>(height * pixelRatio);
-        data = static_cast<uint32_t*>(::operator new(w * h * sizeof(uint32_t)));
-        std::fill(data, data + w * h, 0);
+        data = util::make_unique<uint32_t[]>(w * h);
+        std::fill(data.get(), data.get() + w * h, 0);
     }
 }
 
@@ -158,7 +152,7 @@ void SpriteAtlas::copy(const Rect<dimension>& dst, const SpritePosition& src, co
     const Rect<uint32_t> srcPos { src.x, src.y, src.width, src.height };
 
     allocate();
-    uint32_t *dstData = reinterpret_cast<uint32_t *>(data);
+    uint32_t *const dstData = data.get();
     const vec2<uint32_t> dstSize { static_cast<unsigned int>(width * pixelRatio),
                                    static_cast<unsigned int>(height * pixelRatio) };
     const Rect<uint32_t> dstPos { static_cast<uint32_t>(dst.x * pixelRatio),
@@ -274,7 +268,7 @@ void SpriteAtlas::bind(bool linear) {
                 0, // GLint border
                 GL_RGBA, // GLenum format
                 GL_UNSIGNED_BYTE, // GLenum type
-                data // const GLvoid * data
+                data.get() // const GLvoid * data
             ));
             fullUploadRequired = false;
         } else {
@@ -287,7 +281,7 @@ void SpriteAtlas::bind(bool linear) {
                 height * pixelRatio, // GLsizei height
                 GL_RGBA, // GLenum format
                 GL_UNSIGNED_BYTE, // GLenum type
-                data // const GLvoid *pixels
+                data.get() // const GLvoid *pixels
             ));
         }
 
@@ -303,5 +297,4 @@ SpriteAtlas::~SpriteAtlas() {
     std::lock_guard<std::recursive_mutex> lock(mtx);
     Environment::Get().abandonTexture(texture);
     texture = 0;
-    ::operator delete(data), data = nullptr;
 }

src/mbgl/geometry/sprite_atlas.hpp

@@ -77,7 +77,7 @@ private:
     util::ptr<Sprite> sprite;
     std::map<std::string, Rect<dimension>> images;
     std::set<std::string> uninitialized;
-    uint32_t *data = nullptr;
+    std::unique_ptr<uint32_t[]> data;
     std::atomic<bool> dirty;
     bool fullUploadRequired = true;
     uint32_t texture = 0;