Unix FD Passing

[Havoc Pennington]

Hi,

Thanks for doing this work btw.

Some patch review notes, mostly minor formatting type stuff, I'm
sleepy and won't edit this too much so ignore the cases where I'm just
confused. Hopefully first pass is useful anyway.

Patch: build-system: get rid of config.h inclusion checks

The argument in the commit message seems to be "the assertion
currently passes so it isn't needed," but that seems to miss the point
of an assertion?
These checks are here because at times there were bugs with config.h
not being the first thing included in a given translation unit, which
can sometimes result in silent miscompilation.

Re: config.h in header files, for public header files sure, but I
don't know a problem with it in uninstalled/internal headers.

Patch: memory: remove semicolons from macros

Looks good, though I think "impossible to use them" in commit message
is untrue (they are heavily used, afaik). The semicolon only breaks in
obscure cases we've never hit, apparently, so nobody noticed the bug.

Patch: sysdeps-unix: Use MSG_NOSIGNAL when available

Does this get us anywhere? Seems like it just creates bugs that are
hidden on Linux but appear on OS X, BSD, Solaris.

+  memset(&m, 0, sizeof(m));

The _DBUS_ZERO macro should be used for this. Its primary virtue is
that you can't get the second two args to memset backward.

Patch: dbus_dup

+  int rv;

The existing code would normally call this 'retval' or 'new_fd' or
something like that.
Sometimes it uses "ret" but owen convinced me a couple years ago that
"ret" was ugly.

+_dbus_dup(int fd,
+          DBusError *error)

egtk-format-protos

(though for some reason this whole block of functions has the return
type on the wrong line... not sure what the story is with that... but
the args should be lined up)

Patch: _dbus_socket_can_pass_unix_fd

+  return sa_buf.sa.sa_family == AF_UNIX;
+
+#endif
+
+  return FALSE;

Doing this as #else return FALSE might avoid a warning from a clever
compiler, don't know if gcc cares, probably not.

+  memset(&sa_buf, 0, sizeof(sa_buf));

_DBUS_ZERO here again

Patch: sysdeps-unix-add-basic-IO-primitives

_dbus_read_socket_with_unix_fds should maybe just use DBusError rather
than overloading ENOSPC?
Several memset in here that should be _DBUS_ZERO

The read/write with unix fds functions can unfortunately fail
"partly"; it seems in the fd-read fail case, we'll essentially need to
drop the DBusConnection? I'm not sure how we'd recover. Maybe there's
some way, for example maybe we could say that fds can come through as
"-1" and basically if you get a message with fds in it, some of the
fds could be "failed" fds? (Can failure really happen, i.e. are there
any errors that would not just be bugs?)

If we fail to read fds, do we get confused and read fds that are
supposed to be in a different message? That is going to be a real
security mess on the system bus if so. We definitely would want to
guarantee that fds go only to the intended clients.

+#ifdef MSG_CMSG_CLOEXEC
+                       |MSG_CMSG_CLOEXEC
+#endif

To work if this doesn't exist, we'd need to manually set cloexec later
#ifndef MSG_CMSG_CLOEXEC, right? Or else un-#ifdef this and just
assume we have this if we have fd passing.

The use of manual memcpy, realloc, pointer math, etc. here and later
in DBusMessage are generally intended to be "banned" in dbus code
similar to as in vsftpd, to prevent buffer f-ups (see HACKING). The
problem is usually we avoid these with DBusList or DBusString, and
here you're the first case to need DBusArray, which does not exist.

+/** Type code marking a unix file descriptor */
+#define DBUS_TYPE_UNIX_FD      ((int) 'f')

While I sympathize that other extenders of the protocol should have
gotten their stuff in the spec / in the reference implementation,
maybe it would be kind not to stomp them.

One option could be to go to capital letters ("F") - or otherwise just
use something completely non-mnemonic.

+/** The message meta data does not match the payload. */

This doc comment may as well say "e.g. expected number of file
descriptors were not sent over the socket" since that's going to be
the only actual example of this most likely.

+          _dbus_assert_not_reached("attempted to byteswap unix fds
which makes no sense");

If it's passed as a UINT32 why doesn't it need byteswapping just as a
UINT32 does?

+  if (!(retval->unix_fds = dbus_new(int, message->n_unix_fds)))

The rest of the code I think always writes this explicitly:

retval->unix_fds = dbus_new(int, message->n_unix_fds);
if (retval->unix_fds != NULL)
  goto failed_copy;

which is probably why it never hit the semicolons bug with dbus_new().
May as well keep it consistent. (I don't write new code using exactly
the dbus conventions either, but we should keep the codebase
consistent.)

dbus_message_copy: there are probably places that rely on this failing
due to OOM only. Fortunately, it looks like dbus itself never uses
this function (or, in the test suite only).
At the very least the docs need updating:
* @returns the new message.or #NULL if not enough memory

Strictly speaking, we also need a dbus_message_copy_with_error or some
such. Since dbus-daemon is the only thing that handles OOM and it
doesn't use this function it looks like, we could just make it "@todo
this function can't be used in programs that try to recover from
out-of-memory errors" and wait a decade or so and maybe someone turns
up and cares.

+  for (retval->n_unix_fds = 0; retval->n_unix_fds <
message->n_unix_fds; retval->n_unix_fds++)
+    if ((retval->unix_fds[retval->n_unix_fds] =
_dbus_dup(message->unix_fds[retval->n_unix_fds], NULL)) < 0)
+      goto failed_copy;

If any dup fails, don't the previous dups get leaked?

This would be more readable I think if it just gave up and used an
"int i" instead of the longer retval->n_unix_fds, and didn't mix the
assigment with the "<0" test.

+  } else
+    _dbus_type_reader_read_basic (&real->u.reader,
+                                  value);
 }

This should have the braces around the else{} block

+attach_fds(DBusMessage *m, unsigned n) {

brace on following line, args each on own line

attach_fds() function name confused me, it allocates at least n
additional fds in the array. Something like alloc_more_fds() perhaps.

+      real->message->n_unix_fds ++;
+      u++;

The rest of the code would always use "+= 1" for this

+  else
+    ret = _dbus_type_writer_write_basic (&real->u.writer, type, value);

Missing braces on the else block.

+      /* Final step, update the header accordingly */
+      ret = _dbus_header_set_field_basic (&real->message->header,
+                                          DBUS_HEADER_FIELD_UNIX_FDS,
+                                          DBUS_TYPE_UINT32,
+                                          &u);

If updating the header field fails, you have to roll back the earlier
stuff (close fd and re-decrement n_unix_fds).

@@ -2047,7 +2166,7 @@ dbus_message_iter_get_fixed_array (DBusMessageIter  *iter,
   _dbus_return_if_fail (_dbus_message_iter_check (real));
   _dbus_return_if_fail (value != NULL);
   _dbus_return_if_fail ((subtype == DBUS_TYPE_INVALID) ||
-                         dbus_type_is_fixed (subtype));
+                        (dbus_type_is_fixed (subtype) && subtype !=
DBUS_TYPE_UNIX_FD));

This means that a binding or app can't check dbus_type_is_fixed() to
see if get_fixed_array/append_fixed_array will work. The simplest fix
is probably to just go ahead and support an array of fd.

+#define DBUS_MAX_QUEUED_FDS 1024

This should probably be one of the configurable limits in the bus
config file (see 'man dbus-daemon'), probably handled the same way as
dbus_connection_set_max_message_size() and set_max_received_size().

#ifdef HAVE_UNIX_FD_PASSING
      /* Does fd passing even make sense with encoded data? */
      _dbus_assert(!DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport));
#endif

Agreed, doesn't really make sense, assertion is fine. We don't support
SSL anyhow ;-)

You don't have to #ifdef this though. If we don't HAVE_UNIX_FD_PASSING
then DBUS_TRANSPORT_CAN_SEND_UNIX_FD should always be FALSE.

+#include "dbus-message-private.h"

_dbus_message_get_network_data() basically exists to avoid including
this header. I'd add _dbus_message_get_unix_fd_data().

+          else
 #endif
-
           if (socket_transport->message_bytes_written < header_len)

This should just have the braces and indent the else block so it's
clear it can be an else block; the braces are harmless if there isn't
really an else in front.

Another option is to make _dbus_message_get_unix_fd_data() return
NULL/len=0 array of fds if there are none, and make
_dbus_write_socket_with_unix_fds_two() nicely handle NULL/0 for the fd
array, and then avoid any #ifdef in here.

Hmm. In fact, I think it's probably wrong to have much if any #ifdef
in this file. The runtime checks for CAN_SEND_UNIX_FD should always be
sufficient. The #ifdef should pretty much all be in -sysdeps-unix.c,
plus maybe some small #ifdef to avoid extra struct fields.

+              if (!(transport->unix_fds = dbus_new(int, DBUS_MAX_QUEUED_FDS)))

I would guess the most common number of fds to pass is 1, so
allocating 1024 seems a little harsh. Though it is nice to avoid
realloc headaches, which are error-prone. :-/
(cosmetic: again, avoid test and assign on same line)

+          if (!transport->unix_fds)
+              if (!(transport->unix_fds = dbus_new(int, DBUS_MAX_QUEUED_FDS)))

There should be braces here too, after the first if, and '== NULL'
rather than "!"

+          bytes_read = _dbus_read_socket_with_unix_fds(socket_transport->fd,
+                                                       buffer,
socket_transport->max_bytes_read_per_iteration,
+                                                       fds,
+                                                       &n_fds);

In dbus-transport.c,
+#ifdef HAVE_UNIX_FD_PASSING
+#include "dbus-sysdeps-unix.h"
+#endif

This implies that there's code in dbus-transport.c that should be in
dbus-transport-socket.c or dbus-transport-unix.c. Basically all the
fd-passing-related code should be in those other files. There are two
options I can think of; abstract it for sockets and have the related
functions fail on some sockets (TCP, Windows); or virtualize the
functionality and implement it only in the unix domain socket
transport.

+#ifdef HAVE_UNIX_FD_PASSING
+              /* Remove the requested number of fds from the fd queue
+                 and attach them to the message */

This block of code should really be in dbus-message.c, with a function
called from the transport, rather than having the transport mucking
around in DBusMessage internals.

Thought: the incoming byte buffer is in DBusMessageLoader in
dbus-message.c, not in DBusTransport. It may be that locating the
incoming fds queue in DBusMessageLoader also makes the new code more
analogous to the old code and just means fds are passed around
whenever we pass the byte stream data around.

+              if (!(message->unix_fds =
_dbus_memdup(transport->unix_fds, u * sizeof(int))))

sizeof(message->unix_fds[0]) would avoid a subtle bug if someone ever
changed the type of the array for some reason.
Also the usual point about not mixing assign and test.

Patch: introduce dbus_connection_can_send_type()

Even more generic would be dbus_connection_has_feature(connection,
"UNIX_FD") ... thoughts?

+  _dbus_return_val_if_fail (_dbus_type_is_valid(type), FALSE);

We could simply return FALSE with an invalid type (in a defined way,
without an assertion failure), which would mean that you could check
at runtime whether *libdbus* supported the type, not only whether the
connection did.

patch: unix-fd: when sending a message with unix fds verify that the
connection can do it

As we were discussing a bit earlier, I think this may be failing too
early. I think it might be better to synthesize an error-reply
DBusMessage, similar to what we do if a message times out. That will
also mean that things behave consistently if our connection to the bus
daemon supports fds, but the other app we're talking to does not, in
which case this failure will happen later (inside the bus daemon).

Patch: close on exec cleanup patches

These look sensible. POSIX definitely fucked up the default for this
flag on new fds!

Patch: auth: add fd passing negotiation support

As discussed already, s/-/_/ and maybe we do NEGOTIATE <featurename>
instead of NEGOTIATE_UNIX_FD.

+    case DBUS_AUTH_COMMAND_NEGOTIATE_UNIX_FD:
+      return send_error (auth, "Need to authenticate first");

Before reading the code, I was imagining that negotiation was before
auth. I think this will at least technically break the previous spec
to do it after:

      <para>
        If authentication succeeds after exchanging DATA commands,
        an OK command must be sent to the client.
      </para>
      <para>
        The first octet received by the client after the \r\n of the OK
        command must be the first octet of the authenticated/encrypted
        stream of D-Bus messages.
      </para>
      <para>
        The first octet received by the server after the \r\n of the BEGIN
        command from the client must be the first octet of the
        authenticated/encrypted stream of D-Bus messages.
      </para>

Havoc