core/screen: retain removed screens to avoid use-after-free#879
core/screen: retain removed screens to avoid use-after-free#879sunshowers wants to merge 1 commit into
Conversation
Previously, when a screen was disconnected, `QuickshellTracked::updateScreens` deleted its `QuickshellScreenInfo` wrapper. But a copy of that `QObject*` may have been boxed into a `QVariant` and left a bare, unguarded pointer. Deleting the wrapper then dangles that pointer, and the next time the engine re-wraps it (the process segfaults).
This was observed as intermittent crashes on monitor wakeup under niri with DankMaterialShell, confirmed from two core dumps to be a freed `QuickshellScreenInfo` inside a `Toplevel.screens` list retained by a model.
Both core dumps had the following in them:
```
screens QVariant @ 0x7fecc79e4eb8 metatype='QList<QuickshellScreenInfo*>'
inline QList {d=0x7fecf1921320, ptr=0x7fecf1921330, size=1}
screens[0] = 0x7fed0313b6a0 <-- freed (vtable=0x1 d_ptr=0x7)
```
Fix this by not actually deleting lost screens and instead just putting them in an internal vector. Note that this is a memory leak, in the sense that the list will keep growing in an unbounded fashion. But screens are pretty bounded, so it seems like a relatively straightforward way to handle a somewhat rare occurrence.
Note that `deleteLater` doesn't quite work here, because these raw pointers can be held across event loop iterations. A more structural fix is likely some kind of generational arena + weak index pattern, but that's outside the scope of this PR.
|
(I've patched this locally, and am going to run with it for a while to monitor if I see any other crashes.) |
|
I've never managed to get the QML engine to choke on a freed pointer in any prior testing I've done to trigger this class of bug, but I went and did an extra round of testing and didn't find anything for similar cases. There's really only one point we could reasonably return an invalid |
|
Thanks. One thing to note here is that my monitor is a Samsung G9 57 which I run at 7680x2160 240Hz with an AMD GPU. I've seen that cause some issues on monitor wakeup, with link training failing for a bit then succeeding. Both crashes I saw have this stack trace: https://gist.github.com/sunshowers/e439bd41d78fb5f189088471fc8d3e36 |
From what I can tell it's not Quickshell returning a |
|
From my journalctl, both the crashes I saw have the following pattern: So yeah, it seems like this is more likely to happen if a screen is added, removed, then rapidly added again afterwards. This might be why if you're not on a monitor that sometimes fails to do link training (such as a large DisplayPort MST display) you might not see the crash. I'm not sure what facilities Quickshell or QML has to inject monitor add/remove events, but if there is such a mechanism, reproducing this might be more feasible. |
|
FWIW I've been running with my patch for a few days and it's been rock-solid -- previously I'd see around 2 crashes a day. Obviously not ideal to just leak memory but (from my limited understanding) I feel like the language/libraries/type system just doesn't provide enough knowledge for us to do better. |
|
I had a chance to look at your stack trace. It looks like the QObject pointer is being laundered through a ScriptModel which explains the QVariant. I'll have to take a look at what DMS is doing but the gist of the problem is that updates to the model are non atomic and there are multiple potential ways to observe a dead pointer as is (including some dumb ones that just werent taken into consideration). Its not QScreen specific which is a good thing |
|
One of the easy examples, more complex ones exist through the actual model interface including what DMS uses. import QtQuick
import QtQuick.Controls
import QtQuick.Layouts
import Quickshell
FloatingWindow {
ScriptModel {
id: model
}
ColumnLayout {
RowLayout {
Button {
text: "Update Model"
onClicked: model.values = [...Quickshell.screens]
}
Button {
text: "Update Usage Site"
onClicked: v.text = `Model: ${model.values}`
}
}
Text {
text: `Screens: ${Quickshell.screens}`
}
Text {
id: v
}
}
} |
|
I have a patch for toplevel uafs and a strategy for embedded ones I think, Will give that a shot soon |
|
Thanks! Happy to run with a patch you propose for a while. Glad there's a better solution than the hacky one I have. |
|
There's a good chance 4f2c624 fixes it. |
|
Thanks! Running with that patch now. |
533b7a6 to
ead3b00
Compare
2 participants
Previously, when a screen was disconnected,
QuickshellTracked::updateScreensdeleted itsQuickshellScreenInfowrapper. But a copy of thatQObject*may have been put into aQVariantas a bare, unguarded pointer. Deleting the wrapper leaves that pointer dangling, and the next time the engine accesses it, the process segfaults with a use-after-free error.This was observed as intermittent crashes on monitor wakeup under niri with DankMaterialShell, confirmed from two core dumps to be a freed
QuickshellScreenInfoinside aToplevel.screenslist retained by a model.Both core dumps had the following in them:
Fix this by not actually deleting lost screens and instead just putting them in an internal vector. Note that this is a memory leak, in the sense that the list will keep growing in an unbounded fashion. But over the lifetime of a quickshell process there aren't that many screens connected and disconnected, so it seems like a relatively straightforward way to handle this.
Note that
deleteLaterdoesn't quite work here, because these raw pointers can be held across event loop iterations. A more structural fix is likely some kind of generational arena + weak index pattern, but that's outside the scope of this PR.TODO: