tools/dbus-run-session: fix race between manual and automatically started dbus-daemon on Windows

dbus-run-session starts a dbus-daemon before the client application.
We must avoid letting the application try to connect before the
dbus-daemon's DBusServer is listening for connections.

In the Unix implementation, we already achieved this via the
--print-address option. If the client tried to connect too soon,
the server would not yet be listening and the client would fail.

In the Windows implementation, we communicate the bus address to
the client application as an autolaunch: address, so if the client
tried to connect too soon, it would autolaunch a new dbus-daemon
instead of using the one that it was intended to use.

We can avoid this by using a new option to pass in a Windows event
object, which will be set when the server has started and is ready
to process connections.

Fixes #297

bus/bus.c

@@ -764,11 +764,13 @@ process_config_postinit (BusContext      *context,
   return TRUE;
 }
 
+/* Takes ownership of print_addr_pipe fds, print_pid_pipe fds and ready_event_handle */
 BusContext*
 bus_context_new (const DBusString *config_file,
                  BusContextFlags   flags,
                  DBusPipe         *print_addr_pipe,
                  DBusPipe         *print_pid_pipe,
+                 void             *ready_event_handle,
                  const DBusString *address,
                  DBusError        *error)
 {
@@ -891,6 +893,17 @@ bus_context_new (const DBusString *config_file,
       _dbus_string_free (&addr);
     }
 
+#ifdef DBUS_WIN
+  if (ready_event_handle != NULL)
+    {
+      _dbus_verbose ("Notifying that we are ready to receive connections (event handle=%p)\n", ready_event_handle);
+      if (!_dbus_win_event_set (ready_event_handle, error))
+        goto failed;
+      if (!_dbus_win_event_free (ready_event_handle, error))
+        goto failed;
+    }
+#endif
+
   context->connections = bus_connections_new (context);
   if (context->connections == NULL)
     {

bus/bus.h

@@ -88,6 +88,7 @@ BusContext*       bus_context_new                                (const DBusStri
                                                                   BusContextFlags   flags,
                                                                   DBusPipe         *print_addr_pipe,
                                                                   DBusPipe         *print_pid_pipe,
+                                                                  void             *ready_event_handle,
                                                                   const DBusString *address,
                                                                   DBusError        *error);
 dbus_bool_t       bus_context_reload_config                      (BusContext       *context,

bus/main.c

@@ -48,6 +48,10 @@
 
 static BusContext *context;
 
+#ifdef DBUS_WIN
+#include <dbus/dbus-sysdeps-win.h>
+#endif
+
 #ifdef DBUS_UNIX
 
 /* Despite its name and its unidirectional nature, this is actually
@@ -163,6 +167,9 @@ usage (void)
       " [--syslog]"
       " [--syslog-only]"
       " [--nofork]"
+#ifdef DBUS_WIN
+      " [--ready-event-handle=value]"
+#endif
 #ifdef DBUS_UNIX
       " [--fork]"
       " [--systemd-activation]"
@@ -403,6 +410,8 @@ main (int argc, char **argv)
   dbus_bool_t print_address;
   dbus_bool_t print_pid;
   BusContextFlags flags;
+  void *ready_event_handle;
+
 #ifdef DBUS_UNIX
   const char *error_str;
 
@@ -428,6 +437,7 @@ main (int argc, char **argv)
    * to inherit fds we might have inherited from our caller. */
   _dbus_fd_set_all_close_on_exec ();
 #endif
+  ready_event_handle = NULL;
 
   if (!_dbus_string_init (&config_file))
     return 1;
@@ -619,6 +629,20 @@ main (int argc, char **argv)
         {
           print_pid = TRUE; /* and we'll get the next arg if appropriate */
         }
+#ifdef DBUS_WIN
+      else if (strstr (arg, "--ready-event-handle=") == arg)
+        {
+          const char *desc;
+          desc = strchr (arg, '=');
+          _dbus_assert (desc != NULL);
+          ++desc;
+          if (sscanf (desc, "%p", &ready_event_handle) != 1)
+            {
+              fprintf (stderr, "%s specified, but invalid handle provided\n", arg);
+              exit (1);
+            }
+        }
+#endif
       else
         {
           usage ();
@@ -693,7 +717,7 @@ main (int argc, char **argv)
 
   dbus_error_init (&error);
   context = bus_context_new (&config_file, flags,
-                             &print_addr_pipe, &print_pid_pipe,
+                             &print_addr_pipe, &print_pid_pipe, ready_event_handle,
                              _dbus_string_get_length(&address) > 0 ? &address : NULL,
                              &error);
   _dbus_string_free (&config_file);

bus/test.c

@@ -296,7 +296,7 @@ bus_context_new_test (const DBusString *test_data_dir,
     }
 
   dbus_error_init (&error);
-  context = bus_context_new (&config_file, BUS_CONTEXT_FLAG_NONE, NULL, NULL, NULL, &error);
+  context = bus_context_new (&config_file, BUS_CONTEXT_FLAG_NONE, NULL, NULL, NULL, NULL, &error);
   if (context == NULL)
     {
       _DBUS_ASSERT_ERROR_IS_SET (&error);

dbus/dbus-spawn-win.c

@@ -497,7 +497,8 @@ build_env_string (char** envp)
 HANDLE
 _dbus_spawn_program (const char *name,
                      char      **argv,
-                     char      **envp)
+                     char      **envp,
+                     dbus_bool_t inherit_handles)
 {
   PROCESS_INFORMATION pi = { NULL, 0, 0, 0 };
   STARTUPINFOA si;
@@ -531,7 +532,12 @@ _dbus_spawn_program (const char *name,
 #ifdef DBUS_WINCE
   result = CreateProcessA (name, arg_string, NULL, NULL, FALSE, 0,
 #else
-  result = CreateProcessA (NULL, arg_string, NULL, NULL, FALSE, 0,
+  result = CreateProcessA (NULL,  /* no application name */
+                           arg_string,
+                           NULL, /* no process attributes */
+                           NULL, /* no thread attributes */
+                           inherit_handles, /* inherit handles */
+                           0, /* flags */
 #endif
 			   (LPVOID)env_string, NULL, &si, &pi);
   free (arg_string);
@@ -666,7 +672,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
   _dbus_verbose ("babysitter: spawn child '%s'\n", my_argv[0]);
 
   PING();
-  handle = _dbus_spawn_program (sitter->log_name, my_argv, (char **) envp);
+  handle = _dbus_spawn_program (sitter->log_name, my_argv, (char **) envp, FALSE);
 
   if (my_argv != NULL)
     {

dbus/dbus-sysdeps-win.h

@@ -93,7 +93,10 @@ void        _dbus_threads_windows_ensure_ctor_linked (void);
 DBUS_PRIVATE_EXPORT
 dbus_bool_t _dbus_getsid(char **sid, dbus_pid_t process_id);
 
-HANDLE      _dbus_spawn_program (const char *name, char **argv, char **envp);
+HANDLE      _dbus_spawn_program (const char *name,
+                                 char **argv,
+                                 char **envp,
+                                 dbus_bool_t inherit_handles);
 
 DBUS_PRIVATE_EXPORT
 void _dbus_win_set_error_from_last_error (DBusError  *error,

doc/dbus-daemon.1.xml.in

@@ -32,6 +32,7 @@
     <arg choice='opt'>--nosyslog </arg>
     <arg choice='opt'>--syslog </arg>
     <arg choice='opt'>--syslog-only </arg>
+    <arg choice='opt'>--ready-event-handle=value</arg>
     <sbr/>
 </cmdsynopsis>
 </refsynopsisdiv>
@@ -199,6 +200,29 @@ files.</para>
     </listitem>
   </varlistentry>
 
+  <varlistentry>
+    <term><option>--ready-event-handle=value</option></term>
+    <listitem>
+      <para>With this option, the dbus daemon raises an event when it is ready to process
+        connections. The <replaceable>handle</replaceable> must be the Windows handle
+        for an event object, in the format printed by the <function>printf</function>
+        format string <literal>%p</literal>. The parent process must create this event
+        object (for example with the <function>CreateEvent</function> function) in a
+        nonsignaled state, then configure it to be inherited by the dbus-daemon process.
+        The dbus-daemon will signal the event as if via <function>SetEvent</function>
+        when it is ready to receive connections from clients. The parent process can
+        wait for this to occur by using functions such as
+        <function>WaitForSingleObject</function>.
+        This option is only supported under Windows. On Unix platforms,
+        a similar result can be achieved by waiting for the address and/or
+        process ID to be printed to the inherited file descriptors used
+        for <option>--print-address</option> and/or <option>--print-pid</option>.
+      </para>
+    </listitem>
+  </varlistentry>
+
+
+
 </variablelist>
 </refsect1>
 

tools/CMakeLists.txt

@@ -57,6 +57,8 @@ set(dbus_cleanup_sockets_SOURCES
 
 set(dbus_run_session_SOURCES
     dbus-run-session.c
+    tool-common.c
+    tool-common.h
 )
 
 set(dbus_uuidgen_SOURCES

tools/Makefile.am

@@ -71,7 +71,10 @@ dbus_launch_LDADD = \
 	$(NULL)
 
 dbus_run_session_SOURCES =			\
-	dbus-run-session.c
+	dbus-run-session.c \
+	tool-common.c \
+	tool-common.h
+	$(NULL)
 
 dbus_run_session_LDADD = \
 	$(CODE_COVERAGE_LIBS) \

tools/dbus-run-session.c

@@ -4,7 +4,7 @@
  * Copyright © 2003-2006 Red Hat, Inc.
  * Copyright © 2006 Thiago Macieira <thiago@kde.org>
  * Copyright © 2011-2012 Nokia Corporation
- * Copyright © 2018 Ralf Habacker
+ * Copyright © 2018, 2021 Ralf Habacker
  *
  * Licensed under the Academic Free License version 2.1
  *
@@ -47,6 +47,8 @@
 #include "dbus/dbus.h"
 #include "dbus/dbus-internals.h"
 
+#include "tool-common.h"
+
 #define MAX_ADDR_LEN 512
 #define PIPE_READ_END  0
 #define PIPE_WRITE_END 1
@@ -101,7 +103,7 @@ version (void)
           "Copyright (C) 2003-2006 Red Hat, Inc.\n"
           "Copyright (C) 2006 Thiago Macieira\n"
           "Copyright © 2011-2012 Nokia Corporation\n"
-          "Copyright © 2018 Ralf Habacker\n"
+          "Copyright © 2018, 2021 Ralf Habacker\n"
           "\n"
           "This is free software; see the source for copying conditions.\n"
           "There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n",
@@ -381,10 +383,11 @@ run_session (const char *dbus_daemon,
              char      **argv,
              int         prog_arg)
 {
-  char *dbus_daemon_argv[4];
+  char *dbus_daemon_argv[5];
   int ret = 127;
   HANDLE server_handle = NULL;
   HANDLE app_handle = NULL;
+  HANDLE ready_event_handle = NULL;
   DWORD exit_code;
   DBusString argv_strings[4];
   DBusString address;
@@ -394,6 +397,7 @@ run_session (const char *dbus_daemon,
   dbus_bool_t result = TRUE;
   char *key = NULL;
   char *value = NULL;
+  DBusError error;
 
   if (!_dbus_string_init (&argv_strings[0]))
     result = FALSE;
@@ -401,11 +405,20 @@ run_session (const char *dbus_daemon,
     result = FALSE;
   if (!_dbus_string_init (&argv_strings[2]))
     result = FALSE;
+  if (!_dbus_string_init (&argv_strings[3]))
+    result = FALSE;
   if (!_dbus_string_init (&address))
     result = FALSE;
   if (!result)
     goto out;
 
+  /* The handle of this event is used by the dbus daemon
+   * to signal that connections are ready. */
+  dbus_error_init (&error);
+  ready_event_handle = _dbus_win_event_create_inheritable (&error);
+  if (ready_event_handle == NULL)
+    goto out;
+
   /* run dbus daemon */
   _dbus_get_real_time (&sec, &usec);
   /* On Windows it's difficult to make use of --print-address to
@@ -421,18 +434,29 @@ run_session (const char *dbus_daemon,
   else
     _dbus_string_append_printf (&argv_strings[1], "--session");
   _dbus_string_append_printf (&argv_strings[2], "--address=%s", _dbus_string_get_const_data (&address));
+  _dbus_string_append_printf (&argv_strings[3], "--ready-event-handle=%p", ready_event_handle);
   dbus_daemon_argv[0] = _dbus_string_get_data (&argv_strings[0]);
   dbus_daemon_argv[1] = _dbus_string_get_data (&argv_strings[1]);
   dbus_daemon_argv[2] = _dbus_string_get_data (&argv_strings[2]);
-  dbus_daemon_argv[3] = NULL;
+  dbus_daemon_argv[3] = _dbus_string_get_data (&argv_strings[3]);
+  dbus_daemon_argv[4] = NULL;
 
-  server_handle = _dbus_spawn_program (dbus_daemon, dbus_daemon_argv, NULL);
+  server_handle = _dbus_spawn_program (dbus_daemon, dbus_daemon_argv, NULL, TRUE);
   if (!server_handle)
     {
-      _dbus_win_stderr_win_error (me, "Could not start dbus daemon", GetLastError ());
+      _dbus_win_set_error_from_last_error (&error, "Could not start dbus daemon");
       goto out;
     }
 
+  /* wait until dbus-daemon is ready for connections */
+  if (ready_event_handle != NULL)
+    {
+      _dbus_verbose ("Wait until dbus-daemon is ready for connections (event handle %p)\n", ready_event_handle);
+      if (!_dbus_win_event_wait (ready_event_handle, 30000, &error))
+        goto out;
+      _dbus_verbose ("Got signal that dbus-daemon is ready for connections\n");
+    }
+
   /* run app */
   env = _dbus_get_environment ();
   env_table = _dbus_hash_table_new (DBUS_HASH_STRING,
@@ -474,7 +498,7 @@ run_session (const char *dbus_daemon,
   if (!env)
     goto out;
 
-  app_handle = _dbus_spawn_program (argv[prog_arg], argv + prog_arg, env);
+  app_handle = _dbus_spawn_program (argv[prog_arg], argv + prog_arg, env, FALSE);
   if (!app_handle)
     {
       _dbus_win_stderr_win_error (me, "unable to start child process", GetLastError ());
@@ -490,14 +514,20 @@ run_session (const char *dbus_daemon,
   ret = exit_code;
 
 out:
+  if (dbus_error_is_set (&error))
+    tool_stderr_error (me, &error);
+  dbus_error_free (&error);
   TerminateProcess (server_handle, 0);
   if (server_handle != NULL)
     CloseHandle (server_handle);
   if (app_handle != NULL)
     CloseHandle (app_handle);
+  if (ready_event_handle != NULL)
+    _dbus_win_event_free (ready_event_handle, NULL);
   _dbus_string_free (&argv_strings[0]);
   _dbus_string_free (&argv_strings[1]);
   _dbus_string_free (&argv_strings[2]);
+  _dbus_string_free (&argv_strings[3]);
   _dbus_string_free (&address);
   dbus_free_string_array (env);
   if (env_table != NULL)

tools/tool-common.c

@@ -80,3 +80,10 @@ tool_write_all (int fd,
 
   return TRUE;
 }
+
+void
+tool_stderr_error (const char    *context,
+                   DBusError     *error)
+{
+  fprintf (stderr, "%s: %s: %s\n", context, error->name, error->message);
+}

tools/tool-common.h

@@ -34,5 +34,6 @@
 
 void tool_oom (const char *doing) _DBUS_GNUC_NORETURN;
 dbus_bool_t tool_write_all (int fd, const void *buf, size_t size);
+void tool_stderr_error (const char *context, DBusError *error);
 
 #endif