From a433fb08a3a9255802405a17dd4c8270c68fcb25 Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Tue, 22 Sep 2020 11:53:17 +0300
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
---
 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 f1f497e165..b2117489aa 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 6aa71fc486..fbc35a8b9b 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 489d7bae4a..61fb215a85 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;