Bug 35230 - [PATCH] add new exec transport on Unix
Summary: [PATCH] add new exec transport on Unix
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review-, minor
Keywords: patch
Depends on:
Blocks: dbus-1.5
  Show dependency treegraph
 
Reported: 2011-03-11 13:55 UTC by Lennart Poettering
Modified: 2014-02-03 16:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
the patch (11.73 KB, patch)
2011-03-11 13:55 UTC, Lennart Poettering
Details | Splinter Review
specification update (3.69 KB, patch)
2011-03-11 14:46 UTC, Lennart Poettering
Details | Splinter Review
part1: introduce _dbus_close_all() (3.14 KB, patch)
2011-07-27 20:51 UTC, Lennart Poettering
Details | Splinter Review
part 2: revised version of the actual exec transport patch, with the dbus_close_all() part split out (10.54 KB, patch)
2011-07-27 20:52 UTC, Lennart Poettering
Details | Splinter Review
updated patch for _dbus_close_all() (3.19 KB, patch)
2011-08-24 15:12 UTC, Lennart Poettering
Details | Splinter Review
updated patch, introducing unixexec: transport (10.00 KB, patch)
2011-08-24 15:31 UTC, Lennart Poettering
Details | Splinter Review
updated spec update (3.70 KB, patch)
2011-08-24 15:38 UTC, Lennart Poettering
Details | Splinter Review
Rebased _dbus_close_all() patch (3.36 KB, patch)
2012-02-01 20:42 UTC, Lennart Poettering
Details | Splinter Review
Rebased specification patch (4.25 KB, patch)
2012-02-01 20:51 UTC, Lennart Poettering
Details | Splinter Review
transport: add new unixexec transport on Unix (13.39 KB, patch)
2012-02-09 16:21 UTC, Lennart Poettering
Details | Splinter Review
transport: add new unixexec transport on Unix (13.39 KB, patch)
2012-02-09 16:25 UTC, Lennart Poettering
Details | Splinter Review
sysdeps-unix: introduce dbus_close_all() and make use of it where appropriate (3.36 KB, patch)
2012-02-13 14:40 UTC, Lennart Poettering
Details | Splinter Review
transport: add new unixexec transport on Unix (13.59 KB, patch)
2012-02-13 14:45 UTC, Lennart Poettering
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lennart Poettering 2011-03-11 13:55:22 UTC
Created attachment 44369 [details] [review]
the patch

The "exec:" transport will create a local AF_UNIX socket with
socketpair(), then fork and execute a binary on one side with STDIN and
STDOUT connected to it and then use the other side.

This is useful to implement D-Bus tunneling schemes, for example to get
a D-Bus connection to the system bus on a different host, similar how
udisks is already doing it. (udisks uses SSH TCP tunneling for this,
which is a bit ugly and less secure than this solution).

Suggested use is with connection strings like the following:

  exec:path=ssh,argv1=foobar,argv2=system-bus-bridge

or:

  exec:path=pkexec,argv1=system-bus-bridge

or even:

  exec:path=sudo,argv1=system-bus-bridge

The first line would execute the binary 'system-bus-bridge' on host
'foobar' and then pass D-Bus traffic to it. This (hypothetical) bridge
binary would then forward the information to the local system bus.

The second and third line use this scheme locally to acquire a
privileged connection through pkexec resp. sudo: instead of connecting
directly to the bus, they use the same bridge binary which will forward
all information to the system bus.

The arguments of the protocol are 'path' for the first execlp()
argument, and argv0, argv1, and so on for the following arguments. argv0
can be left out in which case path will be used.

I'd like to see this patch in dbus 1.4, as it is not intrusive at all.
Comment 1 David Zeuthen (not reading bugmail) 2011-03-11 14:12:00 UTC
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.)
Comment 2 Lennart Poettering 2011-03-11 14:27:21 UTC
(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.
Comment 3 Lennart Poettering 2011-03-11 14:46:33 UTC
Created attachment 44371 [details] [review]
specification update

Here's a patch that adds a short text to the specification text about this.
Comment 4 Simon McVittie 2011-03-14 08:51:10 UTC
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()?
Comment 5 Simon McVittie 2011-03-14 08:53:58 UTC
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?
Comment 6 Simon McVittie 2011-03-14 08:57:53 UTC
(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.
Comment 7 Lennart Poettering 2011-07-27 20:51:30 UTC
Created attachment 49638 [details] [review]
part1: introduce _dbus_close_all()
Comment 8 Lennart Poettering 2011-07-27 20:52:56 UTC
Created attachment 49639 [details] [review]
part 2: revised version of the actual exec transport patch, with the dbus_close_all() part split out
Comment 9 Lennart Poettering 2011-07-27 21:00:08 UTC
(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.
Comment 10 Lennart Poettering 2011-07-27 21:01:08 UTC
(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.
Comment 11 Simon McVittie 2011-08-04 09:37:06 UTC
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?
Comment 12 Simon McVittie 2011-08-04 09:59:08 UTC
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.
Comment 13 Simon McVittie 2011-08-04 09:59:26 UTC
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?
Comment 14 Lennart Poettering 2011-08-24 15:10:26 UTC
(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.
Comment 15 Lennart Poettering 2011-08-24 15:12:06 UTC
Created attachment 50552 [details] [review]
updated patch for _dbus_close_all()
Comment 16 Lennart Poettering 2011-08-24 15:29:57 UTC
(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.
Comment 17 Lennart Poettering 2011-08-24 15:31:49 UTC
Created attachment 50553 [details] [review]
updated patch, introducing unixexec: transport

This also renames the thing from "exec:xxx" to "unixexec:xxx" as suggested.
Comment 18 Lennart Poettering 2011-08-24 15:38:39 UTC
Created attachment 50554 [details] [review]
updated spec update

also update the spec with s/exec/unixexec/
Comment 19 Ralf Habacker 2011-10-31 07:07:31 UTC
(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.
Comment 20 Simon McVittie 2011-11-01 09:11:12 UTC
(In reply to comment #15)
> Created attachment 50552 [details] [review]
> updated patch for _dbus_close_all()

Looks good to me
Comment 21 Simon McVittie 2011-11-01 09:13:19 UTC
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 22 Simon McVittie 2011-11-01 09:21:18 UTC
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
Comment 23 Lennart Poettering 2012-02-01 20:42:57 UTC
Created attachment 56495 [details] [review]
Rebased _dbus_close_all() patch
Comment 24 Lennart Poettering 2012-02-01 20:51:28 UTC
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.
Comment 25 Lennart Poettering 2012-02-09 16:21:14 UTC
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.
Comment 26 Lennart Poettering 2012-02-09 16:25:14 UTC
Created attachment 56842 [details] [review]
transport: add new unixexec transport on Unix

Another iteration, fixing another tiny little buglet.
Comment 27 Simon McVittie 2012-02-10 02:35:23 UTC
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 28 Simon McVittie 2012-02-10 02:38:12 UTC
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 29 Simon McVittie 2012-02-10 02:50:36 UTC
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 :-(
Comment 30 Lennart Poettering 2012-02-13 14:40:37 UTC
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.
Comment 31 Lennart Poettering 2012-02-13 14:45:48 UTC
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.
Comment 32 Lennart Poettering 2012-02-13 14:56:43 UTC
(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.
Comment 33 Simon McVittie 2012-02-14 07:34:27 UTC
This looks good now, I'll ship it.
Comment 34 Simon McVittie 2012-03-12 09:44:05 UTC
Sorry, forgot to merge this. Done now, it'll be in 1.5.12.
Comment 35 Alex Kanavin 2014-02-03 16:39:15 UTC
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


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.