QPointer: don't cause UB when checking for nullptr

QPointer is used to track whether a QObject is still alive, and so
it's unsurprising that checking whether the pointer is null is
sometimes done in situations where the QObject is already in the
process of being destroyed and therefore already demoted from T, the
QPointer template argument, to some base class.

Because of the way we made QPointer SCARY¹, calling data() will cast
from QObject* to T*, which is UB if the T was already demoted.

It's hard to fix this in the general case, but the various ways to
spell isNull() should all be equivalent to isNull() (which does not
cause UB, because it doesn't cast).

This patch fixes the relational operators against nullptr and adds an
explicit operator bool() so that if (p) and if(!p) no longer have to
go through (implicit) operator T*(). This does not appear to cause
disambiguities, so it's SC.

Don't document the operator, it's an implementation detail (and not
documenting it lets us pick it back). A follow-up will add the
documentation for 6.10+.

Add tests, even for stuff that's currently still UB (but marked by an
early return). A follow-up patch will try to make as many of these
other operations non-UB as possible, too.

¹ Originally 3f7741fbe7ab4140f8f971c0cf88bb04e7feea6b, then changed in
  cc7239da8d1ab95e68e12a64df3ca3051419cb34 and
  351c738fc4586bf354c9363fb78e190bdfca4617.

Amends the commits mentioned in the footnote.

[ChangeLog][QtCore][QPointer] For `QPointer<Derived> p`, `!p` and
comparing `p` to nullptr no longer perform invalid downcasts when the
object held in `p` is in the process of being destroyed and has
already been demoted from Derived to one of its base classes. Before,
these expressions invoked data(), which casts from QObject* to
Derived*, a cast which is invalid.

Pick-to: 6.8 6.5 5.15
Task-number: QTBUG-135626
Change-Id: I1b59062345e1b6933958c7e030d9253d69e7591c
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 51cd57116b7465f732253d3f38a2dd78fc704088)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/corelib/kernel/qpointer.h

@@ -84,6 +84,7 @@ public:
 
     bool isNull() const noexcept
     { return wp.isNull(); }
+    explicit operator bool() const noexcept { return !isNull(); }
 
     void clear() noexcept
     { wp.clear(); }
@@ -104,7 +105,7 @@ private:
     Q_DECLARE_EQUALITY_COMPARABLE(QPointer, X*, template <typename X>)
 
     friend bool comparesEqual(const QPointer &lhs, std::nullptr_t) noexcept
-    { return lhs.data() == nullptr; }
+    { return lhs.isNull(); }
     Q_DECLARE_EQUALITY_COMPARABLE(QPointer, std::nullptr_t)
 };
 template <class T> Q_DECLARE_TYPEINFO_BODY(QPointer<T>, Q_RELOCATABLE_TYPE);

tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp

@@ -367,6 +367,7 @@ class DerivedChild : public QObject
 {
     Q_OBJECT
 
+protected:
     DerivedParent *parentPointer;
     QPointer<DerivedParent> guardedParentPointer;
 
@@ -394,6 +395,83 @@ DerivedChild::~DerivedChild()
     QCOMPARE(qobject_cast<DerivedParent *>(guardedParentPointer), parentPointer);
 }
 
+//
+// The FurtherDerived... hierarchiy mimicks QWidget subclasses:
+// Like ~QWidget(), ~DerivedParent() deletes its children before ~QObject() would do it,
+// which would have first set QPointers to nullptr. In this situation, therefore,
+// guardedParentPointers are _not_ nullptr, yet, and the object they point to is already
+// demoted from FurtherDerivedParent to DerivedParent, potentially making some operations
+// UB (invalid downcasts).
+//
+
+class FurtherDerivedParent;
+class FurtherDerivedChild : public DerivedChild
+{
+    Q_OBJECT
+
+    FurtherDerivedParent *parentPointer;
+    QPointer<FurtherDerivedParent> guardedParentPointer;
+public:
+    FurtherDerivedChild(FurtherDerivedParent *);
+    ~FurtherDerivedChild() override;
+};
+
+class FurtherDerivedParent : public DerivedParent
+{
+    Q_OBJECT
+public:
+    FurtherDerivedParent() : DerivedParent()
+    {
+        new FurtherDerivedChild(this);
+    }
+};
+
+FurtherDerivedChild::FurtherDerivedChild(FurtherDerivedParent *parent)
+    : DerivedChild(parent),
+      parentPointer(parent),
+      guardedParentPointer(parent)
+{
+}
+
+FurtherDerivedChild::~FurtherDerivedChild()
+{
+    // isNull()
+    QVERIFY(!guardedParentPointer.isNull());
+
+    // operator bool()
+    QVERIFY(guardedParentPointer);
+    QVERIFY(!!guardedParentPointer);
+
+    // Don't use QCOMPARE in these, it may call .data(), which could be UB at this point:
+    #define CHECK_NE(lhs, rhs) do { \
+        QVERIFY(!(lhs == rhs)); \
+        QVERIFY(!(rhs == lhs)); \
+        QVERIFY(lhs != rhs); \
+        QVERIFY(rhs != lhs); \
+    } while (false)
+
+    #define CHECK_EQ(lhs, rhs) do { \
+        QVERIFY(!(lhs != rhs)); \
+        QVERIFY(!(rhs != lhs)); \
+        QVERIFY(lhs == rhs); \
+        QVERIFY(rhs == lhs); \
+    } while (false)
+
+    // operator==/!= against nullptr
+    CHECK_NE(guardedParentPointer, nullptr);
+
+    // operator==/!= against QObject*/DerivedParent*/FutherDerivedParent*
+    CHECK_EQ(parentPointer, parentPointer); // verify it's not UB for raw pointers
+    // UB after this point!
+    return;
+    const QObject *parentPointerAsQObject = parentPointer;
+    CHECK_EQ(guardedParentPointer, parentPointerAsQObject);
+    const DerivedParent *parentPointerAsDerived = parentPointer;
+    CHECK_EQ(guardedParentPointer, parentPointerAsDerived);
+    CHECK_EQ(guardedParentPointer, DerivedChild::guardedParentPointer);
+    CHECK_EQ(guardedParentPointer, parentPointer);
+}
+
 void tst_QPointer::castDuringDestruction()
 {
     {
@@ -413,6 +491,11 @@ void tst_QPointer::castDuringDestruction()
     {
         delete new DerivedParent();
     }
+
+    {
+        [[maybe_unused]]
+        FurtherDerivedParent obj;
+    }
 }
 
 class TestRunnable : public QObject, public QRunnable {