Skip to content

Fix various UB

  • Patch 1 required conflict resolution because context had changed (still straightforward, but please check)
  • Patch 4 required using private function add_overflow because qAddOverflow only exists in >=6.1

[PATCH 1/5] QVarLengthArray: fix UB (precondition violation) in range-erase()

When the range-erase() function is called with an empty, valid range,
it passed equal iterators to both the first and the last arguments of
std::move(f, l, d) (the algorithm, not the cast). This is UB, since it
violates the algorithm's precondition that d ∉ [f,l). For non-empty
ranges, f > d, thus d < f, hence d ∉ [f,l), so everything is ok
_there_.

Fix the empty range case by returning early.

Reviewers may question the precondition, expecting that std::move(f,
l, f) just be a no-op, but the algorithm, as specified, performs
self-move-assignments in that case, which need not be supported. In
fact, the Hinnant criterion, itself only applicable if one calls for
self-swap-safety, asks for self-move-assignment-safety only for
objects in the moved-from state. QVarLengthArray, itself, meets only
the Hinnant criterion; self-move-assignment of non-empty QVLAs invokes
UB.

So, the UB here is real.

Qt 5.15 uses std::copy() here, which has the same precondition as
std::move(), and the same fix applies there, too.

[ChangeLog][QtCore][QVarLengthArray] Fixed a bug where range-erase()
could invoke undefined behavior when called with an empty range.

Pick-to: 6.2 5.15
Change-Id: I656aa09d025168d6d9ef088ad4c6954d216f0d54
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 0800947d7d2127eacaabf66ee1702d4980897041)

[PATCH 2/5] tst_QIODevice: fix UB (precondition violation) in SequentialReadBuffer::readData()

memcpy() mustn't be called with a nullptr, even if the size is zero.

Fixes ubsan error:

	tst_qiodevice.cpp:561:15: runtime error: null pointer passed as argument 1, which is declared to never be null

Even though ubsan only complained about one of them, fix all three
occurrences of the pattern in the test.

Pick-to: 6.3 6.2 5.15
Change-Id: I5c06ab4a20a9e9f8831392c46c6969c05248fdac
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 572b55baa42787447643169811784bc64024fb5e)

[PATCH 3/5] QString: fix UB (pointer arithmetic on nullptr) in qLastIndexOf

Says ubsan:

	qstring.cpp:10484:17: runtime error: applying non-zero offset 18446744073709551614 to null pointer

If we search for a null needle, we stored 0-1 in a size_t variable and
unconditionally appied that offset to the needle's data() pointer. That
being the nullptr, ubsan complained.

To fix, set sl_minus_1 to 0 if it would underflow. In that case,
sl_minus_1, n, and h, are not used, anyway, so their values don't
matter as long as we don't invoke UB.

Pick-to: 6.3 6.2 5.15
Change-Id: Idca4e845c77838dfc84acdb68bbbc98382b5e1d5
Reviewed-by: Sona Kurazyan <sona.kurazyan@qt.io>
Reviewed-by: Anton Kudryavtsev <antkudr@mail.ru>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 6830bdc1401e55680859b74036e9e9d90c359028)

[PATCH 4/5] QDateTime: fix UB (signed overflow) in addDays()

QDateTime: fix UB (signed overflow) in addDays()

The comment indicated that the author expected any overflow to be
caught by a bounds check in the subsequent function, however, signed
overflow is UB, so anything can happen.

Fix by using our API for safe additions instead.

* asturmlechner 2021-12-29: use add_overflow instead of qAddOverflow

Pick-to: 6.3 6.2 5.15
Change-Id: I41909defffa5305b02fdfcf6d5808e0d9fd5924f
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit c47c98ea2b8ec9e8bda51d86f3168bba28c3291a)

[PATCH 5/5] QVariantAnimation: fix UB (FP 0/0) in interpolated() arg calculation

When startProgress, endProgress, and progress were all 0 (as provoked
by tst_QPropertyAnimation::startWithoutStartValue()), we'd calculate
0/0 and ubsan complained:

	qvariantanimation.cpp:284:60: runtime error: division by zero

Fix by detecting progress - startProgress == 0 and setting
localProgress = 0.0 in that case. This is a logical result, even
though it might not be what IEEE754 rules would have yielded.

A more comprehensive change that aims to reliably keep localProgress
∈ [0,1] and thus avoid the infinities when endProgress ==
startProgress, is outside the scope of this patch, which deals only
with the UBSan error.

Pick-to: 6.3 6.2 5.15
Change-Id: I5258b054a2060006795f49fb1cd7604aea3ed46b
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
Reviewed-by: Jan Arve Sæther <jan-arve.saether@qt.io>
(cherry picked from commit 52da10f64542a6c4fda06be1b88e9111331f6500)
Edited by Andreas Sturmlechner

Merge request reports