Merge branch 'dbus-run-session-add-delay' into 'master'

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

Closes #297

See merge request dbus/dbus!195

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-util-win.c

@@ -1674,16 +1674,3 @@ void
 _dbus_daemon_report_stopping (void)
 {
 }
-
-void
-_dbus_win_stderr_win_error (const char    *app,
-                            const char    *message,
-                            unsigned long  code)
-{
-  DBusError error;
-
-  dbus_error_init (&error);
-  _dbus_win_set_error_from_win_error (&error, code);
-  fprintf (stderr, "%s: %s: %s\n", app, message, error.message);
-  dbus_error_free (&error);
-}

dbus/dbus-sysdeps-win.c

@@ -6,7 +6,7 @@
  * Copyright (C) 2005 Novell, Inc.
  * Copyright (C) 2006 Peter Kümmel  <syntheticpp@gmx.net>
  * Copyright (C) 2006 Christian Ehrlicher <ch.ehrlicher@gmx.de>
- * Copyright (C) 2006-2013 Ralf Habacker <ralf.habacker@freenet.de>
+ * Copyright (C) 2006-2021 Ralf Habacker <ralf.habacker@freenet.de>
  *
  * Licensed under the Academic Free License version 2.1
  *
@@ -4009,5 +4009,174 @@ _dbus_get_low_level_socket_errno (void)
   return WSAGetLastError ();
 }
 
+void
+_dbus_win_set_error_from_last_error (DBusError *error,
+                                     const char *format,
+                                     ...)
+{
+  const char *name;
+  char *message = NULL;
+
+  if (error == NULL)
+    return;
+
+  /* make sure to do this first, in case subsequent library calls overwrite GetLastError() */
+  name = _dbus_win_error_from_last_error ();
+  message = _dbus_win_error_string (GetLastError ());
+
+  if (format != NULL)
+    {
+      DBusString str;
+      va_list args;
+      dbus_bool_t retval;
+
+      if (!_dbus_string_init (&str))
+        goto out;
+
+      va_start (args, format);
+      retval = _dbus_string_append_printf_valist (&str, format, args);
+      va_end (args);
+      if (!retval)
+        {
+          _dbus_string_free (&str);
+          goto out;
+        }
+
+      dbus_set_error (error, name, "%s: %s", _dbus_string_get_const_data (&str), message);
+      _dbus_string_free (&str);
+    }
+  else
+    {
+      dbus_set_error (error, name, "%s", message);
+    }
+
+out:
+  if (message != NULL)
+    _dbus_win_free_error_string (message);
+}
+
+/**
+ * Creates a Windows event object and returns the corresponding handle
+ *
+ * The returned object is unnamed, is a manual-reset event object,
+ * is initially in the non-signalled state, and is inheritable by child
+ * processes.
+ *
+ * @param error the error to set
+ * @return handle for the created event
+ * @return #NULL if an error has occurred, the reason is returned in \p error
+ */
+HANDLE
+_dbus_win_event_create_inheritable (DBusError *error)
+{
+  HANDLE handle;
+
+  handle = CreateEvent (NULL, TRUE, FALSE, NULL);
+  if (handle == NULL)
+    {
+      _dbus_win_set_error_from_last_error (error, "Could not create event");
+      return NULL;
+    }
+  else if (GetLastError () == ERROR_ALREADY_EXISTS)
+   {
+      _dbus_win_set_error_from_last_error (error, "Event already exists");
+      return NULL;
+   }
+
+  if (!SetHandleInformation (handle, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT))
+    {
+      _dbus_win_set_error_from_last_error (error, "Could not set inheritance for event %s", handle);
+      CloseHandle (handle);
+      return NULL;
+    }
+  return handle;
+}
+
+/**
+ * Set a Windows event to the signalled state
+ *
+ * @param handle the handle for the event to be set
+ * @return TRUE the event was set successfully
+ * @return FALSE an error has occurred, the reason is returned in \p error
+ */
+dbus_bool_t
+_dbus_win_event_set (HANDLE handle, DBusError *error)
+{
+  _dbus_assert (handle != NULL);
+
+  if (!SetEvent (handle))
+    {
+      _dbus_win_set_error_from_last_error (error, "Could not trigger event (handle %p)", handle);
+      return FALSE;
+    }
+  return TRUE;
+}
+
+/**
+ * Wait for a Windows event to enter the signalled state
+ *
+ * @param handle the handle for the event to wait for
+ * @param timeout the waiting time in milliseconds, or INFINITE to wait forever,
+ *                or 0 to check immediately and not wait (polling)
+ * @param error the error to set
+ * @return TRUE the event was set successfully
+ * @return FALSE an error has occurred, the reason is returned in \p error
+ */
+dbus_bool_t
+_dbus_win_event_wait (HANDLE handle, int timeout, DBusError *error)
+{
+  DWORD status;
+
+  _dbus_assert (handle != NULL);
+
+  status = WaitForSingleObject (handle, timeout);
+  switch (status)
+    {
+      case WAIT_OBJECT_0:
+        return TRUE;
+
+      case WAIT_FAILED:
+        {
+          _dbus_win_set_error_from_last_error (error, "Unable to wait for event (handle %p)", handle);
+          return FALSE;
+        }
+
+      case WAIT_TIMEOUT:
+        /* GetLastError() is not set */
+        dbus_set_error (error, DBUS_ERROR_TIMEOUT, "Timed out waiting for event (handle %p)", handle);
+        return FALSE;
+
+      default:
+        /* GetLastError() is probably not set? */
+        dbus_set_error (error, DBUS_ERROR_FAILED, "Unknown result '%lu' while waiting for event (handle %p)", status, handle);
+        return FALSE;
+    }
+}
+
+/**
+ * Delete a Windows event
+ *
+ * @param handle handle for the event to delete
+ * @param error the error to set (optional)
+ * @return TRUE the event has been deleted successfully or the handle specifies a #NULL or invalid handle
+ * @return FALSE an error has occurred, the reason is returned in \p error if specified
+ */
+dbus_bool_t
+_dbus_win_event_free (HANDLE handle, DBusError *error)
+{
+  if (handle == NULL || handle == INVALID_HANDLE_VALUE)
+    return TRUE;
+
+  if (CloseHandle (handle))
+    return TRUE;
+
+  /* the handle may already be closed */
+  if (GetLastError () == ERROR_INVALID_HANDLE)
+    return TRUE;
+
+  _dbus_win_set_error_from_last_error (error, "Could not close event (handle %p)", handle);
+  return FALSE;
+}
+
 /** @} end of sysdeps-win */
 /* tests in dbus-sysdeps-util.c */

dbus/dbus-sysdeps-win.h

@@ -46,9 +46,6 @@ const char* _dbus_win_error_from_last_error (void);
 dbus_bool_t _dbus_win_startup_winsock (void);
 void _dbus_win_warn_win_error  (const char *message,
                                 unsigned long code);
-void _dbus_win_stderr_win_error (const char *app,
-                                 const char *message,
-                                  unsigned long code);
 DBUS_PRIVATE_EXPORT
 char * _dbus_win_error_string (int error_number);
 DBUS_PRIVATE_EXPORT
@@ -93,7 +90,24 @@ 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,
+                                          const char *format,
+                                          ...);
+
+DBUS_PRIVATE_EXPORT
+HANDLE _dbus_win_event_create_inheritable (DBusError *error);
+DBUS_PRIVATE_EXPORT
+dbus_bool_t _dbus_win_event_set (HANDLE handle, DBusError *error);
+DBUS_PRIVATE_EXPORT
+dbus_bool_t _dbus_win_event_wait (HANDLE handle, int timeout, DBusError *error);
+DBUS_PRIVATE_EXPORT
+dbus_bool_t _dbus_win_event_free (HANDLE handle, DBusError *error);
 
 #endif
 

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,30 +498,36 @@ 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 ());
+      _dbus_win_set_error_from_last_error (&error, "Unable to start child process");
       goto out;
     }
 
   WaitForSingleObject (app_handle, INFINITE);
   if (!GetExitCodeProcess (app_handle, &exit_code))
     {
-      _dbus_win_stderr_win_error (me, "could not fetch exit code", GetLastError ());
+      _dbus_win_set_error_from_last_error (&error, "Could not fetch exit code");
       goto out;
     }
   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