MSVSphere Packaging Team
9 months ago
parent
93d9f73244
commit
efd9328d56
@ -0,0 +1,38 @@
|
||||
From ea63c28efc1d2ecb467b83a34923d12462efa96f Mon Sep 17 00:00:00 2001
|
||||
From: Marc Mutz <marc.mutz@qt.io>
|
||||
Date: Tue, 12 Dec 2023 20:51:56 +0100
|
||||
Subject: [PATCH] HPack: fix a Yoda Condition
|
||||
|
||||
Putting the variable on the LHS of a relational operation makes the
|
||||
expression easier to read. In this case, we find that the whole
|
||||
expression is nonsensical as an overflow protection, because if
|
||||
name.size() + value.size() overflows, the result will exactly _not_
|
||||
be > max() - 32, because UB will have happened.
|
||||
|
||||
To be fixed in a follow-up commit.
|
||||
|
||||
As a drive-by, add parentheses around the RHS.
|
||||
|
||||
Change-Id: I35ce598884c37c51b74756b3bd2734b9aad63c09
|
||||
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
|
||||
(cherry picked from commit 658607a34ead214fbacbc2cca44915655c318ea9)
|
||||
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
|
||||
(cherry picked from commit 4f7efd41740107f90960116700e3134f5e433867)
|
||||
(cherry picked from commit 13c16b756900fe524f6d9534e8a07aa003c05e0c)
|
||||
(cherry picked from commit 1d4788a39668fb2dc5912a8d9c4272dc40e99f92)
|
||||
(cherry picked from commit 87de75b5cc946d196decaa6aef4792a6cac0b6db)
|
||||
---
|
||||
|
||||
diff --git a/src/network/access/http2/hpacktable.cpp b/src/network/access/http2/hpacktable.cpp
|
||||
index 834214f..ab166a6 100644
|
||||
--- a/src/network/access/http2/hpacktable.cpp
|
||||
+++ b/src/network/access/http2/hpacktable.cpp
|
||||
@@ -63,7 +63,7 @@
|
||||
// 32 octets of overhead."
|
||||
|
||||
const unsigned sum = unsigned(name.size() + value.size());
|
||||
- if (std::numeric_limits<unsigned>::max() - 32 < sum)
|
||||
+ if (sum > (std::numeric_limits<unsigned>::max() - 32))
|
||||
return HeaderSize();
|
||||
return HeaderSize(true, quint32(sum + 32));
|
||||
}
|
||||
@ -0,0 +1,59 @@
|
||||
From 23c3fc483e8b6e21012a61f0bea884446f727776 Mon Sep 17 00:00:00 2001
|
||||
From: Marc Mutz <marc.mutz@qt.io>
|
||||
Date: Tue, 12 Dec 2023 22:08:07 +0100
|
||||
Subject: [PATCH] HPack: fix incorrect integer overflow check
|
||||
|
||||
This code never worked:
|
||||
|
||||
For the comparison with max() - 32 to trigger, on 32-bit platforms (or
|
||||
Qt 5) signed interger overflow would have had to happen in the
|
||||
addition of the two sizes. The compiler can therefore remove the
|
||||
overflow check as dead code.
|
||||
|
||||
On Qt 6 and 64-bit platforms, the signed integer addition would be
|
||||
very unlikely to overflow, but the following truncation to uint32
|
||||
would yield the correct result only in a narrow 32-value window just
|
||||
below UINT_MAX, if even that.
|
||||
|
||||
Fix by using the proper tool, qAddOverflow.
|
||||
|
||||
Manual conflict resolutions:
|
||||
- qAddOverflow doesn't exist in Qt 5, use private add_overflow
|
||||
predecessor API instead
|
||||
|
||||
Change-Id: I7599f2e75ff7f488077b0c60b81022591005661c
|
||||
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
|
||||
(cherry picked from commit ee5da1f2eaf8932aeca02ffea6e4c618585e29e3)
|
||||
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
|
||||
(cherry picked from commit debeb8878da2dc706ead04b6072ecbe7e5313860)
|
||||
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
|
||||
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
|
||||
(cherry picked from commit 811b9eef6d08d929af8708adbf2a5effb0eb62d7)
|
||||
(cherry picked from commit f931facd077ce945f1e42eaa3bead208822d3e00)
|
||||
(cherry picked from commit 9ef4ca5ecfed771dab890856130e93ef5ceabef5)
|
||||
Reviewed-by: MÃ¥rten Nordheim <marten.nordheim@qt.io>
|
||||
---
|
||||
|
||||
diff --git a/src/network/access/http2/hpacktable.cpp b/src/network/access/http2/hpacktable.cpp
|
||||
index ab166a6..de91fc0 100644
|
||||
--- a/src/network/access/http2/hpacktable.cpp
|
||||
+++ b/src/network/access/http2/hpacktable.cpp
|
||||
@@ -40,6 +40,7 @@
|
||||
#include "hpacktable_p.h"
|
||||
|
||||
#include <QtCore/qdebug.h>
|
||||
+#include <QtCore/private/qnumeric_p.h>
|
||||
|
||||
#include <algorithm>
|
||||
#include <cstddef>
|
||||
@@ -62,7 +63,9 @@
|
||||
// for counting the number of references to the name and value would have
|
||||
// 32 octets of overhead."
|
||||
|
||||
- const unsigned sum = unsigned(name.size() + value.size());
|
||||
+ size_t sum;
|
||||
+ if (add_overflow(size_t(name.size()), size_t(value.size()), &sum))
|
||||
+ return HeaderSize();
|
||||
if (sum > (std::numeric_limits<unsigned>::max() - 32))
|
||||
return HeaderSize();
|
||||
return HeaderSize(true, quint32(sum + 32));
|
||||
@ -0,0 +1,197 @@
|
||||
diff --git a/src/gui/util/qktxhandler.cpp b/src/gui/util/qktxhandler.cpp
|
||||
index 0d98e97453..6a79e55109 100644
|
||||
--- a/src/gui/util/qktxhandler.cpp
|
||||
+++ b/src/gui/util/qktxhandler.cpp
|
||||
@@ -73,7 +73,7 @@ struct KTXHeader {
|
||||
quint32 bytesOfKeyValueData;
|
||||
};
|
||||
|
||||
-static const quint32 headerSize = sizeof(KTXHeader);
|
||||
+static constexpr quint32 qktxh_headerSize = sizeof(KTXHeader);
|
||||
|
||||
// Currently unused, declared for future reference
|
||||
struct KTXKeyValuePairItem {
|
||||
@@ -103,11 +103,36 @@ struct KTXMipmapLevel {
|
||||
*/
|
||||
};
|
||||
|
||||
-bool QKtxHandler::canRead(const QByteArray &suffix, const QByteArray &block)
|
||||
+static bool qAddOverflow(quint32 v1, quint32 v2, quint32 *r) {
|
||||
+ // unsigned additions are well-defined
|
||||
+ *r = v1 + v2;
|
||||
+ return v1 > quint32(v1 + v2);
|
||||
+}
|
||||
+
|
||||
+// Returns the nearest multiple of 4 greater than or equal to 'value'
|
||||
+static bool nearestMultipleOf4(quint32 value, quint32 *result)
|
||||
+{
|
||||
+ constexpr quint32 rounding = 4;
|
||||
+ *result = 0;
|
||||
+ if (qAddOverflow(value, rounding - 1, result))
|
||||
+ return true;
|
||||
+ *result &= ~(rounding - 1);
|
||||
+ return false;
|
||||
+}
|
||||
+
|
||||
+// Returns a slice with prechecked bounds
|
||||
+static QByteArray safeSlice(const QByteArray& array, quint32 start, quint32 length)
|
||||
{
|
||||
- Q_UNUSED(suffix)
|
||||
+ quint32 end = 0;
|
||||
+ if (qAddOverflow(start, length, &end) || end > quint32(array.length()))
|
||||
+ return {};
|
||||
+ return QByteArray(array.data() + start, length);
|
||||
+}
|
||||
|
||||
- return (qstrncmp(block.constData(), ktxIdentifier, KTX_IDENTIFIER_LENGTH) == 0);
|
||||
+bool QKtxHandler::canRead(const QByteArray &suffix, const QByteArray &block)
|
||||
+{
|
||||
+ Q_UNUSED(suffix);
|
||||
+ return block.startsWith(QByteArray::fromRawData(ktxIdentifier, KTX_IDENTIFIER_LENGTH));
|
||||
}
|
||||
|
||||
QTextureFileData QKtxHandler::read()
|
||||
@@ -115,42 +140,97 @@ QTextureFileData QKtxHandler::read()
|
||||
if (!device())
|
||||
return QTextureFileData();
|
||||
|
||||
- QByteArray buf = device()->readAll();
|
||||
- const quint32 dataSize = quint32(buf.size());
|
||||
- if (dataSize < headerSize || !canRead(QByteArray(), buf)) {
|
||||
- qCDebug(lcQtGuiTextureIO, "Invalid KTX file %s", logName().constData());
|
||||
+ const QByteArray buf = device()->readAll();
|
||||
+ if (size_t(buf.size()) > std::numeric_limits<quint32>::max()) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Too big KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ if (!canRead(QByteArray(), buf)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Invalid KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ if (buf.size() < qsizetype(qktxh_headerSize)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Invalid KTX header size in %s", logName().constData());
|
||||
return QTextureFileData();
|
||||
}
|
||||
|
||||
- const KTXHeader *header = reinterpret_cast<const KTXHeader *>(buf.constData());
|
||||
- if (!checkHeader(*header)) {
|
||||
- qCDebug(lcQtGuiTextureIO, "Unsupported KTX file format in %s", logName().constData());
|
||||
+ KTXHeader header;
|
||||
+ memcpy(&header, buf.data(), qktxh_headerSize);
|
||||
+ if (!checkHeader(header)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Unsupported KTX file format in %s", logName().constData());
|
||||
return QTextureFileData();
|
||||
}
|
||||
|
||||
QTextureFileData texData;
|
||||
texData.setData(buf);
|
||||
|
||||
- texData.setSize(QSize(decode(header->pixelWidth), decode(header->pixelHeight)));
|
||||
- texData.setGLFormat(decode(header->glFormat));
|
||||
- texData.setGLInternalFormat(decode(header->glInternalFormat));
|
||||
- texData.setGLBaseInternalFormat(decode(header->glBaseInternalFormat));
|
||||
-
|
||||
- texData.setNumLevels(decode(header->numberOfMipmapLevels));
|
||||
- quint32 offset = headerSize + decode(header->bytesOfKeyValueData);
|
||||
- const int maxLevels = qMin(texData.numLevels(), 32); // Cap iterations in case of corrupt file.
|
||||
- for (int i = 0; i < maxLevels; i++) {
|
||||
- if (offset + sizeof(KTXMipmapLevel) > dataSize) // Corrupt file; avoid oob read
|
||||
- break;
|
||||
- const KTXMipmapLevel *level = reinterpret_cast<const KTXMipmapLevel *>(buf.constData() + offset);
|
||||
- quint32 levelLen = decode(level->imageSize);
|
||||
- texData.setDataOffset(offset + sizeof(KTXMipmapLevel::imageSize), i);
|
||||
- texData.setDataLength(levelLen, i);
|
||||
- offset += sizeof(KTXMipmapLevel::imageSize) + levelLen + (3 - ((levelLen + 3) % 4));
|
||||
+ texData.setSize(QSize(decode(header.pixelWidth), decode(header.pixelHeight)));
|
||||
+ texData.setGLFormat(decode(header.glFormat));
|
||||
+ texData.setGLInternalFormat(decode(header.glInternalFormat));
|
||||
+ texData.setGLBaseInternalFormat(decode(header.glBaseInternalFormat));
|
||||
+
|
||||
+ texData.setNumLevels(decode(header.numberOfMipmapLevels));
|
||||
+
|
||||
+ const quint32 bytesOfKeyValueData = decode(header.bytesOfKeyValueData);
|
||||
+ quint32 headerKeyValueSize;
|
||||
+ if (qAddOverflow(qktxh_headerSize, bytesOfKeyValueData, &headerKeyValueSize)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Overflow in size of key value data in header of KTX file %s",
|
||||
+ logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ if (headerKeyValueSize >= quint32(buf.size())) {
|
||||
+ qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ // Technically, any number of levels is allowed but if the value is bigger than
|
||||
+ // what is possible in KTX V2 (and what makes sense) we return an error.
|
||||
+ // maxLevels = log2(max(width, height, depth))
|
||||
+ const int maxLevels = (sizeof(quint32) * 8)
|
||||
+ - qCountLeadingZeroBits(std::max(
|
||||
+ { header.pixelWidth, header.pixelHeight, header.pixelDepth }));
|
||||
+
|
||||
+ if (texData.numLevels() > maxLevels) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Too many levels in KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ quint32 offset = headerKeyValueSize;
|
||||
+ for (int level = 0; level < texData.numLevels(); level++) {
|
||||
+ const auto imageSizeSlice = safeSlice(buf, offset, sizeof(quint32));
|
||||
+ if (imageSizeSlice.isEmpty()) {
|
||||
+ qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ const quint32 imageSize = decode(qFromUnaligned<quint32>(imageSizeSlice.data()));
|
||||
+ offset += sizeof(quint32); // overflow checked indirectly above
|
||||
+
|
||||
+ texData.setDataOffset(offset, level);
|
||||
+ texData.setDataLength(imageSize, level);
|
||||
+
|
||||
+ // Add image data and padding to offset
|
||||
+ quint32 padded = 0;
|
||||
+ if (nearestMultipleOf4(imageSize, &padded)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "Overflow in KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ quint32 offsetNext;
|
||||
+ if (qAddOverflow(offset, padded, &offsetNext)) {
|
||||
+ qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData());
|
||||
+ return QTextureFileData();
|
||||
+ }
|
||||
+
|
||||
+ offset = offsetNext;
|
||||
}
|
||||
|
||||
if (!texData.isValid()) {
|
||||
- qCDebug(lcQtGuiTextureIO, "Invalid values in header of KTX file %s", logName().constData());
|
||||
+ qWarning(lcQtGuiTextureIO, "Invalid values in header of KTX file %s",
|
||||
+ logName().constData());
|
||||
return QTextureFileData();
|
||||
}
|
||||
|
||||
@@ -191,7 +271,7 @@ bool QKtxHandler::checkHeader(const KTXHeader &header)
|
||||
(decode(header.numberOfFaces) == 1));
|
||||
}
|
||||
|
||||
-quint32 QKtxHandler::decode(quint32 val)
|
||||
+quint32 QKtxHandler::decode(quint32 val) const
|
||||
{
|
||||
return inverseEndian ? qbswap<quint32>(val) : val;
|
||||
}
|
||||
diff --git a/src/gui/util/qktxhandler_p.h b/src/gui/util/qktxhandler_p.h
|
||||
index f831e59d95..cdf1b2eaf8 100644
|
||||
--- a/src/gui/util/qktxhandler_p.h
|
||||
+++ b/src/gui/util/qktxhandler_p.h
|
||||
@@ -68,7 +68,7 @@ public:
|
||||
|
||||
private:
|
||||
bool checkHeader(const KTXHeader &header);
|
||||
- quint32 decode(quint32 val);
|
||||
+ quint32 decode(quint32 val) const;
|
||||
|
||||
bool inverseEndian = false;
|
||||
};
|
||||
Loading…
Reference in new issue