Differential D1533

guard access to unsafe config pointer
ClosedPublic
Actions

Authored by sebas on May 4 2016, 2:33 AM.

Details

Reviewers
graesslin
Group Reviewers
Plasma
Commits
R104:338a781e71ee: guard access to unsafe config pointer
R104:3cd5c74d5523: guard access to unsafe config pointer
Summary

This fixes a crashing race condition in the kscreen kded module when
the config cannot be deserialized from the filesystem. In this case,
Serializer returns a nullptr which is then derefenced without
validation.

In practice, we just can't be sure the file can be read, so we need to
make sure that we're not passing configs around which may be empty.

Down the road, I think we should be a bit more careful also in
libkscreen, there's some API that can receive ConfigPtrs, which aren't
validated before dereferencing.

BUG:362586
FIXED-IN:5.6.4
CHANGELOG:Fix crasher in kscreen kded daemon

Test Plan

Can't really test this scenario, since I can't reproduce the crash. All
testing I've done passes, and I've added a bunch of autotests for invalid
configs (separate commit)

Diff Detail

Repository
R104 KScreen
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sebas updated this revision to Diff 3632.May 4 2016, 2:33 AM
sebas retitled this revision from to guard access to unsafe config pointer.
sebas updated this object.
sebas edited the test plan for this revision. (Show Details)
sebas added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptMay 4 2016, 2:33 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
graesslin added a subscriber: graesslin.May 4 2016, 5:52 AM
graesslin added inline comments.
kded/daemon.cpp
170

is this change needed at all. Looks to me like D1532 will cover that as well?

293

deleted new line

sebas added a comment.May 4 2016, 1:06 PM

At least one of these two is needed, but I prefer both to be in there, just to be sure. It's not immediately obvious that config can be null here, so the next person to change this code is less likely to screw it up.

sebas updated this revision to Diff 3648.May 4 2016, 11:02 PM

don't kill that poor \newline

sebas marked an inline comment as done.May 4 2016, 11:02 PM
graesslin added a comment.May 6 2016, 5:59 AM

what about a test case for this? If it's too deep inside KScreen just push without.

sebas added a comment.May 6 2016, 11:10 AM

Yes, it's to deep. It would pretty much require running the daemon.

graesslin accepted this revision.May 6 2016, 11:58 AM
graesslin added a reviewer: graesslin.
This revision is now accepted and ready to land.May 6 2016, 11:58 AM
Closed by commit R104:3cd5c74d5523: guard access to unsafe config pointer (authored by sebas). · Explain WhyMay 6 2016, 12:03 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mkded/daemon.cpp (3 lines)

Diff 3669

View Options

kded/daemon.cpp

Loading...