Support passing unix fds #54

michallowasrzechonek-silvair · 2020-05-29T19:48:40Z

Hi,

We've started working on #33. It's still very much in progress, but I wanted to give you heads up and get some feedback about the general approach.

On the other side, we'll be using https://git.kernel.org/pub/scm/libs/ell/ell.git/

acrisci · 2020-06-01T00:26:09Z

Yeah looks ok. Just a few notes.

Try to void the monkey patching if possible.
We need to leave room to support this in the high level interface.
The spec seems to say fds can be passed at any time during the message, not just after the first byte and will definitely not be passed if the header is not present (if that affects performance).
You might need to add the NEGOTIATE_UNIX_FDS command to the auth protocol.

michallowasrzechonek-silvair · 2020-06-01T06:46:50Z

Unfortunately, asyncio doesn't expose sendmsg/recvmsg coroutines :( What we could do is keep these as regular functions, at the cost of slightly ugly invocation.

As for passing fds at any point, I'm pretty sure that all existing implementations do that while sending the initial fixed size header. Allowing fds to be passed along the body is somewhat problematic, because recvmsg is atomic, i.e. it truncates messages if buffer is too small, and has a buffer size limit. And dbus-next parses messages on the fly, so we would need to refactor the whole unmarshalling part to use a in-memory buffer.

High level API and negotiation: ok.

rafalgajda-silvair · 2020-06-24T12:33:04Z

Hi all,
Current version seams to be working and all tests are passing. Could you take a moment to review the code changes and see if I missed something?
Much obliged.

michallowasrzechonek-silvair · 2020-06-26T09:58:52Z

dbus_next/_private/unmarshaller.py

+
+        if self.sock:
+            try:
+                msg, ancdata, *_ = self.sock.recvmsg(1, socket.CMSG_LEN(16 * unix_fds.itemsize),


I think we should replace 1 with the size of fixed message header. Also, why 16? Let's at least extract magic number into a constant.

michallowasrzechonek-silvair · 2020-06-26T09:59:09Z

dbus_next/_private/unmarshaller.py

@@ -228,7 +250,8 @@ def _unmarshall(self):
        sender = header_fields.get(HeaderField.SENDER.name)
        signature = header_fields.get(HeaderField.SIGNATURE.name, '')
        signature_tree = SignatureTree(signature)
-        unix_fds = header_fields.get(HeaderField.UNIX_FDS.name)
+        # TODO: check unix_fds against the header


I'm not sure what to check here

michallowasrzechonek-silvair · 2020-06-26T09:59:37Z

dbus_next/aio/message_bus.py

+            ancdata = [(socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array("i", msg.unix_fds))] \
+                if msg.unix_fds else None
+
+            await self.sock_sendmsg(self._sock, buf[:1], ancdata=ancdata)


Same here, send the whole fixed-size header first, then the rest.

michallowasrzechonek-silvair · 2020-06-27T06:53:12Z

@acrisci Did you have a chance to look at this?

acrisci · 2020-07-01T22:39:33Z

I'm getting a test failure on travis that I can reproduce locally.

acrisci · 2020-07-01T23:20:10Z

dbus_next/_private/unmarshaller.py

+        unix_fds = array.array("i")
+
+        if self.sock:
+            try:


This code path might run multiple times for a single unmarshalling of a message. That will happen in the case there was a blocking error. The previously read data will be read from a buffer cache instead of the socket.

One solution would be to cache the fds on the unmarshaller instance and then skip reading the fds from the socket in the case that the fd cache is present.

I added that cache to unmarshaller, please have a look if this is what you had in mind.

acrisci · 2020-07-01T23:24:46Z

Use make format to run the formatter.

acrisci · 2020-07-01T23:52:25Z

dbus_next/service.py

+        fds = []
+
+        def _replace(obj):
+            if getattr(obj, "fileno", False):


There's hasattr but better might be type(obj) is socket.

I changed this to use hasattr. If I'm not mistaken this doesn't have to be a socket but could for example be an open file.

acrisci · 2020-07-02T00:06:21Z

dbus_next/message.py

@@ -135,7 +135,7 @@ def new_error(msg: 'Message', error_name: str, error_text: str) -> 'Message':
                       body=[error_text])

    @staticmethod
-    def new_method_return(msg: 'Message', signature: str = '', body: List[Any] = []) -> 'Message':
+    def new_method_return(msg: 'Message', signature: str = '', body: List[Any] = [], fds: List[Any] = []) -> 'Message':


Be more specific than List[Any]. I think you only accept a union of socket and int.

This could be something else other than a socket so I changed it to List[Union[int, IO]]

acrisci · 2020-07-02T00:08:35Z

dbus_next/auth.py

-        if response != _AuthResponse.OK:
-            raise AuthError(f'authentication failed: {response.value}: {args}')
+        if response is _AuthResponse.OK:
+            return "NEGOTIATE_UNIX_FD"


This command may only be sent on transports that support Unix file descriptor passing.

Some people use http transport with this even though it's deprecated.

I reworked AuthResponse so if fds negotiation fails with an ERROR we just continue without them (like before).

acrisci · 2020-07-02T00:12:53Z

dbus_next/aio/message_bus.py

@@ -222,6 +225,32 @@ def reply_handler(reply, err):

        return future.result()

+    def sock_sendmsg(self, sock, *buffers, ancdata=None, flags=0):


This should be private.

acrisci · 2020-07-02T00:21:55Z

dbus_next/message.py

@@ -232,3 +235,41 @@ def _marshall(self):
        header_block.marshall()
        header_block.align(8)
        return header_block.buffer + body_block.buffer
+
+
+def replace_fds(body_obj, children, replace_fn):


This should be private.

acrisci · 2020-07-02T00:23:08Z

test/test_fd_passing.py

+import socket
+
+import pytest
+


Be sure these tests are good because someone might come along and do the glib implementation and need to modify this.

acrisci · 2020-08-21T22:11:16Z

Are you guys still working on this? It seems like a good feature. Let me know if I can help.

michallowasrzechonek-silvair · 2020-08-22T16:53:21Z

Hi,

It turned out that our target platform puts restrictions on SCM_RIGHTS, and we can't use it directly. We ended up with a workaround, passing path to an unix socket istead of a descriptor.

So no, we're not actively working on that feature, sorry :(

At least the PR is functional, we've tested it with ELL acting as a service side.

rafalgajda-silvair · 2020-09-12T08:19:33Z

Hi,
I might get some spare time to finish this pull request. It would be great if You could have a look at it again and let me know if there is anything more to be fixed before merging.

acrisci · 2020-10-11T15:44:16Z

Sorry for not getting to this sooner. There are just too many subtle issues here to do this in one PR. But what is here is basically good.

I am going to merge this, make sure it's turned off so it doesn't affect any existing code and mark the feature as experimental so we can make breaking changes for awhile. I'll tag you on any PRs I make in the future.

rafalgajda-silvair · 2020-10-12T07:14:28Z

Sure thing, I'm just glad this code will not get lost :D

michallowasrzechonek-silvair force-pushed the support_unix_fds branch from fb7941c to ea72f91 Compare May 29, 2020 20:31

rafalgajda-silvair force-pushed the support_unix_fds branch from 77f995b to 8737508 Compare June 15, 2020 12:14

rafalgajda-silvair force-pushed the support_unix_fds branch 2 times, most recently from 0daf81e to a5dde99 Compare June 24, 2020 12:01

rafalgajda-silvair force-pushed the support_unix_fds branch from a5dde99 to 6364eb0 Compare June 24, 2020 13:47

michallowasrzechonek-silvair commented Jun 26, 2020

View reviewed changes

acrisci reviewed Jul 1, 2020

View reviewed changes

acrisci reviewed Jul 2, 2020

View reviewed changes

rafalgajda-silvair force-pushed the support_unix_fds branch from 525ee77 to f15f86e Compare July 7, 2020 20:14

acrisci mentioned this pull request Aug 30, 2020

Support for tcp transport #57

Merged

rafalgajda-silvair force-pushed the support_unix_fds branch from f15f86e to cbfee90 Compare September 12, 2020 06:51

michallowasrzechonek-silvair and others added 5 commits September 12, 2020 09:04

Changes to marshalling to support passing fds

07e6b45

Add file descriptor capabilities negotiation

97b1252

Automaticaly replace fds in methods and proxy obj + tests

ab59140

Sendmsg with entire header and pull the value to the constant

d3d212a

Reworked AuthResponse and style changes

f0fcbdc

rafalgajda-silvair force-pushed the support_unix_fds branch from cbfee90 to f0fcbdc Compare September 12, 2020 07:06

Add unix_fds buffer to Unmarshaller

6012957

acrisci merged commit bb0a247 into altdesktop:master Oct 12, 2020

rafalgajda-silvair deleted the support_unix_fds branch October 12, 2020 07:14

michallowasrzechonek-silvair changed the title ~~[WIP] Support passing unix fds~~ Support passing unix fds Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing unix fds #54

Support passing unix fds #54

michallowasrzechonek-silvair commented May 29, 2020

acrisci commented Jun 1, 2020

michallowasrzechonek-silvair commented Jun 1, 2020 •

edited

rafalgajda-silvair commented Jun 24, 2020

michallowasrzechonek-silvair Jun 26, 2020

rafalgajda-silvair Jun 27, 2020

michallowasrzechonek-silvair Jun 26, 2020

rafalgajda-silvair Jun 27, 2020

michallowasrzechonek-silvair Jun 26, 2020

rafalgajda-silvair Jun 27, 2020

michallowasrzechonek-silvair commented Jun 27, 2020

acrisci commented Jul 1, 2020

acrisci Jul 1, 2020 •

edited

rafalgajda-silvair Sep 12, 2020

acrisci commented Jul 1, 2020

acrisci Jul 1, 2020

rafalgajda-silvair Jul 7, 2020 •

edited

acrisci Jul 2, 2020

rafalgajda-silvair Jul 7, 2020 •

edited

acrisci Jul 2, 2020

rafalgajda-silvair Jul 7, 2020 •

edited

acrisci Jul 2, 2020

rafalgajda-silvair Jul 7, 2020

acrisci Jul 2, 2020

rafalgajda-silvair Jul 7, 2020

acrisci Jul 2, 2020

acrisci commented Aug 21, 2020

michallowasrzechonek-silvair commented Aug 22, 2020

rafalgajda-silvair commented Sep 12, 2020

acrisci commented Oct 11, 2020

rafalgajda-silvair commented Oct 12, 2020

		@@ -222,6 +225,32 @@ def reply_handler(reply, err):

		return future.result()

		def sock_sendmsg(self, sock, *buffers, ancdata=None, flags=0):

		import socket

		import pytest

Support passing unix fds #54

Support passing unix fds #54

Conversation

michallowasrzechonek-silvair commented May 29, 2020

acrisci commented Jun 1, 2020

michallowasrzechonek-silvair commented Jun 1, 2020 • edited

rafalgajda-silvair commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michallowasrzechonek-silvair commented Jun 27, 2020

acrisci commented Jul 1, 2020

acrisci Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrisci commented Jul 1, 2020

Choose a reason for hiding this comment

rafalgajda-silvair Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafalgajda-silvair Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafalgajda-silvair Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrisci commented Aug 21, 2020

michallowasrzechonek-silvair commented Aug 22, 2020

rafalgajda-silvair commented Sep 12, 2020

acrisci commented Oct 11, 2020

rafalgajda-silvair commented Oct 12, 2020

michallowasrzechonek-silvair commented Jun 1, 2020 •

edited

acrisci Jul 1, 2020 •

edited

rafalgajda-silvair Jul 7, 2020 •

edited

rafalgajda-silvair Jul 7, 2020 •

edited

rafalgajda-silvair Jul 7, 2020 •

edited