diff mbox series

[2022.08.x] package/qt5base: fix memory leak

Message ID 20231130160710.3146545-1-roy.kollen.svendsen@akersolutions.com
State New
Headers show
Series [2022.08.x] package/qt5base: fix memory leak | expand

Commit Message

Roy Kollen Svendsen Nov. 30, 2023, 4:07 p.m. UTC
We noticed that our qt5 application froze after running for
a few days on our imx8m mini based panel-pc.

We stripped down the applcation to the following while still being able to
recreate the memory leak:

```
git clone git@github.com:roykollensvendsen/recreate-qt5base-memory-leak.git
```

To recreate and monitor the memory leak, run the following
commands:

Terminal (host):
```bash
python -m http.server -d . 8000
```

Terminal 1 (target):
```bash
./recreate-memory-leak http://host-machine:8000
```

Terminal 2 (target):
```bash
top -b -d 75 | grep -v 'grep' | grep recreate-memory-leak
...
  432   413 root     S     237m  12%   5% ./recreate-memory-leak http://192.168.1.1:8000
  432   413 root     S     238m  12%   6% ./recreate-memory-leak http://192.168.1.1:8000
  432   413 root     S     239m  12%   6% ./recreate-memory-leak http://192.168.1.1:8000
  432   413 root     S     240m  12%   6% ./recreate-memory-leak http://192.168.1.1:8000
  432   413 root     S     241m  12%   6% ./recreate-memory-leak http://192.168.1.1:8000
...
```

Before applying the included patches the output from `top`
showed that the memory usage of recreate-memory-leak increased
1 MB every 75 seconds.

After applying the patches the memory usage is constant.

The included patches were cherry-picked from the kde/5.15 branch of
qtbase.

We found these commits while grep-ing for fixes to memory leaks in corelib.

Signed-off-by: Roy Kollen Svendsen <roy.kollen.svendsen@akersolutions.com>
---
 ...ker-Disable-copy-and-provide-explici.patch | 100 +++++++++++++
 ...onnectionsImpl-Allow-to-skip-locking.patch |  99 +++++++++++++
 ...1-Fix-crash-in-concurrent-disconnect.patch | 137 ++++++++++++++++++
 ...the-orphaned-connection-lists-on-des.patch |  86 +++++++++++
 4 files changed, 422 insertions(+)
 create mode 100644 package/qt5/qt5base/0009-QOrderedMutexLocker-Disable-copy-and-provide-explici.patch
 create mode 100644 package/qt5/qt5base/0010-cleanOrphanedConnectionsImpl-Allow-to-skip-locking.patch
 create mode 100644 package/qt5/qt5base/0011-Fix-crash-in-concurrent-disconnect.patch
 create mode 100644 package/qt5/qt5base/0012-QObject-cleanup-the-orphaned-connection-lists-on-des.patch
diff mbox series

Patch

diff --git a/package/qt5/qt5base/0009-QOrderedMutexLocker-Disable-copy-and-provide-explici.patch b/package/qt5/qt5base/0009-QOrderedMutexLocker-Disable-copy-and-provide-explici.patch
new file mode 100644
index 0000000000..72dd6736bb
--- /dev/null
+++ b/package/qt5/qt5base/0009-QOrderedMutexLocker-Disable-copy-and-provide-explici.patch
@@ -0,0 +1,100 @@ 
+From 30b7fc26257292b51ecece9b7c92e7b34208512e Mon Sep 17 00:00:00 2001
+From: Fabian Kosmale <fabian.kosmale@qt.io>
+Date: Fri, 11 Jun 2021 11:31:00 +0200
+Subject: [PATCH 1/5] QOrderedMutexLocker: Disable copy and provide explicit
+ dismiss function
+
+Copying a QOrderedMutexLocker is questionable, and would currenly easily
+lead to UB. Therefore we delete the copy ctor and copy assignment
+operator, and implement well-behaving move operators.
+In addition, provide an explicit dismiss method for cases where we don't
+want the locker to unlock the mutexes, as they have been manually
+unlocked (this could have been implemented previoulsy by using the copy
+assignment operator).
+
+Pick-to: 6.2 6.1 5.15
+Change-Id: If2a888710e1c74277b28fd3e2939ab26fff0c7ae
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+Reviewed-by: Lars Knoll <lars.knoll@qt.io>
+(cherry picked from commit 7fefce73284de4204d64c7e4129f39004a13cdad)
+
+Change-Id: I33777145eecbc33b943a5011a3bd781ba13fee39
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+---
+ src/corelib/thread/qorderedmutexlocker_p.h | 41 ++++++++++++++++++++++
+ 1 file changed, 41 insertions(+)
+
+diff --git a/src/corelib/thread/qorderedmutexlocker_p.h b/src/corelib/thread/qorderedmutexlocker_p.h
+index 83edfd5879..9a9aaefa02 100644
+--- a/src/corelib/thread/qorderedmutexlocker_p.h
++++ b/src/corelib/thread/qorderedmutexlocker_p.h
+@@ -74,6 +74,28 @@ public:
+     {
+         relock();
+     }
++
++    Q_DISABLE_COPY(QOrderedMutexLocker)
++
++    void swap(QOrderedMutexLocker &other) noexcept
++    {
++        qSwap(this->mtx1, other.mtx1);
++        qSwap(this->mtx2, other.mtx2);
++        qSwap(this->locked, other.locked);
++    }
++
++    QOrderedMutexLocker  &operator=(QOrderedMutexLocker &&other) noexcept {
++        QOrderedMutexLocker moved(std::move(other));
++        swap(moved);
++        return *this;
++    }
++
++    QOrderedMutexLocker(QOrderedMutexLocker &&other) noexcept
++        : mtx1(std::exchange(other.mtx1, nullptr))
++        , mtx2(std::exchange(other.mtx2, nullptr))
++        , locked(std::exchange(other.locked, false))
++    {}
++
+     ~QOrderedMutexLocker()
+     {
+         unlock();
+@@ -88,6 +110,21 @@ public:
+         }
+     }
+ 
++    /*!
++        \internal
++        Can be called if the mutexes have been unlocked manually, and sets the
++        state of the QOrderedMutexLocker to unlocked.
++        The caller is expected to have unlocked both of them if they
++        are not the same. Calling this method when the QOrderedMutexLocker is
++        unlocked or when the provided mutexes have not actually been unlocked is
++        UB.
++     */
++    void dismiss()
++    {
++        Q_ASSERT(locked);
++        locked = false;
++    }
++
+     void unlock()
+     {
+         if (locked) {
+@@ -153,11 +190,15 @@ private:
+ class QOrderedMutexLocker
+ {
+ public:
++    Q_DISABLE_COPY(QOrderedMutexLocker)
+     QOrderedMutexLocker(QBasicMutex *, QBasicMutex *) {}
++    QOrderedMutexLocker(QOrderedMutexLocker &&) = default;
++    QOrderedMutexLocker& operator=(QOrderedMutexLocker &&other) = default;
+     ~QOrderedMutexLocker() {}
+ 
+     void relock() {}
+     void unlock() {}
++    void dismiss() {}
+ 
+     static bool relock(QBasicMutex *, QBasicMutex *) { return false; }
+ };
+-- 
+2.43.0
+
diff --git a/package/qt5/qt5base/0010-cleanOrphanedConnectionsImpl-Allow-to-skip-locking.patch b/package/qt5/qt5base/0010-cleanOrphanedConnectionsImpl-Allow-to-skip-locking.patch
new file mode 100644
index 0000000000..77b838aaa1
--- /dev/null
+++ b/package/qt5/qt5base/0010-cleanOrphanedConnectionsImpl-Allow-to-skip-locking.patch
@@ -0,0 +1,99 @@ 
+From dad94691d83da9a3aa256b9fcd9805e69416c24f Mon Sep 17 00:00:00 2001
+From: Fabian Kosmale <fabian.kosmale@qt.io>
+Date: Tue, 8 Jun 2021 17:47:46 +0200
+Subject: [PATCH 2/5] cleanOrphanedConnectionsImpl: Allow to skip locking
+
+This function is/will be used in a few places where we already have a
+lock. Temporarily unlocking and relocking invites all kinds of troubles.
+By adding a flag we can instead tell the function that we already hold
+the lock.
+
+Pick-to: 6.2 6.1 5.15
+Change-Id: Ibca089de61133661d5cd75290f2a55c22c5d013c
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+(cherry picked from commit 556fc646cfac4dca43a34f5c4a4f7e6e3ef9104d)
+
+Change-Id: I328e1be1f63c689dbbd7649cc291ef2c4f2ee6af
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+---
+ src/corelib/kernel/qobject.cpp | 19 ++++++++++++++++---
+ src/corelib/kernel/qobject_p.h | 13 ++++++++++---
+ 2 files changed, 26 insertions(+), 6 deletions(-)
+
+diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
+index 9385419015..4045263cc4 100644
+--- a/src/corelib/kernel/qobject.cpp
++++ b/src/corelib/kernel/qobject.cpp
+@@ -71,6 +71,7 @@
+ #include <qtcore_tracepoints_p.h>
+ 
+ #include <new>
++#include <mutex>
+ 
+ #include <ctype.h>
+ #include <limits.h>
+@@ -406,11 +407,14 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection
+ 
+ }
+ 
+-void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sender)
++void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sender, LockPolicy lockPolicy)
+ {
++    QBasicMutex *senderMutex = signalSlotLock(sender);
+     ConnectionOrSignalVector *c = nullptr;
+     {
+-        QBasicMutexLocker l(signalSlotLock(sender));
++        std::unique_lock<QBasicMutex> lock(*senderMutex, std::defer_lock_t{});
++        if (lockPolicy == NeedToLock)
++            lock.lock();
+         if (ref.loadAcquire() > 1)
+             return;
+ 
+@@ -420,7 +424,16 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende
+         c = orphaned.loadRelaxed();
+         orphaned.storeRelaxed(nullptr);
+     }
+-    deleteOrphaned(c);
++    if (c) {
++        // Deleting c might run arbitrary user code, so we must not hold the lock
++        if (lockPolicy == AlreadyLockedAndTemporarilyReleasingLock) {
++            senderMutex->unlock();
++            deleteOrphaned(c);
++            senderMutex->lock();
++        } else {
++            deleteOrphaned(c);
++        }
++    }
+ }
+ 
+ void QObjectPrivate::ConnectionData::deleteOrphaned(QObjectPrivate::ConnectionOrSignalVector *o)
+diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h
+index 66c19d174e..f738e57eb6 100644
+--- a/src/corelib/kernel/qobject_p.h
++++ b/src/corelib/kernel/qobject_p.h
+@@ -277,12 +277,19 @@ public:
+         // must be called on the senders connection data
+         // assumes the senders and receivers lock are held
+         void removeConnection(Connection *c);
+-        void cleanOrphanedConnections(QObject *sender)
++        enum LockPolicy {
++            NeedToLock,
++            // Beware that we need to temporarily release the lock
++            // and thus calling code must carefully consider whether
++            // invariants still hold.
++            AlreadyLockedAndTemporarilyReleasingLock
++        };
++        void cleanOrphanedConnections(QObject *sender, LockPolicy lockPolicy = NeedToLock)
+         {
+             if (orphaned.loadRelaxed() && ref.loadAcquire() == 1)
+-                cleanOrphanedConnectionsImpl(sender);
++                cleanOrphanedConnectionsImpl(sender, lockPolicy);
+         }
+-        void cleanOrphanedConnectionsImpl(QObject *sender);
++        void cleanOrphanedConnectionsImpl(QObject *sender, LockPolicy lockPolicy);
+ 
+         ConnectionList &connectionsForSignal(int signal)
+         {
+-- 
+2.43.0
+
diff --git a/package/qt5/qt5base/0011-Fix-crash-in-concurrent-disconnect.patch b/package/qt5/qt5base/0011-Fix-crash-in-concurrent-disconnect.patch
new file mode 100644
index 0000000000..b9f148945f
--- /dev/null
+++ b/package/qt5/qt5base/0011-Fix-crash-in-concurrent-disconnect.patch
@@ -0,0 +1,137 @@ 
+From 12b1ed1e2fefe5ff983d7511f21a52e3e7a42437 Mon Sep 17 00:00:00 2001
+From: Fabian Kosmale <fabian.kosmale@qt.io>
+Date: Tue, 3 Aug 2021 11:16:23 +0200
+Subject: [PATCH 3/5] Fix crash in concurrent disconnect
+
+This is a manual cherry-pick of
+71b4d4f150bc3c904a5aceec37513ddc3cd1c150. It has been adjusted to
+reflect the fact that
+QObjectPrivate::disconnect(QObjectPrivate::Connection *c) was only in
+6.0 extracted as a helper function. This made one fix unnecessary, and
+required moving the changes that were done in the helper to
+QObject::disconnect(const QMetaObject::Connection &connection) instead.
+
+Task-number: QTBUG-88248
+Change-Id: I8bc5d058ee02835827425cb722f5a6e17055af97
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+---
+ src/corelib/kernel/qobject.cpp | 39 ++++++++++++++++++++++------------
+ src/corelib/kernel/qobject_p.h | 15 ++++++++++---
+ 2 files changed, 37 insertions(+), 17 deletions(-)
+
+diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
+index 4045263cc4..630f77e2b6 100644
+--- a/src/corelib/kernel/qobject.cpp
++++ b/src/corelib/kernel/qobject.cpp
+@@ -391,8 +391,14 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection
+ 
+     Q_ASSERT(c != orphaned.loadRelaxed());
+     // add c to orphanedConnections
+-    c->nextInOrphanList = orphaned.loadRelaxed();
+-    orphaned.storeRelaxed(c);
++    Connection *o = nullptr;
++    /* No ABA issue here: When adding a node, we only care about the list head, it doesn't
++     * matter if the tail changes.
++     */
++    do {
++        o = orphaned.loadRelaxed();
++        c->nextInOrphanList = o;
++    } while (!orphaned.testAndSetRelease(o, c));
+ 
+ #ifndef QT_NO_DEBUG
+     found = false;
+@@ -421,8 +427,7 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende
+         // Since ref == 1, no activate() is in process since we locked the mutex. That implies,
+         // that nothing can reference the orphaned connection objects anymore and they can
+         // be safely deleted
+-        c = orphaned.loadRelaxed();
+-        orphaned.storeRelaxed(nullptr);
++        c = orphaned.fetchAndStoreRelaxed(nullptr);
+     }
+     if (c) {
+         // Deleting c might run arbitrary user code, so we must not hold the lock
+@@ -1035,7 +1040,7 @@ QObject::~QObject()
+ 
+                 QBasicMutex *m = signalSlotLock(c->receiver.loadRelaxed());
+                 bool needToUnlock = QOrderedMutexLocker::relock(signalSlotMutex, m);
+-                if (c->receiver.loadAcquire()) {
++                if (c == connectionList.first.loadAcquire() && c->receiver.loadAcquire()) {
+                     cd->removeConnection(c);
+                     Q_ASSERT(connectionList.first.loadRelaxed() != c);
+                 }
+@@ -1076,11 +1081,11 @@ QObject::~QObject()
+             if (needToUnlock)
+                 m->unlock();
+ 
+-            if (slotObj) {
+-                locker.unlock();
++            locker.unlock();
++            if (slotObj)
+                 slotObj->destroyIfLastRef();
+-                locker.relock();
+-            }
++            senderData->cleanOrphanedConnections(sender);
++            locker.relock();
+         }
+ 
+         // invalidate all connections on the object and make sure
+@@ -5119,12 +5124,18 @@ bool QObject::disconnect(const QMetaObject::Connection &connection)
+         connections = QObjectPrivate::get(c->sender)->connections.loadRelaxed();
+         Q_ASSERT(connections);
+         connections->removeConnection(c);
+-    }
+ 
+-    connections->cleanOrphanedConnections(c->sender);
+-
+-    c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(),
+-                                                           c->signal_index));
++        c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(), c->signal_index));
++        // We must not hold the receiver mutex, else we risk dead-locking; we also only need the sender mutex
++        // It is however vital to hold the senderMutex before calling cleanOrphanedConnections, as otherwise
++        // another thread might modify/delete the connection
++        if (receiverMutex != senderMutex) {
++            receiverMutex->unlock();
++        }
++        connections->cleanOrphanedConnections(c->sender, QObjectPrivate::ConnectionData::AlreadyLockedAndTemporarilyReleasingLock);
++        senderMutex->unlock();  // now both sender and receiver mutex have been manually unlocked
++        locker.dismiss(); // so we dismiss the QOrderedMutexLocker
++    }
+ 
+     const_cast<QMetaObject::Connection &>(connection).d_ptr = nullptr;
+     c->deref(); // has been removed from the QMetaObject::Connection object
+diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h
+index f738e57eb6..0b827a52ca 100644
+--- a/src/corelib/kernel/qobject_p.h
++++ b/src/corelib/kernel/qobject_p.h
+@@ -268,7 +268,10 @@ public:
+ 
+         ~ConnectionData()
+         {
+-            deleteOrphaned(orphaned.loadRelaxed());
++            Q_ASSERT(ref.loadRelaxed() == 0);
++            auto *c = orphaned.fetchAndStoreRelaxed(nullptr);
++            if (c)
++                deleteOrphaned(c);
+             SignalVector *v = signalVector.loadRelaxed();
+             if (v)
+                 free(v);
+@@ -314,8 +317,14 @@ public:
+ 
+             signalVector.storeRelaxed(newVector);
+             if (vector) {
+-                vector->nextInOrphanList = orphaned.loadRelaxed();
+-                orphaned.storeRelaxed(ConnectionOrSignalVector::fromSignalVector(vector));
++                Connection *o = nullptr;
++                /* No ABA issue here: When adding a node, we only care about the list head, it doesn't
++                 * matter if the tail changes.
++                 */
++                do {
++                    o = orphaned.loadRelaxed();
++                    vector->nextInOrphanList = o;
++                } while (!orphaned.testAndSetRelease(o, ConnectionOrSignalVector::fromSignalVector(vector)));
+             }
+         }
+         int signalVectorCount() const {
+-- 
+2.43.0
+
diff --git a/package/qt5/qt5base/0012-QObject-cleanup-the-orphaned-connection-lists-on-des.patch b/package/qt5/qt5base/0012-QObject-cleanup-the-orphaned-connection-lists-on-des.patch
new file mode 100644
index 0000000000..b7ac27d6a1
--- /dev/null
+++ b/package/qt5/qt5base/0012-QObject-cleanup-the-orphaned-connection-lists-on-des.patch
@@ -0,0 +1,86 @@ 
+From ec01140a89cb4ebb14792526ca6be92f1e9357cf Mon Sep 17 00:00:00 2001
+From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
+Date: Sat, 7 Nov 2020 01:15:34 +0100
+Subject: [PATCH 4/5] QObject: cleanup the orphaned connection lists on
+ destruction
+
+When a signal/slot connection is broken, it gets added to the
+sender's list of "orphaned connections", to clean up later.
+This cleanup happens when the sender gets destroyed or as soon as
+it emits any signal.
+
+This may cause soft memory leaks in case receivers get destroyed,
+and the sender is a long living object and doesn't emit signals
+for a while (e.g. QThread).
+
+For some reason, an explicit disconnection cleans up the list
+(either by using the QMetaObject::Connection object, or in case
+of string-based connect, using a string-based disconnect). This
+raises lots of doubts about why having this list in the first
+place.
+
+Fix the soft-leak by cleaning up the orphaned connection list when
+destroying a receiver.
+
+Note: I still believe that we shouldn't have any "orphaned"
+connection list, and rather cleanup on disconnect/deletion
+(otherwise, emitting a signal may cause a CPU spike because it
+triggers a cleanup).  If we allow for any "impredictability" during
+signal activation we're just admitting that signals/slots aren't
+suitable for e.g. low-latency codepaths. That's why I'm not marking
+the problem as fixed.
+
+Original-patch-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
+Task-number: QTBUG-88248
+Task-number: QTBUG-87774
+Pick-to: 6.2 6.1 5.15
+Change-Id: Id25f67a45dff49f740132a44d36e88740eb12070
+Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
+Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
+(cherry picked from commit c2839843f23fb5c289175cb9577981d48dd273fc)
+
+Change-Id: I40457e989c2d55a5e96a923b08d39088a257d185
+Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
+Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
+---
+ src/corelib/kernel/qobject.cpp | 19 +++++++++++++++++--
+ 1 file changed, 17 insertions(+), 2 deletions(-)
+
+diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
+index 630f77e2b6..d9c324f9f9 100644
+--- a/src/corelib/kernel/qobject.cpp
++++ b/src/corelib/kernel/qobject.cpp
+@@ -1078,13 +1078,28 @@ QObject::~QObject()
+             }
+ 
+             senderData->removeConnection(node);
++            /*
++              When we unlock, another thread has the chance to delete/modify sender data.
++              Thus we need to call cleanOrphanedConnections before unlocking. We use the
++              variant of the function which assumes that the lock is already held to avoid
++              a deadlock.
++              We need to hold m, the sender lock. Considering that we might execute arbitrary user
++              code, we should already release the signalSlotMutex here – unless they are the same.
++            */
++            const bool locksAreTheSame = signalSlotMutex == m;
++            if (!locksAreTheSame)
++                locker.unlock();
++            senderData->cleanOrphanedConnections(
++                        sender,
++                        QObjectPrivate::ConnectionData::AlreadyLockedAndTemporarilyReleasingLock
++                        );
+             if (needToUnlock)
+                 m->unlock();
+ 
+-            locker.unlock();
++            if (locksAreTheSame) // otherwise already unlocked
++                locker.unlock();
+             if (slotObj)
+                 slotObj->destroyIfLastRef();
+-            senderData->cleanOrphanedConnections(sender);
+             locker.relock();
+         }
+ 
+-- 
+2.43.0
+