Bug 35887 - use runtime dirs for ~/.dbus autolaunch crap
use runtime dirs for ~/.dbus autolaunch crap
Status: REOPENED
Product: dbus
Classification: Unclassified
Component: core
1.5
All All
: medium enhancement
Assigned To: D-Bus Maintainers
D-Bus Maintainers
review-
: patch
: 64704 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-01 14:29 UTC by william.jon.mccann
Modified: 2014-12-30 06:25 UTC (History)
5 users (show)

See Also:


Attachments
dbus-launch: Move ~/.dbus autolaunch stuff to /run/user (5.01 KB, patch)
2012-04-27 11:43 UTC, Colin Walters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description william.jon.mccann 2011-04-01 14:29:39 UTC
Modern systems are using /run for system service sockets and /run/user for user agent sockets.  It might be worth considering this for dbus as well.

I suppose this would be something like:
/run/dbus for the system bus
/run/$USER/dbus for the session bus

The latter should probably use the http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html XDG_RUNTIME_DIR.
Comment 1 Simon McVittie 2011-04-08 05:33:05 UTC
For the system bus, sure. I think it should be a /run/dbus/ directory so it can contain both the socket and the pid file (for init systems that still need those), and any other clutter we need later (Colin's hypothetical secondary debugging/eavesdropping socket, maybe).

My only worry about this is that /var/run/dbus/system_bus_socket is probably quite popularly hard-coded, as the way you get to the "real" system bus even when installed in a prefix or whatever. Do in-filesystem sockets work with bind-mounts, symlinks, etc.?

---------------------

For the session bus, we can't use /run/$USER/dbus if we still support parallel sessions for one user. I think the unix:tmpdir transport mode is fine, tbh.

We could consider adding a "xdgruntime" transport which declines to listen and fails over to the next transport if XDG_RUNTIME_DIR is unset, which would make the default fallback order be something like:

    xdgruntime:
    unix:tmpdir=/tmp
    autolaunch:

I think that'd do more or less the right thing?
Comment 2 Colin Walters 2011-04-08 09:34:27 UTC
(In reply to comment #1)
> For the system bus, sure. I think it should be a /run/dbus/ directory so it can
> contain both the socket and the pid file (for init systems that still need
> those), and any other clutter we need later (Colin's hypothetical secondary
> debugging/eavesdropping socket, maybe).
> 
> My only worry about this is that /var/run/dbus/system_bus_socket is probably
> quite popularly hard-coded, as the way you get to the "real" system bus even
> when installed in a prefix or whatever.

In fact I just recently did that in jhbuild...
https://bugzilla.gnome.org/show_bug.cgi?id=645187

It's like you're psychic =)

> Do in-filesystem sockets work with
> bind-mounts, symlinks, etc.?

Yeah.

> For the session bus, we can't use /run/$USER/dbus if we still support parallel
> sessions for one user. I think the unix:tmpdir transport mode is fine, tbh.

I think the conclusion on this was that it's an OS policy decision.  
 
> We could consider adding a "xdgruntime" transport 

I'd rather add a configure flag like --enable-single-session which does whatever we want it to do (that may be /run/$USER/dbus).  However - "single session" needs some more work than that.  Basically consider the local GDM + remote ssh case - if the session bus no longer exits after you log out from GDM, then we're back to processes hanging around.  If it does exit, then it breaks ssh.  So we need logic to close connections only from that particular "head" via ConsoleKit.
Comment 3 Colin Walters 2012-04-27 10:56:15 UTC
So this bug got sidetracked because what Jon is concerned about isn't the user agent socket, it's the autolaunch: bits written into ~/.dbus/ by dbus-launch-x11.

We should move that into XDG_RUNTIME_DIR for sure.
Comment 4 Colin Walters 2012-04-27 11:43:26 UTC
Created attachment 60706 [details] [review]
dbus-launch: Move ~/.dbus autolaunch stuff to /run/user

We shouldn't be polluting the user's home directory when we have a
per-machine per-user directory available in XDG_RUNTIME_DIR.
Comment 5 Simon McVittie 2012-04-30 04:53:41 UTC
Comment on attachment 60706 [details] [review]
dbus-launch: Move ~/.dbus autolaunch stuff to /run/user

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

What reads these files? What writes these files? Are you breaking interoperability by doing this?

If the answer is "only dbus-launch-x11 reads or writes these files" then you're still breaking interop between dbus-launch 1.4 and 1.6. Scenario: I started my session (writing files) under dbus-launch 1.4 then upgraded; now dbus-launch only reads the new location.

If these files are needed by X11 autolaunch (I was under the impression the X-root-window-properties dance was meant to make dealing with files unnecessary?), one fix would be to write the new location, but read both locations. If you start a session under dbus-launch 1.6 and then downgrade to 1.4, I think it's reasonable to expect you to restart your session before things will fully work.

If these files are needed now but don't have to be, because we could use X11 as a synchronization point instead, I think I'd prefer to do that (unless there are Wayland-related reasons not to); dbus-launch-x11 will still have to read the files for interop with sessions started under 1.4, but it can stop writing altogether, and use X11 as "the new location".

If these files are not needed, let's just not write them, and use X11 as the synchronization point.

To be honest I'm pretty confused by dbus-launch-x11, and I think it would benefit greatly from someone writing down exactly how it works. I'll see whether I can reverse-engineer what the spec should have said about it.

While you're in the vicinity of dbus-launch I would really, really appreciate some review on Bug #39196 and Bug #39197.

::: tools/dbus-launch-x11.c
@@ +95,5 @@
> +{
> +  const char *arg;
> +  const char *next;
> +  char *p;
> +  char buf[PATH_MAX+1];

There are three categories of system regarding PATH_MAX:

* those that genuinely do have a fixed limit (Windows is the
  only concrete example I know of)

* those where the kernel has no limit, so as required by POSIX,
  the macro is undefined and naive code like this won't compile
  (Hurd)

* those where the kernel has no limit, but the libc
  implementation disregards POSIX and picks an arbitrary number
  for PATH_MAX as a convenient lie so naive code will compile
  (Linux, Mac OS X, probably *BSD)

For instance see the first Google hit for "path_max":

http://insanecoding.blogspot.co.uk/2007/11/pathmax-simply-isnt.html

If you intend the answer to be "it's OK because every string is limited-length and there are a limited number of them", please verify that on entry.

Or, use DBusString. I'm pretty sure we have sysdeps functions for this.

@@ +106,5 @@
> +  arg = first;
> +  do
> +    {
> +      next = va_arg (args, const char *);
> +      strcpy (p, arg);

Overflows and corrupts the stack if arg is long.

@@ +139,5 @@
> +
> +  while ((arg = va_arg (args, const char *)) != NULL)
> +    {
> +      char *end = result + strlen (result);
> +      result = realloc (result, strlen (result) + strlen (arg) + 1);

Segfaults if realloc returns NULL, which (by policy) D-Bus considers to be a thing that can happen. Please at least call a non-returning oom() function that whines and exits nonzero.

I believe tools/ are allowed to use the static libdbus-internal.la, which gives them access to _dbus_everything. If so, couldn't you just use DBusString?

@@ +167,5 @@
> +        autolaunch_data_dir = wish_i_had_g_build_filename (datadir, "dbus-autolaunch", NULL);
> +      else
> +        {
> +          home = get_homedir ();
> +          autolaunch_data_dir = wish_i_had_g_build_filename (home, ".dbus", "session-bus", NULL);

I think this deserves a comment: "Fall back to the old paths; if you don't want your home directory cluttered with this stuff, set XDG_RUNTIME_DIR", or something.
Comment 6 Thiago Macieira 2012-04-30 06:07:54 UTC
The files are created as a helper for D-Bus autolaunch implementations that wish to avoid connecting to X11 (e.g., the dbus-java implementation). So if you move them without symlinking ~/.dbus to the new location, those implementations will break.

So do not move them.

Simon, here's the info you may need for documenting the launch. The way the X11 autolaunch works is like this:

 - dbus-launch keeps a window open, which contains two properties:
   _DBUS_SESSION_BUS_ADDRESS
   _DBUS_SESSION_BUS_PID
 - the window can be found by querying X for the window holding the selection:
   _DBUS_SESSION_BUS_SELECTION_${USER}_${MACHINEID}

The reason for doing that instead of saving in the root window is that, if the dbus-launch process exits, it auto-cleans up. The window it had open will be closed and the selections connected to that window will be cleared. There's never any stale data.

The files may contain stale data. They are provided as an optimisation for a process that wishes to check if there's a session bus started already before forking and launching dbus-launch. It needs to handle a failed connection to the address read from them.

The protocol for creating such is that dbus-launch will:

 1. check if the properties are there
 2. if they are there, return them and exit
 3. start a daemon, get its address and PID
 4. create the window, create the properties and save them on the window.
 5. XGrabServer, get the selection again and if it is still unset, set it to the window; XUngrabServer
 6. if the selection was empty, also save the files in ~/.dbus
 7. return the contents of the window the selection points to
Comment 7 Simon McVittie 2012-04-30 10:25:27 UTC
(In reply to comment #6)
> The files are created as a helper for D-Bus autolaunch implementations that
> wish to avoid connecting to X11 (e.g., the dbus-java implementation).

The reimplementations that I'm aware of are ndesk-dbus (CLI/C#/.net), dbus-sharp (a fork of ndesk-dbus), dbus-java, GDBus and ruby-dbus.

These bindings use libdbus, so are not affected: Net::DBus (Perl), dbus-python, QtDBus, dbus-glib, dbus-c++, edbus (Enlightenment), libnih (as used in Upstart), dbus-ada, eggdbus, clisp-module-dbus, dbus-ocaml.

(I'm using the approximation that if it's not in Debian, it doesn't exist.)

I wonder whether we can we make those implementations run dbus-launch --autolaunch, like GDBus does, if they notice that they're "probably under X" and they don't already? That does require dbus-launch --autolaunch to be a stable interface - but it basically is already.

> So if you
> move them without symlinking ~/.dbus to the new location, those
> implementations will break.
> 
> So do not move them.

If by "break" you mean "not be optimally fast", I can live with that.

If by "break" you mean cease to work, then I think this probably has to be WONTFIX, and it's up to the people who want this change to happen (Jon and Colin) to research how much will break and how seriously, and if the answer is "only incredibly niche situations", argue in its favour.

> Simon, here's the info you may need for documenting the launch.

Thanks!
Comment 8 Colin Walters 2012-05-02 07:04:12 UTC
(In reply to comment #6)
> The files are created as a helper for D-Bus autolaunch implementations that
> wish to avoid connecting to X11 (e.g., the dbus-java implementation). So if you
> move them without symlinking ~/.dbus to the new location, those implementations
> will break.

Ugh...well, the details of autolaunch: aren't part of the official spec, more an implementation detail of libdbus.  

> The reimplementations that I'm aware of are ndesk-dbus (CLI/C#/.net),
> dbus-sharp (a fork of ndesk-dbus), dbus-java, GDBus and ruby-dbus.

Note however that e.g. GDBus just forks off dbus-launch --autolaunch, so it will transparently pick up the fix here.  So we're interested in the set of dbus reimplementations that:

1) Support autolaunch: at all
2) Reimplement the autolaunch: logic from dbus-launch

Simon since you have the list, do you have their sources handy?

> I wonder whether we can we make those implementations run dbus-launch
> --autolaunch, like GDBus does, if they notice that they're "probably under X"
> and they don't already? That does require dbus-launch --autolaunch to be a
> stable interface - but it basically is already.

OK, yes, this =)

> If by "break" you mean cease to work, then I think this probably has to be
> WONTFIX, and it's up to the people who want this change to happen (Jon and
> Colin) to research how much will break and how seriously, and if the answer is
> "only incredibly niche situations", argue in its favour.

I think most distributions/OSes now properly set up the dbus session bus, so the original use case of "launch KDE app in WindowMaker" is pretty dead.

However, autolaunch: remains for the "su -" and "ssh login" cases.  Even though launching a dbus-daemon under ssh is problematic for many reasons.
Comment 9 Thiago Macieira 2012-05-02 08:35:39 UTC
(In reply to comment #7)
> > So if you
> > move them without symlinking ~/.dbus to the new location, those
> > implementations will break.
> > 
> > So do not move them.
> 
> If by "break" you mean "not be optimally fast", I can live with that.
> 
> If by "break" you mean cease to work, then I think this probably has to be
> WONTFIX, and it's up to the people who want this change to happen (Jon and
> Colin) to research how much will break and how seriously, and if the answer is
> "only incredibly niche situations", argue in its favour.

Those implementations could, so far, assume that if the file wasn't there, then the daemon wasn't running. If that was the case, they'd launch it with --autolaunch, which would start it and return the address.

Therefore, I guess it falls under the "not be optimally fast" case, which means we could go ahead anyway.

Except if the code is broken and tries to read off the files even after launching dbus-launch. That is, if they ignore the output of the launcher process, which is the address that the application should connect to.
Comment 10 Matthias Clasen 2012-06-26 10:52:51 UTC
Is this stuck, or are we still waiting for a final consensus here ?
Comment 11 Colin Walters 2012-06-26 11:34:21 UTC
(In reply to comment #10)
> Is this stuck, or are we still waiting for a final consensus here ?

One thing that sucks about hacking on the dbus tools (dbus-launch, dbus-monitor) is that you can only use libc functions, since the internal "poor man's GLib" bits are private to libdbus.

We could work around that in two ways:

1) Separate out a *private* libpoor-mans-glib-under-AFL.so that's linked to by both libdbus.so and dbus-launch/dbus-monitor.
2) Make a static convenience library of some of the core stuff, link it into both
Comment 12 Simon McVittie 2012-06-27 01:10:40 UTC
(In reply to comment #11)
> 2) Make a static convenience library of some of the core stuff, link it into
> both

That already exists, it's libdbus-internal.la.

Each binary may link (directly or indirectly) either libdbus-1.la (usually shared) or libdbus-internal.la (always static, contains additional objects compiled from *-util.c), but never both.
Comment 13 faispasierch 2012-11-21 12:06:09 UTC
*ping
Comment 14 Simon McVittie 2012-11-21 14:42:28 UTC
(In reply to comment #10)
> Is this stuck, or are we still waiting for a final consensus here ?

For the question of "can we move these without breaking compatibility?" the onus is on people in favour of this change to research what, if anything, it will break, and how that breakage can be avoided.

For the question of "if we do move stuff, is this patch OK?", the answer is "not really"; see Comment #5 from the first ":::" onwards.
Comment 15 Simon McVittie 2013-05-17 12:01:38 UTC
*** Bug 64704 has been marked as a duplicate of this bug. ***
Comment 16 Carl George 2014-12-30 06:24:51 UTC
I wish I knew C better so that I could correct the patch to make it acceptable.

I do have a suggestion for the "breaking compatibility" argument.  Until the patch is fixed and proper testing is done, can the DBUS_DIR variable be exposed in a way that it can be overridden by an environment variable?  Here is what I'm thinking.

export DBUS_DIR=/home/carl/.config/dbus

This approach would serve two purposes.  It would assist in testing for compatibility issues of moving that configuration, and it would (temporarily) address the "polluting the user's home directory" issue.