Bug 30812

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)

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).

This could really benefit from a regression test, or at least a simple example included with dbus-python.

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.

(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).

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.

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.

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).

Any news on this?

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?

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

@Lucas, could you test it for me? I have rebased it but can't retest right now.

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.

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?

(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.

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.

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.

This was released in 0.84.0.