1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QImage: merge the size calculations with proper (non-UB) checks

Browse Source 
This check, which was only done once, was wrong:
    const int bytes_per_line = ((width * depth + 31) >> 5) << 2;

    // sanity check for potential overflows
    if (std::numeric_limits<int>::max()/depth < width

If width*height overflows, then it's already UB and checking afterwards
with a division is pointless and slow.

The other instances weren't properly guarding against overflows.

Change-Id: I343f2beed55440a7ac0bfffd1563350d4cfa639c
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
This commit is contained in:
Thiago Macieira 2018-11-01 21:43:48 -07:00
parent f80ed83cd9
commit 9d90c0edac
3 changed files with 91 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
src/gui/image/qimage.cpp
View File

@ -118,21 +118,14 @@ QImageData::QImageData()
QImageData * QImageData::create(const QSize &size, QImage::Format format)
{
if (!size.isValid() || format == QImage::Format_Invalid)
return 0; // invalid parameter(s)
return nullptr; // invalid parameter(s)
uint width = size.width();
uint height = size.height();
uint depth = qt_depthForFormat(format);
const int bytes_per_line = ((width * depth + 31) >> 5) << 2; // bytes per scanline (must be multiple of 4)
// sanity check for potential overflows
if (std::numeric_limits<int>::max()/depth < width
|| bytes_per_line <= 0
|| height <= 0
|| std::numeric_limits<qsizetype>::max()/uint(bytes_per_line) < height
|| std::numeric_limits<int>::max()/sizeof(uchar *) < uint(height))
return 0;
int width = size.width();
int height = size.height();
int depth = qt_depthForFormat(format);
auto params = calculateImageParameters(width, height, depth);
if (params.bytesPerLine < 0)
return nullptr;
QScopedPointer<QImageData> d(new QImageData);
@ -154,18 +147,15 @@ QImageData * QImageData::create(const QSize &size, QImage::Format format)
d->has_alpha_clut = false;
d->is_cached = false;
d->bytes_per_line = bytes_per_line;
d->nbytes = d->bytes_per_line*height;
d->bytes_per_line = params.bytesPerLine;
d->nbytes = params.totalSize;
d->data = (uchar *)malloc(d->nbytes);
if (!d->data) {
return 0;
}
if (!d->data)
return nullptr;
d->ref.ref();
return d.take();
}
QImageData::~QImageData()
@ -786,27 +776,27 @@ QImage::QImage(const QSize &size, Format format)
QImageData *QImageData::create(uchar *data, int width, int height, int bpl, QImage::Format format, bool readOnly, QImageCleanupFunction cleanupFunction, void *cleanupInfo)
{
QImageData *d = 0;
if (format == QImage::Format_Invalid)
return d;
if (width <= 0 || height <= 0 || !data || format == QImage::Format_Invalid)
return nullptr;
const int depth = qt_depthForFormat(format);
const int calc_bytes_per_line = ((width * depth + 31)/32) * 4;
const int min_bytes_per_line = (width * depth + 7)/8;
auto params = calculateImageParameters(width, height, depth);
if (params.totalSize < 0)
return nullptr;
if (bpl <= 0)
bpl = calc_bytes_per_line;
if (bpl > 0) {
// can't overflow, because has calculateImageParameters already done this multiplication
const int min_bytes_per_line = (width * depth + 7)/8;
if (bpl < min_bytes_per_line)
return nullptr;
if (width <= 0 || height <= 0 || !data
|| INT_MAX/sizeof(uchar *) < uint(height)
|| INT_MAX/uint(depth) < uint(width)
|| bpl <= 0
|| bpl < min_bytes_per_line
|| INT_MAX/uint(bpl) < uint(height))
return d; // invalid parameter(s)
// recalculate the total with this value
params.bytesPerLine = bpl;
if (mul_overflow<qsizetype>(bpl, height, &params.totalSize))
return nullptr;
}
d = new QImageData;
QImageData *d = new QImageData;
d->ref.ref();
d->own_data = false;
@ -817,8 +807,8 @@ QImageData *QImageData::create(uchar *data, int width, int height, int bpl, QIm
d->depth = depth;
d->format = format;
d->bytes_per_line = bpl;
d->nbytes = d->bytes_per_line * height;
d->bytes_per_line = params.bytesPerLine;
d->nbytes = params.totalSize;
d->cleanupFunction = cleanupFunction;
d->cleanupInfo = cleanupInfo;

58
src/gui/image/qimage_conversions.cpp
View File

@ -817,10 +817,10 @@ static bool convert_indexed8_to_ARGB_PM_inplace(QImageData *data, Qt::ImageConve
Q_ASSERT(data->own_data);
const int depth = 32;
const qsizetype dst_bytes_per_line = ((data->width * depth + 31) >> 5) << 2;
const qsizetype nbytes = dst_bytes_per_line * data->height;
uchar *const newData = (uchar *)realloc(data->data, nbytes);
auto params = QImageData::calculateImageParameters(data->width, data->height, depth);
if (params.bytesPerLine < 0)
return false;
uchar *const newData = (uchar *)realloc(data->data, params.totalSize);
if (!newData)
return false;
@ -828,10 +828,10 @@ static bool convert_indexed8_to_ARGB_PM_inplace(QImageData *data, Qt::ImageConve
// start converting from the end because the end image is bigger than the source
uchar *src_data = newData + data->nbytes; // end of src
quint32 *dest_data = (quint32 *) (newData + nbytes); // end of dest > end of src
quint32 *dest_data = (quint32 *) (newData + params.totalSize); // end of dest > end of src
const int width = data->width;
const int src_pad = data->bytes_per_line - width;
const int dest_pad = (dst_bytes_per_line >> 2) - width;
const int dest_pad = (params.bytesPerLine >> 2) - width;
if (data->colortable.size() == 0) {
data->colortable.resize(256);
for (int i = 0; i < 256; ++i)
@ -858,9 +858,9 @@ static bool convert_indexed8_to_ARGB_PM_inplace(QImageData *data, Qt::ImageConve
data->colortable = QVector<QRgb>();
data->format = QImage::Format_ARGB32_Premultiplied;
data->bytes_per_line = dst_bytes_per_line;
data->bytes_per_line = params.bytesPerLine;
data->depth = depth;
data->nbytes = nbytes;
data->nbytes = params.totalSize;
return true;
}
@ -871,10 +871,10 @@ static bool convert_indexed8_to_ARGB_inplace(QImageData *data, Qt::ImageConversi
Q_ASSERT(data->own_data);
const int depth = 32;
const qsizetype dst_bytes_per_line = ((data->width * depth + 31) >> 5) << 2;
const qsizetype nbytes = dst_bytes_per_line * data->height;
uchar *const newData = (uchar *)realloc(data->data, nbytes);
auto params = QImageData::calculateImageParameters(data->width, data->height, depth);
if (params.bytesPerLine < 0)
return false;
uchar *const newData = (uchar *)realloc(data->data, params.totalSize);
if (!newData)
return false;
@ -882,10 +882,10 @@ static bool convert_indexed8_to_ARGB_inplace(QImageData *data, Qt::ImageConversi
// start converting from the end because the end image is bigger than the source
uchar *src_data = newData + data->nbytes;
quint32 *dest_data = (quint32 *) (newData + nbytes);
quint32 *dest_data = (quint32 *) (newData + params.totalSize);
const int width = data->width;
const int src_pad = data->bytes_per_line - width;
const int dest_pad = (dst_bytes_per_line >> 2) - width;
const int dest_pad = (params.bytesPerLine >> 2) - width;
if (data->colortable.size() == 0) {
data->colortable.resize(256);
for (int i = 0; i < 256; ++i)
@ -909,9 +909,9 @@ static bool convert_indexed8_to_ARGB_inplace(QImageData *data, Qt::ImageConversi
data->colortable = QVector<QRgb>();
data->format = QImage::Format_ARGB32;
data->bytes_per_line = dst_bytes_per_line;
data->bytes_per_line = params.bytesPerLine;
data->depth = depth;
data->nbytes = nbytes;
data->nbytes = params.totalSize;
return true;
}
@ -939,10 +939,10 @@ static bool convert_indexed8_to_RGB16_inplace(QImageData *data, Qt::ImageConvers
Q_ASSERT(data->own_data);
const int depth = 16;
const qsizetype dst_bytes_per_line = ((data->width * depth + 31) >> 5) << 2;
const qsizetype nbytes = dst_bytes_per_line * data->height;
uchar *const newData = (uchar *)realloc(data->data, nbytes);
auto params = QImageData::calculateImageParameters(data->width, data->height, depth);
if (params.bytesPerLine < 0)
return false;
uchar *const newData = (uchar *)realloc(data->data, params.totalSize);
if (!newData)
return false;
@ -950,10 +950,10 @@ static bool convert_indexed8_to_RGB16_inplace(QImageData *data, Qt::ImageConvers
// start converting from the end because the end image is bigger than the source
uchar *src_data = newData + data->nbytes;
quint16 *dest_data = (quint16 *) (newData + nbytes);
quint16 *dest_data = (quint16 *) (newData + params.totalSize);
const int width = data->width;
const int src_pad = data->bytes_per_line - width;
const int dest_pad = (dst_bytes_per_line >> 1) - width;
const int dest_pad = (params.bytesPerLine >> 1) - width;
quint16 colorTableRGB16[256];
const int tableSize = data->colortable.size();
@ -983,9 +983,9 @@ static bool convert_indexed8_to_RGB16_inplace(QImageData *data, Qt::ImageConvers
}
data->format = QImage::Format_RGB16;
data->bytes_per_line = dst_bytes_per_line;
data->bytes_per_line = params.bytesPerLine;
data->depth = depth;
data->nbytes = nbytes;
data->nbytes = params.totalSize;
return true;
}
@ -997,6 +997,7 @@ static bool convert_RGB_to_RGB16_inplace(QImageData *data, Qt::ImageConversionFl
const int depth = 16;
// cannot overflow, since we're shrinking the buffer
const qsizetype dst_bytes_per_line = ((data->width * depth + 31) >> 5) << 2;
const qsizetype src_bytes_per_line = data->bytes_per_line;
quint32 *src_data = (quint32 *) data->data;
@ -1013,12 +1014,11 @@ static bool convert_RGB_to_RGB16_inplace(QImageData *data, Qt::ImageConversionFl
data->depth = depth;
data->nbytes = dst_bytes_per_line * data->height;
uchar *const newData = (uchar *)realloc(data->data, data->nbytes);
if (newData) {
if (newData)
data->data = newData;
return true;
} else {
return false;
}
// can't fail, since we're shrinking
return true;
}
static void convert_ARGB_PM_to_ARGB(QImageData *dest, const QImageData *src)

33
src/gui/image/qimage_p.h
View File

@ -52,6 +52,7 @@
//
#include <QtGui/private/qtguiglobal_p.h>
#include <QtCore/private/qnumeric_p.h>
#include <QMap>
#include <QVector>
@ -104,8 +105,40 @@ struct Q_GUI_EXPORT QImageData { // internal image data
bool doImageIO(const QImage *image, QImageWriter* io, int quality) const;
QPaintEngine *paintEngine;
struct ImageSizeParameters {
qsizetype bytesPerLine;
qsizetype totalSize;
};
static ImageSizeParameters calculateImageParameters(qsizetype width, qsizetype height, qsizetype depth);
};
inline QImageData::ImageSizeParameters
QImageData::calculateImageParameters(qsizetype width, qsizetype height, qsizetype depth)
{
ImageSizeParameters invalid = { -1, -1 };
if (height <= 0)
return invalid;
// calculate the size, taking care of overflows
qsizetype bytes_per_line;
if (mul_overflow(width, depth, &bytes_per_line))
return invalid;
if (add_overflow(bytes_per_line, qsizetype(31), &bytes_per_line))
return invalid;
// bytes per scanline (must be multiple of 4)
bytes_per_line = (bytes_per_line >> 5) << 2; // can't overflow
qsizetype total_size;
if (mul_overflow(height, bytes_per_line, &total_size))
return invalid;
qsizetype dummy;
if (mul_overflow(height, qsizetype(sizeof(uchar *)), &dummy))
return invalid; // why is this here?
return { bytes_per_line, total_size };
}
typedef void (*Image_Converter)(QImageData *dest, const QImageData *src, Qt::ImageConversionFlags);
typedef bool (*InPlace_Image_Converter)(QImageData *data, Qt::ImageConversionFlags);