Bug 910315 – dev-qt/qtwayland-5.15.10: use after free memory corruption (from 0027-Reduce-memory-leakage.patch)

• Product: Gentoo Linux
• Status: RESOLVED FIXED

[Description Михаил 2023-07-14 06:37:14 UTC]

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 <[e-mail]>
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 <[e-mail]>
(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.

[Comment 1 Sam James 2023-07-14 06:39:14 UTC]

Thanks.

[Comment 2 Sam James 2023-07-14 07:14:44 UTC]

asturm is away, so I'll handle it.

[Comment 3 Sam James 2023-07-14 07:16:21 UTC]

Fixed upstream in:

commit a76bf824fcd1cc3789f0d3454a0423c0241d9718
Author: David Redondo <[e-mail]>
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 <[e-mail]>

i.e. https://codereview.qt-project.org/c/qt/qtwayland/+/471416.

I'll get this backported in Qt5PatchCollection first.

[Comment 4 Sam James 2023-07-14 07:24:42 UTC]

Qt5PatchCollection backport: https://invent.kde.org/qt/qt/qtwayland/-/merge_requests/73

[Comment 5 Larry the Git Cow 2023-07-14 07:29:49 UTC]

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 <[e-mail]>
AuthorDate: 2023-07-14 07:29:04 +0000
Commit:     Sam James <[e-mail]>
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 <[e-mail]>

 ....15.10-Destroy-frame-queue-before-display.patch | 34 +++++++++++++
 dev-qt/qtwayland/qtwayland-5.15.10-r1.ebuild       | 57 ++++++++++++++++++++++
 2 files changed, 91 insertions(+)

[Comment 6 Sam James 2023-07-14 07:30:06 UTC]

Many thanks for the report and analysis!

[Comment 7 Larry the Git Cow 2023-07-14 07:31:43 UTC]

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 <[e-mail]>
AuthorDate: 2023-07-14 07:31:09 +0000
Commit:     Sam James <[e-mail]>
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 <[e-mail]>

 .../qtwayland-5.15.10-Destroy-frame-queue-before-display.patch   | 9 +++++++++
 1 file changed, 9 insertions(+)