From 44fa80cbd4fabd6d7b87e7a50233f7dbeaf303b4 Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Fri, 11 Jun 2021 09:43:37 +0200
Subject: [PATCH] QObject: Fix memory leak in queued_activate

queued_activate adds a reference to the slot object. It also attempts to
deref it again, but that did not work correctly so far. We could end up
with
T1                              |             T2
queued_activate                 |
  checks isSlotObject           |
    adds ref                    |
  locker.unlock()               |
                                | QObject::~QObject
                                |  //In disconnect all senders loop
                                |  sets isSlotObject to false
                                |  derefs slotObj, but not deleted
  checks isSlotObject           |
  (no deref because it's null)  |

To solve this issue and others caused by early returns, we now use a
RAII helper, which always takes care of calling destroyIfLastRef if the
ref count has been incremented.

Change-Id: I9c011cdb8faa5f344d7e70f024fc13f407e39ccf
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
---
 src/corelib/kernel/qobject.cpp | 46 +++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index fafcd38417e..1659573009b 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -3647,6 +3647,37 @@ void QMetaObject::connectSlotsByName(QObject *o)
     }
 }
 
+/*!
+     \internal
+     A small RAII helper for QSlotObjectBase.
+     Calls ref on construction and destroyLastRef in its dtor.
+     Allows construction from a nullptr in which case it does nothing.
+ */
+struct SlotObjectGuard {
+    SlotObjectGuard() = default;
+    // move would be fine, but we do not need it currently
+    Q_DISABLE_COPY_MOVE(SlotObjectGuard)
+    explicit SlotObjectGuard(QtPrivate::QSlotObjectBase *slotObject)
+        : m_slotObject(slotObject)
+    {
+        if (m_slotObject)
+            m_slotObject->ref();
+    }
+
+    QtPrivate::QSlotObjectBase const *operator->() const
+    { return m_slotObject; }
+
+    QtPrivate::QSlotObjectBase *operator->()
+    { return m_slotObject; }
+
+    ~SlotObjectGuard() {
+        if (m_slotObject)
+            m_slotObject->destroyIfLastRef();
+    }
+private:
+    QtPrivate::QSlotObjectBase *m_slotObject = nullptr;
+};
+
 /*!
     \internal
 
@@ -3678,8 +3709,8 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect
         // the connection has been disconnected before we got the lock
         return;
     }
-    if (c->isSlotObject)
-        c->slotObj->ref();
+
+    SlotObjectGuard slotObjectGuard { c->isSlotObject ? c->slotObj : nullptr };
     locker.unlock();
 
     QMetaCallEvent *ev = c->isSlotObject ?
@@ -3706,8 +3737,6 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect
     }
 
     locker.relock();
-    if (c->isSlotObject)
-        c->slotObj->destroyIfLastRef();
     if (!c->isSingleShot && !c->receiver.loadRelaxed()) {
         // the connection has been disconnected while we were unlocked
         locker.unlock();
@@ -3835,14 +3864,7 @@ void doActivate(QObject *sender, int signal_index, void **argv)
             QObjectPrivate::Sender senderData(receiverInSameThread ? receiver : nullptr, sender, signal_index);
 
             if (c->isSlotObject) {
-                c->slotObj->ref();
-
-                struct Deleter {
-                    void operator()(QtPrivate::QSlotObjectBase *slot) const {
-                        if (slot) slot->destroyIfLastRef();
-                    }
-                };
-                const std::unique_ptr<QtPrivate::QSlotObjectBase, Deleter> obj{c->slotObj};
+                SlotObjectGuard obj{c->slotObj};
 
                 {
                     Q_TRACE_SCOPE(QMetaObject_activate_slot_functor, obj.get());