Early catch potential overflow issue #43

`m_width` and `m_height` are of `int` type in the OpenEXR library. We
currently keep the same types in our class but this may case issue
when mapping 1D memory. In the most favorable case, they are
multiplied together (Y framebuffer). For RGB(A) case, the required
memory can also be 4 time larger. We check if resp. `m_width *
m_height` and `4 * m_width * m_heigh` stay within the `int` higher
limit. Thanks to @GAP-dev for bringing this issue.

This commit also cleans a bit raw memory allocation in favor of
`std::vector` container.

Author: afichet

src/model/framebuffer/FramebufferModel.cpp

@@ -39,7 +39,6 @@
 
 FramebufferModel::FramebufferModel(QObject* parent)
   : QObject(parent)
-  , m_pixelBuffer(nullptr)
   , m_width(0)
   , m_height(0)
   , m_isImageLoaded(false)
@@ -59,7 +58,4 @@ QRect FramebufferModel::getDataWindow() const
     return m_dataWindow;
 }
 
-FramebufferModel::~FramebufferModel()
-{
-    delete[] m_pixelBuffer;
-}
+FramebufferModel::~FramebufferModel() {}

src/model/framebuffer/FramebufferModel.h

@@ -37,7 +37,7 @@
 #include <QObject>
 #include <QRect>
 #include <QVector>
-#include <array>
+#include <vector>
 
 class FramebufferModel: public QObject
 {
@@ -67,8 +67,8 @@ class FramebufferModel: public QObject
     void loadFailed(QString message);
 
   protected:
-    float* m_pixelBuffer;
-    QImage m_image;
+    std::vector<float> m_pixelBuffer;
+    QImage             m_image;
 
     // Right now, the width and height are defined as Vec2i in OpenEXR
     // i.e. int type.

src/model/framebuffer/RGBFramebufferModel.cpp

@@ -81,6 +81,23 @@ void RGBFramebufferModel::load(
             m_displayWindow
               = QRect(dispW.min.x, dispW.min.y, dispW_width, dispW_height);
 
+            // Check to avoid type overflow, width and height are 32bits int
+            // representing a 2 dimentional image. Can overflow the type when
+            // multiplied together.
+            // 0x1FFFFFFF is a save limit for 4 * 0x7FFFFFFF the max
+            // representable int since we need 4 channels.
+            // TODO: Use larger type when manipulating framebuffer
+            const uint64_t partial_size
+              = (uint64_t)m_width * (uint64_t)m_height;
+
+            if (partial_size > 0x1FFFFFFF) {
+                throw std::runtime_error(
+                  "The total image size is too large. May be supported in a "
+                  "future revision.");
+            }
+
+            m_pixelBuffer.resize(4 * m_width * m_height);
+
             // Check if there is specific chromaticities tied to the color
             // representation in this part.
             const Imf::ChromaticitiesAttribute* c
@@ -93,8 +110,6 @@ void RGBFramebufferModel::load(
                 chromaticities = c->value();
             }
 
-            m_pixelBuffer = new float[4 * m_width * m_height];
-
             // Check if there is alpha channel
             if (hasAlpha) {
                 std::string      aLayer = m_parentLayer + "A";
@@ -190,12 +205,12 @@ void RGBFramebufferModel::load(
 
                     Imf::FrameBuffer framebuffer;
 
-                    Imf::Rgba* buff1 = new Imf::Rgba[m_width * m_height];
-                    Imf::Rgba* buff2 = new Imf::Rgba[m_width * m_height];
+                    std::vector<Imf::Rgba> buff1(m_width * m_height);
+                    std::vector<Imf::Rgba> buff2(m_width * m_height);
 
-                    float* yBuffer  = new float[m_width * m_height];
-                    float* ryBuffer = new float[m_width / 2 * m_height / 2];
-                    float* byBuffer = new float[m_width / 2 * m_height / 2];
+                    std::vector<float> yBuffer(m_width * m_height);
+                    std::vector<float> ryBuffer(m_width / 2 * m_height / 2);
+                    std::vector<float> byBuffer(m_width / 2 * m_height / 2);
 
                     Imf::Slice ySlice = Imf::Slice::Make(
                       Imf::PixelType::FLOAT,
@@ -335,12 +350,6 @@ void RGBFramebufferModel::load(
                             m_pixelBuffer[4 * (y * m_width + x) + 2] = rgb.z;
                         }
                     }
-
-                    delete[] yBuffer;
-                    delete[] ryBuffer;
-                    delete[] byBuffer;
-                    delete[] buff1;
-                    delete[] buff2;
                 }
 
                 break;

src/model/framebuffer/YFramebufferModel.cpp

@@ -90,12 +90,25 @@ void YFramebufferModel::load(Imf::MultiPartInputFile& file, int partId)
                   dispW_width / 2,
                   dispW_height / 2);
 
-                m_pixelBuffer = new float[m_width * m_height];
+                // Check to avoid type overflow, width and height are 32bits int
+                // representing a 2 dimentional image. Can overflow the type when
+                // multiplied together
+                // TODO: Use larger type when manipulating framebuffer
+                const uint64_t partial_size
+                  = (uint64_t)m_width * (uint64_t)m_height;
+
+                if (partial_size > 0x7FFFFFFF) {
+                    throw std::runtime_error(
+                      "The total image size is too large. May be supported in "
+                      "a future revision.");
+                }
+
+                m_pixelBuffer.resize(m_width * m_height);
 
                 // Luminance Chroma channels
                 graySlice = Imf::Slice::Make(
                   Imf::PixelType::FLOAT,
-                  m_pixelBuffer,
+                  m_pixelBuffer.data(),
                   datW,
                   sizeof(float),
                   m_width * sizeof(float),
@@ -112,11 +125,11 @@ void YFramebufferModel::load(Imf::MultiPartInputFile& file, int partId)
                 m_displayWindow
                   = QRect(dispW.min.x, dispW.min.y, dispW_width, dispW_height);
 
-                m_pixelBuffer = new float[m_width * m_height];
+                m_pixelBuffer.resize(m_width * m_height);
 
                 graySlice = Imf::Slice::Make(
                   Imf::PixelType::FLOAT,
-                  m_pixelBuffer,
+                  m_pixelBuffer.data(),
                   datW);
             }