Qt Image Decoders Stepping on Each Others

A weird bug

In December last year, Falk Krönert filled a strange bug: Gwenview failed to load some PNG images, which other applications were able to display flawlessly. I couldn’t reproduce it back then with Gwenview 2.7.3, so couldn’t dig any further.

Falk did not give up so easily though. After further investigations, he discovered the bug only happened with images whose width would be 2560 or 2808. Since I still couldn’t reproduce it, he provided me with a QEmu image of his openSUSE installation so that I could investigate (now that is one dedicated bug reporter!)

Investigating

Running the QEmu image, I was finally able to reproduce the bug. I started investigating, installing the necessary packages to build Gwenview and my beloved cgdb, learning how to use Zypper in the process.

Step one, an image format from the past…

I quickly discovered Qt was not able to discover the image format of the PNG. The code looked like this:

QBuffer buffer;
buffer.setBuffer(&mData); // mData contains the first 256 bytes of the image
buffer.open(QIODevice::ReadOnly);

QImageReader reader(&buffer);

mFormat = reader.format();
if (mFormat.isEmpty()) {
    return false;
}

In the case of this image, “mFormat” was supposed to contain “png”, but it was empty instead.

In case you are not familiar with Qt image plugin code, it works like this: Qt provides a base class, QImageIOHandler, which image decoders must inherit from. so there is a QJpegHandler, a QPngHandler and so on. Some of these handlers are provided by Qt directly in QtGui.so, others are provided by Qt and kdelibs (and possibly others) as plugins installed in “plugins/imageformats”.

Here is QImageReader::format() code:

QByteArray QImageReader::format() const
{
    if (d->format.isEmpty()) {
        if (!d->initHandler())
            return QByteArray();
        return d->handler->canRead() ? d->handler->format() : QByteArray();
    }

    return d->format;
}

Debugger was telling me d->handler was correctly created by initHandler(), but QPngHandler::canRead() was never called!

Quite puzzled by this, I finally resorted to place a breakpoint in QImageIOHandler constructor. Once I hit the breakpoints, I printed the backtrace, and was astonished to find myself inside the PCX handler provided by kdelibs! This explained why QPngHandler::canRead() was not called, QImageReader was calling PCXHandler::canRead(), which unsurprisedly returned false.

This does not explain how Qt decided the image was a PCX in the first place. To decide which handler to use, initHandler() calls all implementations of QImageIOHandler::canRead(), the first one to answer true wins. PCXHandler::canRead() implementation is not very selective: it returns true if the first byte of the image is 10 (According to Wikipedia it should be possible to check at least the first two bytes, need to look into this).

Here is an hex-dump of the first bytes of the PNG image:

89 50 4e 47 0d 0a 1a 0a  00 00 00 0d 49 48 44 52  |.PNG........IHDR|
00 00 0a 00 00 00 00 02  08 02 00 00 00 1b 8e 12  |................|

There are a few 10 (0a in the dump) values but the first byte is definitely not 10.

Maybe the PCX loader is looking at the wrong position? I decided to place a breakpoint in PCXHandler::canRead() and print device->pos(). Sure enough, it sayd the position was 18, instead of the expected 0… If you look at position 18 (starting from 0) in the dump there is indeed a 0a value.

It is interesting to look at what this 0a stands for: The PNG specification tells us a PNG starts with an 8 byte signature, followed by a series of chunks. The first chunk must be an IHDR chunk. This chunk starts with “IHDR”, followed by four bytes representing the image width, in most significant byte order. This means the first four bytes in the second line of the dump (00 00 0a 00) are the image width, and indeed, 0a00 is the hexadecimal for 2560… Do you remember the bug was only happening for some image sizes? If the width had been 2559, the IHDR chunk would have started with 00 00 09 ff, and thus the PCX handler would not have identified this image as a PCX.

Another format gets in…

While the PCX handler could be improved to use more than one byte as a signature, it is not the real problem: had device->pos() been correctly set to 0, it wouldn’t have identified the image as PCX. QImageIOHandler::canRead() documentation explicitly says:

When reimplementing canRead(), make sure that the I/O device (device()) is left in its original state (e.g., by using peek() rather than read()).

Someone was not playing by the rules and was altering the device position before PCXHandler::canRead() was reached.

I was starting to suspect one of the other image handlers was doing something nasty. To try to isolate the culprit, I first removed all KDE image plugins but the PCX handler. No luck, still buggy. I then removed all Qt image plugins (PNG was still there as it is one of those built into QtGui.so) Bingo! The image was now being correctly identified. Using a quick dichotomy, I found out the TGA plugin was the nasty boy.

QTgaHandler::canRead() looks like this:

bool QTgaHandler::canRead(QIODevice *device)
{
    if (!device) {
        qWarning("QTgaHandler::canRead() called with no device");
        return false;
    }
    QTgaFile tga(device);
    return tga.isValid();
}

Looking at the code you can probably guess it: QTgaFile constructor does not take care of not altering QIODevice state. Looking at the Git history for this handler shows it was copied from QtQuick3D to Qt before the 4.8.0 release, which explains why there was no problem when running Gwenview on top of Qt 4.7.

It is a bit sad however that this code was brought in instead of using KDE TGA handler, which has been around for a while, does not suffer from this bug and have (limited) write supports whereas the Qt handler is read-only.

Fixing time

I worked around this bug in Gwenview by taking advantage of the fact that QImageReader accept an optional “format” parameter, which can be used as a hint: QImageReader will first try to use an image handler which supports this format.

I was not using this feature until now because I thought passing a format would make QImageReader fail if the image was not of the specified format. It turns out I was wrong: the format parameter is just a hint, if the image cannot be loaded with a handler implementing this format, QImageReader will try the others. I should probably propose a patch for the documentation to reflect that.

The real fix however is to teach QTgaHandler proper manners. I just posted a fix on qt-project.org, hopefully it will get in.

Gwenview 2.8.2 (from upcoming KDE SC 4.8.2), has the work-around in but if you haven’t upgraded yet, you can fix the bug by simply removing the TGA handler plugin. Look for a file named libqtga.so. It should be in:

• openSUSE (64 bits): /usr/lib64/qt4/plugins/imageformats/
• Ubuntu|Debian (64 bits): /usr/lib/x86_64-linux-gnu/qt4/plugins/imageformats/

(I don’t know the location for 32 bits systems)

So to summarize, a rogue TGA handler was tricking a sloppy PCX handler into identifying a PNG file into a PCX file, what a mess!