Skip to content

[3 patches] UB: Assert validity in QStringIterator's unchecked methods / use has{Next,Previous}() more / use std::less to compare pointers

Andreas Sturmlechner requested to merge asturmlechner/qtbase:asturmlechner/QStringIterator-UB into kde/5.15

[PATCH 1/3] Assert validity in QStringIterator's unchecked methods

These methods should never be used on strings not known to be valid UTF-16.
Their optimizations will produce undefined behavior otherwise.

Change-Id: I03a95140dcbdd1f7189eea1be69289ce227331a5
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
Reviewed-by: Konstantin Ritt <ritt.ks@gmail.com>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 7715b186c79a968950fd1c8e0993641f2f58b3be)

[PATCH 2/3] QStringIterator: fix UB [1/2]: use has{Next,Previous}() more

Replace

- pos > i with hasPrevious()
- pos < e with hasNext()

Everything is inline, so there's no difference in assembly, but less
source code lines to fix later.

Pick-to: 6.4 6.3 6.2 5.15
Change-Id: I3f9cf2716c96b811b29b75fa20f88cc3b461771a
Reviewed-by: Mate Barany <mate.barany@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
(cherry picked from commit 34800d1f09447e921203561c0e4804c4f095136f)

[PATCH 3/3] QStringIterator: fix UB [2/2]: use std::less to compare pointers

The relational operators <, >, <=, >= do not form a total order for
pointers. In particular, they are only defined for pointer pairs that
point into the same array (incl. one past the array's last element).

Other uses are undefined behavior.

The iterators the class works on are user-supplied, so we cannot
assume that they are pointing into the same array. In fact, we have a
check in setPosition() that tries to catch such misuses, but similar
checks are absent from the ctors, e.g.

In addition, the implementation itself is checking for next-but-one
and prev-but-one in an unsafe way.

std::less can be used to provide a total order, so use that
everywhere.

Pick-to: 6.4 6.3 6.2 5.15
Change-Id: I32fb0ef9f768d9336ae12cc0542ca18d1bf23adf
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
(cherry picked from commit 7cf2684e6a11892975946bd57d8cf672fe6dd43e)

* asturmlechner 2022-09-17: Resolve conflict with dev branch commit
  3e1d03b1eaf6a5e842bed4ae4f9bb8cca05e5b31

* asturmlechner 2022-09-17: Fix build with c++11 (add missing
  std::less template argument)

Merge request reports