Bug 80603

Created attachment 101878 [details] [review]
[PATCH 1/2] dbus-monitor: do not leak file descriptors from fd-passing

Created attachment 101880 [details] [review]
[PATCH 2/2] dbus-monitor: more details on file descriptors

This one might be a bit more controversial:
- is it ok to add those #includes? Would it compile on Windows?
- printing the socket address family is not generic. Is there a way to do it in a generic way? I would like to print only AF_UNIX, AF_BLUETOOTH, AF_INET{,6} because I know of applications fd-passing them. I'm not sure we need to debug applications passing AF_TIPC or AF_SECURITY over D-Bus.
- I could print the peer address too...

Comment on attachment 101878 [details] [review]
[PATCH 1/2] dbus-monitor: do not leak file descriptors from fd-passing

Review of attachment 101878 [details] [review]:
-----------------------------------------------------------------

Seems fine

Comment on attachment 101880 [details] [review]
[PATCH 2/2] dbus-monitor: more details on file descriptors

Review of attachment 101880 [details] [review]:
-----------------------------------------------------------------

::: tools/dbus-print-message.c
@@ +25,5 @@
>  
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/socket.h>
> +#include <unistd.h>

Those need to be #ifdef HAVE_SYS_TYPES_H etc.; or you could probably just use #ifdef DBUS_UNIX for the whole block.

@@ +156,5 @@
> +   * process recipient of the D-Bus message...
> +   */
> +  printf ("unix fd %d\n", fd);
> +  if (fd == -1)
> +    return;

Everything in this function from here down (as well as the variables) should be #ifdef DBUS_UNIX.

@@ +158,5 @@
> +  printf ("unix fd %d\n", fd);
> +  if (fd == -1)
> +    return;
> +
> +  err = fstat (fd, &statbuf);

I would use "ret", "res" or "result" for the thing that is either 0 or -1 returned by a syscall, and "err" for a saved errno.

Will this work on TCP or abstract-Unix sockets that don't exist as files on the filesystem?

I think it might be better to try getsockname() first; if that succeeds, take the socket code path; and if it fails, try fstat() to see whether it's a filesystem file. getsockname() fails with ENOTSOCK on non-sockets.

@@ +194,5 @@
> +	    {
> +	      case AF_UNIX:
> +		printf ("unix\n");
> +		break;
> +	      case AF_BLUETOOTH:

This is unlikely to be portable. #ifdef AF_BLUETOOTH please

@@ +200,5 @@
> +		break;
> +	      case AF_INET:
> +		printf ("inet\n");
> +		break;
> +	      case AF_INET6:

#ifdef AF_INET6, unless we have code elsewhere that uses AF_INET6 unguarded.

Created attachment 102025 [details] [review]
[PATCH] dbus-monitor: more details on file descriptors

Patch updated with the changes mentioned in Simon's review.

I kept fstat first: fstat on tcp or abstract unix socket works fine and I get the same inode as netstat.

I removed levels of indentation by using getsockname/getpeername return value.

Comment on attachment 102025 [details] [review]
[PATCH] dbus-monitor: more details on file descriptors

Review of attachment 102025 [details] [review]:
-----------------------------------------------------------------

::: tools/dbus-print-message.c
@@ +31,5 @@
> +#include <unistd.h>
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <arpa/inet.h>
> +#endif

We'll probably get bug reports saying that $my_pet_unix doesn't have one of these headers, but it seems reasonable to defer dealing with that until the actual bug report, because these are all things that (surely?) any sensible Unix should have.

@@ +159,5 @@
> +#ifdef DBUS_UNIX
> +  int ret;
> +  struct stat statbuf = {0,};
> +  union {
> +      struct sockaddr sa;

This union demonstrates why people like to wrap the BSD sockets API in less ugly abstractions, but your socket-wrangling code all looks valid.

@@ +230,5 @@
> +             * See manual page unix(7)
> +             */
> +            indent (depth+1);
> +            printf ("name @%.*s\n",
> +                    (int) (addrlen - sizeof (sa_family_t) - 1),

Strictly speaking this should probably be

(int) (addrlen - (&(addr.un.sun_path[1]) - (char *) &addr))

or some similar horror that takes account of "in theory there might be something between addr.un.sun_family and addr.un.sun_path"... but abstract sockets are a Linux- and Solaris-specific API, Linux presumably implements it in the only sane way (nothing between those two members), and if Solaris doesn't, someone can report a bug.

Created attachment 102066 [details]
dbus-monitor file descriptor test program

This is the test program I used to test the patches. It calls a non-existent D-Bus method with various fds attached. It does not test everything, but it's better than nothing.

Fixed in git for 1.9.0, thanks