auth: Support sd-bus authentication pipelining #48
Conversation
evan-a-a
force-pushed
the
fix_sdbus_clients
branch
3 times, most recently
from
4a39412 to
73acc2e
Compare
|
@alexlarsson or @smcv, can you take a look at this? I believe it solves the concerns present in #26. This change is important for flathub/com.microsoft.Edge#252 and other applications that rely on sd-bus. |
Since sd-dbus clients send the BEGIN before receiving OK, ensure that we look for both to come across the bus in either order. Fixes flatpak#21
evan-a-a
force-pushed
the
fix_sdbus_clients
branch
from
73acc2e to
29dab4a
Compare
| num_messages, | ||
| flags, cancellable, error); | ||
|
|
||
| if (num_read <= 0) |
There was a problem hiding this comment.
This feels a bit wrong. We're reading N bytes, but then get an error, so we throw away the N bytes and report just the error. Typically network code doesn't want to lose data like that. However, maybe in this case it is fine, as on error we stop everything. Should have a comment about it though.
There was a problem hiding this comment.
Actually, this isn't quite true. The code does handle G_IO_ERROR_WOULD_BLOCK. So, you need to handle a partial read that returns a WOULD_BLOCK and return the partial results. And then you need to remember for the next call whether you have seen the CR.
There was a problem hiding this comment.
Honestly, Its probably best to just always return error only on the first byte read, and otherwise return what you read up to now.
|
|
||
| if (!auth_line_is_valid (line_start, line_end)) | ||
| return FIND_AUTH_END_ABORT; | ||
| gssize new_state = client->auth_state; |
There was a problem hiding this comment.
This code relies on the buffer being a complete line (and nothing more than a line). The later is now true, but if you handle errors per the above that is no longer true. So you would need to handle cases where the CRLF is not yet read.
|
It feels a bit lame to read a byte at a time. Thats quite a lot of syscalls... Did you look at how this affects the time spent on authentication? Anyway, i would like @smcv to look at this too. He knows the dbus stuff best. |
| * the final line from the client | ||
| */ | ||
| static gssize | ||
| _my_g_socket_receive_message_safe (GSocket* socket, |
There was a problem hiding this comment.
I would call this something like _g_socket_receive_message_auth_line.
|
The reading of one line a time will also cause the writing to the proxy client to be split up, so the performance is not just in the number of read calls. It will become multiple "network packets" too. |
I agree, it's quite lame. Handling the over-read with the extra buffer management seems to be a "less clean" option to me. I actually based this on how gdbus' server code fixes the handling of sd-bus clients. In GNOME/glib@8f02681, they modified their code to only read one byte at a time, which prevents the over-read case. I didn't benchmark exactly how much this slows down the auth process, but I didn't notice any responsiveness issues in the one sd-bus client I tested. I can try to set up a test harness to collect some timing info. |
|
Actually, after looking at the reference implementation, I have a better idea about how to tackle this while avoiding the single byte reads. I will work on that improvement. |
|
A variant of this has been merged in #54 |
Since sd-dbus clients send the BEGIN before receiving OK, ensure that we look for both to come across the bus in either order.
Fixes #21