Use after free during destruction. valgring: ==169635== Invalid read of size 4 ==169635== at 0x617BA10: pthread_mutex_lock@@GLIBC_2.2.5 (in /usr/lib64/libc.so.6) ==169635== by 0x898BE3B: wl_event_queue_destroy (in /usr/lib64/libwayland-client.so.0.22.0) ==169635== by 0x88AD427: QtWaylandClient::QWaylandDisplay::~QWaylandDisplay() (qwaylanddisplay.cpp:386) ==169635== by 0x88AD594: QtWaylandClient::QWaylandDisplay::~QWaylandDisplay() (qwaylanddisplay.cpp:387) ==169635== by 0x88A1CA5: cleanup (qscopedpointer.h:60) ==169635== by 0x88A1CA5: QScopedPointer<QtWaylandClient::QWaylandDisplay, QScopedPointerDeleter<QtWaylandClient::QWaylandDisplay> >::~QScopedPointer() (qscopedpointer.h:107) ==169635== by 0x88A0041: QtWaylandClient::QWaylandIntegration::~QWaylandIntegration() (qwaylandintegration.cpp:135) ==169635== by 0x88A005A: QtWaylandClient::QWaylandIntegration::~QWaylandIntegration() (qwaylandintegration.cpp:135) ==169635== by 0x5422CBC: QGuiApplicationPrivate::~QGuiApplicationPrivate() (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x4C66AF8: QApplicationPrivate::~QApplicationPrivate() (in /usr/lib64/libQt5Widgets.so.5.15.10) ==169635== by 0x117485: ??? (in /usr/bin/kmines) ==169635== by 0x61178CB: (below main) (in /usr/lib64/libc.so.6) ==169635== Address 0xbd43b98 is 296 bytes inside a block of size 376 free'd ==169635== at 0x484300B: free (vg_replace_malloc.c:974) ==169635== by 0x88AD419: QtWaylandClient::QWaylandDisplay::~QWaylandDisplay() (qwaylanddisplay.cpp:383) ==169635== by 0x88AD594: QtWaylandClient::QWaylandDisplay::~QWaylandDisplay() (qwaylanddisplay.cpp:387) ==169635== by 0x88A1CA5: cleanup (qscopedpointer.h:60) ==169635== by 0x88A1CA5: QScopedPointer<QtWaylandClient::QWaylandDisplay, QScopedPointerDeleter<QtWaylandClient::QWaylandDisplay> >::~QScopedPointer() (qscopedpointer.h:107) ==169635== by 0x88A0041: QtWaylandClient::QWaylandIntegration::~QWaylandIntegration() (qwaylandintegration.cpp:135) ==169635== by 0x88A005A: QtWaylandClient::QWaylandIntegration::~QWaylandIntegration() (qwaylandintegration.cpp:135) ==169635== by 0x5422CBC: QGuiApplicationPrivate::~QGuiApplicationPrivate() (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x4C66AF8: QApplicationPrivate::~QApplicationPrivate() (in /usr/lib64/libQt5Widgets.so.5.15.10) ==169635== by 0x117485: ??? (in /usr/bin/kmines) ==169635== by 0x61178CB: (below main) (in /usr/lib64/libc.so.6) ==169635== Block was alloc'd at ==169635== at 0x48456FF: calloc (vg_replace_malloc.c:1554) ==169635== by 0x898C7EE: wl_display_connect_to_fd (in /usr/lib64/libwayland-client.so.0.22.0) ==169635== by 0x898CB29: wl_display_connect (in /usr/lib64/libwayland-client.so.0.22.0) ==169635== by 0x88ABF6E: QtWaylandClient::QWaylandDisplay::QWaylandDisplay(QtWaylandClient::QWaylandIntegration*) (qwaylanddisplay.cpp:340) ==169635== by 0x88A06A7: QtWaylandClient::QWaylandIntegration::QWaylandIntegration() (qwaylandintegration.cpp:115) ==169635== by 0x485420D: QtWaylandClient::QWaylandIntegrationPlugin::create(QString const&, QStringList const&) (main.cpp:59) ==169635== by 0x541A01C: QPlatformIntegrationFactory::create(QString const&, QStringList const&, int&, char**, QString const&) (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x542649C: QGuiApplicationPrivate::createPlatformIntegration() (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x54278FF: QGuiApplicationPrivate::createEventDispatcher() (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x5B62FC4: QCoreApplicationPrivate::init() (in /usr/lib64/libQt5Core.so.5.15.10) ==169635== by 0x542A1FB: QGuiApplicationPrivate::init() (in /usr/lib64/libQt5Gui.so.5.15.10) ==169635== by 0x4C6D4A8: QApplicationPrivate::init() (in /usr/lib64/libQt5Widgets.so.5.15.10) Issue introduced by gentoo patch 0027-Reduce-memory-leakage.patch: From b981fc82eb37700353949c72d3fd6d0887c8c107 Mon Sep 17 00:00:00 2001 From: Ulf Hermann <ulf.hermann@qt.io> Date: Tue, 22 Feb 2022 12:31:08 +0100 Subject: [PATCH 27/51] Reduce memory leakage We need to clean up the event queue when we're done. Change-Id: I13a9eb77e978f4eab227a3a28dab8ebc8de94405 Reviewed-by: David Edmundson <davidedmundson@kde.org> (cherry picked from commit 1264e5f565d8fb7ac200e4b00531ab876922458f) --- src/client/qwaylanddisplay.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/client/qwaylanddisplay.cpp b/src/client/qwaylanddisplay.cpp index d371ffec..1b9ec699 100644 --- a/src/client/qwaylanddisplay.cpp +++ b/src/client/qwaylanddisplay.cpp @@ -381,6 +381,9 @@ QWaylandDisplay::~QWaylandDisplay(void) #endif if (mDisplay) wl_display_disconnect(mDisplay); + + if (m_frameEventQueue) + wl_event_queue_destroy(m_frameEventQueue); } // Steps which is called just after constructor. This separates registry_global() out of the constructor -- 2.41.0 This patch is incorrect because wl_event_queue_destroy() must be called before wl_display_disconnect() https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__event__queue I've tested what happens if move wl_event_queue_destroy(m_frameEventQueue) before wl_display_disconnect(). Everything works and use after free disappears.
Thanks.
asturm is away, so I'll handle it.
Fixed upstream in: commit a76bf824fcd1cc3789f0d3454a0423c0241d9718 Author: David Redondo <qt@david-redondo.de> Date: Tue Apr 11 14:27:27 2023 +0200 Destroy frame queue before display wl_event_queue_destroy accesses the display. Found by running a test under valgrind. Pick-to: 6.5 Change-Id: Ic89cbd3b6e98b4fc9561b0e63b5fab4886a1ec50 Reviewed-by: David Edmundson <davidedmundson@kde.org> i.e. https://codereview.qt-project.org/c/qt/qtwayland/+/471416. I'll get this backported in Qt5PatchCollection first.
Qt5PatchCollection backport: https://invent.kde.org/qt/qt/qtwayland/-/merge_requests/73
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=efca1f2c0288304eb5cc06500d01d9847da48dc7 commit efca1f2c0288304eb5cc06500d01d9847da48dc7 Author: Sam James <sam@gentoo.org> AuthorDate: 2023-07-14 07:29:04 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-07-14 07:29:41 +0000 dev-qt/qtwayland: backport use-after-free fix to 5.15.10-r1 Closes: https://bugs.gentoo.org/910315 Signed-off-by: Sam James <sam@gentoo.org> ....15.10-Destroy-frame-queue-before-display.patch | 34 +++++++++++++ dev-qt/qtwayland/qtwayland-5.15.10-r1.ebuild | 57 ++++++++++++++++++++++ 2 files changed, 91 insertions(+)
Many thanks for the report and analysis!
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=a7f4aecbf52c48a09536a13e85122ddae60613e4 commit a7f4aecbf52c48a09536a13e85122ddae60613e4 Author: Sam James <sam@gentoo.org> AuthorDate: 2023-07-14 07:31:09 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-07-14 07:31:09 +0000 dev-qt/qtwayland: throw in some more detail to patch description I've put this in the MR but not in the commit message for the MR because I'm not sure they want commentary there. Bug: https://bugs.gentoo.org/910315 Signed-off-by: Sam James <sam@gentoo.org> .../qtwayland-5.15.10-Destroy-frame-queue-before-display.patch | 9 +++++++++ 1 file changed, 9 insertions(+)
