From 8562ce18f1abdff44e9ae0670f76a8060603f9ae Mon Sep 17 00:00:00 2001 From: "Martin T. H. Sandsmark" Date: Wed, 2 Oct 2019 17:39:59 +0200 Subject: [PATCH] Add some sanity and bounds checking Since QImage does sanity checking for overflows and stuff wrt. dimensions and depth, check for QImage::isNull() as early as possible to see if there's some funky business going on. Also tried to add some checks wherever we wrote to "raw" memory. Unit tests pass, and tested converting some files from https://samples.ffmpeg.org/image-samples/ to pngs, and that seemed to work. Reviewed By: aacid Differential Revision: https://phabricator.kde.org/D24367 --- src/imageformats/exr.cpp | 11 ++++++----- src/imageformats/pcx.cpp | 21 ++++++++++++++++++++- src/imageformats/pic.cpp | 6 ++++++ src/imageformats/psd.cpp | 15 +++++++++++++++ src/imageformats/rgb.cpp | 37 +++++++++++++++++++++++++++++++++---- src/imageformats/tga.cpp | 28 ++++++++++++++++++++++++++-- 6 files changed, 106 insertions(+), 12 deletions(-) diff --git a/src/imageformats/exr.cpp b/src/imageformats/exr.cpp index 3f60515..b64dc9e 100644 --- a/src/imageformats/exr.cpp +++ b/src/imageformats/exr.cpp @@ -172,17 +172,18 @@ bool EXRHandler::read(QImage *outImage) width = dw.max.x - dw.min.x + 1; height = dw.max.y - dw.min.y + 1; + QImage image(width, height, QImage::Format_RGB32); + if (image.isNull()) { + qWarning() << "Failed to allocate image, invalid size?" << QSize(width, height); + return false; + } + Imf::Array2D pixels; pixels.resizeErase(height, width); file.setFrameBuffer(&pixels[0][0] - dw.min.x - dw.min.y * width, 1, width); file.readPixels(dw.min.y, dw.max.y); - QImage image(width, height, QImage::Format_RGB32); - if (image.isNull()) { - return false; - } - // somehow copy pixels into image for (int y = 0; y < height; y++) { for (int x = 0; x < width; x++) { diff --git a/src/imageformats/pcx.cpp b/src/imageformats/pcx.cpp index a45ba96..fd376a0 100644 --- a/src/imageformats/pcx.cpp +++ b/src/imageformats/pcx.cpp @@ -253,8 +253,10 @@ static void readImage1(QImage &img, QDataStream &s, const PCXHEADER &header) img = QImage(header.width(), header.height(), QImage::Format_Mono); img.setColorCount(2); - if (img.isNull()) + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(header.width(), header.height()); return; + } for (int y = 0; y < header.height(); ++y) { if (s.atEnd()) { @@ -282,6 +284,10 @@ static void readImage4(QImage &img, QDataStream &s, const PCXHEADER &header) img = QImage(header.width(), header.height(), QImage::Format_Indexed8); img.setColorCount(16); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(header.width(), header.height()); + return; + } for (int y = 0; y < header.height(); ++y) { if (s.atEnd()) { @@ -301,6 +307,9 @@ static void readImage4(QImage &img, QDataStream &s, const PCXHEADER &header) } uchar *p = img.scanLine(y); + if (!p) { + qWarning() << "Failed to get scanline for" << y << "might be out of bounds"; + } for (int x = 0; x < header.width(); ++x) { p[ x ] = pixbuf[ x ]; } @@ -319,6 +328,11 @@ static void readImage8(QImage &img, QDataStream &s, const PCXHEADER &header) img = QImage(header.width(), header.height(), QImage::Format_Indexed8); img.setColorCount(256); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(header.width(), header.height()); + return; + } + for (int y = 0; y < header.height(); ++y) { if (s.atEnd()) { img = QImage(); @@ -360,6 +374,11 @@ static void readImage24(QImage &img, QDataStream &s, const PCXHEADER &header) img = QImage(header.width(), header.height(), QImage::Format_RGB32); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(header.width(), header.height()); + return; + } + for (int y = 0; y < header.height(); ++y) { if (s.atEnd()) { img = QImage(); diff --git a/src/imageformats/pic.cpp b/src/imageformats/pic.cpp index 22bd8b4..6566e27 100644 --- a/src/imageformats/pic.cpp +++ b/src/imageformats/pic.cpp @@ -253,6 +253,11 @@ bool SoftimagePICHandler::read(QImage *image) } QImage img(m_header.width, m_header.height, fmt); + if (img.isNull()) { + qDebug() << "Failed to allocate image, invalid dimensions?" << QSize(m_header.width, m_header.height) << fmt; + return false; + } + img.fill(qRgb(0,0,0)); for (int y = 0; y < m_header.height; y++) { @@ -362,6 +367,7 @@ bool SoftimagePICHandler::readHeader() m_state = ReadHeader; } } + return m_state != Error; } diff --git a/src/imageformats/psd.cpp b/src/imageformats/psd.cpp index 85f3aeb..e33271e 100644 --- a/src/imageformats/psd.cpp +++ b/src/imageformats/psd.cpp @@ -171,10 +171,20 @@ static bool LoadPSD(QDataStream &stream, const PSDHeader &header, QImage &img) channel_num = 4; } img = QImage(header.width, header.height, fmt); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(header.width, header.height); + return false; + } img.fill(qRgb(0,0,0)); const quint32 pixel_count = header.height * header.width; + // Verify this, as this is used to write into the memory of the QImage + if (pixel_count > img.sizeInBytes() / sizeof(QRgb)) { + qWarning() << "Invalid pixel count!" << pixel_count << "bytes available:" << img.sizeInBytes(); + return false; + } + QRgb *image_data = reinterpret_cast(img.bits()); if (!image_data) { @@ -276,6 +286,11 @@ bool PSDHandler::canRead(QIODevice *device) char head[4]; qint64 readBytes = device->read(head, sizeof(head)); + if (readBytes < 0) { + qWarning() << "Read failed" << device->errorString(); + return false; + } + if (readBytes != sizeof(head)) { if (device->isSequential()) { while (readBytes > 0) { diff --git a/src/imageformats/rgb.cpp b/src/imageformats/rgb.cpp index a288eef..77ac1bb 100644 --- a/src/imageformats/rgb.cpp +++ b/src/imageformats/rgb.cpp @@ -313,6 +313,10 @@ bool SGIImage::readImage(QImage &img) } img = QImage(_xsize, _ysize, QImage::Format_RGB32); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(_xsize, _ysize); + return false; + } if (_zsize == 0 ) return false; @@ -470,7 +474,14 @@ bool SGIImage::scanData(const QImage &img) uint len; for (y = 0; y < _ysize; y++) { - c = reinterpret_cast(img.scanLine(_ysize - y - 1)); + const int yPos = _ysize - y - 1; // scanline doesn't do any sanity checking + if (yPos >= img.height()) { + qWarning() << "Failed to get scanline for" << yPos; + return false; + } + + c = reinterpret_cast(img.scanLine(yPos)); + for (x = 0; x < _xsize; x++) { buf[x] = intensity(qRed(*c++)); } @@ -484,7 +495,13 @@ bool SGIImage::scanData(const QImage &img) if (_zsize != 2) { for (y = 0; y < _ysize; y++) { - c = reinterpret_cast(img.scanLine(_ysize - y - 1)); + const int yPos = _ysize - y - 1; + if (yPos >= img.height()) { + qWarning() << "Failed to get scanline for" << yPos; + return false; + } + + c = reinterpret_cast(img.scanLine(yPos)); for (x = 0; x < _xsize; x++) { buf[x] = intensity(qGreen(*c++)); } @@ -493,7 +510,13 @@ bool SGIImage::scanData(const QImage &img) } for (y = 0; y < _ysize; y++) { - c = reinterpret_cast(img.scanLine(_ysize - y - 1)); + const int yPos = _ysize - y - 1; + if (yPos >= img.height()) { + qWarning() << "Failed to get scanline for" << yPos; + return false; + } + + c = reinterpret_cast(img.scanLine(yPos)); for (x = 0; x < _xsize; x++) { buf[x] = intensity(qBlue(*c++)); } @@ -507,7 +530,13 @@ bool SGIImage::scanData(const QImage &img) } for (y = 0; y < _ysize; y++) { - c = reinterpret_cast(img.scanLine(_ysize - y - 1)); + const int yPos = _ysize - y - 1; + if (yPos >= img.height()) { + qWarning() << "Failed to get scanline for" << yPos; + return false; + } + + c = reinterpret_cast(img.scanLine(yPos)); for (x = 0; x < _xsize; x++) { buf[x] = intensity(qAlpha(*c++)); } diff --git a/src/imageformats/tga.cpp b/src/imageformats/tga.cpp index 6b0b600..bbd380e 100644 --- a/src/imageformats/tga.cpp +++ b/src/imageformats/tga.cpp @@ -178,6 +178,10 @@ static bool LoadTGA(QDataStream &s, const TgaHeader &tga, QImage &img) { // Create image. img = QImage(tga.width, tga.height, QImage::Format_RGB32); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(tga.width, tga.height); + return false; + } TgaHeaderInfo info(tga); @@ -186,6 +190,10 @@ static bool LoadTGA(QDataStream &s, const TgaHeader &tga, QImage &img) // However alpha exists only in the 32 bit format. if ((tga.pixel_size == 32) && (tga.flags & 0xf)) { img = QImage(tga.width, tga.height, QImage::Format_ARGB32); + if (img.isNull()) { + qWarning() << "Failed to allocate image, invalid dimensions?" << QSize(tga.width, tga.height); + return false; + } if (numAlphaBits > 8) { return false; @@ -229,9 +237,10 @@ static bool LoadTGA(QDataStream &s, const TgaHeader &tga, QImage &img) if (info.rle) { // Decode image. char *dst = (char *)image; + char *imgEnd = dst + size; qint64 num = size; - while (num > 0) { + while (num > 0 && valid) { if (s.atEnd()) { valid = false; break; @@ -257,6 +266,12 @@ static bool LoadTGA(QDataStream &s, const TgaHeader &tga, QImage &img) memset(&pixel[dataRead], 0, pixel_size - dataRead); } do { + if (dst + pixel_size > imgEnd) { + qWarning() << "Trying to write out of bounds!" << ptrdiff_t(dst) << (ptrdiff_t(imgEnd) - ptrdiff_t(pixel_size)); + valid = false; + break; + } + memcpy(dst, pixel, pixel_size); dst += pixel_size; } while (--count); @@ -268,8 +283,17 @@ static bool LoadTGA(QDataStream &s, const TgaHeader &tga, QImage &img) free(image); return false; } + + if ((uint)dataRead < count) { - memset(&dst[dataRead], 0, count - dataRead); + const size_t toCopy = count - dataRead; + if (&dst[dataRead] + toCopy > imgEnd) { + qWarning() << "Trying to write out of bounds!" << ptrdiff_t(image) << ptrdiff_t(&dst[dataRead]);; + valid = false; + break; + } + + memset(&dst[dataRead], 0, toCopy); } dst += count; }