From d423fe985137be1283c0998f54b6ba307a8cf923 Mon Sep 17 00:00:00 2001 From: Andrei Golubev Date: Tue, 1 Sep 2020 15:48:00 +0200 Subject: QIcc: fix alignment concerns in ICC profile parsing Updated QIcc::fromIccProfile() and friends to not rely on QByteArray pointer alignment. Used qFromUnaligned() instead Task-number: QTBUG-84267 Pick-to: 5.15 Change-Id: I69ef7e011707bec27cd84693e7f0e92d79a577d1 Reviewed-by: Qt CI Bot Reviewed-by: Allan Sandfeld Jensen --- src/gui/painting/qcolorspace.cpp | 3 - src/gui/painting/qicc.cpp | 186 ++++++++++++++++++++++----------------- 2 files changed, 103 insertions(+), 86 deletions(-) (limited to 'src') diff --git a/src/gui/painting/qcolorspace.cpp b/src/gui/painting/qcolorspace.cpp index 86cb5db2ff..5ae071f480 100644 --- a/src/gui/painting/qcolorspace.cpp +++ b/src/gui/painting/qcolorspace.cpp @@ -666,9 +666,6 @@ QByteArray QColorSpace::iccProfile() const If the ICC profile is not supported an invalid QColorSpace is returned where you can still read the original ICC profile using iccProfile(). - \note If the QByteArray data is created from external sources it should be - at least 4 byte aligned. - \sa iccProfile() */ QColorSpace QColorSpace::fromIccProfile(const QByteArray &iccProfile) diff --git a/src/gui/painting/qicc.cpp b/src/gui/painting/qicc.cpp index 832b732bf6..7cf5e36073 100644 --- a/src/gui/painting/qicc.cpp +++ b/src/gui/painting/qicc.cpp @@ -1,6 +1,6 @@ /**************************************************************************** ** -** Copyright (C) 2016 The Qt Company Ltd. +** Copyright (C) 2020 The Qt Company Ltd. ** Contact: https://www.qt.io/licensing/ ** ** This file is part of the QtGui module of the Qt Toolkit. @@ -49,6 +49,8 @@ #include "qcolorspace_p.h" #include "qcolortrc_p.h" +#include + QT_BEGIN_NAMESPACE Q_LOGGING_CATEGORY(lcIcc, "qt.gui.icc") @@ -448,14 +450,14 @@ bool parseXyzData(const QByteArray &data, const TagEntry &tagEntry, QColorVector qCWarning(lcIcc) << "Undersized XYZ tag"; return false; } - const XYZTagData *xyz = reinterpret_cast(data.constData() + tagEntry.offset); - if (xyz->type != quint32(Tag::XYZ_)) { + const XYZTagData xyz = qFromUnaligned(data.constData() + tagEntry.offset); + if (xyz.type != quint32(Tag::XYZ_)) { qCWarning(lcIcc) << "Bad XYZ content type"; return false; } - const float x = fromFixedS1516(xyz->fixedX); - const float y = fromFixedS1516(xyz->fixedY); - const float z = fromFixedS1516(xyz->fixedZ); + const float x = fromFixedS1516(xyz.fixedX); + const float y = fromFixedS1516(xyz.fixedY); + const float z = fromFixedS1516(xyz.fixedZ); colorVector = QColorVector(x, y, z); return true; @@ -463,26 +465,29 @@ bool parseXyzData(const QByteArray &data, const TagEntry &tagEntry, QColorVector bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma) { - const GenericTagData *trcData = reinterpret_cast(data.constData() + tagEntry.offset); - if (trcData->type == quint32(Tag::curv)) { - const CurvTagData *curv = static_cast(trcData); - if (curv->valueCount > (1 << 16)) + const GenericTagData trcData = qFromUnaligned(data.constData() + + tagEntry.offset); + if (trcData.type == quint32(Tag::curv)) { + const CurvTagData curv = qFromUnaligned(data.constData() + tagEntry.offset); + if (curv.valueCount > (1 << 16)) return false; - if (tagEntry.size - 12 < 2 * curv->valueCount) + if (tagEntry.size - 12 < 2 * curv.valueCount) return false; - if (curv->valueCount == 0) { + if (curv.valueCount == 0) { gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction(); // Linear - } else if (curv->valueCount == 1) { - float g = curv->value[0] * (1.0f / 256.0f); + } else if (curv.valueCount == 1) { + float g = curv.value[0] * (1.0f / 256.0f); gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction::fromGamma(g); } else { QList tabl; - tabl.resize(curv->valueCount); - for (uint i = 0; i < curv->valueCount; ++i) - tabl[i] = curv->value[i]; - QColorTransferTable table = QColorTransferTable(curv->valueCount, std::move(tabl)); + tabl.resize(curv.valueCount); + static_assert(sizeof(GenericTagData) == 2 * sizeof(quint32_be), + "GenericTagData has padding. The following code is a subject to UB."); + const auto offset = tagEntry.offset + sizeof(GenericTagData) + sizeof(quint32_be); + qFromBigEndian(data.constData() + offset, curv.valueCount, tabl.data()); + QColorTransferTable table = QColorTransferTable(curv.valueCount, std::move(tabl)); QColorTransferFunction curve; if (!table.asColorTransferFunction(&curve)) { gamma.m_type = QColorTrc::Type::Table; @@ -495,13 +500,18 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma } return true; } - if (trcData->type == quint32(Tag::para)) { + if (trcData.type == quint32(Tag::para)) { if (tagEntry.size < sizeof(ParaTagData)) return false; - const ParaTagData *para = static_cast(trcData); - switch (para->curveType) { + static_assert(sizeof(GenericTagData) == 2 * sizeof(quint32_be), + "GenericTagData has padding. The following code is a subject to UB."); + const ParaTagData para = qFromUnaligned(data.constData() + tagEntry.offset); + // re-read first parameter for consistency: + const auto parametersOffset = tagEntry.offset + sizeof(GenericTagData) + + 2 * sizeof(quint16_be); + switch (para.curveType) { case 0: { - float g = fromFixedS1516(para->parameter[0]); + float g = fromFixedS1516(para.parameter[0]); gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction::fromGamma(g); break; @@ -509,9 +519,11 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma case 1: { if (tagEntry.size < sizeof(ParaTagData) + 2 * 4) return false; - float g = fromFixedS1516(para->parameter[0]); - float a = fromFixedS1516(para->parameter[1]); - float b = fromFixedS1516(para->parameter[2]); + std::array parameters = + qFromUnaligned(data.constData() + parametersOffset); + float g = fromFixedS1516(parameters[0]); + float a = fromFixedS1516(parameters[1]); + float b = fromFixedS1516(parameters[2]); float d = -b / a; gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction(a, b, 0.0f, d, 0.0f, 0.0f, g); @@ -520,10 +532,12 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma case 2: { if (tagEntry.size < sizeof(ParaTagData) + 3 * 4) return false; - float g = fromFixedS1516(para->parameter[0]); - float a = fromFixedS1516(para->parameter[1]); - float b = fromFixedS1516(para->parameter[2]); - float c = fromFixedS1516(para->parameter[3]); + std::array parameters = + qFromUnaligned(data.constData() + parametersOffset); + float g = fromFixedS1516(parameters[0]); + float a = fromFixedS1516(parameters[1]); + float b = fromFixedS1516(parameters[2]); + float c = fromFixedS1516(parameters[3]); float d = -b / a; gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction(a, b, 0.0f, d, c, c, g); @@ -532,11 +546,13 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma case 3: { if (tagEntry.size < sizeof(ParaTagData) + 4 * 4) return false; - float g = fromFixedS1516(para->parameter[0]); - float a = fromFixedS1516(para->parameter[1]); - float b = fromFixedS1516(para->parameter[2]); - float c = fromFixedS1516(para->parameter[3]); - float d = fromFixedS1516(para->parameter[4]); + std::array parameters = + qFromUnaligned(data.constData() + parametersOffset); + float g = fromFixedS1516(parameters[0]); + float a = fromFixedS1516(parameters[1]); + float b = fromFixedS1516(parameters[2]); + float c = fromFixedS1516(parameters[3]); + float d = fromFixedS1516(parameters[4]); gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction(a, b, c, d, 0.0f, 0.0f, g); break; @@ -544,19 +560,21 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma case 4: { if (tagEntry.size < sizeof(ParaTagData) + 6 * 4) return false; - float g = fromFixedS1516(para->parameter[0]); - float a = fromFixedS1516(para->parameter[1]); - float b = fromFixedS1516(para->parameter[2]); - float c = fromFixedS1516(para->parameter[3]); - float d = fromFixedS1516(para->parameter[4]); - float e = fromFixedS1516(para->parameter[5]); - float f = fromFixedS1516(para->parameter[6]); + std::array parameters = + qFromUnaligned(data.constData() + parametersOffset); + float g = fromFixedS1516(parameters[0]); + float a = fromFixedS1516(parameters[1]); + float b = fromFixedS1516(parameters[2]); + float c = fromFixedS1516(parameters[3]); + float d = fromFixedS1516(parameters[4]); + float e = fromFixedS1516(parameters[5]); + float f = fromFixedS1516(parameters[6]); gamma.m_type = QColorTrc::Type::Function; gamma.m_fun = QColorTransferFunction(a, b, c, d, e, f, g); break; } default: - qCWarning(lcIcc) << "Unknown para type" << uint(para->curveType); + qCWarning(lcIcc) << "Unknown para type" << uint(para.curveType); return false; } return true; @@ -567,73 +585,70 @@ bool parseTRC(const QByteArray &data, const TagEntry &tagEntry, QColorTrc &gamma bool parseDesc(const QByteArray &data, const TagEntry &tagEntry, QString &descName) { - const GenericTagData *tag = (const GenericTagData *)(data.constData() + tagEntry.offset); + const GenericTagData tag = qFromUnaligned(data.constData() + tagEntry.offset); // Either 'desc' (ICCv2) or 'mluc' (ICCv4) - if (tag->type == quint32(Tag::desc)) { + if (tag.type == quint32(Tag::desc)) { if (tagEntry.size < sizeof(DescTagData)) return false; - const DescTagData *desc = (const DescTagData *)(data.constData() + tagEntry.offset); - const quint32 len = desc->asciiDescriptionLength; + const DescTagData desc = qFromUnaligned(data.constData() + tagEntry.offset); + const quint32 len = desc.asciiDescriptionLength; if (len < 1) return false; if (tagEntry.size - 12 < len) return false; - if (desc->asciiDescription[len - 1] != '\0') + static_assert(sizeof(GenericTagData) == 2 * sizeof(quint32_be), + "GenericTagData has padding. The following code is a subject to UB."); + const char *asciiDescription = data.constData() + tagEntry.offset + sizeof(GenericTagData) + + sizeof(quint32_be); + if (asciiDescription[len - 1] != '\0') return false; - descName = QString::fromLatin1(desc->asciiDescription, len - 1); + descName = QString::fromLatin1(asciiDescription, len - 1); return true; } - if (tag->type != quint32(Tag::mluc)) + if (tag.type != quint32(Tag::mluc)) return false; if (tagEntry.size < sizeof(MlucTagData)) return false; - const MlucTagData *mluc = (const MlucTagData *)(data.constData() + tagEntry.offset); - if (mluc->recordCount < 1) + const MlucTagData mluc = qFromUnaligned(data.constData() + tagEntry.offset); + if (mluc.recordCount < 1) return false; - if (mluc->recordSize < 12) + if (mluc.recordSize < 12) return false; // We just use the primary record regardless of language or country. - const quint32 stringOffset = mluc->records[0].offset; - const quint32 stringSize = mluc->records[0].size; + const quint32 stringOffset = mluc.records[0].offset; + const quint32 stringSize = mluc.records[0].size; if (tagEntry.size < stringOffset || tagEntry.size - stringOffset < stringSize ) return false; if ((stringSize | stringOffset) & 1) return false; quint32 stringLen = stringSize / 2; - const ushort *unicodeString = (const ushort *)(data.constData() + tagEntry.offset + stringOffset); + QVarLengthArray utf16hostendian(stringLen); + qFromBigEndian(data.constData() + tagEntry.offset + stringOffset, stringLen, + utf16hostendian.data()); // The given length shouldn't include 0-termination, but might. - if (stringLen > 1 && unicodeString[stringLen - 1] == 0) + if (stringLen > 1 && utf16hostendian[stringLen - 1] == 0) --stringLen; - QVarLengthArray utf16hostendian(stringLen); - qFromBigEndian(unicodeString, stringLen, utf16hostendian.data()); descName = QString::fromUtf16(utf16hostendian.data(), stringLen); return true; } bool fromIccProfile(const QByteArray &data, QColorSpace *colorSpace) { - Q_ASSERT((reinterpret_cast(data.constData()) & 0x3) == 0); - if (reinterpret_cast(data.constData()) & 0x3) { - qCWarning(lcIcc) << "fromIccProfile: Unaligned profile data"; - return false; - } if (data.size() < qsizetype(sizeof(ICCProfileHeader))) { qCWarning(lcIcc) << "fromIccProfile: failed size sanity 1"; return false; } - const ICCProfileHeader *header = (const ICCProfileHeader *)data.constData(); - if (!isValidIccProfile(*header)) + const ICCProfileHeader header = qFromUnaligned(data.constData()); + if (!isValidIccProfile(header)) return false; // if failed we already printing a warning - if (qsizetype(header->profileSize) > data.size()) { + if (qsizetype(header.profileSize) > data.size()) { qCWarning(lcIcc) << "fromIccProfile: failed size sanity 2"; return false; } - // Read tag index - const TagTableEntry *tagTable = (const TagTableEntry *)(data.constData() + sizeof(ICCProfileHeader)); - const qsizetype offsetToData = sizeof(ICCProfileHeader) + header->tagCount * sizeof(TagTableEntry); + const qsizetype offsetToData = sizeof(ICCProfileHeader) + header.tagCount * sizeof(TagTableEntry); Q_ASSERT(offsetToData > 0); if (offsetToData > data.size()) { qCWarning(lcIcc) << "fromIccProfile: failed index size sanity"; @@ -641,37 +656,42 @@ bool fromIccProfile(const QByteArray &data, QColorSpace *colorSpace) } QHash tagIndex; - for (uint i = 0; i < header->tagCount; ++i) { + for (uint i = 0; i < header.tagCount; ++i) { + // Read tag index + const qsizetype tableOffset = sizeof(ICCProfileHeader) + i * sizeof(TagTableEntry); + const TagTableEntry tagTable = qFromUnaligned(data.constData() + + tableOffset); + // Sanity check tag sizes and offsets: - if (qsizetype(tagTable[i].offset) < offsetToData) { + if (qsizetype(tagTable.offset) < offsetToData) { qCWarning(lcIcc) << "fromIccProfile: failed tag offset sanity 1"; return false; } // Checked separately from (+ size) to handle overflow. - if (tagTable[i].offset > header->profileSize) { + if (tagTable.offset > header.profileSize) { qCWarning(lcIcc) << "fromIccProfile: failed tag offset sanity 2"; return false; } - if (tagTable[i].size < 12) { + if (tagTable.size < 12) { qCWarning(lcIcc) << "fromIccProfile: failed minimal tag size sanity"; return false; } - if (tagTable[i].size > header->profileSize - tagTable[i].offset) { + if (tagTable.size > header.profileSize - tagTable.offset) { qCWarning(lcIcc) << "fromIccProfile: failed tag offset + size sanity"; return false; } - if (tagTable[i].offset & 0x03) { + if (tagTable.offset & 0x03) { qCWarning(lcIcc) << "fromIccProfile: invalid tag offset alignment"; return false; } -// printf("'%4s' %d %d\n", (const char *)&tagTable[i].signature, -// quint32(tagTable[i].offset), -// quint32(tagTable[i].size)); - tagIndex.insert(Tag(quint32(tagTable[i].signature)), { tagTable[i].offset, tagTable[i].size }); +// printf("'%4s' %d %d\n", (const char *)&tagTable.signature, +// quint32(tagTable.offset), +// quint32(tagTable.size)); + tagIndex.insert(Tag(quint32(tagTable.signature)), { tagTable.offset, tagTable.size }); } // Check the profile is three-component matrix based (what we currently support): - if (header->inputColorSpace == uint(ColorSpaceType::Rgb)) { + if (header.inputColorSpace == uint(ColorSpaceType::Rgb)) { if (!tagIndex.contains(Tag::rXYZ) || !tagIndex.contains(Tag::gXYZ) || !tagIndex.contains(Tag::bXYZ) || !tagIndex.contains(Tag::rTRC) || !tagIndex.contains(Tag::gTRC) || !tagIndex.contains(Tag::bTRC) || !tagIndex.contains(Tag::wtpt)) { @@ -679,7 +699,7 @@ bool fromIccProfile(const QByteArray &data, QColorSpace *colorSpace) return false; } } else { - Q_ASSERT(header->inputColorSpace == uint(ColorSpaceType::Gray)); + Q_ASSERT(header.inputColorSpace == uint(ColorSpaceType::Gray)); if (!tagIndex.contains(Tag::kTRC) || !tagIndex.contains(Tag::wtpt)) { qCWarning(lcIcc) << "fromIccProfile: Invalid ICC profile - not valid gray scale based"; return false; @@ -688,7 +708,7 @@ bool fromIccProfile(const QByteArray &data, QColorSpace *colorSpace) QColorSpacePrivate *colorspaceDPtr = QColorSpacePrivate::getWritable(*colorSpace); - if (header->inputColorSpace == uint(ColorSpaceType::Rgb)) { + if (header.inputColorSpace == uint(ColorSpaceType::Rgb)) { // Parse XYZ tags if (!parseXyzData(data, tagIndex[Tag::rXYZ], colorspaceDPtr->toXyz.r)) return false; @@ -748,7 +768,7 @@ bool fromIccProfile(const QByteArray &data, QColorSpace *colorSpace) TagEntry rTrc; TagEntry gTrc; TagEntry bTrc; - if (header->inputColorSpace == uint(ColorSpaceType::Gray)) { + if (header.inputColorSpace == uint(ColorSpaceType::Gray)) { rTrc = tagIndex[Tag::kTRC]; gTrc = tagIndex[Tag::kTRC]; bTrc = tagIndex[Tag::kTRC]; -- cgit v1.2.3