From 5b72329d7d5947e0613a69a4b2d69975bd4a5b46 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 20 Dec 2018 15:17:13 +0000 Subject: [PATCH 1/6] dbus-spawn-win: Move _dbus_spawn_program() to main thread We would like to stop taking ownership of envp, but we can't do that without a deep copy if a different thread might still be using it (asynchronously) after the main thread has returned from _dbus_spawn_async_with_babysitter(). Originally part of a larger commit by Ralf Habacker. Signed-off-by: Simon McVittie --- dbus/dbus-spawn-win.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 5ad561c2..c4b356c5 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -600,26 +600,7 @@ babysitter (void *parameter) { int ret = 0; DBusBabysitter *sitter = (DBusBabysitter *) parameter; - HANDLE handle; - PING(); - _dbus_verbose ("babysitter: spawning %s\n", sitter->log_name); - - PING(); - handle = _dbus_spawn_program (sitter->log_name, sitter->argv, sitter->envp); - - PING(); - if (handle != INVALID_HANDLE_VALUE) - { - sitter->child_handle = handle; - } - else - { - sitter->child_handle = NULL; - sitter->have_spawn_errno = TRUE; - sitter->spawn_errno = GetLastError(); - } - PING(); SetEvent (sitter->start_sync_event); @@ -663,7 +644,8 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, { DBusBabysitter *sitter; DWORD sitter_thread_id; - + HANDLE handle; + _DBUS_ASSERT_ERROR_IS_CLEAR (error); _dbus_assert (argv[0] != NULL); @@ -731,6 +713,24 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, } sitter->envp = envp; + PING(); + _dbus_verbose ("babysitter: spawn child '%s'\n", sitter->argv[0]); + + PING(); + handle = _dbus_spawn_program (sitter->log_name, sitter->argv, sitter->envp); + + PING(); + if (handle != INVALID_HANDLE_VALUE) + { + sitter->child_handle = handle; + } + else + { + sitter->child_handle = NULL; + sitter->have_spawn_errno = TRUE; + sitter->spawn_errno = GetLastError(); + } + PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); From 88032367a960c96989a964006d28aa2f6851e4a8 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 20 Dec 2018 15:27:03 +0000 Subject: [PATCH 2/6] dbus-spawn: Don't take ownership of envp It's unexpected for a function to take ownership of its arguments without indicating that in its name, or at least documenting it. The only caller with envp != NULL is in bus_activation_activate_service(), which has been updated. Based on part of a larger commit by Ralf Habacker. Signed-off-by: Simon McVittie --- bus/activation.c | 1 + dbus/dbus-spawn-unix.c | 4 +--- dbus/dbus-spawn-win.c | 18 +++--------------- dbus/dbus-spawn.h | 2 +- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 05dbfce4..30950636 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -2301,6 +2301,7 @@ bus_activation_activate_service (BusActivation *activation, } dbus_free_string_array (argv); + dbus_free_string_array (envp); envp = NULL; _dbus_assert (pending_activation->babysitter != NULL); diff --git a/dbus/dbus-spawn-unix.c b/dbus/dbus-spawn-unix.c index b6a4acd5..742c3a7a 100644 --- a/dbus/dbus-spawn-unix.c +++ b/dbus/dbus-spawn-unix.c @@ -1269,7 +1269,7 @@ dbus_bool_t _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, const char *log_name, char * const *argv, - char **env, + char * const *env, DBusSpawnFlags flags, DBusSpawnChildSetupFunc child_setup, void *user_data, @@ -1513,8 +1513,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, else _dbus_babysitter_unref (sitter); - dbus_free_string_array (env); - _DBUS_ASSERT_ERROR_IS_CLEAR (error); return TRUE; diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index c4b356c5..ee03102f 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -68,7 +68,6 @@ struct DBusBabysitter int argc; char **argv; - char **envp; HANDLE thread_handle; HANDLE child_handle; @@ -127,7 +126,6 @@ _dbus_babysitter_new (void) sitter->argc = 0; sitter->argv = NULL; - sitter->envp = NULL; sitter->watches = _dbus_watch_list_new (); if (sitter->watches == NULL) @@ -224,16 +222,6 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) sitter->argv = NULL; } - if (sitter->envp != NULL) - { - char **e = sitter->envp; - - while (*e) - dbus_free (*e++); - dbus_free (sitter->envp); - sitter->envp = NULL; - } - if (sitter->child_handle != NULL) { CloseHandle (sitter->child_handle); @@ -636,7 +624,7 @@ dbus_bool_t _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, const char *log_name, char * const *argv, - char **envp, + char * const *envp, DBusSpawnFlags flags _DBUS_GNUC_UNUSED, DBusSpawnChildSetupFunc child_setup _DBUS_GNUC_UNUSED, void *user_data _DBUS_GNUC_UNUSED, @@ -711,13 +699,13 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, _DBUS_SET_OOM (error); goto out0; } - sitter->envp = envp; PING(); _dbus_verbose ("babysitter: spawn child '%s'\n", sitter->argv[0]); PING(); - handle = _dbus_spawn_program (sitter->log_name, sitter->argv, sitter->envp); + handle = _dbus_spawn_program (sitter->log_name, sitter->argv, + (char **) envp); PING(); if (handle != INVALID_HANDLE_VALUE) diff --git a/dbus/dbus-spawn.h b/dbus/dbus-spawn.h index b36b1d86..03458d0a 100644 --- a/dbus/dbus-spawn.h +++ b/dbus/dbus-spawn.h @@ -47,7 +47,7 @@ typedef enum { dbus_bool_t _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, const char *log_name, char * const *argv, - char **env, + char * const *env, DBusSpawnFlags flags, DBusSpawnChildSetupFunc child_setup, void *user_data, From 61c76cae028686e558bca2059f11c8ae3129b871 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 16:38:02 +0100 Subject: [PATCH 3/6] dbus-spawn-win.c: Move out argv copy from DBusSitter struct Since the child program is started in the main thread, there is no need to pass a copy of argv to the thread waiting for the child's termination. Signed-off-by: Ralf Habacker --- dbus/dbus-spawn-win.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index ee03102f..9cfdf478 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -66,9 +66,6 @@ struct DBusBabysitter char *log_name; - int argc; - char **argv; - HANDLE thread_handle; HANDLE child_handle; DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ @@ -124,9 +121,6 @@ _dbus_babysitter_new (void) sitter->socket_to_babysitter = sitter->socket_to_main = _dbus_socket_get_invalid (); - sitter->argc = 0; - sitter->argv = NULL; - sitter->watches = _dbus_watch_list_new (); if (sitter->watches == NULL) { @@ -189,7 +183,6 @@ close_socket_to_babysitter (DBusBabysitter *sitter) void _dbus_babysitter_unref (DBusBabysitter *sitter) { - int i; dbus_int32_t old_refcount; PING(); @@ -209,19 +202,6 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) sitter->socket_to_main.sock = INVALID_SOCKET; } - PING(); - if (sitter->argv != NULL) - { - for (i = 0; i < sitter->argc; i++) - if (sitter->argv[i] != NULL) - { - dbus_free (sitter->argv[i]); - sitter->argv[i] = NULL; - } - dbus_free (sitter->argv); - sitter->argv = NULL; - } - if (sitter->child_handle != NULL) { CloseHandle (sitter->child_handle); @@ -633,6 +613,8 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, DBusBabysitter *sitter; DWORD sitter_thread_id; HANDLE handle; + int argc; + char **my_argv = NULL; _DBUS_ASSERT_ERROR_IS_CLEAR (error); _dbus_assert (argv[0] != NULL); @@ -693,19 +675,22 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, goto out0; } - sitter->argc = protect_argv (argv, &sitter->argv); - if (sitter->argc == -1) + argc = protect_argv (argv, &my_argv); + if (argc == -1) { _DBUS_SET_OOM (error); goto out0; } - PING(); - _dbus_verbose ("babysitter: spawn child '%s'\n", sitter->argv[0]); + _dbus_verbose ("babysitter: spawn child '%s'\n", my_argv[0]); PING(); - handle = _dbus_spawn_program (sitter->log_name, sitter->argv, - (char **) envp); + handle = _dbus_spawn_program (sitter->log_name, my_argv, (char **) envp); + + if (my_argv != NULL) + { + dbus_free_string_array (my_argv); + } PING(); if (handle != INVALID_HANDLE_VALUE) From bcd750fa2e9b1a23e5cadfe08a906a69d6f50dca Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 17:03:27 +0100 Subject: [PATCH 4/6] dbus-spawn-win.c: Don't wait for babysitter thread to start Now that we start the spawned program from the main thread, there is no need to wait for it before dereferencing `sitter->child_handle`. Signed-off-by: Ralf Habacker --- dbus/dbus-spawn-win.c | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 9cfdf478..1cbe84cb 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -61,9 +61,6 @@ struct DBusBabysitter { DBusAtomic refcount; - - HANDLE start_sync_event; - char *log_name; HANDLE thread_handle; @@ -110,13 +107,6 @@ _dbus_babysitter_new (void) _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__); - sitter->start_sync_event = CreateEvent (NULL, FALSE, FALSE, NULL); - if (sitter->start_sync_event == NULL) - { - _dbus_babysitter_unref (sitter); - return NULL; - } - sitter->child_handle = NULL; sitter->socket_to_babysitter = sitter->socket_to_main = _dbus_socket_get_invalid (); @@ -218,13 +208,6 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (sitter->watches) _dbus_watch_list_free (sitter->watches); - if (sitter->start_sync_event != NULL) - { - PING(); - CloseHandle (sitter->start_sync_event); - sitter->start_sync_event = NULL; - } - if (sitter->thread_handle) { CloseHandle (sitter->thread_handle); @@ -570,8 +553,6 @@ babysitter (void *parameter) DBusBabysitter *sitter = (DBusBabysitter *) parameter; PING(); - SetEvent (sitter->start_sync_event); - if (sitter->child_handle != NULL) { DWORD status; @@ -716,9 +697,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, goto out0; } - PING(); - WaitForSingleObject (sitter->start_sync_event, INFINITE); - PING(); if (sitter_p != NULL) *sitter_p = sitter; From 4cae5cb81cf7e6960aabdaa26afc61b4075f1a17 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 17:54:16 +0100 Subject: [PATCH 5/6] dbus-spawn-win.c: Return valid error if child could not be spawned Signed-off-by: Ralf Habacker --- dbus/dbus-spawn-win.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 1cbe84cb..87255964 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -683,6 +683,9 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, sitter->child_handle = NULL; sitter->have_spawn_errno = TRUE; sitter->spawn_errno = GetLastError(); + dbus_set_error_const (error, DBUS_ERROR_SPAWN_EXEC_FAILED, + "Failed to spawn child"); + goto out0; } PING(); From 0c0056c7d6ccded9962193298edfdcbba1dee579 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 17:55:29 +0100 Subject: [PATCH 6/6] dbus-spawn-win.c: Simplify logic of return value from call to _dbus_spawn_program() Signed-off-by: Ralf Habacker --- dbus/dbus-spawn-win.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 87255964..ca796055 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -674,11 +674,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, } PING(); - if (handle != INVALID_HANDLE_VALUE) - { - sitter->child_handle = handle; - } - else + if (handle == INVALID_HANDLE_VALUE) { sitter->child_handle = NULL; sitter->have_spawn_errno = TRUE; @@ -688,6 +684,8 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, goto out0; } + sitter->child_handle = handle; + PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, _dbus_babysitter_ref (sitter), 0, &sitter_thread_id);