From 70cc9412ef3c713f74dbd78761971e2a09bcb580 Mon Sep 17 00:00:00 2001 From: Alban Fichet Date: Sat, 9 Dec 2023 13:33:01 +0100 Subject: [PATCH 1/2] Adding a comment explaining the choice for int types instead of uint of a larger size --- src/model/framebuffer/FramebufferModel.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/model/framebuffer/FramebufferModel.h b/src/model/framebuffer/FramebufferModel.h index b0067db..f80085a 100644 --- a/src/model/framebuffer/FramebufferModel.h +++ b/src/model/framebuffer/FramebufferModel.h @@ -70,6 +70,8 @@ class FramebufferModel: public QObject float* m_pixelBuffer; QImage m_image; + // Right now, the width and height are defined as Vec2i in OpenEXR + // i.e. int type. int m_width, m_height; bool m_isImageLoaded; From d0a7e85dfeb519951fb8a8d70f73f30d41cdd3d9 Mon Sep 17 00:00:00 2001 From: Alban Fichet Date: Sat, 9 Dec 2023 14:13:36 +0100 Subject: [PATCH 2/2] 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. --- src/model/framebuffer/FramebufferModel.cpp | 6 +--- src/model/framebuffer/FramebufferModel.h | 6 ++-- src/model/framebuffer/RGBFramebufferModel.cpp | 35 ++++++++++++------- src/model/framebuffer/YFramebufferModel.cpp | 21 ++++++++--- 4 files changed, 43 insertions(+), 25 deletions(-) diff --git a/src/model/framebuffer/FramebufferModel.cpp b/src/model/framebuffer/FramebufferModel.cpp index 4f21ba7..379b669 100644 --- a/src/model/framebuffer/FramebufferModel.cpp +++ b/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() {} diff --git a/src/model/framebuffer/FramebufferModel.h b/src/model/framebuffer/FramebufferModel.h index f80085a..7993fb5 100644 --- a/src/model/framebuffer/FramebufferModel.h +++ b/src/model/framebuffer/FramebufferModel.h @@ -37,7 +37,7 @@ #include #include #include -#include +#include 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 m_pixelBuffer; + QImage m_image; // Right now, the width and height are defined as Vec2i in OpenEXR // i.e. int type. diff --git a/src/model/framebuffer/RGBFramebufferModel.cpp b/src/model/framebuffer/RGBFramebufferModel.cpp index f345470..66fd78c 100644 --- a/src/model/framebuffer/RGBFramebufferModel.cpp +++ b/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 buff1(m_width * m_height); + std::vector 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 yBuffer(m_width * m_height); + std::vector ryBuffer(m_width / 2 * m_height / 2); + std::vector 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; diff --git a/src/model/framebuffer/YFramebufferModel.cpp b/src/model/framebuffer/YFramebufferModel.cpp index 06f388c..7310c6e 100644 --- a/src/model/framebuffer/YFramebufferModel.cpp +++ b/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); }