Differential D10549

Don't crash if the cursor theme fails to create
ClosedPublic
Actions

Authored by graesslin on Feb 15 2018, 5:08 PM.

Details

Reviewers
davidedmundson
romangg
Group Reviewers
KWin
Plasma
Commits
R108:2ea5153e1c97: Don't crash if the cursor theme fails to create
Summary

If the cursor theme failed to create KWin crashed due to an endless
recursion. There are two reasons for this fault:

  1. When the physical size does not exist we perform a division by 0

which results in an invalid size going into wl_cursor_theme_load

  1. We emit the signal that the cursor theme changed even if it didn't

change thus creating an endless recursion

This change addresses both problems: it checks that the size is not 0
and changes the handling for theme update to only destroy the previous
theme if the new theme could be created and only emits the signal if
things change.

BUG: 390314
FIXED-IN: 5.12.2

Test Plan

Added a new test case which crashed with old code

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.Feb 15 2018, 5:08 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 15 2018, 5:08 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
graesslin requested review of this revision.Feb 15 2018, 5:08 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 15 2018, 5:08 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 21 2018, 5:10 PM
davidedmundson accepted this revision.Feb 22 2018, 7:57 PM
This revision is now accepted and ready to land.Feb 22 2018, 7:57 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 22 2018, 7:57 PM
romangg requested changes to this revision.Feb 24 2018, 7:48 PM
romangg added a subscriber: romangg.
romangg added inline comments.
wayland_cursor_theme.cpp
69–78

Only do if theme really changes (theme && m_theme != theme).

79

On !theme: Add debug output that themecould not be loaded.

This revision now requires changes to proceed.Feb 24 2018, 7:48 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 24 2018, 7:48 PM
graesslin added inline comments.Feb 25 2018, 8:02 AM
wayland_cursor_theme.cpp
69–78

As it returns a pointer it is impossible that we get the same pointer back.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 25 2018, 8:02 AM
romangg added inline comments.Feb 25 2018, 9:18 AM
wayland_cursor_theme.cpp
69–78

Can we check differently if it is the same theme? Or should we in any case emit themeChanged when we are at this point already?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 25 2018, 9:18 AM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 25 2018, 9:27 AM
graesslin added inline comments.Feb 25 2018, 1:26 PM
wayland_cursor_theme.cpp
69–78

As it's connected to Cursor::themeChanged we can assume that we don't have to check here. If it would be invoked for the same theme the bug would be in the Cursor class. Given that I would say we can ignore it here. I just checked in cursor.cpp and there it is properly guarded to only emit the themeChanged signal if the theme truly changed.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 25 2018, 1:26 PM
romangg accepted this revision.Feb 26 2018, 11:19 AM
This revision is now accepted and ready to land.Feb 26 2018, 11:19 AM
Closed by commit R108:2ea5153e1c97: Don't crash if the cursor theme fails to create (authored by graesslin). · Explain WhyMar 4 2018, 8:43 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28560

View Options

autotests/integration/CMakeLists.txt

Loading...
View Options

autotests/integration/dont_crash_cursor_physical_size_empty.cpp

Loading...
View Options

wayland_cursor_theme.cpp

Loading...