From 8922ead397093f3ab6fa5b4de04614c9f6bb4dcc Mon Sep 17 00:00:00 2001
From: Marc Mutz <marc.mutz@qt.io>
Date: Wed, 20 Jul 2022 16:23:50 +0200
Subject: [PATCH] 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.

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)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/corelib/text/qstringiterator_p.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/corelib/text/qstringiterator_p.h b/src/corelib/text/qstringiterator_p.h
index 8d13778186c..bb1a8616378 100644
--- a/src/corelib/text/qstringiterator_p.h
+++ b/src/corelib/text/qstringiterator_p.h
@@ -25,6 +25,8 @@ class QStringIterator
 {
     QString::const_iterator i, pos, e;
     static_assert((std::is_same<QString::const_iterator, const QChar *>::value));
+    static bool less(const QChar *lhs, const QChar *rhs) noexcept
+    { return std::less{}(lhs, rhs); }
 public:
     explicit QStringIterator(QStringView string, qsizetype idx = 0)
         : i(string.begin()),
@@ -59,7 +61,8 @@ public:
 
     inline void setPosition(QString::const_iterator position)
     {
-        Q_ASSERT_X(i <= position && position <= e, Q_FUNC_INFO, "position out of bounds");
+        Q_ASSERT_X(!less(position, i) && !less(e, position),
+                   Q_FUNC_INFO, "position out of bounds");
         pos = position;
     }
 
@@ -67,7 +70,7 @@ public:
 
     inline bool hasNext() const
     {
-        return pos < e;
+        return less(pos, e);
     }
 
     inline void advance()
@@ -95,7 +98,7 @@ public:
         Q_ASSERT_X(hasNext(), Q_FUNC_INFO, "iterator hasn't a next item");
 
         if (Q_UNLIKELY(pos->isHighSurrogate())) {
-            Q_ASSERT(pos + 1 < e && pos[1].isLowSurrogate());
+            Q_ASSERT(less(pos + 1, e) && pos[1].isLowSurrogate());
             return QChar::surrogateToUcs4(pos[0], pos[1]);
         }
 
@@ -148,7 +151,7 @@ public:
 
     inline bool hasPrevious() const
     {
-        return pos > i;
+        return less(i, pos);
     }
 
     inline void recede()
@@ -177,7 +180,7 @@ public:
         Q_ASSERT_X(hasPrevious(), Q_FUNC_INFO, "iterator hasn't a previous item");
 
         if (Q_UNLIKELY(pos[-1].isLowSurrogate())) {
-            Q_ASSERT(pos > i + 1 && pos[-2].isHighSurrogate());
+            Q_ASSERT(less(i + 1, pos) && pos[-2].isHighSurrogate());
             return QChar::surrogateToUcs4(pos[-2], pos[-1]);
         }
         return pos[-1].unicode();