Bug 50962 - dbus passes wrong address to spawned services if launched with --address=systemd:
dbus passes wrong address to spawned services if launched with --address=syst...
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
unspecified
All Linux (All)
: medium enhancement
Assigned To: Havoc Pennington
John (J5) Palmieri
review-
: patch
: 53639 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-11 05:23 UTC by Simon Peeters
Modified: 2013-02-22 12:36 UTC (History)
6 users (show)

See Also:


Attachments
first patch to detect address from systemd socket (4.09 KB, patch)
2012-06-11 12:14 UTC, Simon Peeters
Details | Splinter Review
second version of the patch, including error handling (5.13 KB, patch)
2012-06-12 06:23 UTC, Simon Peeters
Details | Splinter Review
redone formatting of the ip dresses (4.35 KB, patch)
2012-06-13 15:01 UTC, Simon Peeters
Details | Splinter Review
4th version of the patch (4.86 KB, patch)
2012-08-09 01:57 UTC, Simon Peeters
Details | Splinter Review
5th version, fixed coding style and did "git format-patch" (5.77 KB, patch)
2012-10-07 15:09 UTC, Simon Peeters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Peeters 2012-06-11 05:23:38 UTC
if one launches a dbus session bus from systemd with
dbus-daemon --session --systemd-activation --address=systemd:
the dbus-activated services get passed DBUS_SESSION_BUS_ADDRESS=systemd: which makes the service unable to connect to the dbus daemon.
Comment 1 Simon McVittie 2012-06-11 06:36:41 UTC
(In reply to comment #0)
> if one launches a dbus session bus from systemd

I don't consider this to be a supported thing to do: the systemd transport is currently only suitable for the system bus.

To make it suitable for the session bus, after _dbus_server_listen_platform_specific() in dbus-server-unix.c calls _dbus_listen_systemd_sockets(), instead of passing "systemd:" to _dbus_server_new_for_socket(), it should:

* determine the real address of each fd (probably via getsockname())
* construct a corresponding D-Bus address
  (e.g. "unix:abstract=/tmp/dbus-LI35ckm")
* pass the sockets' addresses to _dbus_server_new_for_socket()
  separated by ";" (e.g.
  "tcp:host=127.0.0.1,port=34653;unix:abstract=/tmp/dbus-LI35ckm"
  if we discover that it was listening on both those sockets)

The whole block with systemd support should probably become Linux-specific (since systemd is), so that the code to turn socket addresses into D-Bus addresses does not need to be portable; the code to turn socket addresses into D-Bus addresses should probably be a Linux-specific utility function.

Patches to do this would be appreciated.

There are likely to be other changes needed to make the systemd transport suitable for the session bus.
Comment 2 Simon Peeters 2012-06-11 07:05:54 UTC
(In reply to comment #1)
> I don't consider this to be a supported thing to do: the systemd transport is
> currently only suitable for the system bus.

well, it is one of the only ways i know to get dbus running inside a systemd --user session

> To make it suitable for the session bus, after
> _dbus_server_listen_platform_specific() in dbus-server-unix.c calls
> _dbus_listen_systemd_sockets(), instead of passing "systemd:" to
> _dbus_server_new_for_socket(), it should:
> 
> * determine the real address of each fd (probably via getsockname())
> * construct a corresponding D-Bus address
>   (e.g. "unix:abstract=/tmp/dbus-LI35ckm")
> * pass the sockets' addresses to _dbus_server_new_for_socket()
>   separated by ";" (e.g.
>   "tcp:host=127.0.0.1,port=34653;unix:abstract=/tmp/dbus-LI35ckm"
>   if we discover that it was listening on both those sockets)

thanks for these pointers, i will take a look at implementing this

> Patches to do this would be appreciated.

then you can expect them.

> There are likely to be other changes needed to make the systemd transport
> suitable for the session bus.

as far as i know only the address thing is an issue, i fixed that on my current machine(in a slightly less elegant way) and it now runs fine. (even though i don't think the systemd-dbus activation works correctly, but the dbus activation it self works)
Comment 3 Simon Peeters 2012-06-11 12:14:23 UTC
Created attachment 62899 [details] [review]
first patch to detect address from systemd socket

i created a first patch
i still have to add error handling, but want to know whether i am going in the right direction.

(any comments welcome)
Comment 4 Simon McVittie 2012-06-12 04:17:17 UTC
(In reply to comment #2)
> well, it is one of the only ways i know to get dbus running inside a systemd
> --user session

Can't you run dbus-launch --exit-with-session (or, after I merge the patches from Bug #39197, dbus-launch --exit-with-x11) with $DISPLAY set, like you would in a non-systemd environment? (See, for instance, /etc/X11/Xsession.d/75dbus_dbus-launch on Debian/Ubuntu systems.)

Or, if you want to avoid dbus-launch, dbus-daemon --address=unix:path=/run/user/smcv/dbus_session_bus or something like that, with a well-known per-user address?

The systemd: transport is only needed for socket activation: it would be nice for socket activation to be possible in sessions, but its absence shouldn't prevent you from using dbus-daemon under systemd.

(In reply to comment #3)
> i still have to add error handling, but want to know whether i am going in the
> right direction.

Yes, this is roughly what I had in mind.
Comment 5 Simon Peeters 2012-06-12 06:23:07 UTC
Created attachment 62932 [details] [review]
second version of the patch, including error handling

added error handling and some documentation for the _dbus_append_address_from_socket function

i have tested this only with normal unix sockets, and it works fine.
somebody still needs to test this with inet/inet6 sockets (or at least look at the code i use to format the address, since i never used inet sockets before)
Comment 6 Simon Peeters 2012-06-13 15:01:25 UTC
Created attachment 62992 [details] [review]
redone formatting of the ip dresses

forget about my earlier patches, the byte-order for AF_INET and AF_INET6 is all wrong, and there exist functions that do the string formatting for you.

this should be working for inet sockets as well (still not tested)
Comment 7 Simon McVittie 2012-06-15 07:59:34 UTC
Comment on attachment 62992 [details] [review]
redone formatting of the ip dresses

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

This looks good, but a few bits aren't quite there.

I'd like a regression test for this too - I'll get round to writing one at some point if nobody else does.

The systemd LISTEN_FDS protocol is pretty simple, so a regression test can fake it fairly easily without actually being systemd - it just needs to listen on some sockets, make them not close-on-exec, and run dbus-daemon --address=systemd: --print-address with the LISTEN_FDS environment variable set appropriately. I'd recommend using GLib for the test, like test/dbus-daemon.c does.

::: dbus-1.6.0/dbus/dbus-server-unix.c
@@ +159,4 @@
>            return DBUS_SERVER_LISTEN_DID_NOT_CONNECT;
>          }
>  
> +      if(!_dbus_string_init(&address))

Whitespace nitpicking: if (!_dbus_string_init (&address))

@@ +166,2 @@
>          {
> +          if ( i > 0)

Whitespace nitpicking: if (i > 0)

@@ +171,2 @@
>              }
> +          if(!_dbus_append_address_from_socket (fds[i], &address))

Whitespace nitpicking: "if (", twice

@@ +180,4 @@
>        dbus_free (fds);
>  
>        return DBUS_SERVER_LISTEN_OK;
> +  oom:

I'd call this systemd_oom or something, so it doesn't get mixed up with a more general "oom" label.

If you make _dbus_append_address_from_socket raise a DBusError, I think you'll actually end up with:

systemd_oom:
  _DBUS_SET_OOM (error);
systemd_err:
  /* ... clean up ... */

where you "goto systemd_oom" if the error is not set yet, or "goto systemd_err" if it is set (even if what it's set to happens to be OOM too).

@@ +188,5 @@
> +        }
> +      dbus_free (fds);
> +      _dbus_string_free (&address);
> +
> +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);

You've done this twice. If that OOM is hit, that'll be an assertion failure.

You should use _DBUS_SET_OOM (error); to set a non-NULL message.

::: dbus-1.6.0/dbus/dbus-sysdeps-unix.c
@@ +4086,5 @@
>      close (i);
>  }
>  
> +/**
> + * Read the addres from the socket and append it to the string

Nitpicking: "address"

@@ +4091,5 @@
> + *
> + * @param fd the socket
> + * @param address
> + */
> +dbus_bool_t _dbus_append_address_from_socket (int fd, DBusString *address)

Really pedantic layout nitpicking:

dbus_bool_t
_dbus_append_address_from_socket (int         fd,
                                  DBusString *address)

This function can fail for reasons that aren't OOM, so it should return a DBusError (which it sets with _DBUS_SET_OOM or from errno whenever it returns FALSE), just like (e.g.) _dbus_open_socket() does.

@@ +4096,5 @@
> +{
> +  struct sockaddr_storage socket;
> +  struct sockaddr_in *inet_addr = (struct sockaddr_in*)&socket;
> +  struct sockaddr_in6 *inet6_addr = (struct sockaddr_in6*)&socket;
> +  struct sockaddr_un *unix_addr = (struct sockaddr_un*)&socket;

Unfortunately, sockaddr_storage is smaller than sockaddr_un. (Yeah, I know, that defeats the point...)

I think sockaddr code usually benefits from using a union, to avoid all the casts:

typedef union {
  struct sockaddr sa;
  struct sockaddr_storage storage;
  /* can't be called unix because that's often a predefined
   * macro */
  struct sockaddr_un un;
  struct sockaddr_in ipv4;
  struct sockaddr_in6 ipv6;
} AnySockAddr;

AnySockAddr socket;
int size = sizeof (AnySockAddr);
...
getsockname (fd, &socket.sa, &size);

switch (socket.sa.sa_family)
...

@@ +4101,5 @@
> +  char host[71];
> +  char hostip[INET6_ADDRSTRLEN];
> +  int size = sizeof(struct sockaddr_storage);
> +
> +  getsockname(fd, (struct sockaddr*)&socket, &size);

This should probably be error-checked too.

@@ +4122,5 @@
> +      break;
> +    case AF_INET:
> +      if(inet_ntop(AF_INET, inet_addr, hostip, INET6_ADDRSTRLEN))
> +        snprintf(host, 71, "tcp:family=ipv4,host=%s,port=%hu",
> +                 hostip, ntohs(inet_addr->sin_port));

You don't need "host" or snprintf, just use:

   if (inet_ntop (AF_INET, inet_addr, hostip, sizeof (hostip)))
     ...

   if (!_dbus_string_append_printf (address,
       "tcp:family=ipv4,host=%s,port=%u", hostip, ntohs (...)))
     ...

There's no point in using %hu since varargs smaller than int are always "promoted" to the size of an int (the "default promotions"). %u seems less likely to be missing on a rubbish Unix from the 70s or something.

I'd prefer sizeof (hostip) rather than INET6_ADDRSTRLEN (even though they're equal) because the former is obviously correct at a glance.

@@ +4127,5 @@
> +      else
> +        return FALSE;
> +      if(!_dbus_string_append (address, host))
> +        return FALSE;
> +    case AF_INET6:

This case should be #ifdef AF_INET6.
Comment 8 Auke Kok 2012-06-18 22:22:34 UTC
Comment on attachment 62992 [details] [review]
redone formatting of the ip dresses

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

*** Tested this patch against dbus-1.6.0 release, systemd-git-v185-39-gb30b426 ***

No unexpected behavior - functioned as intended.

performed dbus-activation test with `xfconfd`, the xfce settings daemon. Killed the session bus, started xfce-settings-editor, dbus session bus ran as intended, xfconfd started as intended. xfconfd had the proper DBUS_SESSION_BUS_ADDRESS in it's /proc/<pid>/environ.

Thumbs up from here, many thanks for writing this patch.
Comment 9 Simon Peeters 2012-08-09 01:57:52 UTC
Created attachment 65321 [details] [review]
4th version of the patch

taken into account all issues pointed out by Simon McVittie.

I have not yet got the chance to test this patch, but it compiles fine and should run better than the previous patches.

I hope I finally have the formatting right, and that I will never have to touch dbus code again.
Comment 10 Simon McVittie 2012-08-21 07:44:39 UTC
*** Bug 53639 has been marked as a duplicate of this bug. ***
Comment 11 Patrick McCarty 2012-09-07 00:04:13 UTC
Comment on attachment 65321 [details] [review]
4th version of the patch

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

Tested this version of patch on ArchLinux with dbus 1.6.4, under a systemd --user session.

DBus activation of gconfd works as expected after launching 'gconf-editor'.  Without the patch, gconfd does not activate.
Comment 12 Thiago Macieira 2012-09-07 12:38:35 UTC
Comment on attachment 65321 [details] [review]
4th version of the patch

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

This looks fine to me.
Comment 13 Colin Walters 2012-09-18 18:00:29 UTC
(In reply to comment #11)
> Comment on attachment 65321 [details] [review] [review]
> 4th version of the patch
> 
> Review of attachment 65321 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Tested this version of patch on ArchLinux with dbus 1.6.4, under a systemd
> --user session.
> 
> DBus activation of gconfd works as expected after launching 'gconf-editor'. 
> Without the patch, gconfd does not activate.

Sorry to bikeshed on this, but I have two comments:

1) The coding style is inconsistent.  DBus is GNU style, which in particular means space between identifier and parenthesis.
2) Can you please make this a "git format-patch" style patch?  See https://live.gnome.org/GnomeLove/SubmittingPatches for how to write commit messages.  I think you're in the best position to summarize this change.

I *can* fix both of these for you if needed.
Comment 14 Simon Peeters 2012-10-07 15:09:49 UTC
Created attachment 68211 [details] [review]
5th version, fixed coding style and did "git format-patch"
Comment 15 Colin Walters 2012-10-08 21:11:39 UTC
Comment on attachment 68211 [details] [review]
5th version, fixed coding style and did "git format-patch"

Committed, thanks for jumping through all of the hoops =)

(I did add a space between the inet_ntop calls and the paren)
Comment 16 Auke Kok 2012-10-09 21:24:45 UTC
Thanks folks. This is great.

Would someone change the state on this bug to reflect the change please? :)
Comment 17 Simon McVittie 2012-11-09 10:46:32 UTC
Fixed in git for dbus-1.7.0.

(Auke: I believe anyone can close bugs as FIXED here, you don't need to be a maintainer or anything :-)
Comment 18 Ross Burton 2013-02-22 12:13:31 UTC
Can this be cherry-picked back to 1.6?
Comment 19 Simon McVittie 2013-02-22 12:36:52 UTC
(In reply to comment #18)
> Can this be cherry-picked back to 1.6?

The patch applies cleanly, but I am not comfortable with including it in dbus-1.6 upstream. It's more new code than I'd like to add to a stable-branch: stable-branches ought to be stable.

I've been working on user bus activation myself recently, but it appears to be rather unfinished and has a number of awkward corner cases (Bug #61129, <http://lists.freedesktop.org/archives/systemd-devel/2013-February/009119.html> - participation welcome).

If user bus activation is important in a particular distribution (and you can work around the subtleties, for instance by hard-coding that DISPLAY is always :0 on your platform), I would suggest applying this change as a patch, and doing some fairly serious testing/QA on the result.

I should do a dbus-1.7.0 release at some point, which would include this change and be suitable for sufficiently adventurous downstream distributions. My advice would be not to cherry-pick patches or update to a development release unless you are prepared to keep up with development until the corresponding stable branch is reached.

I don't have a great deal of time to work on D-Bus at the moment, so I don't have a fixed timescale for the corresponding stable branch.