Unix FD Passing

[Havoc Pennington]

Hi,

On Mon, May 4, 2009 at 9:18 PM, Lennart Poettering <mzqohf at 0pointer.de> wrote:
> 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.

I'm guessing the history is that config.h wasn't in the header files,
as you suggest would be better style. So the #error was intended to
ensure that it was always in the .c files.

I don't have a super strong opinion I suppose.

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

Sure, but it's not portable, so few if any apps can rely on it.

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

Well, except the portability issues here only arise when interacting
with other unix code.

I don't know. I don't think this makes sense, because it's Linux-only
so seems more likely to me to hide a bug, than get anyone anywhere. In
any case, it's orthogonal to this patch series, so let's not block the
fd stuff on it.

Maybe it fixes it to always disable SIGPIPE for consistency, but still
also use MSG_NOSIGNAL so if you tell libdbus not to set the SIGPIPE
handler it does still work on Linux. Which could be useful for a few
specialized linux-only apps, possibly.

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

Thanks!

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

Yeah, complain to owen. It's been bugging me for years but not worth
fighting elisp to fix ;-)

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

No, that does not sound worth it to change all dbus_read/dbus_write. I
didn't think that would be needed.

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

Agreed.

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

No, you don't have to change it... nobody is going to go back and
factor out DBusArray ever though, since the next person to patch
things is just going to make the same argument that it's minimal and
low-level ;-) so you don't have to change it but we should all feel
guilty and be larted if a security exploit turns up.

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

I see, I didn't make the connection. I think it needs a comment, like
/* fds can only be passed on a local machine, so byte order must always match */

One thing to consider is that dbus only lazily byteswaps, iirc; so say
a remote machine is attached to the bus via TCP, the bus will not
necessarily swap messages from the remote machine before forwarding
them on. However I think all fds messages have to be from a local app
to local bus to another local app, so it doesn't matter.

Havoc