Don't include the pid in the dbus path when on flatpak
ClosedPublic

Authored by apol on May 8 2017, 5:44 PM.

Details

Summary

Flatpak requests us to list the services that will be exposed to the outside,
better have it the name of the application without a random number in the end.
In these systems the pid doesn't add much anyway, because two instances of
the application will always end up with the same pid anyway, from the point
of view of the application.

Test Plan

Applied the patch to flatpak-kde-runtime, fixes the issue for katomic and blinken.

Diff Detail

Repository
R271 KDBusAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.May 8 2017, 5:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 8 2017, 5:44 PM
apol edited the test plan for this revision. (Show Details)May 8 2017, 6:56 PM
jgrulich edited edge metadata.May 9 2017, 4:56 AM

Looks good, it really doesn't make sense to use pid when running app in flatpak, because you cannot guarantee what pid it will get and without that you cannot allow dbus access in app's flatpak manifest.

aacid added a subscriber: aacid.May 9 2017, 8:36 AM

This means we get the same path if we start the process twice, does it still work? Don't you get some kind of collision at the dbus level?

apol added a comment.May 10 2017, 12:59 PM
In D5775#108315, @aacid wrote:

This means we get the same path if we start the process twice, does it still work? Don't you get some kind of collision at the dbus level?

Yes, with this patch you can only open 1 of the applications (despite multiple). This is the error I get:

katomic(3)/(default) unknown: "Couldn't register name 'org.kde.katomic' with DBUS - another process owns it already!"

While this is a regression, we'll still need to come up with an alternative, it will be an alternative ad-hoc to flatpak (and potentially anything else wrapping dbus).

aacid added a comment.May 10 2017, 2:36 PM

I don't think this is a fix.

I can't really i good faith recommend people to use Okular from a flatpak if that means they can't start two okular instances.

In D5775#108558, @aacid wrote:

I don't think this is a fix.

I can't really i good faith recommend people to use Okular from a flatpak if that means they can't start two okular instances.

You have this problem even without this patch. When you now run two instances of Okular in flatpak then both will probably have same pid. Point of this patch is to make the dbus name predictable so you can allow access to it in flatpak manifest.

apol added a comment.May 11 2017, 3:29 PM
In D5775#108558, @aacid wrote:

I don't think this is a fix.

I can't really i good faith recommend people to use Okular from a flatpak if that means they can't start two okular instances.

You have this problem even without this patch. When you now run two instances of Okular in flatpak then both will probably have same pid. Point of this patch is to make the dbus name predictable so you can allow access to it in flatpak manifest.

Albert has a point. Maybe it's something to bring to flatpak upstream, for example it could make sense to expose the actual pid as an env variable? Or a different but unique identifier?

In D5775#108825, @apol wrote:
In D5775#108558, @aacid wrote:

I don't think this is a fix.

I can't really i good faith recommend people to use Okular from a flatpak if that means they can't start two okular instances.

You have this problem even without this patch. When you now run two instances of Okular in flatpak then both will probably have same pid. Point of this patch is to make the dbus name predictable so you can allow access to it in flatpak manifest.

Albert has a point. Maybe it's something to bring to flatpak upstream, for example it could make sense to expose the actual pid as an env variable? Or a different but unique identifier?

Problem is that the name, even when the pid would be unique, won't be predictable. You still need to change it to something like org.kde.appName.pid so you can use org.kde.appName.* in flatpak manifest. Problem is that you need to allow to own those dbus services and because of that the first instance won't let the other instance to register itself.

apol added a comment.May 12 2017, 8:32 AM

Problem is that the name, even when the pid would be unique, won't be predictable. You still need to change it to something like org.kde.appName.pid so you can use org.kde.appName.* in flatpak manifest. Problem is that you need to allow to own those dbus services and because of that the first instance won't let the other instance to register itself.

Right, so the PID is not what we want maybe. I have the impression that the usage of PID here is a bit random though, the important thing is to be able to expose all okular instances to dbus.

Alternate suggestion:
register app-name + QDbus::connection()->baseServiceName.

it'll be guaranteed unique.

(though does anything even use the DBus service name in multi mode? It doesn't seem to be very useful. kquitapp5 doesn't support it.)

dfaure added a subscriber: dfaure.May 13 2017, 12:22 PM

d_ed: I use the DBus name in multi mode to talk to running processes ;-)

(for introspection, debugging, automation, etc.)
apol updated this revision to Diff 14541.May 15 2017, 11:00 AM

Fix regression

I think this still doesn't solve the general problem we have with sandboxed applications as you cannot still allow this dbus service in app manifest. You can use "org.kde.AppName.*" , but then the dbus name would need to be in form of "org.kde.AppName.appPid" or "org.kde.AppName.baseServiceName", mean that you have to replace dash with a dot. When you use the dash the wildcard won't apply. On the other hand you also cannot use o.k.AppName.appPid or so because it has to start with a letter and not with a number or any special character which is case of baseService() returning e.g. :1:30.

apol updated this revision to Diff 14642.May 17 2017, 5:49 PM

Fix reverse domain notation

+1

good idea, it should solve all the problems, we should use similar approach in KNotifications for SNI support.

+1

good idea, it should solve all the problems, we should use similar approach in KNotifications for SNI support.

Actually this won't work for KNotifications and SNI, still problem with --own-name=o.k.StatusNotifierItem.appName.* which won't let you register another SNI as the previous one owns all the service names. This however should work for this as org.kde.appName.* is allowed by default.

apol added a comment.May 18 2017, 8:40 AM

+1

good idea, it should solve all the problems, we should use similar approach in KNotifications for SNI support.

Actually this won't work for KNotifications and SNI, still problem with --own-name=o.k.StatusNotifierItem.appName.* which won't let you register another SNI as the previous one owns all the service names. This however should work for this as org.kde.appName.* is allowed by default.

Are you sure that we can't change the status notifier name to something else e.g. org.kde.appname.StatusNotifierItem.<stuff>?

In D5775#110551, @apol wrote:

+1

good idea, it should solve all the problems, we should use similar approach in KNotifications for SNI support.

Actually this won't work for KNotifications and SNI, still problem with --own-name=o.k.StatusNotifierItem.appName.* which won't let you register another SNI as the previous one owns all the service names. This however should work for this as org.kde.appName.* is allowed by default.

Are you sure that we can't change the status notifier name to something else e.g. org.kde.appname.StatusNotifierItem.<stuff>?

You are totally right, I didn't realize that we can change it to have name which is already allowed by flatpak.

apol added a comment.May 22 2017, 6:45 PM

Can somebody point out a problem with this approach? Or approve instead?
If nobody does I'll just push the patch.

aacid added a comment.May 22 2017, 9:25 PM
In D5775#111218, @apol wrote:

Can somebody point out a problem with this approach? Or approve instead?
If nobody does I'll just push the patch.

maybe not change the serviceName if not using flatpack?

apol added a comment.May 23 2017, 12:00 AM
In D5775#111230, @aacid wrote:

maybe not change the serviceName if not using flatpack?

We are not changing it, that's why we do the inSandbox bit.

aacid added a comment.May 23 2017, 7:21 PM

.kdbus

In D5775#111234, @apol wrote:
In D5775#111230, @aacid wrote:

maybe not change the serviceName if not using flatpack?

We are not changing it, that's why we do the inSandbox bit.

What about the .kdbus part?

apol updated this revision to Diff 14782.May 23 2017, 9:38 PM

Keep the old reverse.domain-pid notation when not sandboxed

aacid accepted this revision.May 24 2017, 9:16 PM
This revision is now accepted and ready to land.May 24 2017, 9:16 PM
This revision was automatically updated to reflect the committed changes.
dfaure added inline comments.Aug 23 2017, 8:36 PM
src/kdbusservice.cpp
98

this if() seems useless (double lookup). You already get the value, and then you even check it for empty.

105

KF5 coding style: { ... } even around single-line statements.

apol marked 2 inline comments as done.Sep 8 2017, 3:39 PM
aacid added a comment.Sep 8 2017, 5:49 PM

So if i have two okular open, how do i access the second via dbus?