1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QHeaderView: fix more UB (signed integer overflow) in setOffset()

Browse Source 
We fixed the first line of defense in
03d1e81516be9af37fa08900f9a2d88d34abc4df, but that commit didn't rule
out ndelta == INT_MIN, in which case -ndelta overflows a few lines
below.

Coverity pointed this out.

Add a check that exposes this problem to ubsan, and avoid the overflow
by using qMulOverflow<-1>()¹ and not scrolling when it overflows, but
emitting a qWarning().

¹ There's no qNegateOverflow()...

When state == QHeaderViewPrivate::ResizeSection, we assume that
everything happens on the actual screen, which has physical limits to
the setOffset() argument, and therefore these arithmetic operations
don't need to be protected.

I fully expect that this will just be a rat's tail, one we can only
hope to control by using Peppe's safe integers everywhere, at which
point we've probably blown our executable code size out of any
proportions. So leave it at this, for the time being.

Amends 03d1e81516be9af37fa08900f9a2d88d34abc4df.

Coverity-Id: 479557
Pick-to: 6.5
Change-Id: I2e31fc9be21e7d59563b67f3cd26c29dcea61b55
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>
(cherry picked from commit 49fcac99deea390901000a74deea1c0c690b6ae2)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit f98c49666a518df3ac182e1f4920b581d1a6bda7)
This commit is contained in:
Marc Mutz 2025-03-31 14:27:10 +02:00 committed by Qt Cherry-pick Bot
parent 45e8c81c52
commit dd934d675a
2 changed files with 46 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/widgets/itemviews/qheaderview.cpp
View File

@ -34,6 +34,14 @@
QT_BEGIN_NAMESPACE
Q_DECL_COLD_FUNCTION
static void warn_overflow(const char *caller, const char *callee, int value)
{
qWarning("Integer argument %d causes overflow in %s when calling %s, "
"results may not be as you expect",
value, caller, callee);
}
#ifndef QT_NO_DATASTREAM
QDataStream &operator<<(QDataStream &out, const QHeaderViewPrivate::SectionItem &section)
{
@ -408,10 +416,18 @@ void QHeaderView::setOffset(int newOffset)
// don't overflow; this function is checked with both INT_MIN and INT_MAX...
const int ndelta = q26::saturate_cast<int>(d->headerOffset - qint64{newOffset});
d->headerOffset = newOffset;
if (d->orientation == Qt::Horizontal)
d->viewport->scroll(isRightToLeft() ? -ndelta : ndelta, 0);
if (d->orientation == Qt::Horizontal) {
if (isLeftToRight()) {
if (int r; !qMulOverflow<-1>(ndelta, &r))
d->viewport->scroll(r, 0);
else
warn_overflow("QHeaderView::setOffset", "QWidget::scroll", newOffset);
} else {
d->viewport->scroll(ndelta, 0);
}
} else {
d->viewport->scroll(0, ndelta);
}
if (d->state == QHeaderViewPrivate::ResizeSection && !d->preventCursorChangeInSetOffset) {
QPoint cursorPos = QCursor::pos();
if (d->orientation == Qt::Horizontal)

27
tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp
View File

@ -381,6 +381,22 @@ public:
bool m_bMultiLine = false;
};
static Qt::LayoutDirection otherLayoutDirection(Qt::LayoutDirection current)
{
switch (current) {
case Qt::LayoutDirection::LeftToRight: return Qt::LayoutDirection::RightToLeft;
case Qt::LayoutDirection::RightToLeft: return Qt::LayoutDirection::LeftToRight;
case Qt::LayoutDirection::LayoutDirectionAuto:
;
}
Q_UNREACHABLE_RETURN(Qt::LayoutDirection::LayoutDirectionAuto);
}
static void swapLayoutDirection(QWidget &w)
{
w.setLayoutDirection(otherLayoutDirection(w.layoutDirection()));
}
// Testing get/set functions
void tst_QHeaderView::getSetCheck()
{
@ -426,9 +442,20 @@ void tst_QHeaderView::getSetCheck()
QCOMPARE(0, obj1.offset());
obj1.setOffset(std::numeric_limits<int>::min());
QCOMPARE(std::numeric_limits<int>::min(), obj1.offset());
QTest::ignoreMessage(QtWarningMsg, // one of the INT_MAX will hit this; not necessarily the first one
"Integer argument 2147483647 causes overflow in QHeaderView::setOffset "
"when calling QWidget::scroll, results may not be as you expect");
obj1.setOffset(std::numeric_limits<int>::max());
QCOMPARE(std::numeric_limits<int>::max(), obj1.offset());
// and again with RTL (or LTR, if it was RTL before):
swapLayoutDirection(obj1);
obj1.setOffset(0);
QCOMPARE(0, obj1.offset());
obj1.setOffset(std::numeric_limits<int>::min());
QCOMPARE(std::numeric_limits<int>::min(), obj1.offset());
obj1.setOffset(std::numeric_limits<int>::max());
QCOMPARE(std::numeric_limits<int>::max(), obj1.offset());
}
tst_QHeaderView::tst_QHeaderView()