Bug 35230

Should this go into the spec, e.g. should other D-Bus implementations (such as GDBus) be required to implement something like this?

If yes, should probably not specifically mention/require socketpair(2), it should work just fine with pipe(2) or some named pipe thing on Windows or whatever an OS uses for IPC

If no, would the libdbus-1 implementation only work on Unix/POSIX?

(Btw, In GDBus you can already do this because of the fact that GDBus already works over any valid GIOStream implementation. So it'd be almost trivial to implement this GDBus.)

(In reply to comment #1)
> Should this go into the spec, e.g. should other D-Bus implementations (such as
> GDBus) be required to implement something like this?

I figure GDBus should implement this too.

I'll prep a patch for the spec.

> If yes, should probably not specifically mention/require socketpair(2), it
> should work just fine with pipe(2) or some named pipe thing on Windows or
> whatever an OS uses for IPC

I have no clue whether you can do these things at all on Windows, and I am not sure how that would be relevant on Windows anyway, given the lack of tools like pkexec, ssh, sudo... I'd like to leave that out for now. If Windows folks care enough we can make this cross-platform later.

I think it is worth mentioning that on Unix this is guaranteed to be a proper socket, so that people know they can rely on SCM_CREDENTIAL based auth, and on unix fd passing.

> If no, would the libdbus-1 implementation only work on Unix/POSIX?

Yes. It does exec() and fork() and all kinds of other stuff, which does not translate at all to Windows.

Created attachment 44371 [details] [review]
specification update

Here's a patch that adds a short text to the specification text about this.

Review of attachment 44369 [details] [review]:

I think new features should be 1.5 material, even if they're this small; we can do a 1.5.0 whenever.

I'd love to see dbus-daemon grow support for being used like this, with some way to either retrieve the PID to kill it, or tell it to exit with a D-Bus message: it'd be great for "dbus-daemon on a stick" regression tests like those in telepathy-glib.

::: dbus/dbus-sysdeps-unix.c
@@ +843,2 @@
 /**
+ * Closes all file descriptors except the first three.

I did something remarkably similar (although Linux-specific) for Bug #35173, and the same code may exist elsewhere.

Could you make this into internal API (if needed outside this file), and check for any other implementations / convert them to it? Ideally, also use /proc on Linux like I did for Bug #35173.

@@ +865,3 @@
+ * process to execute.
+ *
+ * This will set FD_CLOEXEC for the socket returned.

ifndef SOCK_CLOEXEC you don't do that: you'll need to do a fcntl or something.

@@ +929,3 @@
+      execvp (path, argv);
+
+      _exit(1);

Shouldn't this report the failure to exec somehow? stderr would be better than nothing.

::: dbus/dbus-transport-unix.c
@@ +163,3 @@
+            }
+
+          success = _dbus_string_append_printf (&address, "exec:argv%i=%s", i, escaped);

%u to printf an unsigned int

@@ +198,3 @@
+  _dbus_close_socket (fd, NULL);
+ failed_0:
+  _dbus_string_free (&address);

I'd prefer to have a single "except clause":

failed:
  if (fd != -1)
    _dbus_close_socket (fd, NULL);

  _dbus_string_free (&address);

The "failed_4" pattern gets a bit mad in more complex functions, and using -1 or NULL to mark not-used-yet variables lets you have a single code path.

@@ +275,3 @@
+        {
+          _dbus_set_bad_address (error, NULL, NULL,
+                                 "No process pass specified");

process path?

@@ +281,3 @@
+      /* First count argv arguments */
+      for (i = 1;; i++)
+        {

I'd prefer a space between the semicolons to make the empty statement more obvious.

@@ +318,3 @@
+
+          l = strlen (p)+1;
+          argv[i] = dbus_new (char, l);

Why not _dbus_strdup()?

Review of attachment 44371 [details] [review]:

I like the look of this for 1.5. The spec text itself could go into 1.4 if you prefer (but the dbus-specification on the website should be the one from 1.5 - after we release 1.5.0, we should disable the HTML-uploading makefile functionality in 1.4).

::: doc/dbus-specification.xml
@@ +2718,3 @@
+      </para>
+      <para>
+        Executed subprocesses are not available on windows.

"not currently available" if we'd like them to be?

(In reply to comment #1)
> If yes, should probably not specifically mention/require socketpair(2), it
> should work just fine with pipe(2) or some named pipe thing on Windows or
> whatever an OS uses for IPC

pipe(2) isn't always bidirectional, so it might take more than one fd to use those.

If we want pipe(2) to be possible, it'd be worth specifying that the exec'd process's DBusServer-or-equivalent must read from stdin and write to stdout, and must not assume that getting them backwards will work. If we want to mandate bidi pipes it'd be worth specifying that stdin and stdout must point to the same underlying socket.

(In reply to comment #2)
> I think it is worth mentioning that on Unix this is guaranteed to be a proper
> socket, so that people know they can rely on SCM_CREDENTIAL based auth, and on
> unix fd passing.

I do like the sound of this.

Created attachment 49638 [details] [review]
part1: introduce _dbus_close_all()

Created attachment 49639 [details] [review]
part 2: revised version of the actual exec transport patch, with the dbus_close_all() part split out

(In reply to comment #4)

> ::: dbus/dbus-sysdeps-unix.c
> @@ +843,2 @@
>  /**
> + * Closes all file descriptors except the first three.
> 
> I did something remarkably similar (although Linux-specific) for Bug #35173,
> and the same code may exist elsewhere.

Actually it doesn't. Couldn't find any. (when we spawn bus activation services we rely on O_CLOEXEC being set properly). Nonetheless I have split this call off now, so that if it is needed somewhere people can just use it.

> Could you make this into internal API (if needed outside this file), and check
> for any other implementations / convert them to it? Ideally, also use /proc on
> Linux like I did for Bug #35173.

Done and done.

> @@ +865,3 @@
> + * process to execute.
> + *
> + * This will set FD_CLOEXEC for the socket returned.
> 
> ifndef SOCK_CLOEXEC you don't do that: you'll need to do a fcntl or something.

fixed.
 
> @@ +929,3 @@
> +      execvp (path, argv);
> +
> +      _exit(1);
> 
> Shouldn't this report the failure to exec somehow? stderr would be better than
> nothing.

fixed.

> ::: dbus/dbus-transport-unix.c
> @@ +163,3 @@
> +            }
> +
> +          success = _dbus_string_append_printf (&address, "exec:argv%i=%s", i,
> escaped);
> 
> %u to printf an unsigned int

fixed.

> @@ +198,3 @@
> +  _dbus_close_socket (fd, NULL);
> + failed_0:
> +  _dbus_string_free (&address);
> 
> I'd prefer to have a single "except clause":
> 
> failed:
>   if (fd != -1)
>     _dbus_close_socket (fd, NULL);
> 
>   _dbus_string_free (&address);
> 
> The "failed_4" pattern gets a bit mad in more complex functions, and using -1
> or NULL to mark not-used-yet variables lets you have a single code path.

fixed. (note that this was just a carbon copy of the style done a few lines above the patch, but anyway, i hate the label creeperitis, too)

> 
> @@ +275,3 @@
> +        {
> +          _dbus_set_bad_address (error, NULL, NULL,
> +                                 "No process pass specified");
> 
> process path?

fixed.
 
> @@ +281,3 @@
> +      /* First count argv arguments */
> +      for (i = 1;; i++)
> +        {
> 
> I'd prefer a space between the semicolons to make the empty statement more
> obvious.

fixed. 

> @@ +318,3 @@
> +
> +          l = strlen (p)+1;
> +          argv[i] = dbus_new (char, l);
> 
> Why not _dbus_strdup()?

uh, dunno what i was smoking there. fixed.

(In reply to comment #6)
> (In reply to comment #1)
> > If yes, should probably not specifically mention/require socketpair(2), it
> > should work just fine with pipe(2) or some named pipe thing on Windows or
> > whatever an OS uses for IPC
> 
> pipe(2) isn't always bidirectional, so it might take more than one fd to use
> those.

Pipes *never* are bidirectional.

Review of attachment 49638 [details] [review]:

::: dbus/dbus-sysdeps-unix.c
@@ +3866,3 @@
+ */
+void
+_dbus_close_all (void)

I'd prefer it documented as "except stdin, stdout and stderr" to explain the magic number 3.

If you put the entire body of this function in

{
#ifdef HAVE_CLOSEFROM
  closefrom (3);
#else
  ... what you wrote ...
#endif
}

and add closefrom to the giant AC_CHECK_FUNCS call in configure.ac, then it'll also close Bug #29526 at the same time.

(closefrom() is a Solaris 10'ism, also available on *BSD; I'm surprised that glibc doesn't have it, but somewhat predictably, it turns out that's because Ulrich Drepper WONTFIX'd it. <http://sourceware.org/bugzilla/show_bug.cgi?id=10353>)

@@ +3910,3 @@
+      closedir (d);
+    }
+#endif

On Linux, don't you want to early-return just after closedir(), rather than re-doing it the slow way?

Review of attachment 49639 [details] [review]:

I'd like a regression test, really. Perhaps something similar to test/loopback.c, using "exec:path=${abs_top_builddir}/test/exec-test-helper,argv0=argv0,argv1=--argv1,argv2=argv2", where exec-test-helper is a new executable that just asserts that its arguments are as expected, then writes stdin to stdout until EOF?

::: dbus/dbus-sysdeps-unix.c
@@ +840,3 @@
+ * This will set FD_CLOEXEC for the socket returned.
+ *
+ * @param path the path to UNIX domain socket

You mean "the path to the executable".

argv[] is not documented. I think it's worth mentioning explicitly that it has the main()-like "argv[0] is typically the same as path" semantics.

@@ +841,3 @@
+ *
+ * @param path the path to UNIX domain socket
+ * @param abstract #TRUE to use abstract namespace

This parameter is documented but does not exist. (I think it's OK for it not to exist - you should just use whatever socketpair() returns.)

@@ +903,3 @@
+      if (path[0] == '/')
+        execv (path, argv);
+      execvp (path, argv);

You could just call execvp() unconditionally, removing the "if"? If the man page is to be believed, execvp() already behaves the same as execv() for paths with a slash.

::: dbus/dbus-transport-unix.h
@@ +32,3 @@
                                                       DBusError        *error);
 
+DBusTransport* _dbus_transport_new_for_exec (const char *path,

Is there any reason this should be exported? It looks to me as though it could just be static.

Review of attachment 44371 [details] [review]:

::: doc/dbus-specification.xml
@@ +2709,3 @@
+        This transport forks off a process and connects its standard
+        input and standard output with an anonymous Unix domain
+        socket. This socket is then used for communication by the

I agree that it's valuable to guarantee that this is a "proper" Unix socket. It might be worth calling it unixexec: to make this completely obvious?

(In reply to comment #11)
> Review of attachment 49638 [details] [review]:
> 
> ::: dbus/dbus-sysdeps-unix.c
> @@ +3866,3 @@
> + */
> +void
> +_dbus_close_all (void)
> 
> I'd prefer it documented as "except stdin, stdout and stderr" to explain the
> magic number 3.

Added.
> 
> If you put the entire body of this function in
> 
> {
> #ifdef HAVE_CLOSEFROM
>   closefrom (3);
> #else
>   ... what you wrote ...
> #endif
> }
> 
> and add closefrom to the giant AC_CHECK_FUNCS call in configure.ac, then it'll
> also close Bug #29526 at the same time.
> 
> (closefrom() is a Solaris 10'ism, also available on *BSD; I'm surprised that
> glibc doesn't have it, but somewhat predictably, it turns out that's because
> Ulrich Drepper WONTFIX'd it.
> <http://sourceware.org/bugzilla/show_bug.cgi?id=10353>)

Sorry, I cannot test this. It's a pointless excercise trying to write code for a specific platform when you cannot test your code on it. This patch has to come from people actually running Solaris. Also, I don't care for Solaris... So nope, won't include this. Solaris people should fix this on their own.

> 
> @@ +3910,3 @@
> +      closedir (d);
> +    }
> +#endif
> 
> On Linux, don't you want to early-return just after closedir(), rather than
> re-doing it the slow way?

Ouch. This is a bug. Fixed.

Created attachment 50552 [details] [review]
updated patch for _dbus_close_all()

(In reply to comment #12)
> Review of attachment 49639 [details] [review]:
> 
> I'd like a regression test, really. Perhaps something similar to
> test/loopback.c, using
> "exec:path=${abs_top_builddir}/test/exec-test-helper,argv0=argv0,argv1=--argv1,argv2=argv2",
> where exec-test-helper is a new executable that just asserts that its arguments
> are as expected, then writes stdin to stdout until EOF?

Hmm, to be frank I am too lazy for this. What about this: I am working on a generic forwarder tool which clients can start via ssh on some other machine which does nothing but connect stdio to the system bus. THis would then be useful for tools like udisks, systemd and so on to connect to a remote system bus via ssh. For that tool I'll add a regression test, which would implicitly test this transport too.

> ::: dbus/dbus-sysdeps-unix.c
> @@ +840,3 @@
> + * This will set FD_CLOEXEC for the socket returned.
> + *
> + * @param path the path to UNIX domain socket
> 
> You mean "the path to the executable".

Fixed.

> 
> argv[] is not documented. I think it's worth mentioning explicitly that it has
> the main()-like "argv[0] is typically the same as path" semantics.

Fixed.
> 
> @@ +841,3 @@
> + *
> + * @param path the path to UNIX domain socket
> + * @param abstract #TRUE to use abstract namespace
> 
> This parameter is documented but does not exist. (I think it's OK for it not to
> exist - you should just use whatever socketpair() returns.)

Oops, copy&paste mistake. Fixed.

> 
> @@ +903,3 @@
> +      if (path[0] == '/')
> +        execv (path, argv);
> +      execvp (path, argv);
> 
> You could just call execvp() unconditionally, removing the "if"? If the man
> page is to be believed, execvp() already behaves the same as execv() for paths
> with a slash.

Oh, right. I just had copied that from somewhere else in the dbus code. Fixed.

> 
> ::: dbus/dbus-transport-unix.h
> @@ +32,3 @@
>                                                        DBusError       
> *error);
> 
> +DBusTransport* _dbus_transport_new_for_exec (const char *path,
> 
> Is there any reason this should be exported? It looks to me as though it could
> just be static.

Fixed.

Created attachment 50553 [details] [review]
updated patch, introducing unixexec: transport

This also renames the thing from "exec:xxx" to "unixexec:xxx" as suggested.

Created attachment 50554 [details] [review]
updated spec update

also update the spec with s/exec/unixexec/

(In reply to comment #2)
> 
> I have no clue whether you can do these things at all on Windows, and I am not
> sure how that would be relevant on Windows anyway, given the lack of tools like
> pkexec, ssh, sudo... 

there is a ssh replacement named plink, see http://www.chiark.greenend.org.uk/~sgtatham/putty/, which is used for example by subversion and git to create ssh based protocols.

(In reply to comment #15)
> Created attachment 50552 [details] [review]
> updated patch for _dbus_close_all()

Looks good to me

Comment on attachment 50554 [details] [review]
updated spec update

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

Looks good

::: doc/dbus-specification.xml
@@ +2810,5 @@
> +        The forked process will inherit the standard error output and
> +        process group from the parent process.
> +      </para>
> +      <para>
> +        Executed subprocesses are not available on windows.

nitpicking: it's called Windows

Comment on attachment 50553 [details] [review]
updated patch, introducing unixexec: transport

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

I still think this deserves a regression test.

I assume "fixo" at the end of the commit message is a relic of rebasing?

::: dbus/dbus-sysdeps-unix.c
@@ +907,5 @@
> +
> +      _exit(1);
> +    }
> +
> +  close (fds[1]);

maybe worth commenting /* parent */ just above here

::: dbus/dbus-transport-unix.c
@@ +161,5 @@
> +              dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
> +              goto failed;
> +            }
> +
> +          success = _dbus_string_append_printf (&address, "unixexec:argv%u=%s", i, escaped);

Won't this come out as something like

    unixexec:path=/foounixexec:argv0=bar

or whatever? A good regression test would have caught this.

@@ +162,5 @@
> +              goto failed;
> +            }
> +
> +          success = _dbus_string_append_printf (&address, "unixexec:argv%u=%s", i, escaped);
> +          dbus_free(escaped);

Space before opening parenthesis

Created attachment 56495 [details] [review]
Rebased _dbus_close_all() patch

Created attachment 56496 [details] [review]
Rebased specification patch

(In reply to comment #21)

> ::: doc/dbus-specification.xml
> @@ +2810,5 @@
> > +        The forked process will inherit the standard error output and
> > +        process group from the parent process.
> > +      </para>
> > +      <para>
> > +        Executed subprocesses are not available on windows.
> 
> nitpicking: it's called Windows

Fixed.

Created attachment 56841 [details] [review]
transport: add new unixexec transport on Unix

(In reply to comment #22)
> Comment on attachment 50553 [details] [review] [review]
> updated patch, introducing unixexec: transport
> 
> Review of attachment 50553 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I still think this deserves a regression test.

I have now added a simple regression test, which tests the non-trivial bits of the patch (though not actually the real transport): the parsing of the address specification, as well as the proper formatting of it later on.
 
> I assume "fixo" at the end of the commit message is a relic of rebasing?

Indeed, fixed.

> 
> ::: dbus/dbus-sysdeps-unix.c
> @@ +907,5 @@
> > +
> > +      _exit(1);
> > +    }
> > +
> > +  close (fds[1]);
> 
> maybe worth commenting /* parent */ just above here

Added.

> 
> ::: dbus/dbus-transport-unix.c
> @@ +161,5 @@
> > +              dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
> > +              goto failed;
> > +            }
> > +
> > +          success = _dbus_string_append_printf (&address, "unixexec:argv%u=%s", i, escaped);
> 
> Won't this come out as something like
> 
>     unixexec:path=/foounixexec:argv0=bar
> 
> or whatever? A good regression test would have caught this.

Fixed.

> @@ +162,5 @@
> > +              goto failed;
> > +            }
> > +
> > +          success = _dbus_string_append_printf (&address, "unixexec:argv%u=%s", i, escaped);
> > +          dbus_free(escaped);
> 
> Space before opening parenthesis

Fixed.

Created attachment 56842 [details] [review]
transport: add new unixexec transport on Unix

Another iteration, fixing another tiny little buglet.

Comment on attachment 56495 [details] [review]
Rebased _dbus_close_all() patch

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

"appropriate" is not spelled "appropirate" :-)

Otherwise looks fine, and if someone whose platform has closeall() wants to add it, they can.

Comment on attachment 56496 [details] [review]
Rebased specification patch

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

Looks fine.

::: doc/dbus-specification.xml
@@ +2837,5 @@
> +        The forked process will inherit the standard error output and
> +        process group from the parent process.
> +      </para>
> +      <para>
> +        Executed subprocesses are not available on Windows.

(Implementation note: if we wanted to have a Windows-compatible pipe: transport, we'd have to use a pair of pipes, and be aware that "normal Unix things" like fd-passing and credentials-passing do not work on that transport.)

Comment on attachment 56842 [details] [review]
transport: add new unixexec transport on Unix

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

::: dbus/dbus-connection-internal.h
@@ +117,5 @@
>                                   dbus_uint32_t  *out_peak_fds);
>  
> +
> +/* if DBUS_BUILD_TESTS */
> +DBusTransport* _dbus_connection_get_transport (DBusConnection *connection);

I'd really prefer _dbus_connection_get_address(), or even a public dbus_connection_get_address().

::: dbus/dbus-sysdeps-unix.c
@@ +935,5 @@
> +      _dbus_close_all ();
> +
> +      execvp (path, argv);
> +
> +      fprintf (stderr, "Failed to execute process %s: %s\n", path, _dbus_strerror(errno));

Space before parenthesis (but, yeah, whatever)

::: dbus/dbus-transport-unix.c
@@ +142,5 @@
> +
> +  fd = -1;
> +
> +  if (!_dbus_string_append (&address, "unixexec:path=") ||
> +      !_dbus_string_append (&address, path))

Shouldn't the path get escaped too?

@@ +162,5 @@
> +              dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
> +              goto failed;
> +            }
> +
> +          success = _dbus_string_append_printf (&address, ",unixexec:argv%u=%s", i, escaped);

Normally address strings look like

    unixexec:path=x,argv0=y,argv1=z

rather than this, which would produce

    unixexec:path=x,unixexec:argv0=y,unixexec:argv1=z

Is this intentional?

@@ +421,5 @@
> +  t = _dbus_connection_get_transport (c);
> +  _dbus_assert (t != NULL);
> +
> +  /* Let's see if the address got parsed, reordered and formatted correctly */
> +  ret = strcmp (_dbus_transport_get_address (t), "unixexec:path=/bin/false,unixexec:argv0=false,unixexec:argv1=foobar") == 0;

... tested, but doesn't look right?

I thought there was already a dbus_connection_get_address() you could use to test this, but apparently not :-(

Created attachment 56995 [details] [review]
sysdeps-unix: introduce dbus_close_all() and make use of it where appropriate

(In reply to comment #27)

> "appropriate" is not spelled "appropirate" :-)

You must admit though that "appropirate" is a much nicer word! I has this delicious smell of adventure, wide and open seas and Disney movies!

Anyway, fixed now.

Created attachment 56996 [details] [review]
transport: add new unixexec transport on Unix

(In reply to comment #29)

> ::: dbus/dbus-connection-internal.h
> @@ +117,5 @@
> >                                   dbus_uint32_t  *out_peak_fds);
> >  
> > +
> > +/* if DBUS_BUILD_TESTS */
> > +DBusTransport* _dbus_connection_get_transport (DBusConnection *connection);
> 
> I'd really prefer _dbus_connection_get_address(), or even a public dbus_connection_get_address().

OK, intrdouced _dbus_connection_get_address() now. I didn't make this public, since I tend to be very conservative about API additions and the D-Bus API is way to big already anyway. If we need this we can easily export it later.

> ::: dbus/dbus-sysdeps-unix.c
> @@ +935,5 @@
> > +      _dbus_close_all ();
> > +
> > +      execvp (path, argv);
> > +
> > +      fprintf (stderr, "Failed to execute process %s: %s\n", path, _dbus_strerror(errno));
> 
> Space before parenthesis (but, yeah, whatever)

Fixed.
> 
> ::: dbus/dbus-transport-unix.c
> @@ +142,5 @@
> > +
> > +  fd = -1;
> > +
> > +  if (!_dbus_string_append (&address, "unixexec:path=") ||
> > +      !_dbus_string_append (&address, path))
> 
> Shouldn't the path get escaped too?

Well, for normal AF_UNIX it currently isn't which this case is based on. I know added the escaping here, since you are right, but we probably should do the same for normal AF_UNIX too one day.

> @@ +162,5 @@
> > +              dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
> > +              goto failed;
> > +            }
> > +
> > +          success = _dbus_string_append_printf (&address, ",unixexec:argv%u=%s", i, escaped);
> 
> Normally address strings look like
> 
>     unixexec:path=x,argv0=y,argv1=z
> 
> rather than this, which would produce
> 
>     unixexec:path=x,unixexec:argv0=y,unixexec:argv1=z
> 
> Is this intentional?

Well, they are considered equivalent by the parser. However in order to minimize surprises I have changed the output to the shorter version.

Anyway, should all be fixed now.

(In reply to comment #31)

> > > +  if (!_dbus_string_append (&address, "unixexec:path=") ||
> > > +      !_dbus_string_append (&address, path))
> > 
> > Shouldn't the path get escaped too?
> 
> Well, for normal AF_UNIX it currently isn't which this case is based on. I know added the escaping here, since you are right, but we probably should do the
> same for normal AF_UNIX too one day.

Filed bug 46013 now about this, so that we don't forget.

This looks good now, I'll ship it.

Sorry, forgot to merge this. Done now, it'll be in 1.5.12.

Hello,

the patch covers the parent-side of the unixexec transport, but it's missing the child side - making a dbus connection out of input and output file descriptors. Something like dbus_connection_open_using_fds() is needed. I opened bug 74454 for this, but wanted to notify everyone here too.

Alex