From 18cef3798053b69e0f418e756c3dda45aae3f67f Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 17:07:51 +0100 Subject: [PATCH 01/15] Protect access to DBusBabysitter structure members by using a mutex --- dbus/dbus-spawn-win.c | 77 ++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index ca7960559..e72d147db 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -64,6 +64,7 @@ struct DBusBabysitter char *log_name; HANDLE thread_handle; + HANDLE mutex; HANDLE child_handle; DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ DBusSocket socket_to_main; @@ -184,6 +185,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (old_refcount == 1) { + WaitForSingleObject (sitter->mutex, INFINITE); close_socket_to_babysitter (sitter); if (sitter->socket_to_main.sock != INVALID_SOCKET) @@ -216,6 +218,9 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) dbus_free (sitter->log_name); + ReleaseMutex (sitter->mutex); + CloseHandle (sitter->mutex); + dbus_free (sitter); } } @@ -224,11 +229,16 @@ void _dbus_babysitter_kill_child (DBusBabysitter *sitter) { PING(); + WaitForSingleObject (sitter->mutex, INFINITE); + if (sitter->child_handle == NULL) - return; /* child is already dead, or we're so hosed we'll never recover */ + goto out; PING(); TerminateProcess (sitter->child_handle, 12345); + +out: + ReleaseMutex (sitter->mutex); } /** @@ -239,8 +249,12 @@ _dbus_babysitter_kill_child (DBusBabysitter *sitter) dbus_bool_t _dbus_babysitter_get_child_exited (DBusBabysitter *sitter) { + dbus_bool_t state; PING(); - return (sitter->child_handle == NULL); + WaitForSingleObject (sitter->mutex, INFINITE); + state = (sitter->child_handle == NULL); + ReleaseMutex (sitter->mutex); + return state; } /** @@ -259,14 +273,19 @@ dbus_bool_t _dbus_babysitter_get_child_exit_status (DBusBabysitter *sitter, int *status) { + WaitForSingleObject (sitter->mutex, INFINITE); if (!_dbus_babysitter_get_child_exited (sitter)) _dbus_assert_not_reached ("Child has not exited"); if (!sitter->have_child_status || sitter->child_status == STILL_ACTIVE) - return FALSE; + { + ReleaseMutex (sitter->mutex); + return FALSE; + } *status = sitter->child_status; + ReleaseMutex (sitter->mutex); return TRUE; } @@ -288,6 +307,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, return; PING(); + WaitForSingleObject (sitter->mutex, INFINITE); if (sitter->have_spawn_errno) { char *emsg = _dbus_win_error_string (sitter->spawn_errno); @@ -311,6 +331,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, sitter->log_name); } PING(); + ReleaseMutex (sitter->mutex); } dbus_bool_t @@ -321,13 +342,17 @@ _dbus_babysitter_set_watch_functions (DBusBabysitter *sitter, void *data, DBusFreeFunction free_data_function) { + dbus_bool_t result; PING(); - return _dbus_watch_list_set_functions (sitter->watches, - add_function, - remove_function, - toggled_function, - data, - free_data_function); + WaitForSingleObject (sitter->mutex, INFINITE); + result = _dbus_watch_list_set_functions (sitter->watches, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + ReleaseMutex (sitter->mutex); + return result; } static dbus_bool_t @@ -347,9 +372,11 @@ handle_watch (DBusWatch *watch, * struct. */ + WaitForSingleObject (sitter->mutex, INFINITE); PING(); close_socket_to_babysitter (sitter); PING(); + ReleaseMutex (sitter->mutex); if (_dbus_babysitter_get_child_exited (sitter) && sitter->finished_cb != NULL) @@ -545,37 +572,35 @@ _dbus_spawn_program (const char *name, return pi.hProcess; } - +/* This function is started in a different thread and waits until spawned child exit. */ static DWORD __stdcall babysitter (void *parameter) { int ret = 0; DBusBabysitter *sitter = (DBusBabysitter *) parameter; + DWORD status; PING(); - if (sitter->child_handle != NULL) - { - DWORD status; + _dbus_verbose ("babysitter: waiting for spawn end of '%s'\n", sitter->log_name); - PING(); - // wait until process finished - WaitForSingleObject (sitter->child_handle, INFINITE); + // wait until process finished + WaitForSingleObject (sitter->child_handle, INFINITE); - PING(); - ret = GetExitCodeProcess (sitter->child_handle, &status); - if (ret) - { - sitter->child_status = status; - sitter->have_child_status = TRUE; - } + PING(); + ret = GetExitCodeProcess (sitter->child_handle, &status); - CloseHandle (sitter->child_handle); - sitter->child_handle = NULL; + WaitForSingleObject (sitter->mutex, INFINITE); + if (ret) + { + sitter->child_status = status; + sitter->have_child_status = TRUE; } - PING(); + CloseHandle (sitter->child_handle); + sitter->child_handle = NULL; send (sitter->socket_to_main.sock, " ", 1, 0); + ReleaseMutex (sitter->mutex); _dbus_babysitter_unref (sitter); return ret ? 0 : 1; -- GitLab From bea41fcec80d688355e9bd016dce549372624e37 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 17:09:29 +0100 Subject: [PATCH 02/15] Indentation fixup --- dbus/dbus-spawn-win.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index e72d147db..1d6abc5fe 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -713,7 +713,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, - _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); + _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); if (sitter->thread_handle == NULL) { -- GitLab From b8bb467def56bae5ee5317c5dba4a2b7ee5283bc Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 21 Dec 2018 19:33:23 +0100 Subject: [PATCH 03/15] babysitter(): Add more debug information on Windows --- dbus/dbus-spawn-win.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 1d6abc5fe..bf6983912 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -581,10 +581,11 @@ babysitter (void *parameter) DWORD status; PING(); - _dbus_verbose ("babysitter: waiting for spawn end of '%s'\n", sitter->log_name); + _dbus_verbose ("waiting for spawn end of '%s'\n", sitter->log_name); // wait until process finished WaitForSingleObject (sitter->child_handle, INFINITE); + _dbus_verbose ("got spawn end of '%s'\n", sitter->log_name); PING(); ret = GetExitCodeProcess (sitter->child_handle, &status); @@ -688,7 +689,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, goto out0; } - _dbus_verbose ("babysitter: spawn child '%s'\n", my_argv[0]); + _dbus_verbose ("spawn child '%s'\n", my_argv[0]); PING(); handle = _dbus_spawn_program (sitter->log_name, my_argv, (char **) envp); -- GitLab From 321749f89c26326bd724ae3e9fcd46b0b7e10762 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Mon, 7 Jan 2019 10:59:48 +0100 Subject: [PATCH 04/15] Add doc to DBusBabySitter struct members --- dbus/dbus-spawn-win.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index bf6983912..4d7f7aeaa 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -60,24 +60,31 @@ */ struct DBusBabysitter { + /* only used by the main thread */ DBusAtomic refcount; - char *log_name; - HANDLE thread_handle; - HANDLE mutex; - HANDLE child_handle; - DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ - DBusSocket socket_to_main; - DBusWatchList *watches; DBusWatch *sitter_watch; DBusBabysitterFinishedFunc finished_cb; void *finished_data; - dbus_bool_t have_spawn_errno; int spawn_errno; + + /* read-only while the babysitter thread is running */ + HANDLE mutex; + + /* all read or write accesses while the babysitter thread + * is running must be protected by the mutex */ + HANDLE child_handle; dbus_bool_t have_child_status; int child_status; + + /* pipe connection to the babysitter thread protected by the mutex */ + DBusSocket socket_to_babysitter; + DBusSocket socket_to_main; + + /* only used by the babysitter thread */ + char *log_name; }; static void -- GitLab From 9c497527cf6b85989c1b9774796add88cfc5ae10 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 21 Jan 2019 19:41:55 +0000 Subject: [PATCH 05/15] dbus-spawn: Add a function to cancel a babysitter At the moment this does exactly the same thing on Windows and on Unix, but a subsequent commit will do extra cleanup on Windows. Signed-off-by: Simon McVittie --- bus/activation.c | 7 +------ dbus/dbus-spawn-unix.c | 8 ++++++++ dbus/dbus-spawn-win.c | 8 ++++++++ dbus/dbus-spawn.h | 1 + 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 19ac869af..2fce9c060 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -194,12 +194,7 @@ bus_pending_activation_unref (BusPendingActivation *pending_activation) #ifdef ENABLE_TRADITIONAL_ACTIVATION if (pending_activation->babysitter) { - if (!_dbus_babysitter_set_watch_functions (pending_activation->babysitter, - NULL, NULL, NULL, - pending_activation->babysitter, - NULL)) - _dbus_assert_not_reached ("setting watch functions to NULL failed"); - + _dbus_babysitter_cancel (pending_activation->babysitter); _dbus_babysitter_unref (pending_activation->babysitter); } #endif diff --git a/dbus/dbus-spawn-unix.c b/dbus/dbus-spawn-unix.c index 5550ffac1..557a803d6 100644 --- a/dbus/dbus-spawn-unix.c +++ b/dbus/dbus-spawn-unix.c @@ -1554,3 +1554,11 @@ _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter) while (LIVE_CHILDREN (sitter)) babysitter_iteration (sitter, TRUE); } + +void +_dbus_babysitter_cancel (DBusBabysitter *sitter) +{ + if (!_dbus_babysitter_set_watch_functions (sitter, NULL, NULL, NULL, + sitter, NULL)) + _dbus_assert_not_reached ("setting watch functions to NULL failed"); +} diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 4d7f7aeaa..69a401b99 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -767,3 +767,11 @@ _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter) * its memory. */ WaitForSingleObject (sitter->thread_handle, INFINITE); } + +void +_dbus_babysitter_cancel (DBusBabysitter *sitter) +{ + if (!_dbus_babysitter_set_watch_functions (sitter, NULL, NULL, NULL, + sitter, NULL)) + _dbus_assert_not_reached ("setting watch functions to NULL failed"); +} diff --git a/dbus/dbus-spawn.h b/dbus/dbus-spawn.h index 03458d0a5..f4cbab81f 100644 --- a/dbus/dbus-spawn.h +++ b/dbus/dbus-spawn.h @@ -70,6 +70,7 @@ dbus_bool_t _dbus_babysitter_set_watch_functions (DBusBabysitter *si void *data, DBusFreeFunction free_data_function); void _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter); +void _dbus_babysitter_cancel (DBusBabysitter *sitter); DBUS_END_DECLS -- GitLab From b64241f9a9ca418e45c0cc8e34ea771e1789ec1c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 20 Dec 2018 15:36:11 +0000 Subject: [PATCH 06/15] dbus-spawn-win: Don't release babysitter reference until watch is removed If the thread that was running babysitter() owns the last reference to the DBusBabysitter, and the main thread loses the race with the babysitter, then handle_watch() could be called on a dangling pointer: main thread babysitter <---- send on socket_to_main _dbus_babysitter_unref refcount drops from 1 to 0 DBusBabysitter destroyed socket_to_main signals ready <---- handle_watch dereference sitter->mutex crash Even if that can't happen, it would be bad for the babysitter to be the one calling the last-unref (which can happen if the main thread wins the race), because the dbus-daemon's remove_function for the DBusWatchList is a thin wrapper around _dbus_loop_remove_watch(), which is explicitly not thread-safe: main thread babysitter <---- send on socket_to_main socket_to_main signals ready <---- handle_watch report process status to caller _dbus_babysitter_unref refcount drops from 2 to 1 _dbus_babysitter_unref refcount drops from 1 to 0 _dbus_watch_list_free remove_babysitter_watch _dbus_loop_remove_watch To avoid this, don't release the reference in babysitter(). Release it in the main thread instead, in response to the watch being freed, which happens either when handle_watch() handles the notification from the babysitter thread or after we cancel the babysitter thread. Either way, the babysitter thread either already exited, or will exit very soon. Signed-off-by: Simon McVittie --- dbus/dbus-spawn-win.c | 63 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 69a401b99..2ca70539c 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -72,6 +72,7 @@ struct DBusBabysitter /* read-only while the babysitter thread is running */ HANDLE mutex; + HANDLE cancel_babysitter_event; /* all read or write accesses while the babysitter thread * is running must be protected by the mutex */ @@ -119,6 +120,14 @@ _dbus_babysitter_new (void) sitter->socket_to_babysitter = sitter->socket_to_main = _dbus_socket_get_invalid (); + sitter->cancel_babysitter_event = CreateEvent (0, FALSE, FALSE, NULL); + + if (sitter->cancel_babysitter_event == NULL) + { + _dbus_babysitter_unref (sitter); + return NULL; + } + sitter->watches = _dbus_watch_list_new (); if (sitter->watches == NULL) { @@ -223,6 +232,12 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) sitter->thread_handle = NULL; } + if (sitter->cancel_babysitter_event != NULL) + { + CloseHandle (sitter->cancel_babysitter_event); + sitter->cancel_babysitter_event = NULL; + } + dbus_free (sitter->log_name); ReleaseMutex (sitter->mutex); @@ -369,6 +384,8 @@ handle_watch (DBusWatch *watch, { DBusBabysitter *sitter = data; + _dbus_babysitter_ref (sitter); + /* On Unix dbus-spawn uses a babysitter *process*, thus it has to * actually send the exit statuses, error codes and whatnot through * sockets and/or pipes. On Win32, the babysitter is jus a thread, @@ -381,6 +398,9 @@ handle_watch (DBusWatch *watch, WaitForSingleObject (sitter->mutex, INFINITE); PING(); + /* Note that this removes the watch, which releases the reference to + * sitter that the watch holds, allowing sitter to reach its + * last-unref. */ close_socket_to_babysitter (sitter); PING(); ReleaseMutex (sitter->mutex); @@ -392,6 +412,8 @@ handle_watch (DBusWatch *watch, sitter->finished_cb = NULL; } + _dbus_babysitter_unref (sitter); + return TRUE; } @@ -586,12 +608,23 @@ babysitter (void *parameter) int ret = 0; DBusBabysitter *sitter = (DBusBabysitter *) parameter; DWORD status; + HANDLE handles[2]; PING(); _dbus_verbose ("waiting for spawn end of '%s'\n", sitter->log_name); + handles[0] = sitter->child_handle; + handles[1] = sitter->cancel_babysitter_event; + // wait until process finished - WaitForSingleObject (sitter->child_handle, INFINITE); + ret = WaitForMultipleObjectsEx (2, handles, FALSE, INFINITE, FALSE); + + if (ret != WAIT_OBJECT_0) + { + /* cancelled or failed */ + return 1; + } + _dbus_verbose ("got spawn end of '%s'\n", sitter->log_name); PING(); @@ -609,7 +642,6 @@ babysitter (void *parameter) send (sitter->socket_to_main.sock, " ", 1, 0); ReleaseMutex (sitter->mutex); - _dbus_babysitter_unref (sitter); return ret ? 0 : 1; } @@ -666,12 +698,20 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, FALSE, error)) goto out0; + /* This reference to the DBusBabysitter is released when the watch is + * removed from the main loop: either in handle_watch(), or when the + * caller cancels the babysitter. */ sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter, DBUS_WATCH_READABLE, - TRUE, handle_watch, sitter, NULL); + TRUE, handle_watch, + _dbus_babysitter_ref (sitter), + (DBusFreeFunction) _dbus_babysitter_unref); PING(); if (sitter->sitter_watch == NULL) { + /* _dbus_watch_new() doesn't call its free-function in this case */ + _dbus_babysitter_unref (sitter); + _DBUS_SET_OOM (error); goto out0; } @@ -721,7 +761,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, - _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); + sitter, 0, &sitter_thread_id); if (sitter->thread_handle == NULL) { @@ -771,7 +811,22 @@ _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter) void _dbus_babysitter_cancel (DBusBabysitter *sitter) { + _dbus_babysitter_ref (sitter); + if (!_dbus_babysitter_set_watch_functions (sitter, NULL, NULL, NULL, sitter, NULL)) _dbus_assert_not_reached ("setting watch functions to NULL failed"); + + /* Take the lock, to make sure the babysitter either does its whole + * locked section, or none of it */ + WaitForSingleObject (sitter->mutex, INFINITE); + + /* Interrupt the thread's WaitForSingleObject on the child + * process, if running */ + SetEvent (sitter->cancel_babysitter_event); + + ReleaseMutex (sitter->mutex); + + close_socket_to_babysitter (sitter); + _dbus_babysitter_unref (sitter); } -- GitLab From 51c47be5021e93fedeccaa99a3d64361bb4ca3ae Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 21 Jan 2019 20:20:29 +0000 Subject: [PATCH 07/15] Revert "Avoid memory leaks on running check_shell_service_success_auto_start on Windows" Now that we are not relying on the other thread to unref the DBusBabysitter, we shouldn't need this. This reverts commit 02bb1c2cfadf5028e40d8fd8256f6c78bb74937d. Signed-off-by: Simon McVittie --- bus/dispatch.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/bus/dispatch.c b/bus/dispatch.c index c3019b1e8..ca162c949 100644 --- a/bus/dispatch.c +++ b/bus/dispatch.c @@ -4952,11 +4952,6 @@ bus_dispatch_test_conf (const DBusString *test_data_dir, _dbus_test_fatal ("shell success service auto start failed"); _dbus_test_ok ("%s:%s - check_shell_service_success_auto_start", _DBUS_FUNCTION_NAME, filename); -#ifdef DBUS_WIN - _dbus_verbose("TODO: Fix memory leaks after running check_shell_service_success_auto_start\n"); - _dbus_sleep_milliseconds (500); -#endif - _dbus_verbose ("Disconnecting foo, bar, and baz\n"); kill_client_connection_unchecked (foo); -- GitLab From ef1f80e791178fb76587cff5b1a70464aed21822 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 15 Feb 2019 20:20:41 +0100 Subject: [PATCH 08/15] Make sure that handle_watch unrefs the sitter --- dbus/dbus-spawn-win.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 2ca70539c..596ea6d61 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -384,8 +384,6 @@ handle_watch (DBusWatch *watch, { DBusBabysitter *sitter = data; - _dbus_babysitter_ref (sitter); - /* On Unix dbus-spawn uses a babysitter *process*, thus it has to * actually send the exit statuses, error codes and whatnot through * sockets and/or pipes. On Win32, the babysitter is jus a thread, -- GitLab From f1112c1597448705200928a4b1f47cb6d919a3d7 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 15 Feb 2019 20:36:29 +0100 Subject: [PATCH 09/15] _dbus_spawn_async_with_babysitter: Create watch only in case we really spawned a child --- dbus/dbus-spawn-win.c | 72 ++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 596ea6d61..4137012f2 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -696,37 +696,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, FALSE, error)) goto out0; - /* This reference to the DBusBabysitter is released when the watch is - * removed from the main loop: either in handle_watch(), or when the - * caller cancels the babysitter. */ - sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter, - DBUS_WATCH_READABLE, - TRUE, handle_watch, - _dbus_babysitter_ref (sitter), - (DBusFreeFunction) _dbus_babysitter_unref); - PING(); - if (sitter->sitter_watch == NULL) - { - /* _dbus_watch_new() doesn't call its free-function in this case */ - _dbus_babysitter_unref (sitter); - - _DBUS_SET_OOM (error); - goto out0; - } - - PING(); - if (!_dbus_watch_list_add_watch (sitter->watches, sitter->sitter_watch)) - { - /* we need to free it early so the destructor won't try to remove it - * without it having been added, which DBusLoop doesn't allow */ - _dbus_watch_invalidate (sitter->sitter_watch); - _dbus_watch_unref (sitter->sitter_watch); - sitter->sitter_watch = NULL; - - _DBUS_SET_OOM (error); - goto out0; - } - argc = protect_argv (argv, &my_argv); if (argc == -1) { @@ -757,6 +726,30 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, sitter->child_handle = handle; + /* This reference to the DBusBabysitter is released when the watch is + * removed from the main loop: either in handle_watch(), or when the + * caller cancels the babysitter. */ + sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter, + DBUS_WATCH_READABLE, + TRUE, handle_watch, + _dbus_babysitter_ref (sitter), + (DBusFreeFunction) _dbus_babysitter_unref); + PING(); + if (sitter->sitter_watch == NULL) + { + PING(); + _DBUS_SET_OOM (error); + goto out1; + } + + PING(); + if (!_dbus_watch_list_add_watch (sitter->watches, sitter->sitter_watch)) + { + PING(); + _DBUS_SET_OOM (error); + goto out2; + } + PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, sitter, 0, &sitter_thread_id); @@ -766,7 +759,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, PING(); dbus_set_error_const (error, DBUS_ERROR_SPAWN_FORK_FAILED, "Failed to create new thread"); - goto out0; + goto out2; } PING(); @@ -780,9 +773,24 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, PING(); return TRUE; +out2: + /* we need to free it early so the destructor won't try to remove it + * without it having been added, which DBusLoop doesn't allow */ + _dbus_watch_invalidate (sitter->sitter_watch); + +out1: + /* _dbus_watch_new() doesn't call its free-function in this case */ + _dbus_watch_unref (sitter->sitter_watch); + sitter->sitter_watch = NULL; + // child has been spawned, kill it + TerminateProcess (sitter->child_handle, 12345); + CloseHandle (sitter->child_handle); + sitter->child_handle = NULL; + out0: _dbus_babysitter_unref (sitter); + PING(); return FALSE; } -- GitLab From 4a5841e3d4acba4424e618ec1b231fd8ea6aa24f Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 16 Feb 2019 00:32:57 +0100 Subject: [PATCH 10/15] _dbus_babysitter_kill_child: Simplify code --- dbus/dbus-spawn-win.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 4137012f2..c6657d9c4 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -253,13 +253,10 @@ _dbus_babysitter_kill_child (DBusBabysitter *sitter) PING(); WaitForSingleObject (sitter->mutex, INFINITE); - if (sitter->child_handle == NULL) - goto out; - PING(); - TerminateProcess (sitter->child_handle, 12345); + if (sitter->child_handle != NULL) + TerminateProcess (sitter->child_handle, 12345); -out: ReleaseMutex (sitter->mutex); } -- GitLab From 5d562eda93798703b25ab1a40e44cd906bc4ff73 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 16 Feb 2019 00:33:44 +0100 Subject: [PATCH 11/15] _dbus_babysitter_cancel: Do really unreferencing sitter --- dbus/dbus-spawn-win.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index c6657d9c4..8c3b4348e 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -814,8 +814,6 @@ _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter) void _dbus_babysitter_cancel (DBusBabysitter *sitter) { - _dbus_babysitter_ref (sitter); - if (!_dbus_babysitter_set_watch_functions (sitter, NULL, NULL, NULL, sitter, NULL)) _dbus_assert_not_reached ("setting watch functions to NULL failed"); -- GitLab From 94281842f565c08d08433198a930a9578cafea08 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 16 Feb 2019 00:34:49 +0100 Subject: [PATCH 12/15] Add reference tracing for DBusBabySitter to verbose output --- dbus/dbus-spawn-win.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 8c3b4348e..8f6599653 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -98,7 +98,7 @@ _dbus_babysitter_trace_ref (DBusBabysitter *sitter, static int enabled = -1; _dbus_trace_ref ("DBusBabysitter", sitter, old_refcount, new_refcount, why, - "DBUS_BABYSITTER_TRACE", &enabled); + "DBUS_VERBOSE", &enabled); #endif } -- GitLab From fd6bdd2d52912b63eb451305c38a819b587fb924 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 16 Feb 2019 00:38:21 +0100 Subject: [PATCH 13/15] add xmi diagram with DBusBabySitter related sequence diagram --- doc/dbus-spawn-win.xmi | 224 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 doc/dbus-spawn-win.xmi diff --git a/doc/dbus-spawn-win.xmi b/doc/dbus-spawn-win.xmi new file mode 100644 index 000000000..9d06895dd --- /dev/null +++ b/doc/dbus-spawn-win.xmi @@ -0,0 +1,224 @@ + + + + + umbrello uml modeller http://umbrello.kde.org + 1.6.16 + UnicodeUTF8 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- GitLab From cc2415ed62e11d6063f5b3914aa3c66cb6552a16 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 18 Apr 2020 11:23:20 +0200 Subject: [PATCH 14/15] Add memory allocation tracer support based on stl The tracer hooks memory related functions and stores file and location of (re)allocations and freeing in an internal list. The tracer is enabled with the cmake variable DBUS_MEMORY_DEBUG, which is enabled on Windows by default. Conflicts: dbus/dbus-sysdeps-win.h --- CMakeLists.txt | 10 ++++- bus/config-loader-expat.c | 13 +++++- dbus/CMakeLists.txt | 2 +- dbus/dbus-init-win.cpp | 87 +++++++++++++++++++++++++++++++++++++++ dbus/dbus-internals.c | 8 ++++ dbus/dbus-internals.h | 6 +++ dbus/dbus-memory.c | 37 +++++++++++++++-- dbus/dbus-memory.h | 23 +++++++---- dbus/dbus-sysdeps-win.h | 10 +++++ 9 files changed, 179 insertions(+), 17 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d86b8ac46..067a3af42 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,7 +5,7 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/modules") # we do not need to have WIN32 defined set(CMAKE_LEGACY_CYGWIN_WIN32 0) -project(dbus) +project(dbus C CXX) # we need to be up to date cmake_minimum_required(VERSION 3.0.2 FATAL_ERROR) @@ -196,6 +196,14 @@ if(GLIB2_FOUND) option(DBUS_WITH_GLIB "build with glib" ON) endif() +if(WIN32) + set(DBUS_MEMORY_DEBUG 1) +endif() +if(DBUS_MEMORY_DEBUG) + add_definitions(-DDBUS_MEMORY_DEBUG) + set(DBUS_MEMORY_DEBUG_LIBRARY stdc++) +endif() + # analogous to AC_USE_SYSTEM_EXTENSIONS in configure.ac add_definitions(-D_GNU_SOURCE) diff --git a/bus/config-loader-expat.c b/bus/config-loader-expat.c index 6cbd37eb1..376719f56 100644 --- a/bus/config-loader-expat.c +++ b/bus/config-loader-expat.c @@ -157,6 +157,15 @@ expat_CharacterDataHandler (void *userData, } } +static void *my_dbus_malloc(size_t bytes) +{ + return dbus_malloc(bytes); +} + +static void *my_dbus_realloc(void *p, size_t bytes) +{ + return dbus_realloc(p, bytes); +} BusConfigParser* bus_config_load (const DBusString *file, @@ -192,8 +201,8 @@ bus_config_load (const DBusString *file, return NULL; } - memsuite.malloc_fcn = dbus_malloc; - memsuite.realloc_fcn = dbus_realloc; + memsuite.malloc_fcn = my_dbus_malloc; + memsuite.realloc_fcn = my_dbus_realloc; memsuite.free_fcn = dbus_free; expat = XML_ParserCreate_MM ("UTF-8", &memsuite, NULL); diff --git a/dbus/CMakeLists.txt b/dbus/CMakeLists.txt index a603a1aa1..d6b904f66 100644 --- a/dbus/CMakeLists.txt +++ b/dbus/CMakeLists.txt @@ -279,7 +279,7 @@ if(WIN32) if(WINCE) target_link_libraries(dbus-1 ws2) else(WINCE) - target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi dbghelp) + target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi dbghelp ${DBUS_MEMORY_DEBUG_LIBRARY}) endif() else(WIN32) if(DEFINED DBUS_LIBRARY_REVISION) diff --git a/dbus/dbus-init-win.cpp b/dbus/dbus-init-win.cpp index 687f248ca..66a64d406 100644 --- a/dbus/dbus-init-win.cpp +++ b/dbus/dbus-init-win.cpp @@ -26,8 +26,89 @@ extern "C" { #include "dbus-sysdeps-win.h" +#ifdef DBUS_MEMORY_DEBUG +void dbus_track_malloc(void *p, int size, const char *file, int line); +void dbus_track_free(void *p); +void dbus_track_calloc(void *p, int size, const char *file, int line); +void dbus_track_realloc(void *p, int size, const char *file, int line); +#endif } +#ifdef DBUS_MEMORY_DEBUG +#include +#include + +class Data { +public: + Data() : size(0), line(0) + {} + std::string caller; + int size; + std::string file; + int line; +}; + +typedef std::map Map; +static Map map; + +void dbus_track_malloc(void *p, int size, const char *file, int line) +{ + Data &d = map[p]; + d.caller = "malloc"; + d.size = size; + d.file = file; + d.line = line; +} + +void dbus_track_free(void *p) +{ + map[p].caller = "free"; +} + +void dbus_track_calloc(void *p, int size, const char *file, int line) +{ + Data &d = map[p]; + d.caller = "calloc"; + d.size = size; + d.file = file; + d.line = line; +} + +void dbus_track_realloc(void *p, int size, const char *file, int line) +{ + Data &d = map[p]; + d.caller = "realloc"; + d.size = size; + d.file = file; + d.line = line; +} + +void _dbus_track_clear() +{ + fprintf(stderr, "%d allocations cleared\n", map.size()); + map.clear(); +} + +void _dbus_track_dump() +{ + fprintf(stderr, "%d allocations found\n", map.size()); + for (Map::const_iterator i = map.begin(); i != map.end(); i++) + { + if (i->second.caller != "free") + { + fprintf(stderr, "[%s:%d] %s(%d) %p \"%s\"\n", + i->second.file.c_str(), + i->second.line, + i->second.caller.c_str(), + i->second.size, + i->first, + (char *)i->first + ); + } + } +} +#endif + class DBusInternalInit { public: @@ -39,6 +120,12 @@ class DBusInternalInit void must_not_be_omitted () { } +#ifdef DBUS_MEMORY_DEBUG + ~DBusInternalInit () + { + _dbus_track_dump (); + } +#endif }; static DBusInternalInit init; diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index ab498b159..e13fca24d 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -586,7 +586,11 @@ _dbus_trace_ref (const char *obj_name, * @returns newly-allocated copy. */ char* +#ifdef DBUS_MEMORY_DEBUG +__dbus_strdup (const char *str, char *file, int line) +#else _dbus_strdup (const char *str) +#endif { size_t len; char *copy; @@ -596,7 +600,11 @@ _dbus_strdup (const char *str) len = strlen (str); +#ifdef DBUS_MEMORY_DEBUG + copy = _dbus_malloc (len + 1, file, line); +#else copy = dbus_malloc (len + 1); +#endif if (copy == NULL) return NULL; diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index 73443e232..b855a89d9 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -279,9 +279,15 @@ _dbus_assert_error_xor_bool (const DBusError *error, #define _DBUS_ALIGN_ADDRESS(this, boundary) \ ((void*)_DBUS_ALIGN_VALUE(this, boundary)) +#ifdef DBUS_MEMORY_DEBUG +#define _dbus_strdup(s) __dbus_strdup(s, __FILE__, __LINE__) +DBUS_PRIVATE_EXPORT +char* __dbus_strdup (const char *str, char *file, int line); +#else DBUS_PRIVATE_EXPORT char* _dbus_strdup (const char *str); +#endif void* _dbus_memdup (const void *mem, size_t n_bytes); DBUS_PRIVATE_EXPORT diff --git a/dbus/dbus-memory.c b/dbus/dbus-memory.c index 96df2357f..1dc7a130f 100644 --- a/dbus/dbus-memory.c +++ b/dbus/dbus-memory.c @@ -30,6 +30,18 @@ #include #include +#ifdef DBUS_MEMORY_DEBUG +void dbus_track_malloc(void *p, int size, const char *file, int line); +void dbus_track_free(void *p); +void dbus_track_calloc(void *p, int size, const char *file, int line); +void dbus_track_realloc(void *p, int size, const char *file, int line); +#else +static inline void dbus_track_malloc(void *p, int size, const char *file, int line) {} +static inline void dbus_track_free(void *p) {} +static inline void dbus_track_calloc(void *p, int size, const char *file, int line) {} +static inline void dbus_track_realloc(void *p, int size, const char *file, int line) {} +#endif + /** * @defgroup DBusMemory Memory Allocation * @ingroup DBus @@ -461,7 +473,9 @@ set_guards (void *real_block, * @return allocated memory, or #NULL if the allocation fails. */ void* -dbus_malloc (size_t bytes) +_dbus_malloc (size_t bytes, + const char *file, + int line) { #ifdef DBUS_ENABLE_EMBEDDED_TESTS _dbus_initialize_malloc_debug (); @@ -486,6 +500,7 @@ dbus_malloc (size_t bytes) if (block) { _dbus_atomic_inc (&n_blocks_outstanding); + dbus_track_malloc(block, bytes + GUARD_EXTRA_SIZE, file, line); } else if (malloc_cannot_fail) { @@ -506,6 +521,7 @@ dbus_malloc (size_t bytes) if (mem) { _dbus_atomic_inc (&n_blocks_outstanding); + dbus_track_malloc (mem, bytes, file, line); } else if (malloc_cannot_fail) { @@ -531,7 +547,9 @@ dbus_malloc (size_t bytes) * @return allocated memory, or #NULL if the allocation fails. */ void* -dbus_malloc0 (size_t bytes) +_dbus_malloc0 (size_t bytes, + const char *file, + int line) { #ifdef DBUS_ENABLE_EMBEDDED_TESTS _dbus_initialize_malloc_debug (); @@ -558,6 +576,7 @@ dbus_malloc0 (size_t bytes) if (block) { _dbus_atomic_inc (&n_blocks_outstanding); + dbus_track_calloc (block, bytes + GUARD_EXTRA_SIZE, file, line); } else if (malloc_cannot_fail) { @@ -578,6 +597,7 @@ dbus_malloc0 (size_t bytes) if (mem) { _dbus_atomic_inc (&n_blocks_outstanding); + dbus_track_calloc (mem, bytes, file, line); } else if (malloc_cannot_fail) { @@ -601,8 +621,10 @@ dbus_malloc0 (size_t bytes) * @return allocated memory, or #NULL if the resize fails. */ void* -dbus_realloc (void *memory, - size_t bytes) +_dbus_realloc (void *memory, + size_t bytes, + const char *file, + int line) { #ifdef DBUS_ENABLE_EMBEDDED_TESTS _dbus_initialize_malloc_debug (); @@ -646,6 +668,8 @@ dbus_realloc (void *memory, return NULL; } + dbus_track_free (((unsigned char*)memory) - GUARD_START_OFFSET); + dbus_track_realloc (block, bytes + GUARD_EXTRA_SIZE, file, line); old_bytes = *(dbus_uint32_t*)block; if (bytes >= old_bytes) @@ -663,6 +687,7 @@ dbus_realloc (void *memory, if (block) { _dbus_atomic_inc (&n_blocks_outstanding); + dbus_track_malloc(block, bytes + GUARD_EXTRA_SIZE, file, line); } else if (malloc_cannot_fail) { @@ -690,6 +715,8 @@ dbus_realloc (void *memory, if (memory == NULL && mem != NULL) _dbus_atomic_inc (&n_blocks_outstanding); #endif + dbus_track_free (memory); + dbus_track_realloc (mem, bytes, file, line); return mem; } } @@ -719,6 +746,7 @@ dbus_free (void *memory) #endif free (((unsigned char*)memory) - GUARD_START_OFFSET); + dbus_track_free (memory); } return; @@ -739,6 +767,7 @@ dbus_free (void *memory) #endif free (memory); + dbus_track_free (memory); } } diff --git a/dbus/dbus-memory.h b/dbus/dbus-memory.h index 4dcf2f92f..7c330b3a7 100644 --- a/dbus/dbus-memory.h +++ b/dbus/dbus-memory.h @@ -39,23 +39,28 @@ DBUS_BEGIN_DECLS DBUS_EXPORT DBUS_MALLOC -DBUS_ALLOC_SIZE(1) -void* dbus_malloc (size_t bytes); +DBUS_ALLOC_SIZE(3) +void* _dbus_malloc (size_t bytes, const char *file, int line); DBUS_EXPORT DBUS_MALLOC -DBUS_ALLOC_SIZE(1) -void* dbus_malloc0 (size_t bytes); +DBUS_ALLOC_SIZE(3) +void* _dbus_malloc0 (size_t bytes, const char *file, int line); DBUS_EXPORT -DBUS_ALLOC_SIZE(2) -void* dbus_realloc (void *memory, - size_t bytes); +DBUS_MALLOC +DBUS_ALLOC_SIZE(4) +void* _dbus_realloc (void *memory, size_t bytes, const char *file, int line); + DBUS_EXPORT void dbus_free (void *memory); -#define dbus_new(type, count) ((type*)dbus_malloc (sizeof (type) * (count))) -#define dbus_new0(type, count) ((type*)dbus_malloc0 (sizeof (type) * (count))) +#define dbus_malloc(bytes) _dbus_malloc (bytes, __FILE__, __LINE__) +#define dbus_malloc0(bytes) _dbus_malloc0 (bytes, __FILE__, __LINE__) +#define dbus_realloc(p, size) _dbus_realloc (p, size, __FILE__, __LINE__) + +#define dbus_new(type, count) ((type*)_dbus_malloc (sizeof (type) * (count), __FILE__, __LINE__)) +#define dbus_new0(type, count) ((type*)_dbus_malloc0 (sizeof (type) * (count), __FILE__, __LINE__)) DBUS_EXPORT void dbus_free_string_array (char **str_array); diff --git a/dbus/dbus-sysdeps-win.h b/dbus/dbus-sysdeps-win.h index edce99a31..dd2609567 100644 --- a/dbus/dbus-sysdeps-win.h +++ b/dbus/dbus-sysdeps-win.h @@ -95,6 +95,16 @@ dbus_bool_t _dbus_getsid(char **sid, dbus_pid_t process_id); HANDLE _dbus_spawn_program (const char *name, char **argv, char **envp); +DBUS_PRIVATE_EXPORT +void _dbus_win_set_autolaunch_command_line_parameter (const char *path); + +#ifdef DBUS_MEMORY_DEBUG +DBUS_PRIVATE_EXPORT +void _dbus_track_clear (); +DBUS_PRIVATE_EXPORT +void _dbus_track_dump (); +#endif + #endif /** @} end of sysdeps-win.h */ -- GitLab From 86a8554ecc471e9e303da691e3840d41665334fe Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Sat, 18 Apr 2020 13:32:14 +0200 Subject: [PATCH 15/15] Add _dbus_track_enable_dump_on_exit () --- dbus/dbus-init-win.cpp | 49 +++++++++++++++++++++++------------------ dbus/dbus-sysdeps-win.h | 2 ++ 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/dbus/dbus-init-win.cpp b/dbus/dbus-init-win.cpp index 66a64d406..1a53d4e1f 100644 --- a/dbus/dbus-init-win.cpp +++ b/dbus/dbus-init-win.cpp @@ -38,6 +38,29 @@ void dbus_track_realloc(void *p, int size, const char *file, int line); #include #include +class DBusInternalInit + { +public: + dbus_bool_t _dump_on_exit{TRUE}; + DBusInternalInit () + { + _dbus_threads_windows_init_global (); + } + + void must_not_be_omitted () + { + } +#ifdef DBUS_MEMORY_DEBUG + ~DBusInternalInit () + { + if (_dump_on_exit != FALSE) + _dbus_track_dump (); + } +#endif + }; + +static DBusInternalInit init; + class Data { public: Data() : size(0), line(0) @@ -51,6 +74,11 @@ public: typedef std::map Map; static Map map; +void _dbus_track_enable_dump_on_exit (dbus_bool_t state) +{ + init._dump_on_exit = state; +} + void dbus_track_malloc(void *p, int size, const char *file, int line) { Data &d = map[p]; @@ -109,27 +137,6 @@ void _dbus_track_dump() } #endif -class DBusInternalInit - { - public: - DBusInternalInit () - { - _dbus_threads_windows_init_global (); - } - - void must_not_be_omitted () - { - } -#ifdef DBUS_MEMORY_DEBUG - ~DBusInternalInit () - { - _dbus_track_dump (); - } -#endif - }; - -static DBusInternalInit init; - extern "C" void _dbus_threads_windows_ensure_ctor_linked () { diff --git a/dbus/dbus-sysdeps-win.h b/dbus/dbus-sysdeps-win.h index dd2609567..396e6631e 100644 --- a/dbus/dbus-sysdeps-win.h +++ b/dbus/dbus-sysdeps-win.h @@ -100,6 +100,8 @@ void _dbus_win_set_autolaunch_command_line_parameter (const char *path); #ifdef DBUS_MEMORY_DEBUG DBUS_PRIVATE_EXPORT +void _dbus_track_enable_dump_on_exit (dbus_bool_t state); +DBUS_PRIVATE_EXPORT void _dbus_track_clear (); DBUS_PRIVATE_EXPORT void _dbus_track_dump (); -- GitLab