From 6a8b3344c09344dfb66ec0e80cd2b4a60999ffeb Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 4 Apr 2025 10:12:24 +0200 Subject: [PATCH] QPointer: don't cause UB when checking for nullptr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 (cherry picked from commit 51cd57116b7465f732253d3f38a2dd78fc704088) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qpointer.h | 3 +- .../corelib/kernel/qpointer/tst_qpointer.cpp | 83 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/corelib/kernel/qpointer.h b/src/corelib/kernel/qpointer.h index 9659fcf9376..d876c8d7616 100644 --- a/src/corelib/kernel/qpointer.h +++ b/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 ) 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 Q_DECLARE_TYPEINFO_BODY(QPointer, Q_RELOCATABLE_TYPE); diff --git a/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp b/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp index 5ac9cb3af13..fa95fe85f7b 100644 --- a/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp +++ b/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp @@ -367,6 +367,7 @@ class DerivedChild : public QObject { Q_OBJECT +protected: DerivedParent *parentPointer; QPointer guardedParentPointer; @@ -394,6 +395,83 @@ DerivedChild::~DerivedChild() QCOMPARE(qobject_cast(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 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 {