Attachment 123830 Details for Bug 95191 – Use mutex to protect access to DBusBabysitter struct members on Windows.

[patch] Use mutex to protect access to DBusBabysitter struct members on Windows.

0001-Use-mutex-to-protect-access-to-DBusBabysitter-struct-m.txt (text/plain), 10.47 KB, created by Ralf Habacker on 2016-05-17 14:39:24 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralf Habacker

Created: 2016-05-17 14:39:24 UTC

Size: 10.47 KB

patch

obsolete

>From 8d50f1f43df66b6f743972471e73c87f722eb875 Mon Sep 17 00:00:00 2001
>From: Ralf Habacker <ralf.habacker@freenet.de>
>Date: Tue, 17 May 2016 16:39:02 +0200
>Subject: [PATCH] Use mutex to protect access to DBusBabysitter struct members
> on Windows.
>
>We are using the dbus provided mutex wrapper instead of native funtions
>to avoid confusion of WaitForSingleObject() calls used for events too.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95191
>---
> dbus/dbus-spawn-win.c        | 84 ++++++++++++++++++++++++++++++++++----------
> dbus/dbus-threads-internal.h |  4 +++
> 2 files changed, 70 insertions(+), 18 deletions(-)
>
>diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c
>index f9a4207..03dc351 100644
>--- a/dbus/dbus-spawn-win.c
>+++ b/dbus/dbus-spawn-win.c
>@@ -61,6 +61,7 @@
> struct DBusBabysitter
>   {
>     DBusAtomic refcount;
>+    DBusCMutex *mutex;
> 
>     HANDLE start_sync_event;
> 
>@@ -110,6 +111,8 @@ _dbus_babysitter_new (void)
>   if (sitter == NULL)
>     return NULL;
> 
>+  _dbus_cmutex_new_at_location (&sitter->mutex);
>+
>   old_refcount = _dbus_atomic_inc (&sitter->refcount);
> 
>   _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__);
>@@ -203,6 +206,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
> 
>   if (old_refcount == 1)
>     {
>+      _dbus_cmutex_lock (sitter->mutex);
>       close_socket_to_babysitter (sitter);
> 
>       if (sitter->socket_to_main.sock != INVALID_SOCKET)
>@@ -264,7 +268,8 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
>         }
> 
>       dbus_free (sitter->log_name);
>-
>+      _dbus_cmutex_unlock (sitter->mutex);
>+      _dbus_cmutex_free_at_location (&sitter->mutex);
>       dbus_free (sitter);
>     }
> }
>@@ -272,12 +277,16 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
> void
> _dbus_babysitter_kill_child (DBusBabysitter *sitter)
> {
>+  HANDLE handle;
>   PING();
>-  if (sitter->child_handle == NULL)
>+  _dbus_cmutex_lock (sitter->mutex);
>+  handle = sitter->child_handle;
>+  _dbus_cmutex_unlock (sitter->mutex);
>+  if (handle == NULL)
>     return; /* child is already dead, or we're so hosed we'll never recover */
> 
>   PING();
>-  TerminateProcess (sitter->child_handle, 12345);
>+  TerminateProcess (handle, 12345);
> }
> 
> /**
>@@ -288,8 +297,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);
>+  _dbus_cmutex_lock (sitter->mutex);
>+  state = sitter->child_handle == NULL;
>+  _dbus_cmutex_unlock (sitter->mutex);
>+  return state;
> }
> 
> /**
>@@ -311,11 +324,16 @@ _dbus_babysitter_get_child_exit_status (DBusBabysitter *sitter,
>   if (!_dbus_babysitter_get_child_exited (sitter))
>     _dbus_assert_not_reached ("Child has not exited");
> 
>+  _dbus_cmutex_lock (sitter->mutex);
>   if (!sitter->have_child_status ||
>       sitter->child_status == STILL_ACTIVE)
>-    return FALSE;
>+    {
>+      _dbus_cmutex_unlock (sitter->mutex);
>+      return FALSE;
>+    }
> 
>   *status = sitter->child_status;
>+  _dbus_cmutex_unlock (sitter->mutex);
>   return TRUE;
> }
> 
>@@ -337,6 +355,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter,
>     return;
> 
>   PING();
>+  _dbus_cmutex_lock (sitter->mutex);
>   if (sitter->have_spawn_errno)
>     {
>       char *emsg = _dbus_win_error_string (sitter->spawn_errno);
>@@ -360,6 +379,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter,
>                       sitter->log_name);
>     }
>   PING();
>+  _dbus_cmutex_unlock (sitter->mutex);
> }
> 
> dbus_bool_t
>@@ -370,13 +390,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);
>+  _dbus_cmutex_lock (sitter->mutex);
>+  result = _dbus_watch_list_set_functions (sitter->watches,
>+                                           add_function,
>+                                           remove_function,
>+                                           toggled_function,
>+                                           data,
>+                                           free_data_function);
>+  _dbus_cmutex_unlock (sitter->mutex);
>+  return result;
> }
> 
> static dbus_bool_t
>@@ -396,6 +420,7 @@ handle_watch (DBusWatch       *watch,
>    * struct.
>    */
> 
>+  _dbus_cmutex_lock (sitter->mutex);
>   PING();
>   close_socket_to_babysitter (sitter);
>   PING();
>@@ -406,6 +431,7 @@ handle_watch (DBusWatch       *watch,
>       sitter->finished_cb (sitter, sitter->finished_data);
>       sitter->finished_cb = NULL;
>     }
>+  _dbus_cmutex_unlock (sitter->mutex);
> 
>   return TRUE;
> }
>@@ -591,6 +617,7 @@ babysitter (void *parameter)
>   DBusBabysitter *sitter = (DBusBabysitter *) parameter;
>   HANDLE handle;
> 
>+  _dbus_cmutex_lock (sitter->mutex);
>   PING();
>   _dbus_verbose ("babysitter: spawning %s\n", sitter->log_name);
> 
>@@ -611,17 +638,19 @@ babysitter (void *parameter)
>   
>   PING();
>   SetEvent (sitter->start_sync_event);
>+  _dbus_cmutex_unlock (sitter->mutex);
> 
>-  if (sitter->child_handle != NULL)
>+  if (handle != INVALID_HANDLE_VALUE)
>     {
>       DWORD status;
> 
>       PING();
>       // wait until process finished
>-      WaitForSingleObject (sitter->child_handle, INFINITE);
>+      WaitForSingleObject (handle, INFINITE);
> 
>       PING();
>-      ret = GetExitCodeProcess (sitter->child_handle, &status);
>+      ret = GetExitCodeProcess (handle, &status);
>+      _dbus_cmutex_lock (sitter->mutex);
>       if (ret)
>         {
>           sitter->child_status = status;
>@@ -630,10 +659,13 @@ babysitter (void *parameter)
> 
>       CloseHandle (sitter->child_handle);
>       sitter->child_handle = NULL;
>+      _dbus_cmutex_unlock (sitter->mutex);
>     }
> 
>   PING();
>+  _dbus_cmutex_lock (sitter->mutex);
>   send (sitter->socket_to_main.sock, " ", 1, 0);
>+  _dbus_cmutex_unlock (sitter->mutex);
> 
>   _dbus_babysitter_unref (sitter);
> 
>@@ -651,6 +683,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
> {
>   DBusBabysitter *sitter;
>   DWORD sitter_thread_id;
>+  HANDLE event;
>   
>   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>   _dbus_assert (argv[0] != NULL);
>@@ -666,10 +699,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>       return FALSE;
>     }
> 
>+  _dbus_cmutex_lock (sitter->mutex);
>   sitter->log_name = _dbus_strdup (log_name);
>   if (sitter->log_name == NULL && log_name != NULL)
>     {
>       _DBUS_SET_OOM (error);
>+      _dbus_cmutex_unlock (sitter->mutex);
>       goto out0;
>     }
> 
>@@ -679,6 +714,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>   if (sitter->log_name == NULL)
>     {
>       _DBUS_SET_OOM (error);
>+      _dbus_cmutex_unlock (sitter->mutex);
>       goto out0;
>     }
> 
>@@ -686,7 +722,10 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>   if (!_dbus_socketpair (&sitter->socket_to_babysitter,
>                          &sitter->socket_to_main,
>                          FALSE, error))
>-    goto out0;
>+    {
>+      _dbus_cmutex_unlock (sitter->mutex);
>+      goto out0;
>+    }
> 
>   sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter,
>                                           DBUS_WATCH_READABLE,
>@@ -695,6 +734,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>   if (sitter->sitter_watch == NULL)
>     {
>       _DBUS_SET_OOM (error);
>+      _dbus_cmutex_unlock (sitter->mutex);
>       goto out0;
>     }
> 
>@@ -708,6 +748,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>       sitter->sitter_watch = NULL;
> 
>       _DBUS_SET_OOM (error);
>+      _dbus_cmutex_unlock (sitter->mutex);
>       goto out0;
>     }
> 
>@@ -715,10 +756,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>   if (sitter->argc == -1)
>     {
>       _DBUS_SET_OOM (error);
>+      _dbus_cmutex_unlock (sitter->mutex);
>       goto out0;
>     }
>   sitter->envp = envp;
> 
>+  _dbus_cmutex_unlock (sitter->mutex);
>   PING();
>   sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter,
>                   _dbus_babysitter_ref (sitter), 0, &sitter_thread_id);
>@@ -732,7 +775,10 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>     }
> 
>   PING();
>-  WaitForSingleObject (sitter->start_sync_event, INFINITE);
>+  _dbus_cmutex_lock (sitter->mutex);
>+  event = sitter->start_sync_event;
>+  _dbus_cmutex_unlock (sitter->mutex);
>+  WaitForSingleObject (event, INFINITE);
> 
>   PING();
>   if (sitter_p != NULL)
>@@ -756,8 +802,11 @@ _dbus_babysitter_set_result_function  (DBusBabysitter             *sitter,
>                                        DBusBabysitterFinishedFunc  finished,
>                                        void                       *user_data)
> {
>+  PING();
>+  _dbus_cmutex_lock (sitter->mutex);
>   sitter->finished_cb = finished;
>   sitter->finished_data = user_data;
>+  _dbus_cmutex_unlock (sitter->mutex);
> }
> 
> #ifdef DBUS_ENABLE_EMBEDDED_TESTS
>@@ -786,11 +835,10 @@ get_test_exec (const char *exe,
>   return _dbus_string_get_data (scratch_space);
> }
> 
>-#define LIVE_CHILDREN(sitter) ((sitter)->child_handle != NULL)
>-
> static void
> _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter)
> {
>+    PING();
>   /* The thread terminates after the child does. We want to wait for the thread,
>    * not just the child, to avoid data races and ensure that it has freed all
>    * its memory. */
>diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h
>index 076acd2..29edad5 100644
>--- a/dbus/dbus-threads-internal.h
>+++ b/dbus/dbus-threads-internal.h
>@@ -55,9 +55,13 @@ void         _dbus_rmutex_unlock             (DBusRMutex       *mutex);
> void         _dbus_rmutex_new_at_location    (DBusRMutex      **location_p);
> void         _dbus_rmutex_free_at_location   (DBusRMutex      **location_p);
> 
>+DBUS_PRIVATE_EXPORT
> void         _dbus_cmutex_lock               (DBusCMutex       *mutex);
>+DBUS_PRIVATE_EXPORT
> void         _dbus_cmutex_unlock             (DBusCMutex       *mutex);
>+DBUS_PRIVATE_EXPORT
> void         _dbus_cmutex_new_at_location    (DBusCMutex      **location_p);
>+DBUS_PRIVATE_EXPORT
> void         _dbus_cmutex_free_at_location   (DBusCMutex      **location_p);
> 
> DBusCondVar* _dbus_condvar_new               (void);
>-- 
>1.8.4.5
>

Actions: View | Splinter Review

Attachments on bug 95191: 123326 | 123585 | 123606 | 123607 | 123615 | 123616 | 123655 | 123656 | 123657 | 123660 | 123661 | 123662 | 123682 | 123695 | 123738 | 123739 | 123740 | 123741 | 123742 | 123754 | 123779 | 123782 | 123783 | 123830 | 123913 | 123936 | 125072 | 125075

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.