From: Ralf Habacker Date: Thu, 11 Nov 2021 09:01:29 +0000 (+0100) Subject: tools/dbus-run-session: fix race between manual and automatically started dbus-daemon... X-Git-Tag: dbus-1.13.20~27^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=79df3d2811343eadcf2f95bacb372729fde1fddd;p=thirdparty%2Fdbus.git 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 --- diff --git a/bus/bus.c b/bus/bus.c index db20bbbc2..ea508d726 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 99625ca31..b44111809 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 84f601b30..5f756d5c9 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 42651e774..8d413fb58 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 ca7960559..07d03bace 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 e9932ff5c..cbacd60cb 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 a9c0b5d5e..80fe94531 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 43d4df439..5caf5de5f 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 1337ebcee..f8660c06b 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 d401e23eb..c8b2920e6 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 32020324a..4fcfcb9fe 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 e6397ffb0..12a3fd6c2 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