QLatin1String: Fix UB (nullptr passed) in relational operators

Found by UBSan: qstring.h:1160:44: runtime error: null pointer passed as argument 1, which is declared to never be null qstring.h:1160:44: runtime error: null pointer passed as argument 2, which is declared to never be null Fix by avoiding the memcmp() calls if there's a chance that they might be called with nullptr. While at it, also implement !=, >, <=, >= in terms of ==, <, and add a test, because this particular UB was not fingered by any of the QtCore test cases, but by a Qt3D one. Change-Id: I413792dcc8431ef14f0c79f26e89a3e9fab69465 Reviewed-by: Thiago Macieira <[email protected]> Reviewed-by: Edward Welbourne <[email protected]>
RDevCore · Sep 15, 2016 · a54d442 · a54d442
1 parent 7529c39
commit a54d442
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 11 deletions.
diff --git a/src/corelib/tools/qstring.h b/src/corelib/tools/qstring.h
@@ -1127,21 +1127,21 @@ inline bool operator!=(QString::Null, const QString &s) { return !s.isNull(); }
 inline bool operator!=(const QString &s, QString::Null) { return !s.isNull(); }
 
 inline bool operator==(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ return (s1.size() == s2.size() && !memcmp(s1.latin1(), s2.latin1(), s1.size())); }
+{ return s1.size() == s2.size() && (!s1.size() || !memcmp(s1.latin1(), s2.latin1(), s1.size())); }
 inline bool operator!=(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ return (s1.size() != s2.size() || memcmp(s1.latin1(), s2.latin1(), s1.size())); }
+{ return !operator==(s1, s2); }
 inline bool operator<(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ int r = memcmp(s1.latin1(), s2.latin1(), qMin(s1.size(), s2.size()));
-  return (r < 0) || (r == 0 && s1.size() < s2.size()); }
-inline bool operator<=(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ int r = memcmp(s1.latin1(), s2.latin1(), qMin(s1.size(), s2.size()));
-  return (r < 0) || (r == 0 && s1.size() <= s2.size()); }
+{
+    const int len = qMin(s1.size(), s2.size());
+    const int r = len ? memcmp(s1.latin1(), s2.latin1(), len) : 0;
+    return r < 0 || (r == 0 && s1.size() < s2.size());
+}
 inline bool operator>(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ int r = memcmp(s1.latin1(), s2.latin1(), qMin(s1.size(), s2.size()));
-  return (r > 0) || (r == 0 && s1.size() > s2.size()); }
+{ return operator<(s2, s1); }
+inline bool operator<=(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
+{ return !operator>(s1, s2); }
 inline bool operator>=(QLatin1String s1, QLatin1String s2) Q_DECL_NOTHROW
-{ int r = memcmp(s1.latin1(), s2.latin1(), qMin(s1.size(), s2.size()));
-  return (r > 0) || (r == 0 && s1.size() >= s2.size()); }
+{ return !operator<(s1, s2); }
 
 inline bool QLatin1String::operator==(const QString &s) const Q_DECL_NOTHROW
 { return s == *this; }

diff --git a/tests/auto/corelib/tools/qlatin1string/tst_qlatin1string.cpp b/tests/auto/corelib/tools/qlatin1string/tst_qlatin1string.cpp
@@ -35,13 +35,24 @@
 
 #include <QString>
 
+// Preserve QLatin1String-ness (QVariant(QLatin1String) creates a QVariant::String):
+struct QLatin1StringContainer {
+    QLatin1String l1;
+};
+QT_BEGIN_NAMESPACE
+Q_DECLARE_TYPEINFO(QLatin1StringContainer, Q_MOVABLE_TYPE);
+QT_END_NAMESPACE
+Q_DECLARE_METATYPE(QLatin1StringContainer)
+
 class tst_QLatin1String : public QObject
 {
     Q_OBJECT
 
 private Q_SLOTS:
     void nullString();
     void emptyString();
+    void relationalOperators_data();
+    void relationalOperators();
 };
 
 void tst_QLatin1String::nullString()
@@ -119,7 +130,53 @@ void tst_QLatin1String::emptyString()
     }
 }
 
+void tst_QLatin1String::relationalOperators_data()
+{
+    QTest::addColumn<QLatin1StringContainer>("lhs");
+    QTest::addColumn<int>("lhsOrderNumber");
+    QTest::addColumn<QLatin1StringContainer>("rhs");
+    QTest::addColumn<int>("rhsOrderNumber");
 
+    struct Data {
+        QLatin1String l1;
+        int order;
+    } data[] = {
+        { QLatin1String(),     0 },
+        { QLatin1String(""),   0 },
+        { QLatin1String("a"),  1 },
+        { QLatin1String("aa"), 2 },
+        { QLatin1String("b"),  3 },
+    };
+
+    for (Data *lhs = data; lhs != data + sizeof data / sizeof *data; ++lhs) {
+        for (Data *rhs = data; rhs != data + sizeof data / sizeof *data; ++rhs) {
+            QLatin1StringContainer l = { lhs->l1 }, r = { rhs->l1 };
+            QTest::newRow(qPrintable(QString::asprintf("\"%s\" <> \"%s\"",
+                                                       lhs->l1.data() ? lhs->l1.data() : "nullptr",
+                                                       rhs->l1.data() ? rhs->l1.data() : "nullptr")))
+                << l << lhs->order << r << rhs->order;
+        }
+    }
+}
+
+void tst_QLatin1String::relationalOperators()
+{
+    QFETCH(QLatin1StringContainer, lhs);
+    QFETCH(int, lhsOrderNumber);
+    QFETCH(QLatin1StringContainer, rhs);
+    QFETCH(int, rhsOrderNumber);
+
+#define CHECK(op) \
+    QCOMPARE(lhs.l1 op rhs.l1, lhsOrderNumber op rhsOrderNumber) \
+    /*end*/
+    CHECK(==);
+    CHECK(!=);
+    CHECK(< );
+    CHECK(> );
+    CHECK(<=);
+    CHECK(>=);
+#undef CHECK
+}
 
 QTEST_APPLESS_MAIN(tst_QLatin1String)