flatpak / xdg-dbus-proxy

GNU Lesser General Public License v2.1
53 stars 21 forks source link

gets confused by sd-bus clients pipelining the SASL handshake #21

Open smcv opened 3 years ago

smcv commented 3 years ago

On systemd/systemd#16610, @refi64 writes:

sd-bus sends the BEGIN command right along with AUTH EXTERNAL, rather than using it to acknowledge an OK. I found this because it confuses Flatpak's xdg-dbus-proxy, which generally expects BEGIN to be sent after the auth is complete.

sd-bus does this pipelining deliberately to reduce round-trips, and @poettering considers it to be an xdg-dbus-proxy bug that it gets confused by pipelining the SASL handshake.

It is not at all clear to me that the D-Bus Specification ever intended to allow pipelining the SASL handshake, and its current wording can be interpreted as forbidding pipelining the SASL handshake, but it does work in practice against the major server implementations (the reference libdbus, GDBus and sd-bus). Ideally xdg-dbus-proxy should only process as much text as it is ready to deal with (in practice this means only the next line), and defer the rest until its next return to the main loop.

20 might be related to this.

refi64 commented 3 years ago

Working on a fix for this, #20 was sort of related in that I was also debugging sd-bus not working, but it's technically a separate issue.

zeenix commented 2 months ago

Just FYI, zbus started pipelining the handshake in its latest releases (4.1.2 and 4.2) and this means zbus flatpak apps breaking. Seems @mardy already provided a fix a month ago but it was never reviewed. Can a maintainer have a look and merge if it makes sense?

zeenix commented 2 months ago

Btw while I added a workaround for this in zbus 4.2.1, I wanted to point out that I wouldn't want to keep this workaround forever in zbus. I'm thinking ideally only for 6 months (but could be much longer if i don't need to touch the client-side handshake part much).

Also sdbus remains broken (because of Lennart not being as cooperative as me 😂) even though that probably just means you can't use busctl, which may not be a biggie.

If other libs would also start pipelining handshakes (maybe some already do?), we'll have an even bigger problem.

smcv commented 2 months ago

while I added a workaround for this in zbus 4.2.1, I wanted to point out that I wouldn't want to keep this workaround forever in zbus. I'm thinking ideally only for 6 months

If that's the case, then Flatpak apps that use zbus are unlikely to be runnable on older LTS distros (Debian, Ubuntu LTS, RHEL family, SLES family, etc.) because any fix for this that might get merged into xdg-dbus-proxy will only fix future x-d-p and Flatpak versions, and will not somehow retroactively fix the older versions that already exist in LTS distros.

zeenix commented 2 months ago

If that's the case, then Flatpak apps that use zbus are unlikely to be runnable on older LTS distros (Debian, Ubuntu LTS, RHEL family, SLES family, etc.) because any fix for this that might get merged into xdg-dbus-proxy will only fix future x-d-p and Flatpak versions, and will not somehow retroactively fix the older versions that already exist in LTS distros.

Why not? If an issue breaks Flatpak completely against at least (that we know of) 2 separate D-Bus client implementations, then IMO it should be considered critical and hence backported. The same goes for #46, which I don't even have any idea how to workaround.

zeenix commented 2 months ago

and hence backported

Actually I'm not sure there is even backporting needed. At least Debian stable seems to already have xdg-dbus-proxy 0.1.4, which is quite recent. If the coming release is going to be 0.1.6, it's a micro update. I'm no Debian packager but doesn't micro update qualify to be pushed to stable?

Update: I thought 0.1.4 is very recently because it appears at the top in the git log but I only realized after writing the comment that it's only because this project hasn't been super active.

matthiasclasen commented 2 months ago

That is just not how supporting multiple platforms and stables releases works, @zeenix.

Interoperability requires some restraint on the embracing every latest cool idea that Lennart comes up with.

These roundtrips are saving you a few microseconds at best. Is that really worth breaking the majority of existing flatpak users?

zeenix commented 2 months ago

Interoperability requires some restraint on the embracing every latest cool idea that Lennart comes up with.

I don't think this is new in sdbus. I think sdbus has been doing this for years afaik. I could be wrong though but he's been telling me I should do that for a long while now. :)

These roundtrips are saving you a few microseconds at best.

slomo did some very basic benchmarking and he saw that my workaround for flatpak, adds 13% increase in time needed to establish a single connection. I wouldn't call a 13% difference insignificant. Also to keep in mind that zbus isn't completely pipelining the handshake (yet).

X-m7 commented 2 months ago

I don't think this is new in sdbus. I think sdbus has been doing this for years afaik. I could be wrong though but he's been telling me I should do that for a long while now. :)

sd-bus has been doing it since mid 2020 or so at least, judging by when this issue report was opened.

matthiasclasen commented 2 months ago

slomo did some very basic benchmarking and he saw that my workaround for flatpak, adds 13% increase in time needed to establish a single connection. I wouldn't call a 13% difference insignificant.

13% of a few microseconds is very much insignificant, in particular for a thing that happens once in an application's run.

zeenix commented 2 months ago

13% of a few microseconds is very much insignificant

It's not a few microseconds even on high end machines.

in particular for a thing that happens once in an application's run.

That's true for typical applications but not always. If I could assume that's always the case, I also won't see any value in this optimisation.

poettering commented 2 months ago

Interoperability requires some restraint on the embracing every latest cool idea that Lennart comes up with.

Ummm. sd-bus has been pipelining the auth logic since day one, i.e. March 2013, it never did it differently. I think it did that even before this flatpak dbus proxy hack existed, no? It's like more than a decade.

I mean, I have no beef here, I don't think the audience for flatpak and for sd-bus really overlap that much, but just wanted to correct this here.

And also, it does make a major difference to pipeline this, when we switched from old libdbus over to sd-bus it did shove multiple 100ms off our boot times, since we had so many tools going through this that we synced on. If you have many short-lived connections it's the only thing that makes D-Bus manageable at all I think, the roundtrips traditional dbus libraries did just were prohibitively expensive.

If you care about performance at all for IPC, then it really is the roundtrips that kill everything. The time you spend on marshalling really never matter, it all comes down to roundtrips, hence pipelining is really what you have to do.

(i figure gnome mostly uses gio/gdbus as dbus implementation. it's probably the worst implementation performance-wise you can use, because it moves dbus communication to a thread, and thus adds another level of roundtrips to the whole thing, tanking latencies in a way that I guess auth pipelining just can't really recover from).