From 79df3d2811343eadcf2f95bacb372729fde1fddd Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Thu, 11 Nov 2021 10:01:29 +0100 Subject: [PATCH] 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 | 13 ++++++++++++ bus/bus.h | 1 + bus/main.c | 26 +++++++++++++++++++++++- bus/test.c | 2 +- dbus/dbus-spawn-win.c | 12 ++++++++--- dbus/dbus-sysdeps-win.h | 5 ++++- doc/dbus-daemon.1.xml.in | 24 ++++++++++++++++++++++ tools/CMakeLists.txt | 2 ++ tools/Makefile.am | 5 ++++- tools/dbus-run-session.c | 44 +++++++++++++++++++++++++++++++++------- tools/tool-common.c | 7 +++++++ tools/tool-common.h | 1 + 12 files changed, 128 insertions(+), 14 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index db20bbbc..ea508d72 100644 --- a/bus/bus.c +++ b/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) { diff --git a/bus/bus.h b/bus/bus.h index 99625ca3..b4411180 100644 --- a/bus/bus.h +++ b/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, diff --git a/bus/main.c b/bus/main.c index 84f601b3..5f756d5c 100644 --- a/bus/main.c +++ b/bus/main.c @@ -48,6 +48,10 @@ static BusContext *context; +#ifdef DBUS_WIN +#include +#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); diff --git a/bus/test.c b/bus/test.c index 42651e77..8d413fb5 100644 --- a/bus/test.c +++ b/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); diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index ca796055..07d03bac 100644 --- a/dbus/dbus-spawn-win.c +++ b/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) { diff --git a/dbus/dbus-sysdeps-win.h b/dbus/dbus-sysdeps-win.h index e9932ff5..cbacd60c 100644 --- a/dbus/dbus-sysdeps-win.h +++ b/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, diff --git a/doc/dbus-daemon.1.xml.in b/doc/dbus-daemon.1.xml.in index a9c0b5d5..80fe9453 100644 --- a/doc/dbus-daemon.1.xml.in +++ b/doc/dbus-daemon.1.xml.in @@ -32,6 +32,7 @@ --nosyslog --syslog --syslog-only + --ready-event-handle=value @@ -199,6 +200,29 @@ files. + + + + With this option, the dbus daemon raises an event when it is ready to process + connections. The handle must be the Windows handle + for an event object, in the format printed by the printf + format string %p. The parent process must create this event + object (for example with the CreateEvent 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 SetEvent + when it is ready to receive connections from clients. The parent process can + wait for this to occur by using functions such as + WaitForSingleObject. + 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 and/or . + + + + + + diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index 43d4df43..5caf5de5 100644 --- a/tools/CMakeLists.txt +++ b/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 diff --git a/tools/Makefile.am b/tools/Makefile.am index 1337ebce..f8660c06 100644 --- a/tools/Makefile.am +++ b/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) \ diff --git a/tools/dbus-run-session.c b/tools/dbus-run-session.c index d401e23e..c8b2920e 100644 --- a/tools/dbus-run-session.c +++ b/tools/dbus-run-session.c @@ -4,7 +4,7 @@ * Copyright © 2003-2006 Red Hat, Inc. * Copyright © 2006 Thiago Macieira * 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) diff --git a/tools/tool-common.c b/tools/tool-common.c index 32020324..4fcfcb9f 100644 --- a/tools/tool-common.c +++ b/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); +} diff --git a/tools/tool-common.h b/tools/tool-common.h index e6397ffb..12a3fd6c 100644 --- a/tools/tool-common.h +++ b/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