Unix FD Passing

[Lennart Poettering]

On Sat, 25.04.09 23:41, Havoc Pennington (havoc.pennington at gmail.com) wrote:

Heya!

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

The thing is that just a few lines before each of these tests there was
an explicit "#include <config.h>". Having asserts is nice and stuff, but
checking things that you did just two lines before and which cannot
even fail is certainly overzealous and just noise.

Either put the "#include <config.h>" everywhere OR put checks for
proper inclusion. But doing both is bad.

I generally find it pretty bad style to include config.h in header
files. However, that seems to be the normal case for D-Bus' internal
headers, so I chose to stick to it and instead of removing those
includes i removed the needless asserts.

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

I now changed the commit message to mention that it breaks only in some cases.

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

Firstly I generally believe it is much cleaner to use MSG_NOSIGNAL
instead of setting SIGPIPE to SIG_IGN.

More specifically, this enables correct behaviour when used inside of
tools that are intended to be used in unix pipes, i.e. where the
original reasons why SIGPIPE was invented apply.

Secondly, because of the scarcity of general purpose signals some
daemons use SIGPIPE for similar purposes as SIGUSR1, SIGUSR2,
SIGHUP. Since D-Bus is often pulled in indirectly via some library or
plugin I think it would be good if D-Bus would fiddle as little as
possible with global process state so that this cannot lead to
unpleasant surprises of code fighting for SIGPIPE.

Windows never generated SIGPIPE anyway, so you could turn your
argument around and say this increases portability because it makes
the Windows and the Linux versions more alike.

Also, this change actually makes dbus_connection_set_change_sigpipe()
useful I'd say. Without this change you could always trip over
spurious SIGPIPEs you didn't expect.

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

Hmm, there's actually a lot of other code using memset. While I am at
it I'll fix that too.

Done.

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

/me prefers 'ret' or 'r'. But uh, changed to 'new_fd' now.

> +_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)

Bah. Did I mention that this kind of alignment make things really
unreadable? Brrh. 

Anyway. Changed that now.

btw, egtk-format-protos doesn't work on the function definitions themselves.

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

Fixed. 

> +  memset(&sa_buf, 0, sizeof(sa_buf));
> 
> _DBUS_ZERO here again

Fixed.

> Patch: sysdeps-unix-add-basic-IO-primitives
> 
> _dbus_read_socket_with_unix_fds should maybe just use DBusError rather
> than overloading ENOSPC?

Hmm, dunno. The reason I put this there is because I found some
SCM_RIGHTS related code on the net that had a comment that some unixes
return ENOSPC there and my code just unifies the handling of that
error by using this trick. No sure if that issue mentioned is actually
true though.

If you insist that I change the read()/write() prototypes to take a
DBusError than I'd have to make quite substantial changes everyhwere,
including to the Windows code which I have no access to and cannot
test. I am a lazy person and I am not sure if this would be worth
it... So, what do you say?

> Several memset in here that should be _DBUS_ZERO

Fixed.

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

Unfortunately SCM_RIGHTS sucks in this area: if there is no space in
our fd array the remaining fds won't be read at all and all we get is
notification via MSG_CTRUNC. On Linux the remaining fds are just
dropped, and we don't even know how many got dropped.

It would be good if SCM_RIGHTS would allow us to retry when the fd
array is too short. But unfortunately it doesn't.

I don't really see any other option than making sure that this error
never happens: i.e. always allocate enough space for all fds, so that
we can always be sure that the read will succeed. And if the error
nevertheless does happen we should consider it fatal for the
connection since we have no chance of recovering from that without
having to drop messages or to pass broken messages to the process. But
even if we did that I wouldn't be sure that this would really be
portable.

If the other side sends us more than a certain number of fds at once
this should certainly be considered an unfriendly act which I think 
justifies the connection to be terminated.

In summary, I think we should consider dropped fds a fatal error. And
to make sure that this error doesn't happen we need to make
appropriate resource limit checks, i.e. by extending the byte counting
framework to fd counting, too -- which we need to do for security
reasons anyway.

> +#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.

SCM_RIGHTS dates back from Linux 1.x times IIRC. MSG_CMSG_CLOEXEC
however is a pretty recent addition. We hence need to handle the case
where SCM_RIGHTS is a round, but MSG_CMSG_CLOEXEC is not.

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

So, do you ask me to add DBusArray? Abstracting things like this away
makes only sense to me if there are at least two users of this stuff,
and right now there is only one. Also the uses here are quite minimal
and in very low-level code, so do I really need to change this?

> +/** 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.

As mentioned I am now using "h".

> +/** 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.

Fixed.

> +          _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?

This is a local transport after all. There are processors which can be
configured to run in either LE or BE mode, but AFAIK there is no OS
that actually allows running processes in LE and BE side-by-side.

Running 32bit and 64bit envs side-by-side on a single processors is a
use-case we need to keep in mind. But running LE and BE processes
side-by-side on the same CPU? I am pretty sure that loonies who'd do
such a thing will run into a lot of other problems before they run
into this particular issue with D-Bus. And then, if it matters to them
they can send us a patch!

Ok, need to got to bed now, I'll look at your other comments tomorrow
or so. Before you do a second round of reviews you might want to wait
a until I looked into the remaining issues, too.

Thanks a lot for the review!

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4