Unix FD Passing

[Lennart Poettering]

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

(I haven't looked into all the issues you raised yet, will need a
third reply for that.)

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

Changed.

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

Added.

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

Added.

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

No, because we use revtal->n_unix_fds as counter, hence further down
when we call close_unix_fds that variable is properly initialize.

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

Sure, one could add a seperate counter variable there. However, that
would break the cleanup logic (see above), unless we'd update
n_unix_ifds from i on error or loop exit. Which is why I decided to
use that counter directly, since in the end, this really what is
intended here.

I have now rearranged this a bit, but left n_unix_fds as counter, I
hope you find it more readable now.

> 
> +  } else
> +    _dbus_type_reader_read_basic (&real->u.reader,
> +                                  value);
>  }
> 
> This should have the braces around the else{} block

Fixed. I don't really understand the logic when you put braces and
when not, though.

> +attach_fds(DBusMessage *m, unsigned n) {
> 
> brace on following line, args each on own line

Fixed.

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

Renamed this to expand_fd_array() now.

> +      real->message->n_unix_fds ++;
> +      u++;
> 
> The rest of the code would always use "+= 1" for this

That is not true. Lot's of "++" everywhere. On iterators,
on other counters.

But anyway, changed to += 1 now.

> 
> +  else
> +    ret = _dbus_type_writer_write_basic (&real->u.writer, type, value);
> 
> Missing braces on the else block.

Again, what is your logic behind that? When do you want braces on
if/else, and when not?

Changed.

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

Why?

The message is hosed anyway on OOM and the fd is not lost. It has been
added to the array and will be freed properly when the message itself
is freed.

I mean, sure I can rollback the dup() call here, but doing that would
create the wrong idea that this function was actually able to deal
properly with OOM which it is not. As long as no fds/memory is leaked
when we free the message on oom i think this should be safe enough.

Hmm, need got to bed now. 

Lennart

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