https://bugs.kde.org/show_bug.cgi?id=510116
Terry Raimondo <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REPORTED |CONFIRMED CC| |[email protected] Ever confirmed|0 |1 --- Comment #4 from Terry Raimondo <[email protected]> --- Root cause analysis, with a symbolised coredump. I hit this on Plasma 6.7.3 (kwin-6.7.3-2.fc43, Fedora Kinoite 43, Qt 6.10.3, glibc 2.42, Mesa 25.3.6, AMD RX 7900 XTX, Wayland session). Twice, both times a few seconds to a minute after launching a 32-bit Windows game (World of Warcraft 3.3.5a) through Steam + Proton, i.e. an Xwayland client that constantly grabs and releases the pointer for mouselook. The abort site is not the bug site. This is a 24-byte heap buffer overflow, and glibc aborts at whatever free() next touches the clobbered chunk header. My two cores abort in completely different places for that reason: one inside SurfaceState::mergeInto, the other inside Mesa's amdgpu_bo_fence_wait. The original report's abort inside removeExtension -> extensions.erase() is a third detection point for the same corruption. THE OVERFLOW ============ Stack of the crashing thread (core 1): malloc_printerr ("free(): invalid next size (fast)") malloc.c:5895 _int_free_chunk malloc.c:4670 std::default_delete<KWin::RawSurfaceAttachedState>::operator() std::unique_ptr<KWin::RawSurfaceAttachedState>::~unique_ptr std::unordered_map<KWin::RawSurfaceExtension*, std::unique_ptr<KWin::RawSurfaceAttachedState>>::~unordered_map KWin::SurfaceState::mergeInto (this=0x558301589b30, target=0x5583014ebc60) src/wayland/surface.cpp:667 KWin::SurfaceInterfacePrivate::applyState KWin::Transaction::apply src/wayland/transaction.cpp:169 QtWaylandServer::wl_surface::handle_commit (via ffi_call/wl_closure_invoke) KWin::Display::dispatchEvents src/wayland/display.cpp:138 Line 667 is the closing brace of SurfaceState::mergeInto, so this is the scope exit that destroys the local `previousExtensions` map created on line 652. The node being destroyed has a type-mismatched key and value: key 0x558301564600 -> vtable for KWin::SurfaceExtension<KWin::LockedPointerV1InterfacePrivate, KWin::LockedPointerV1Commit> value 0x55830156a130 -> vtable for KWin::ConfinedPointerV1Commit The value object's memory shows the overflow directly: 0x55830156a120: <malloc chunk header> size = 0x51 -> 72 usable bytes 0x55830156a130: vtable ConfinedPointerV1Commit 0x55830156a138 .. 0x55830156a170: zeros (in bounds; last in-bounds word 0x...170) 0x55830156a178: 0x40932c0000000000 = double 1226.0 <- offset 72, first OOB word 0x55830156a180: 0x4083780000000000 = double 623.0 0x55830156a188: 0x0000000000000001 and gdb confirms the size delta: (gdb) print sizeof(KWin::ConfinedPointerV1Commit) -> 72 (gdb) print sizeof(KWin::LockedPointerV1Commit) -> 96 LockedPointerV1Commit adds std::optional<QPointF> hint at offset 72. ConfinedPointerV1Commit ends at 72 and its 0x50 chunk has exactly 72 usable bytes, zero slack. So writing a LockedPointerV1Commit into a ConfinedPointerV1Commit puts the cursor position hint straight onto the next chunk's size field. Those two doubles are a set_cursor_position_hint(1226, 623). 0x50 is fastbin-sized, which is where the "(fast)" in the glibc message comes from. HOW THE TYPES GET CONFUSED ========================== 1. SurfaceInterface::removeExtension() (src/wayland/surface.cpp:501) erases the extension from d->pending->extensions, from d->subsurface.transaction, and from the d->firstTransaction chain. It never erases it from d->current->extensions. So after an extension is destroyed, `current` keeps {dangling RawSurfaceExtension* -> stale state} until the next mergeInto happens to prune it. 2. The map is keyed by raw RawSurfaceExtension*. Once the extension object is freed, a different extension can be allocated at the same address. 3. SurfaceState::mergeInto (surface.cpp:652-664) looks the key up with no type check: auto previousExtensions = std::exchange(target->extensions, {}); for (const auto &[extension, sourceState] : extensions) { std::unique_ptr<RawSurfaceAttachedState> targetState; if (auto it = previousExtensions.find(extension); it != previousExtensions.end()) { targetState = std::move(it->second); } else { targetState = extension->createState(); } sourceState->mergeInto(targetState.get()); target->extensions[extension] = std::move(targetState); } With a recycled address, find() hits the stale entry, so targetState is the old state object of the wrong type. 4. SurfaceAttachedState<Self>::mergeInto (src/wayland/surface.h:507) then does an unchecked downcast and a full-object assignment: auto self = static_cast<Self *>(this); auto other = static_cast<Self *>(target); *other = std::exchange(*self, Self{}); sizeof(Self) bytes are written into an allocation sized for a different type. LockedPointerV1InterfacePrivate and ConfinedPointerV1InterfacePrivate are the pair that makes this reachable in practice: they are the only two SurfaceExtension users that are created and destroyed repeatedly over a single live surface, and their Commit types differ in size by exactly the 24 bytes seen above. An Xwayland client doing mouselook churns lock/confine constraints continuously on one surface, which is why this reproduces with Proton games and not with ordinary applications. Two details in the dump corroborate the ordering. Implicit assignment does not copy the vptr, so the victim keeps its ConfinedPointerV1Commit vtable while holding LockedPointer data. And ~SurfaceExtension resets the base subobject's vptr to the template's own vtable before the memory is freed, which is why the dangling key resolves to "vtable for SurfaceExtension<LockedPointerV1InterfacePrivate, LockedPointerV1Commit>" rather than to "vtable for LockedPointerV1InterfacePrivate + N". That symbol is the fingerprint of freed memory. Consistent with: a) a confined-pointer extension at address A registers a ConfinedPointerV1Commit in current->extensions[A]; b) it is destroyed; removeExtension leaves {A -> state} in current; c) a locked-pointer extension is allocated at A; d) the next commit merges a LockedPointerV1Commit into the 72-byte confined state -> 24-byte overflow; e) some later free() touching that region aborts. SECOND DEFECT IN THE SAME AREA ============================== SurfaceInterfacePrivate::applyState iterates current->extensions and calls extension->applyState(state) on the key. After step (b) above that key is dangling, so this is a use-after-free on the next commit. This looks like what comment #3 hit: SIGSEGV in KWin::ConfinedPointerV1Interface::region() reached via KWin::Transaction::apply(). FIX === The minimal fix is for removeExtension() to also drop the entry from the current state: void SurfaceInterface::removeExtension(RawSurfaceExtension *extension) { d->current->extensions.erase(extension); // <-- missing d->pending->extensions.erase(extension); ... } That closes both the type confusion and the applyState use-after-free. Note that MR !9606 / commit 8f0882b2 ("pointer_input: rework pointer constraints", milestone 6.8) removes LockedPointerV1Commit and ConfinedPointerV1Commit entirely and moves pointer-constraint state into SurfaceState. That makes this crash unreachable on master, but only incidentally: the underlying SurfaceExtension key-lifetime defect is still there on master, and the six remaining SurfaceExtension users (XdgToplevel, XdgPopup, XwaylandSurfaceV1, LayerSurfaceV1, PlasmaShellSurface, XXPipV1) are only safer because they are not created and destroyed repeatedly on a live surface. The defect was introduced by 192cbfdc ("wayland: Rework surface attached state", MR !7426), first released in Plasma 6.4.0. Plasma/6.7 HEAD still has the incomplete removeExtension and still declares both Commit types, so 6.4.0 through 6.7.x are all affected and !9606 was not backported. I can provide the coredumps or run further gdb queries against them on request. -- You are receiving this mail because: You are watching all bug changes.
