Send keyboard map from a read only source
AbandonedPublic

Authored by davidedmundson on Mar 17 2020, 5:25 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

We don't want one client to modify the keymap sent to others. It will
cause crashes or worse.

This patch reopens the file after it has been set to read only, then
unlinks the original file.

BUG: 381674

Test Plan

Ran test attached to the bug
future dolphin instances start up without crashing
those instances seem to have a working keymap

Diff Detail

Repository
R108 KWin
Branch
davidedmundson/simplify_outputs
Lint
Lint ErrorsExcuse: adf
SeverityLocationCodeMessage
Errorinput.h:310CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 23831
Build 23849: arc lint + arc unit
davidedmundson created this revision.Mar 17 2020, 5:25 PM
Restricted Application added a project: KWin. · View Herald TranscriptMar 17 2020, 5:25 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Mar 17 2020, 5:25 PM
zzag added a subscriber: zzag.Mar 17 2020, 6:25 PM
zzag added inline comments.
xkb.cpp
297

Would it be okay if the QTemporaryFile is allocated on the stack? e.g.

QTemporaryFile keymapFile;

(asking out of curiosity)

313

Noob question: why do we need another QFile object?

I'm just trying to understand why we can't do something along these lines

tmp->setPermissions(QFileDevice::ReadOwner);
m_seat->setKeymap(tmp->handle(), size);
apol added a subscriber: apol.Mar 17 2020, 6:51 PM
apol added inline comments.
xkb.cpp
297

Yes it should be possible, we would need to find a way to do the reset part though, so just removing the file.

davidedmundson added inline comments.Mar 17 2020, 7:09 PM
xkb.cpp
313

I tired that first.

Both with Qt API as well as fchmod directly on this fd and on a dup'd version.

It didn't work.

There's a really good test in the bug report

Fwiw, I checked wlroots.

They seem to have a mmaped address per client and copy keymap contents to everyone individually, which obviously also is safe.

zzag added a comment.Mar 18 2020, 7:43 AM

They seem to have a mmaped address per client and copy keymap contents to everyone individually, which obviously also is safe.

Like this patch does? https://phabricator.kde.org/D14910

zzag added a comment.EditedMar 18 2020, 2:08 PM

I've rebased D14910 on master for test purposes. I wouldn't mind either solution (this one or D14910).

davidedmundson abandoned this revision.Mar 18 2020, 2:28 PM

I wouldn't mind either solution (this one or D14910).

His has a unit test. Lets do that.