Bug 80603 - dbus-monitor should handle file descriptors better
Summary: dbus-monitor should handle file descriptors better
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Alban Crequy
QA Contact:
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-06-27 15:38 UTC by Alban Crequy
Modified: 2014-09-15 17:48 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/2] dbus-monitor: do not leak file descriptors from fd-passing (992 bytes, patch)
2014-06-27 15:43 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/2] dbus-monitor: more details on file descriptors (2.91 KB, patch)
2014-06-27 15:51 UTC, Alban Crequy
Details | Splinter Review
[PATCH] dbus-monitor: more details on file descriptors (6.00 KB, patch)
2014-06-30 17:56 UTC, Alban Crequy
Details | Splinter Review
dbus-monitor file descriptor test program (3.21 KB, text/plain)
2014-07-01 10:21 UTC, Alban Crequy
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alban Crequy 2014-06-27 15:38:03 UTC
dbus-monitor leaks file descriptor when receiving them.

Also it should print more details about the received fds.
Comment 1 Alban Crequy 2014-06-27 15:43:06 UTC
Created attachment 101878 [details] [review]
[PATCH 1/2] dbus-monitor: do not leak file descriptors from fd-passing
Comment 2 Alban Crequy 2014-06-27 15:51:36 UTC
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 3 Simon McVittie 2014-06-27 16:24:07 UTC
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 4 Simon McVittie 2014-06-27 16:33:15 UTC
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.
Comment 5 Alban Crequy 2014-06-30 17:56:54 UTC
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 6 Simon McVittie 2014-06-30 18:11:55 UTC
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.
Comment 7 Alban Crequy 2014-07-01 10:21:26 UTC
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.
Comment 8 Simon McVittie 2014-09-15 17:48:27 UTC
Fixed in git for 1.9.0, thanks


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.