Skip to content

Commit

Permalink
Early catch potential overflow issue #43
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
afichet committed Dec 9, 2023
1 parent 70cc941 commit d0a7e85
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 25 deletions.
6 changes: 1 addition & 5 deletions src/model/framebuffer/FramebufferModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@

FramebufferModel::FramebufferModel(QObject* parent)
: QObject(parent)
, m_pixelBuffer(nullptr)
, m_width(0)
, m_height(0)
, m_isImageLoaded(false)
Expand All @@ -59,7 +58,4 @@ QRect FramebufferModel::getDataWindow() const
return m_dataWindow;
}

FramebufferModel::~FramebufferModel()
{
delete[] m_pixelBuffer;
}
FramebufferModel::~FramebufferModel() {}
6 changes: 3 additions & 3 deletions src/model/framebuffer/FramebufferModel.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
#include <QObject>
#include <QRect>
#include <QVector>
#include <array>
#include <vector>

class FramebufferModel: public QObject
{
Expand Down Expand Up @@ -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.
Expand Down
35 changes: 22 additions & 13 deletions src/model/framebuffer/RGBFramebufferModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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";
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
21 changes: 17 additions & 4 deletions src/model/framebuffer/YFramebufferModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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);
}

Expand Down

0 comments on commit d0a7e85

Please sign in to comment.