From aeb904e829fcc6f5fc7b3ff4ad95621675eabab8 Mon Sep 17 00:00:00 2001 From: Rain Date: Mon, 29 Jun 2026 12:22:38 -0700 Subject: [PATCH] wayland/toplevel: defer Toplevel/handle destruction to avoid use-after-free Attempt to fix an issue where quickshell sometimes crashes on monitor wakeup. The stack trace [can be found here](https://gist.github.com/sunshowers/e439bd41d78fb5f189088471fc8d3e36). Normally, when the V4 engine hands a `QObject*` to JavaScript it creates a `QObjectWrapper` guarded through the object's `QQmlData`. This allows a later delete to fire `QObject::destroyed` and to invalidate the wrapper. But if the object is turned into a `QVariant`, it goes back to being a bare, untracked pointer with none of that functionality. This can happen if a `Toplevel` is embedded in a `QVariantList` or `QVariantMap`, such as what `ScriptModel` stores. Now, if `Toplevel` and `ToplevelHandle` were then destroyed synchronously the moment the compositor closed a toplevel, a consumer holding the corresponding `QObject*` would hold on to a dangling reference. Accessing this later would then segfault. This patch makes it so that both destructions are deferred with, `deleteLater()`. This matches how screen wrappers are already freed in `QuickshellTracked::updateScreens`, and should hopefully fix this issue. (Note that this issue is only fixed on the read path, not the action path which still has a use-after-free. My thoughts are the action path is relatively rare, so it might be worth waiting to design a better fix for this which focuses on making invalid states unrepresentable, though naturally the ability to do so is more limited than it would be in a language with affine typing like Rust. One option is to do a larger rework like using a generational arena with indexes.) Why must both `Toplevel` and `ToplevelHandle` be deferred? If `Toplevel` were deferred but `ToplevelHandle` were not, then `Toplevel` would be stuck holding a dangling pointer itself. --- changelog/next.md | 1 + src/wayland/toplevel/qml.cpp | 2 +- src/wayland/toplevel/wlr_toplevel.cpp | 2 +- src/wayland/toplevel/wlr_toplevel.hpp | 3 ++- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/changelog/next.md b/changelog/next.md index ede8442d..564f6991 100644 --- a/changelog/next.md +++ b/changelog/next.md @@ -6,3 +6,4 @@ - Fixed missing/wrong change signals on various properties. - Fixed session lock crashes on sleep, wake, DPMS, and unlocking. - QsWindow.updatesEnabled makes sure windows are redrawn when set to true. +- Fixed a crash that could occur when a Toplevel was closed while still referenced, such as on monitor wakeup. diff --git a/src/wayland/toplevel/qml.cpp b/src/wayland/toplevel/qml.cpp index ea09e095..2ccbcef1 100644 --- a/src/wayland/toplevel/qml.cpp +++ b/src/wayland/toplevel/qml.cpp @@ -31,7 +31,7 @@ Toplevel::Toplevel(wlr::ToplevelHandle* handle, QObject* parent): QObject(parent void Toplevel::onClosed() { emit this->closed(); - delete this; + this->deleteLater(); } void Toplevel::activate() { this->handle->activate(); } diff --git a/src/wayland/toplevel/wlr_toplevel.cpp b/src/wayland/toplevel/wlr_toplevel.cpp index fe6f1b3e..d2f30c13 100644 --- a/src/wayland/toplevel/wlr_toplevel.cpp +++ b/src/wayland/toplevel/wlr_toplevel.cpp @@ -173,7 +173,7 @@ void ToplevelHandle::zwlr_foreign_toplevel_handle_v1_closed() { qCDebug(logToplevelManagement) << this << "closed"; this->destroy(); emit this->closed(); - delete this; + this->deleteLater(); } void ToplevelHandle::zwlr_foreign_toplevel_handle_v1_app_id(const QString& appId) { diff --git a/src/wayland/toplevel/wlr_toplevel.hpp b/src/wayland/toplevel/wlr_toplevel.hpp index 2c69fcdf..515ece76 100644 --- a/src/wayland/toplevel/wlr_toplevel.hpp +++ b/src/wayland/toplevel/wlr_toplevel.hpp @@ -45,7 +45,8 @@ class ToplevelHandle signals: // sent after the first done event. void ready(); - // sent right before delete this. + // sent right before this object is scheduled for deletion (via deleteLater) + // -- the wayland handle is already gone by this point. void closed(); void appIdChanged();