Bug 30812

Summary: Add Unix FD passing support to Python bindings of D-BUS
Product: dbus Reporter: Elvis Pfützenreuter <epx>
Component: pythonAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: enhancement    
Priority: medium CC: gustavo
Version: 1.4.xKeywords: patch
Hardware: All   
OS: All   
URL: http://cgit.collabora.com/git/user/smcv/dbus-python-smcv.git/log/?h=fd-passing
Whiteboard:
i915 platform: i915 features:
Attachments: Patch to add UNIX FD support to dbus-python
Patch to add UNIX FD support to dbus-python, with fixed fd sending support
New version of unix fd support implementation
Add comment to unix-fd-client
Add comment to Unix FD client (and example)

Description Elvis Pfützenreuter 2010-10-12 15:05:39 UTC
Created attachment 39399 [details] [review]
Patch to add UNIX FD support to dbus-python

Attached is a tenative patch to add Unix FD passing support for the Python bindings, following the feature in D-Bus 1.4.0.

The patch has been well tested for Unix FD *receiving*, not for Unix FD *sending*.
Comment 1 Elvis Pfützenreuter 2010-10-13 20:42:50 UTC
Created attachment 39423 [details] [review]
Patch to add UNIX FD support to dbus-python, with fixed fd sending support

Now I have tested FD *sending* (using sample server written in Python) and added a couple fixes to the patch, so it implements FD support in both roles (sending and receiving fd's)
Comment 2 Simon McVittie 2010-10-14 05:29:19 UTC
Review of attachment 39423 [details] [review]:

::: _dbus_bindings/int.c
@@ +436,3 @@
+    PyObject *self = (DBusPyIntBase_Type.tp_new)(cls, args, kwargs);
+    if (self && dbus_py_unixfd_range_check(self) == -1 && PyErr_Occurred()) {
+        Py_DECREF(self);

I'm not sure that range-checking makes much sense for file descriptors. What really makes sense is to check that it is, as claimed, an operating system file descriptor.

The only way I can think of to do that would be to os.dup() the file descriptor. On success, it really is a file descriptor: close the duplicate. On exception, raise that exception.

That still doesn't protect us from a user of dbus-python giving us a file descriptor that they then close before the message is serialized: if they do that, dbus_message_iter_append_basic will return FALSE, which is helpfully documented as "@returns #FALSE if not enough memory". Sigh.

Alternatively, you could have UnixFd be an opaque object (*not* a subtype of int) which contains a duplicate (os.dup()) of the file descriptor, which is close()d in the UnixFd's destructor, and has a dup() method which returns the C equivalent of os.dup(self.fileno)? But perhaps that's excessive.

I think GDBus' API for this is nicer, but dbus-python is stuck with libdbus at least for the medium term.

::: _dbus_bindings/message-append.c
@@ +953,3 @@
+#if defined(DBUS_TYPE_UNIX_FD)
+      case DBUS_TYPE_UNIX_FD:
+          PROCESS_INTEGER(int32)

A file descriptor is an int, not an int32 (although the two are the same on most platforms).

(The thing in the message payload is an int32 index into an out-of-band array of fds, but dbus_message_get_args magically transforms it into an int that is a copy of the file descriptor.)

::: _dbus_bindings/message-get-args.c
@@ +327,3 @@
+            DBG("%s", "found an unix fd");
+            dbus_message_iter_get_basic(iter, &u.i32);
+            args = Py_BuildValue("(i)", u.i32);

This should be a new union member that is exactly a C int (not an int of a specified size).
Comment 3 Simon McVittie 2010-10-14 05:30:12 UTC
This could really benefit from a regression test, or at least a simple example included with dbus-python.
Comment 4 Elvis Pfützenreuter 2010-10-14 05:40:44 UTC
I will improve the patch following the comments' ideas.

One reason I kept the UnixFd type as a subtype of int type, is that we don't know the type of file descriptor. So the user may want to encapsulate it in different ways e.g. with os.fdopen() or with sock.fromfd(). This of course leaves the possibility of a resource leak, if the user receives a file descriptor and does nothing with it, the fd will stay open until program exits.

In that sense, an  opaque UnixFd with a fileno() ou fd() function that allows the user to get the int fd would be best, because UnixFd can close() the fd upon garbage collection.
Comment 5 Simon McVittie 2010-10-14 05:48:46 UTC
(In reply to comment #4)
> In that sense, an  opaque UnixFd with a fileno() ou fd() function that allows
> the user to get the int fd would be best, because UnixFd can close() the fd
> upon garbage collection.

If you do this, you should probably have a dup() method to avoid this failure mode:

- receive a fd, kernel allocates number 23 for it
- dbus-python creates a UnixFd object containing os.dup(23) (let's say the result is 24), and calls os.close(23)
- user says f = os.fdopen(fd.fileno), now f.fileno == 24
- user discards the UnixFd
- f.fileno has now been closed and f is useless

in favour of:

- receive a fd, kernel allocates number 23 for it
- dbus-python creates a UnixFd object containing os.dup(23) (let's say the result is 24), and calls os.close(23)
- user says f = os.fdopen(fd.dup()), now f.fileno == 25 (say)
- user discards the UnixFd, fd 24 is closed
- f is still useful

To streamline step 2 you could provide some C API, DBusPyUnixFd_NewTakingFd or something, which takes ownership of the fd instead of calling os.dup() on it (and requiring the caller to os.close() their copy).
Comment 6 Elvis Pfützenreuter 2010-10-14 06:09:53 UTC
I guess I will go for a method like UniFd.take() or UnixFd.acquire() that passes ownership of fd to the user, and he is therefore responsible by doing the right thing with it.

We could add methods to UnixFd that call os.fdopen() or socket.fromfd() internally, but IMHO file descriptor passing is advanced enough and we can expect the programmer to know such calls himself. Having a ownership-passing method in UnixFd is enough to ensure that a file descriptor that is received and discarded won't leak.

I don't see the need to dup() a received fd, since it is already 'validated' by D-Bus lib, that does the SCM_RIGHTS drudgery. I see the usefulness of dup() check upon sending fds, but not upon receiving.
Comment 7 Elvis Pfützenreuter 2010-10-14 11:01:47 UTC
Created attachment 39451 [details] [review]
New version of unix fd support implementation

In this version:

* UnixFd is an opaque type that closes fd automatically in case it is destroyed;
* At receiving side, UnixFd.take() may be called to take ownership of (numeric) file descriptor, appliction is responsible by closing it after that.
* At transmitting side, UnixFd(file_object or socket_object) may be used to encapsulate parameter. Passing a numeric fd where signature expects an fd is also supported.
* Added example scripts (service and client), using disk file descriptors. (But module has been tested with Bluetooth sockets too)
* dup() is used to check whether file descriptor is valid.
Comment 8 Elvis Pfützenreuter 2010-10-14 17:40:19 UTC
Created attachment 39456 [details] [review]
Add comment to unix-fd-client

This patch adds a comment to unix-fd-client, advising to close the fd right after socket.fromfd() because this method dup()s the socket (while os.fdopen() merely encapsulates the given number).
Comment 9 Elvis Pfützenreuter 2010-10-25 04:21:53 UTC
Any news on this?
Comment 10 Lucas De Marchi 2011-02-22 11:37:21 UTC
Any news on this? In oFono we use FD passing and I'd like to use it on our python scripts.

Current patch does not apply anymore, though it's super easy to rebase it on git-HEAD. Elvis, are you going to update it?
Comment 11 Elvis Pfützenreuter 2011-02-22 11:51:43 UTC
Created attachment 43676 [details] [review]
Add comment to Unix FD client (and example)

New version rebased to latest master, example patch squashed into main patch
Comment 12 Elvis Pfützenreuter 2011-02-22 11:52:22 UTC
@Lucas, could you test it for me? I have rebased it but can't retest right now.
Comment 13 Simon McVittie 2011-05-17 10:27:55 UTC
Review of attachment 43676 [details] [review]:

Sorry about the delay, I've been busy with various other projects and I rarely use dbus-python these days.

::: _dbus_bindings/unixfd.c
@@ +72,3 @@
+    UnixFdObject* self = NULL;
+    PyObject* arg;
+    PyObject* fdnumber;

Coding style: * for pointers attaches to the variable name, not to the type (because that's how C precedence works)

@@ +87,3 @@
+	}
+	/* takes ownership of original fd */
+	close(fd_original);

I'm very suspicious about this close() - it seems astonishing. Why this difference between the "int argument" and "file-like argument" cases?

If the answer is "so message-get-args.c can call it with an int argument" then I'd prefer either of these:

1) do the close() in message-get-args.c instead

2) add C API DBusPyUnixFd_NewTakingFd, as I suggested in Comment #5, which forcibly creates a blank DBusPyUnixFd without calling its constructor; make that bypass the dup() and close()

DBusPyConnection_NewConsumingDBusConnection is a similar constructor-bypassing static method.
Comment 14 Elvis Pfützenreuter 2011-05-17 11:36:14 UTC
Looking in retrospect, I don't see why handling int and file-object arguments should be different. IMHO if the user manages to get a int-like file descriptor, he should know what he's doing, and that fd will leak if not manually closed. There are many other ways to get hurt by manipulating int fds (e.g. using os module) anyway.

In the other hand, if the user does it the nice way -- passing a file object -- nothing leaks because both unixfd and file objects are eventually collected.

So, would it be acceptable to remove the close()ing of fd for the int case?
Comment 15 Simon McVittie 2011-05-18 02:04:52 UTC
(In reply to comment #14)
> So, would it be acceptable to remove the close()ing of fd for the int case?

Yes - but this will mean that message-get-args.c needs to close() the fd received from the DBusMessage. (Not difficult, obviously.) I'll prepare a patch.
Comment 16 Simon McVittie 2011-05-18 03:10:10 UTC
Here's a branch (see bugzilla URL field for the address of a cgit viewer). If you have some more thorough tests than running the examples, please give it a try. If you have no objections, I'll apply that branch and release it.
Comment 17 Elvis Pfützenreuter 2011-05-18 06:25:44 UTC
I have tested here in my use case as well as some manual tests (to see if there are leaked fds etc.) and looks good for me.
Comment 18 Simon McVittie 2011-07-19 04:25:39 UTC
This was released in 0.84.0.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.