kwin: Unwrap executable name for desktop file search #116549

samueldr · 2021-03-16T19:52:26Z

KWin for wayland uses the .desktop file to determine whether a process
is allowed to access some wayland services.

This would be fine if there was a stable interface to map a process to a
.desktop file.

Since there is no such interface, they are scanning .desktop files for
one where the executable path matches the resolved file "exe" from
/proc/$PID/exe.

This would be fine, if we didn't wrap many (most?) KDE/Plasma binaries.

Since we are wrapping binaries, the exe symlink points to a wrapped
binary. No .desktop file will match for the wrapped binary.

The solution here is to peel away at the .${name}-wrapped layers until
we have the intended name for the executable.

It is expected that no .desktop file will ever point to a wrapped
binary.

Motivation for this change

Fixing plasma mobile (PR will be coming later), and fixing plasma wayland session in general.

Should fix some of the issues in this linked comment:

nixos/plasma: use upstream xsession/wayland files #100057 (comment)

As for plasma mobile shell, it fixes all of its "tasks" management. Without this, it cannot get the appropriate services from the compositor, meaning it cannot spy at the tasks running, so it cannot switch between tasks, or ask to go back to the desktop, among other things.

The patch was written in a way where it would be the least instrusive possible.

It is highly likely other KDE or Plasma components or apps use similar trickeries to get information from the desktop files. Coordination with upstream to produce a common interface those apps should use instead of readin exe themselves is likely the better solution.

Things done

Ran this with a VM with a fresh plasma mobile shell setup. Worked quite well.

cc @oxalica if you want to test for the issues you were having
cc @worldofpeace @ttuegel as plasma5 peeps

SuperSandro2000 · 2021-03-16T20:16:25Z

pkgs/desktops/plasma-5/kwin/default.nix

@@ -37,6 +37,7 @@ mkDerivation {
  patches = [
    ./0001-follow-symlinks.patch
    ./0002-xwayland.patch
+    ./0001-NixOS-Unwrap-executable-name-for-.desktop-search.patch


Suggested change

./0001-NixOS-Unwrap-executable-name-for-.desktop-search.patch

./0003-NixOS-Unwrap-executable-name-for-.desktop-search.patch

I don't think this is needed. git format-patch handles naming the file from the commit message. The prefix is to order patches in a group, so that e.g. patch 0002 happens after 0001, because the reverse will not necessarily apply.

0001 here is fine as it is not linked to the other two patches. It can be applied before or after the other two patches.

Or, let's say, let's see what the maintainer of the package thinks about that. But forcing arbitrary naming of the patch files compared to what the tooling gives is going to make it more awkward to work with.

Shouldn't git name them continues when you export them all at once?

I think @samueldr is not applying all the patches at once. I usually do git am *.patch when I'm working on a package. I don't have feelings about the file name. I expect it will get renamed the next time anyone works on this package's patches, and that's fine.

r-rmcgibbo · 2021-03-16T20:49:13Z

Result of nixpkgs-review pr 116549 at e583c2a0 run on aarch64-linux 1

30 packages marked as broken and skipped:

libsForQt512.kde-cli-tools
libsForQt512.kdeplasma-addons
libsForQt512.khotkeys
libsForQt512.kinfocenter
libsForQt512.kmenuedit
libsForQt512.krohnkite
libsForQt512.kwin
libsForQt512.kwin-dynamic-workspaces
libsForQt512.kwin-tiling
libsForQt512.parachute
...

5 packages skipped due to time constraints:

kdev-php
kdev-python
kdevelop-unwrapped
libsForQt5.kdeplasma-addons (libsForQt515.kdeplasma-addons ,plasma5Packages.kdeplasma-addons)
libsForQt5.plasma-desktop (libsForQt515.plasma-desktop ,plasma5Packages.plasma-desktop)

14 packages built successfully:

libsForQt5.kde-cli-tools
libsForQt5.khotkeys
libsForQt5.kinfocenter
libsForQt5.kmenuedit
libsForQt5.krohnkite
libsForQt5.kwin
libsForQt5.kwin-dynamic-workspaces
libsForQt5.kwin-tiling
libsForQt5.parachute
libsForQt5.plasma-disks
libsForQt5.plasma-workspace
libsForQt5.powerdevil
libsForQt5.systemsettings
wacomtablet

Result of nixpkgs-review pr 116549 at e583c2a0 run on x86_64-linux 1

30 packages marked as broken and skipped:

libsForQt512.kde-cli-tools
libsForQt512.kdeplasma-addons
libsForQt512.khotkeys
libsForQt512.kinfocenter
libsForQt512.kmenuedit
libsForQt512.krohnkite
libsForQt512.kwin
libsForQt512.kwin-dynamic-workspaces
libsForQt512.kwin-tiling
libsForQt512.parachute
...

6 packages skipped due to time constraints:

kdev-php
kdev-python
kdevelop
kdevelop-unwrapped
libsForQt5.kdeplasma-addons (libsForQt515.kdeplasma-addons ,plasma5Packages.kdeplasma-addons)
libsForQt5.plasma-desktop (libsForQt515.plasma-desktop ,plasma5Packages.plasma-desktop)

14 packages built successfully:

libsForQt5.kde-cli-tools
libsForQt5.khotkeys
libsForQt5.kinfocenter
libsForQt5.kmenuedit
libsForQt5.krohnkite
libsForQt5.kwin
libsForQt5.kwin-dynamic-workspaces
libsForQt5.kwin-tiling
libsForQt5.parachute
libsForQt5.plasma-disks
libsForQt5.plasma-workspace
libsForQt5.powerdevil
libsForQt5.systemsettings
wacomtablet

worldofpeace

I've wondered about this for a while, thanks for fixing it ✨
As I don't know a thing about this kind of code, I will leave the review of the patch to @ttuegel. But from looking it over, you for sure made the patch non-intrusive and commented in a very understandable way A+

worldofpeace · 2021-03-17T03:31:31Z

Ran this with a VM with a fresh plasma mobile shell setup. Worked quite well.

An excellent choice to get working with Mobile NixOS ❤️

oxalica · 2021-03-17T13:28:44Z

Screenshot and Task Manager both work now. ❤️

worldofpeace · 2021-03-21T03:43:38Z

@ofborg test plasma5

(not that this test has any real coverage other than it started)

ttuegel · 2021-03-23T00:32:35Z

What processes are going to call the patched function?

I think this is likely the only approach that will work, but path trickey usually ends, so I have to make sure there's no other way. 😅

samueldr · 2021-03-23T02:02:22Z

The particular instance I was working against was internal to kwin:

https://invent.kde.org/plasma/kwin/-/blob/86c6066551ab536e2bd20f719a8f19578aad0cb0/wayland_server.cpp#L153

Where you can see it uses client->executablePath(). It uses the executable path for the wayland client to lookup the desktop file, to ascertain whether the client has elevated privileges or not.

Logs from the kde-plasmamobile IRC channel:

[19:46:14] <bshah|matrix> So for some reason kwin can not read/find desktop file of plasmashell
[19:46:53] <bshah|matrix> And then it just assumes it to be normal executable and doesn't announce the plasmashell interface to it.
[19:47:53] <samueldr> sounds somewhat plausible
[19:48:10] <samueldr> I'll eat then dig into kwin to see if your suppositions are plausible
[19:48:17] <bshah|matrix> samueldr: how hard it is to test kwin patches for you?
[19:48:30] <bshah|matrix> We can dumb patch this and see if this works.
[19:48:38] <samueldr> relatively trivial
[19:48:47] <samueldr> it's easy to apply, it needs to rebuild
[19:52:56] <bshah|matrix> https://invent.kde.org/plasma/kwin/-/blob/Plasma/5.21/wayland_server.cpp#L150 try shorting rest of code down here by doing early `return true;` here

bshah helped me find a place to work from and figure out where the issue was.

[20:54:19] <bshah|matrix> samueldr: https://phabricator.kde.org/D22571
[20:55:02] <samueldr> thanks, that will help putting a name on things
[20:56:13] <samueldr> should I have seen "Interace [...] not in X-KDE-Wayland-Interfaces of [...]" in the error logs?
[20:56:20] -*- samueldr tries again without the fix
[21:03:47] <samueldr> oh, indeed, a lot of "interface [...] not in X-KDE-Wayland-Interfaces of [...]" in the log

Then I extrapolated from there, figuring out where it went wrong. Traced it to service_util which I guess would be used in other places to sniff out a process.

pkgs/desktops/plasma-5/kwin/0001-NixOS-Unwrap-executable-name-for-.desktop-search.patch

ttuegel · 2021-03-23T10:46:50Z

pkgs/desktops/plasma-5/kwin/default.nix

@@ -37,6 +37,7 @@ mkDerivation {
  patches = [
    ./0001-follow-symlinks.patch
    ./0002-xwayland.patch
+    ./0001-NixOS-Unwrap-executable-name-for-.desktop-search.patch


I think @samueldr is not applying all the patches at once. I usually do git am *.patch when I'm working on a package. I don't have feelings about the file name. I expect it will get renamed the next time anyone works on this package's patches, and that's fine.

samueldr · 2021-03-24T19:45:58Z

Applied both suggestions and verified it still worked as expected, by being able to use Plasma Mobile.

worldofpeace · 2021-03-25T11:05:03Z

Aargh, conflicts. Feel free to merge once you resolve them.

KWin for wayland uses the `.desktop` file to determine whether a process is allowed to access some wayland services. This would be fine if there was a stable interface to map a process to a `.desktop` file. Since there is no such interface, they are scanning `.desktop` files for one where the executable path matches the resolved file "exe" from `/proc/$PID/exe`. This would be fine, if we didn't wrap many (most?) KDE/Plasma binaries. Since we are wrapping binaries, the `exe` symlink points to a wrapped binary. No `.desktop` file will match for the wrapped binary. The solution here is to peel away at the `.${name}-wrapped` layers until we have the intended name for the executable. It is expected that no `.desktop` file will ever point to a wrapped binary.

samueldr · 2021-03-25T22:33:52Z

(Currently waiting on the mass rebuild on my end to confirm things do still work...)

see NixOS/nixpkgs#116549 * gnu/packages/patches/kwin-unwrap-executable-name-for-dot-desktop-search.patch: New file. * gnu/local.mk (dist_patch_DATA): Add it. * gnu/packages/kde-plasma.scm (kwin)[origin]: Use it. Signed-off-by: Ludovic Courtès <ludo@gnu.org>

samueldr added the 6.topic: qt/kde label Mar 16, 2021

samueldr requested review from ttuegel and worldofpeace March 16, 2021 19:52

samueldr mentioned this pull request Mar 16, 2021

General problems with environment variable wrappers #60260

Open

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 labels Mar 16, 2021

SuperSandro2000 reviewed Mar 16, 2021

View reviewed changes

worldofpeace approved these changes Mar 17, 2021

View reviewed changes

oxalica mentioned this pull request Mar 17, 2021

nixos/plasma: use upstream xsession/wayland files #100057

Merged

10 tasks

ttuegel reviewed Mar 23, 2021

View reviewed changes

peterhoeg mentioned this pull request Mar 24, 2021

nixos/plasma5: make it run using systemd and wayland #117102

Merged

10 tasks

samueldr force-pushed the fix/kwin-wayland-desktop-files branch from e583c2a to 7d714d1 Compare March 24, 2021 19:45

ttuegel approved these changes Mar 24, 2021

View reviewed changes

worldofpeace approved these changes Mar 25, 2021

View reviewed changes

samueldr force-pushed the fix/kwin-wayland-desktop-files branch from 7d714d1 to 100db99 Compare March 25, 2021 18:15

samueldr force-pushed the fix/kwin-wayland-desktop-files branch from 100db99 to 1ba2080 Compare March 25, 2021 18:17

samueldr merged commit df63ef4 into NixOS:master Mar 26, 2021

samueldr deleted the fix/kwin-wayland-desktop-files branch March 26, 2021 01:31

peterhoeg mentioned this pull request Apr 5, 2021

Plasma Night Color is broken #118579

Closed

Artturin mentioned this pull request Sep 23, 2021

plasma-workspace: fix app_id by moving bin/.plasmashell-wrapped to #139213

Closed

12 tasks

jansol mentioned this pull request Nov 15, 2021

Setup hook to make Exec= paths absolute in desktop files #68035

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kwin: Unwrap executable name for desktop file search #116549

kwin: Unwrap executable name for desktop file search #116549

samueldr commented Mar 16, 2021

SuperSandro2000 Mar 16, 2021

samueldr Mar 16, 2021 •

edited

Loading

SuperSandro2000 Mar 17, 2021

ttuegel Mar 23, 2021

r-rmcgibbo commented Mar 16, 2021 •

edited

Loading

worldofpeace left a comment

worldofpeace commented Mar 17, 2021

oxalica commented Mar 17, 2021

worldofpeace commented Mar 21, 2021

ttuegel commented Mar 23, 2021

samueldr commented Mar 23, 2021 •

edited

Loading

ttuegel Mar 23, 2021

samueldr commented Mar 24, 2021 •

edited

Loading

worldofpeace commented Mar 25, 2021

samueldr commented Mar 25, 2021

	./0001-NixOS-Unwrap-executable-name-for-.desktop-search.patch
	./0003-NixOS-Unwrap-executable-name-for-.desktop-search.patch

kwin: Unwrap executable name for desktop file search #116549

kwin: Unwrap executable name for desktop file search #116549

Conversation

samueldr commented Mar 16, 2021

Motivation for this change

Things done

SuperSandro2000 Mar 16, 2021

Choose a reason for hiding this comment

samueldr Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

SuperSandro2000 Mar 17, 2021

Choose a reason for hiding this comment

ttuegel Mar 23, 2021

Choose a reason for hiding this comment

r-rmcgibbo commented Mar 16, 2021 • edited Loading

worldofpeace left a comment

Choose a reason for hiding this comment

worldofpeace commented Mar 17, 2021

oxalica commented Mar 17, 2021

worldofpeace commented Mar 21, 2021

ttuegel commented Mar 23, 2021

samueldr commented Mar 23, 2021 • edited Loading

ttuegel Mar 23, 2021

Choose a reason for hiding this comment

samueldr commented Mar 24, 2021 • edited Loading

worldofpeace commented Mar 25, 2021

samueldr commented Mar 25, 2021

samueldr Mar 16, 2021 •

edited

Loading

r-rmcgibbo commented Mar 16, 2021 •

edited

Loading

samueldr commented Mar 23, 2021 •

edited

Loading

samueldr commented Mar 24, 2021 •

edited

Loading