GIT_SILENT Upgrade ECM and KF5 version requirements for 5.72.0 release.

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

.arcconfig

@@ -1,3 +0,0 @@
-{
-  "phabricator.uri" : "https://phabricator.kde.org/"
-}

CMakeLists.txt

@@ -2,8 +2,10 @@ cmake_minimum_required(VERSION 3.5)
 
 project(KImageFormats)
 
+set (CMAKE_CXX_STANDARD 14)
+
 include(FeatureSummary)
-find_package(ECM 5.68.0  NO_MODULE)
+find_package(ECM 5.72.0  NO_MODULE)
 set_package_properties(ECM PROPERTIES TYPE REQUIRED DESCRIPTION "Extra CMake Modules." URL "https://commits.kde.org/extra-cmake-modules")
 feature_summary(WHAT REQUIRED_PACKAGES_NOT_FOUND FATAL_ON_MISSING_REQUIRED_PACKAGES)
 
@@ -49,7 +51,7 @@ add_definitions(-DQT_NO_FOREACH)
 # 050d00 (5.13) triggers a BIC in qimageiohandler.h, in Qt 5.13, so do not enable that until we can require 5.14
 # https://codereview.qt-project.org/c/qt/qtbase/+/279215
 add_definitions(-DQT_DISABLE_DEPRECATED_BEFORE=0x050c00)
-add_definitions(-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054300)
+add_definitions(-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054700)
 add_subdirectory(src)
 if (BUILD_TESTING)
     add_subdirectory(autotests)

autotests/readtest.cpp

@@ -127,6 +127,12 @@ int main(int argc, char ** argv)
         << "Starting basic read tests for "
         << suffix << " images *********\n";
 
+    const QList<QByteArray> formats = QImageReader::supportedImageFormats();
+    QStringList formatStrings;
+    formatStrings.reserve(formats.size());
+    std::transform(formats.begin(), formats.end(), std::back_inserter(formatStrings), [](const QByteArray &format) { return QString(format); });
+    QTextStream(stdout) << "QImageReader::supportedImageFormats: " << formatStrings.join(", ") << "\n";
+
     const QFileInfoList lstImgDir = imgdir.entryInfoList();
     for (const QFileInfo &fi : lstImgDir) {
         int suffixPos = fi.filePath().count() - suffix.count();
@@ -134,7 +140,7 @@ int main(int argc, char ** argv)
         QString expfile = fi.filePath().replace(suffixPos, suffix.count(), QStringLiteral("png"));
         QString expfilename = QFileInfo(expfile).fileName();
 
-        QImageReader inputReader(inputfile, format.constData());
+        QImageReader inputReader(inputfile, format);
         QImageReader expReader(expfile, "png");
 
         QImage inputImage;

src/imageformats/eps.cpp

@@ -17,7 +17,7 @@
 #include <QCoreApplication>
 
 // logging category for this framework, default: log stuff >= warning
-Q_LOGGING_CATEGORY(EPSPLUGIN, "epsplugin", QtWarningMsg)
+Q_LOGGING_CATEGORY(EPSPLUGIN, "kf5.kimageformats.epsplugin", QtWarningMsg)
 
 //#define EPS_PERFORMANCE_DEBUG 1
 

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<Imf::Rgba> 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++) {

src/imageformats/gimp_p.h

@@ -56,16 +56,6 @@ typedef enum {
     INDEXED
 } GimpImageBaseType;
 
-//! Type of individual layers in an XCF file.
-
-typedef enum {
-    RGB_GIMAGE,
-    RGBA_GIMAGE,
-    GRAY_GIMAGE,
-    GRAYA_GIMAGE,
-    INDEXED_GIMAGE,
-    INDEXEDA_GIMAGE
-} GimpImageType;
 
 // From GIMP "libgimp/gimpenums.h" v2.4
 
@@ -96,49 +86,6 @@ typedef enum {
     GRAIN_MERGE_MODE
 } LayerModeEffects;
 
-// From GIMP "xcf.c" v1.2
-
-//! Properties which can be stored in an XCF file.
-
-typedef enum {
-    PROP_END = 0,
-    PROP_COLORMAP = 1,
-    PROP_ACTIVE_LAYER = 2,
-    PROP_ACTIVE_CHANNEL = 3,
-    PROP_SELECTION = 4,
-    PROP_FLOATING_SELECTION = 5,
-    PROP_OPACITY = 6,
-    PROP_MODE = 7,
-    PROP_VISIBLE = 8,
-    PROP_LINKED = 9,
-    PROP_PRESERVE_TRANSPARENCY = 10,
-    PROP_APPLY_MASK = 11,
-    PROP_EDIT_MASK = 12,
-    PROP_SHOW_MASK = 13,
-    PROP_SHOW_MASKED = 14,
-    PROP_OFFSETS = 15,
-    PROP_COLOR = 16,
-    PROP_COMPRESSION = 17,
-    PROP_GUIDES = 18,
-    PROP_RESOLUTION = 19,
-    PROP_TATTOO = 20,
-    PROP_PARASITES = 21,
-    PROP_UNIT = 22,
-    PROP_PATHS = 23,
-    PROP_USER_UNIT = 24,
-    MAX_SUPPORTED_PROPTYPE // should always be at the end so its value is last + 1
-} PropType;
-
-// From GIMP "xcf.c" v1.2
-
-//! Compression type used in layer tiles.
-
-typedef enum {
-    COMPRESS_NONE = 0,
-    COMPRESS_RLE = 1,
-    COMPRESS_ZLIB = 2,
-    COMPRESS_FRACTAL = 3  /* Unused. */
-} CompressionType;
 
 // From GIMP "paint_funcs.c" v1.2
 

src/imageformats/hdr.cpp

@@ -12,11 +12,15 @@
 
 #include <QImage>
 #include <QDataStream>
+#include <QLoggingCategory>
+#include <QRegularExpressionMatch>
 
 #include <QDebug>
 
 typedef unsigned char uchar;
 
+Q_LOGGING_CATEGORY(HDRPLUGIN, "kf5.kimageformats.hdrplugin", QtWarningMsg)
+
 namespace   // Private.
 {
 
@@ -95,6 +99,7 @@ static bool LoadHDR(QDataStream &s, const int width, const int height, QImage &i
     // Create dst image.
     img = QImage(width, height, QImage::Format_RGB32);
     if (img.isNull()) {
+        qCDebug(HDRPLUGIN) << "Couldn't create image with size" << width << height << "and format RGB32";
         return false;
     }
 
@@ -141,6 +146,7 @@ static bool LoadHDR(QDataStream &s, const int width, const int height, QImage &i
         }
 
         if ((image[2] << 8 | image[3]) != width) {
+            qCDebug(HDRPLUGIN) << "Line of pixels had width" << (image[2] << 8 | image[3]) << "instead of" << width;
             return false;
         }
 
@@ -149,6 +155,7 @@ static bool LoadHDR(QDataStream &s, const int width, const int height, QImage &i
             for (int j = 0; j < width;) {
                 s >> code;
                 if (s.atEnd()) {
+                    qCDebug(HDRPLUGIN) << "Truncated HDR file";
                     return false;
                 }
                 if (code > 128) {
@@ -182,39 +189,48 @@ static bool LoadHDR(QDataStream &s, const int width, const int height, QImage &i
 bool HDRHandler::read(QImage *outImage)
 {
     int len;
-    char line[MAXLINE];
-    //bool validHeader = false;
-    bool validFormat = false;
+    QByteArray line(MAXLINE + 1, Qt::Uninitialized);
+    QByteArray format;
 
     // Parse header
     do {
-        len = device()->readLine(line, MAXLINE);
+        len = device()->readLine(line.data(), MAXLINE);
 
-        /*if (strcmp(line, "#?RADIANCE\n") == 0 || strcmp(line, "#?RGBE\n") == 0)
-        {
-            validHeader = true;
-        }*/
-        if (strcmp(line, "FORMAT=32-bit_rle_rgbe\n") == 0) {
-            validFormat = true;
+        if (line.startsWith("FORMAT=")) {
+            format = line.mid(7, len - 7 - 1 /*\n*/);
         }
 
     } while ((len > 0) && (line[0] != '\n'));
 
-    if (/*!validHeader ||*/ !validFormat) {
-        // qDebug() << "Unknown HDR format.";
+    if (format != "32-bit_rle_rgbe") {
+        qCDebug(HDRPLUGIN) << "Unknown HDR format:" << format;
         return false;
     }
 
-    device()->readLine(line, MAXLINE);
+    len = device()->readLine(line.data(), MAXLINE);
+    line.resize(len);
 
-    char s1[3], s2[3];
-    int width, height;
-    if (sscanf(line, "%2[+-XY] %d %2[+-XY] %d\n", s1, &height, s2, &width) != 4)
-        //if( sscanf(line, "-Y %d +X %d", &height, &width) < 2 )
-    {
-        // qDebug() << "Invalid HDR file.";
+    /*
+       TODO: handle flipping and rotation, as per the spec below
+       The single resolution line consists of 4 values, a X and Y label each followed by a numerical
+       integer value. The X and Y are immediately preceded by a sign which can be used to indicate
+       flipping, the order of the X and Y indicate rotation. The standard coordinate system for
+       Radiance images would have the following resolution string -Y N +X N. This indicates that the
+       vertical axis runs down the file and the X axis is to the right (imagining the image as a
+       rectangular block of data). A -X would indicate a horizontal flip of the image. A +Y would
+       indicate a vertical flip. If the X value appears before the Y value then that indicates that
+       the image is stored in column order rather than row order, that is, it is rotated by 90 degrees.
+       The reader can convince themselves that the 8 combinations cover all the possible image orientations
+       and rotations.
+    */
+    QRegularExpression resolutionRegExp(QStringLiteral("([+\\-][XY]) ([0-9]+) ([+\\-][XY]) ([0-9]+)\n"));
+    QRegularExpressionMatch match = resolutionRegExp.match(QString::fromLatin1(line));
+    if (!match.hasMatch()) {
+        qCDebug(HDRPLUGIN) << "Invalid HDR file, the first line after the header didn't have the expected format:" << line;
         return false;
     }
+    const int width = match.captured(2).toInt();
+    const int height = match.captured(4).toInt();
 
     QDataStream s(device());
 

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();

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;
 }
 

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<QRgb*>(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) {

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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(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<const QRgb *>(img.scanLine(yPos));
         for (x = 0; x < _xsize; x++) {
             buf[x] = intensity(qAlpha(*c++));
         }

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;
             }