From c5880833d94330d022c4b6fc84c175aadeaf9632 Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Tue, 22 Sep 2020 08:53:17 +0000
Subject: [PATCH] x11: Make removal of X11 event filters safe

If an X11 event filter has been activated and it unregisters another X11
event filter, then the window manager may crash because the foreach macro
in Workspace::workspaceEvent() makes a copy of m_genericEventFilters or
m_eventFilters and we can call the event() method for an already defunct
filter.

With this change, X11 event filters can be safely removed and installed
at any particular moment.

BUG: 423319


(cherry picked from commit a433fb08a3a9255802405a17dd4c8270c68fcb25)
---
 events.cpp    | 52 +++++++++++++++++++++++++++++++++++++++++----------
 workspace.cpp | 10 ++++++++++
 workspace.h   | 17 +++++++++++++++--
 3 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/events.cpp b/events.cpp
index eb3572d13..2e8885d76 100644
--- a/events.cpp
+++ b/events.cpp
@@ -165,18 +165,34 @@ QVector<QByteArray> s_xcbEerrors({
 
 void Workspace::registerEventFilter(X11EventFilter *filter)
 {
-    if (filter->isGenericEvent())
-        m_genericEventFilters.append(filter);
-    else
-        m_eventFilters.append(filter);
+    if (filter->isGenericEvent()) {
+        m_genericEventFilters.append(new X11EventFilterContainer(filter));
+    } else {
+        m_eventFilters.append(new X11EventFilterContainer(filter));
+    }
+}
+
+static X11EventFilterContainer *takeEventFilter(X11EventFilter *eventFilter,
+                                                QList<QPointer<X11EventFilterContainer>> &list)
+{
+    for (int i = 0; i < list.count(); ++i) {
+        X11EventFilterContainer *container = list.at(i);
+        if (container->filter() == eventFilter) {
+            return list.takeAt(i);
+        }
+    }
+    return nullptr;
 }
 
 void Workspace::unregisterEventFilter(X11EventFilter *filter)
 {
-    if (filter->isGenericEvent())
-        m_genericEventFilters.removeOne(filter);
-    else
-        m_eventFilters.removeOne(filter);
+    X11EventFilterContainer *container = nullptr;
+    if (filter->isGenericEvent()) {
+        container = takeEventFilter(filter, m_genericEventFilters);
+    } else {
+        container = takeEventFilter(filter, m_eventFilters);
+    }
+    delete container;
 }
 
 
@@ -219,13 +235,29 @@ bool Workspace::workspaceEvent(xcb_generic_event_t *e)
     if (eventType == XCB_GE_GENERIC) {
         xcb_ge_generic_event_t *ge = reinterpret_cast<xcb_ge_generic_event_t *>(e);
 
-        foreach (X11EventFilter *filter, m_genericEventFilters) {
+        // We need to make a shadow copy of the event filter list because an activated event
+        // filter may mutate it by removing or installing another event filter.
+        const auto eventFilters = m_genericEventFilters;
+
+        for (X11EventFilterContainer *container : eventFilters) {
+            if (!container) {
+                continue;
+            }
+            X11EventFilter *filter = container->filter();
             if (filter->extension() == ge->extension && filter->genericEventTypes().contains(ge->event_type) && filter->event(e)) {
                 return true;
             }
         }
     } else {
-        foreach (X11EventFilter *filter, m_eventFilters) {
+        // We need to make a shadow copy of the event filter list because an activated event
+        // filter may mutate it by removing or installing another event filter.
+        const auto eventFilters = m_eventFilters;
+
+        for (X11EventFilterContainer *container : eventFilters) {
+            if (!container) {
+                continue;
+            }
+            X11EventFilter *filter = container->filter();
             if (filter->eventTypes().contains(eventType) && filter->event(e)) {
                 return true;
             }
diff --git a/workspace.cpp b/workspace.cpp
index a87a622e9..fd3634b16 100644
--- a/workspace.cpp
+++ b/workspace.cpp
@@ -66,6 +66,16 @@ namespace KWin
 extern int screen_number;
 extern bool is_multihead;
 
+X11EventFilterContainer::X11EventFilterContainer(X11EventFilter *filter)
+    : m_filter(filter)
+{
+}
+
+X11EventFilter *X11EventFilterContainer::filter() const
+{
+    return m_filter;
+}
+
 ColorMapper::ColorMapper(QObject *parent)
     : QObject(parent)
     , m_default(kwinApp()->x11DefaultScreen()->default_colormap)
diff --git a/workspace.h b/workspace.h
index 489d7bae4..61fb215a8 100644
--- a/workspace.h
+++ b/workspace.h
@@ -55,6 +55,19 @@ class X11Client;
 class X11EventFilter;
 enum class Predicate;
 
+class X11EventFilterContainer : public QObject
+{
+    Q_OBJECT
+
+public:
+    explicit X11EventFilterContainer(X11EventFilter *filter);
+
+    X11EventFilter *filter() const;
+
+private:
+    X11EventFilter *m_filter;
+};
+
 class KWIN_EXPORT Workspace : public QObject
 {
     Q_OBJECT
@@ -654,8 +667,8 @@ private:
 
     QScopedPointer<KillWindow> m_windowKiller;
 
-    QList<X11EventFilter *> m_eventFilters;
-    QList<X11EventFilter *> m_genericEventFilters;
+    QList<QPointer<X11EventFilterContainer>> m_eventFilters;
+    QList<QPointer<X11EventFilterContainer>> m_genericEventFilters;
     QScopedPointer<X11EventFilter> m_movingClientFilter;
     QScopedPointer<X11EventFilter> m_syncAlarmFilter;
 
-- 
GitLab