mirror of
https://gitlab.gnome.org/GNOME/glib.git
synced 2025-02-21 09:42:10 +01:00
gio/subprocess: Lock while writing on shared pid and status values
The process PID is initialized by the initable vfunc, while
g_subprocess_exited sets it again, when we're protecting it via a lock.
The status is set when the process exits instead, again while locking.
This makes the thread sanitizer unhappy (even if it shouldn't really be
a race for the PID init case), but still locking during initialization is
not a bad thing to do.
At the same time g_subprocess_wait() and friends were using the pid and status
values without any protection, so let's ensure this is not the case anymore.
WARNING: ThreadSanitizer: data race (pid=8213)
Write of size 4 at 0x7b200000084c by thread T1:
#0 g_subprocess_exited ../gio/gsubprocess.c:284
#1 g_child_watch_dispatch ../glib/gmain.c:5963
#2 g_main_dispatch ../glib/gmain.c:3373
#3 g_main_context_dispatch_unlocked ../glib/gmain.c:4224
#4 g_main_context_iterate_unlocked ../glib/gmain.c:4289
#5 g_main_context_iteration ../glib/gmain.c:4354
#6 glib_worker_main ../glib/gmain.c:6553
#7 g_thread_proxy ../glib/gthread.c:892
Previous read of size 4 at 0x7b200000084c by main thread:
#0 g_subprocess_wait ../gio/gsubprocess.c:908
#1 g_subprocess_wait_check ../gio/gsubprocess.c:939
#2 end_element ../gio/glib-compile-resources.c:342
#3 emit_end_element ../glib/gmarkup.c:1045
#4 g_markup_parse_context_parse ../glib/gmarkup.c:1603
#5 parse_resource_file ../gio/glib-compile-resources.c:578
#6 main ../gio/glib-compile-resources.c:967
Location is heap block of size 120 at 0x7b2000000800 allocated by main
thread:
#0 calloc <null>
#1 g_malloc0 ../glib/gmem.c:133
#2 g_type_create_instance ../gobject/gtype.c:1933
#3 g_object_new_internal ../gobject/gobject.c:2621
#4 g_object_new_valist ../gobject/gobject.c:2960
#5 g_initable_new_valist ../gio/ginitable.c:245
#6 g_initable_new ../gio/ginitable.c:163
#7 g_subprocess_newv ../gio/gsubprocess.c:619
#8 g_subprocess_new ../gio/gsubprocess.c:590
#9 end_element ../gio/glib-compile-resources.c:334
#10 emit_end_element ../glib/gmarkup.c:1045
#11 g_markup_parse_context_parse ../glib/gmarkup.c:1603
#12 parse_resource_file ../gio/glib-compile-resources.c:578
#13 main ../gio/glib-compile-resources.c:967
Thread T1 'gmain' (tid=8228, running) created by main thread at:
#0 pthread_create <null>
#1 g_system_thread_new ../glib/gthread-posix.c:762
#2 g_thread_new_internal ../glib/gthread.c:996
#3 g_thread_new ../glib/gthread.c:949
#4 g_get_worker_context ../glib/gmain.c:6580
#5 initable_init ../gio/gsubprocess.c:443
#6 g_initable_init ../gio/ginitable.c:129
#7 g_initable_new_valist ../gio/ginitable.c:249
#8 g_initable_new ../gio/ginitable.c:163
#9 g_subprocess_newv ../gio/gsubprocess.c:619
#10 g_subprocess_new ../gio/gsubprocess.c:590
#11 end_element ../gio/glib-compile-resources.c:334
#12 emit_end_element ../glib/gmarkup.c:1045
#13 g_markup_parse_context_parse ../glib/gmarkup.c:1603
#14 parse_resource_file ../gio/glib-compile-resources.c:578
#15 main ../gio/glib-compile-resources.c:967
SUMMARY: ThreadSanitizer: data race ../gio/gsubprocess.c:284 in
g_subprocess_exited
======================================
WARNING: ThreadSanitizer: data race (pid=15959)
Read of size 4 at 0x7b200000084c by main thread:
#0 g_subprocess_wait ../gio/gsubprocess.c:913
#1 g_subprocess_wait_check ../gio/gsubprocess.c:944
#2 test_cat_utf8 ../gio/tests/gsubprocess.c:489
#3 test_case_run ../glib/gtestutils.c:3115
#4 g_test_run_suite_internal ../glib/gtestutils.c:3210
#5 g_test_run_suite_internal ../glib/gtestutils.c:3229
#6 g_test_run_suite ../glib/gtestutils.c:3310
#7 g_test_run ../glib/gtestutils.c:2379
#8 main ../gio/tests/gsubprocess.c:2266
Previous write of size 4 at 0x7b200000084c by thread T1:
#0 g_subprocess_exited ../gio/gsubprocess.c:284
#1 g_child_watch_dispatch ../glib/gmain.c:5963
#2 g_main_dispatch ../glib/gmain.c:3373
#3 g_main_context_dispatch_unlocked ../glib/gmain.c:4224
#4 g_main_context_iterate_unlocked ../glib/gmain.c:4289
#5 g_main_context_iteration ../glib/gmain.c:4354
#6 glib_worker_main ../glib/gmain.c:6553
#7 g_thread_proxy ../glib/gthread.c:892
This commit is contained in:
Marco Trevisan (Treviño)
parent
31a906a7a6
commit
5fd0825958
@ -275,9 +275,8 @@ g_subprocess_exited (GPid pid,
|
||||
GSubprocess *self = user_data;
|
||||
GSList *tasks;
|
||||
|
||||
g_assert (self->pid == pid);
|
||||
|
||||
g_mutex_lock (&self->pending_waits_lock);
|
||||
g_assert (self->pid == pid);
|
||||
self->status = status;
|
||||
tasks = self->pending_waits;
|
||||
self->pending_waits = NULL;
|
||||
@ -306,6 +305,7 @@ initable_init (GInitable *initable,
|
||||
gint *pipe_ptrs[3] = { NULL, NULL, NULL };
|
||||
gint pipe_fds[3] = { -1, -1, -1 };
|
||||
gint close_fds[3] = { -1, -1, -1 };
|
||||
GPid pid;
|
||||
#ifdef G_OS_UNIX
|
||||
gint stdin_fd = -1, stdout_fd = -1, stderr_fd = -1;
|
||||
#endif
|
||||
@ -415,19 +415,23 @@ initable_init (GInitable *initable,
|
||||
-1, -1, -1,
|
||||
NULL, NULL, 0,
|
||||
#endif
|
||||
&self->pid,
|
||||
&pid,
|
||||
pipe_ptrs[0], pipe_ptrs[1], pipe_ptrs[2],
|
||||
error);
|
||||
g_assert (success == (self->pid != 0));
|
||||
g_assert (success == (pid != 0));
|
||||
|
||||
g_mutex_lock (&self->pending_waits_lock);
|
||||
self->pid = pid;
|
||||
g_mutex_unlock (&self->pending_waits_lock);
|
||||
|
||||
{
|
||||
guint64 identifier;
|
||||
gint s G_GNUC_UNUSED /* when compiling with G_DISABLE_ASSERT */;
|
||||
|
||||
#ifdef G_OS_WIN32
|
||||
identifier = (guint64) GetProcessId (self->pid);
|
||||
identifier = (guint64) GetProcessId (pid);
|
||||
#else
|
||||
identifier = (guint64) self->pid;
|
||||
identifier = (guint64) pid;
|
||||
#endif
|
||||
|
||||
s = g_snprintf (self->identifier, sizeof self->identifier, "%"G_GUINT64_FORMAT, identifier);
|
||||
@ -441,7 +445,7 @@ initable_init (GInitable *initable,
|
||||
GSource *source;
|
||||
|
||||
worker_context = GLIB_PRIVATE_CALL (g_get_worker_context) ();
|
||||
source = g_child_watch_source_new (self->pid);
|
||||
source = g_child_watch_source_new (pid);
|
||||
g_source_set_callback (source, (GSourceFunc) g_subprocess_exited, g_object_ref (self), g_object_unref);
|
||||
g_source_attach (source, worker_context);
|
||||
g_source_unref (source);
|
||||
@ -637,12 +641,15 @@ g_subprocess_newv (const gchar * const *argv,
|
||||
const gchar *
|
||||
g_subprocess_get_identifier (GSubprocess *subprocess)
|
||||
{
|
||||
const char *identifier;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), NULL);
|
||||
|
||||
if (subprocess->pid)
|
||||
return subprocess->identifier;
|
||||
else
|
||||
return NULL;
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
identifier = subprocess->pid ? subprocess->identifier : NULL;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
return identifier;
|
||||
}
|
||||
|
||||
/**
|
||||
@ -905,7 +912,11 @@ g_subprocess_wait (GSubprocess *subprocess,
|
||||
/* We can shortcut in the case that the process already quit (but only
|
||||
* after we checked the cancellable).
|
||||
*/
|
||||
if (subprocess->pid == 0)
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
success = subprocess->pid == 0;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
if (success)
|
||||
return TRUE;
|
||||
|
||||
/* Otherwise, we need to do this the long way... */
|
||||
@ -936,8 +947,16 @@ g_subprocess_wait_check (GSubprocess *subprocess,
|
||||
GCancellable *cancellable,
|
||||
GError **error)
|
||||
{
|
||||
return g_subprocess_wait (subprocess, cancellable, error) &&
|
||||
g_spawn_check_wait_status (subprocess->status, error);
|
||||
gint status;
|
||||
|
||||
if (!g_subprocess_wait (subprocess, cancellable, error))
|
||||
return FALSE;
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
return g_spawn_check_wait_status (status, error);
|
||||
}
|
||||
|
||||
/**
|
||||
@ -980,8 +999,16 @@ g_subprocess_wait_check_finish (GSubprocess *subprocess,
|
||||
GAsyncResult *result,
|
||||
GError **error)
|
||||
{
|
||||
return g_subprocess_wait_finish (subprocess, result, error) &&
|
||||
g_spawn_check_wait_status (subprocess->status, error);
|
||||
gint status;
|
||||
|
||||
if (!g_subprocess_wait_finish (subprocess, result, error))
|
||||
return FALSE;
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
return g_spawn_check_wait_status (status, error);
|
||||
}
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
@ -999,8 +1026,10 @@ g_subprocess_actually_send_signal (gpointer user_data)
|
||||
/* The pid is set to zero from the worker thread as well, so we don't
|
||||
* need to take a lock in order to prevent it from changing under us.
|
||||
*/
|
||||
g_mutex_lock (&signal_record->subprocess->pending_waits_lock);
|
||||
if (signal_record->subprocess->pid)
|
||||
kill (signal_record->subprocess->pid, signal_record->signalnum);
|
||||
g_mutex_unlock (&signal_record->subprocess->pending_waits_lock);
|
||||
|
||||
g_object_unref (signal_record->subprocess);
|
||||
|
||||
@ -1082,7 +1111,9 @@ g_subprocess_force_exit (GSubprocess *subprocess)
|
||||
#ifdef G_OS_UNIX
|
||||
g_subprocess_dispatch_signal (subprocess, SIGKILL);
|
||||
#else
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
TerminateProcess (subprocess->pid, 1);
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
#endif
|
||||
}
|
||||
|
||||
@ -1109,10 +1140,19 @@ g_subprocess_force_exit (GSubprocess *subprocess)
|
||||
gint
|
||||
g_subprocess_get_status (GSubprocess *subprocess)
|
||||
{
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE);
|
||||
g_return_val_if_fail (subprocess->pid == 0, FALSE);
|
||||
gint status;
|
||||
GPid pid;
|
||||
|
||||
return subprocess->status;
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, FALSE);
|
||||
|
||||
return status;
|
||||
}
|
||||
|
||||
/**
|
||||
@ -1133,13 +1173,22 @@ g_subprocess_get_status (GSubprocess *subprocess)
|
||||
gboolean
|
||||
g_subprocess_get_successful (GSubprocess *subprocess)
|
||||
{
|
||||
GPid pid;
|
||||
gint status;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE);
|
||||
g_return_val_if_fail (subprocess->pid == 0, FALSE);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, FALSE);
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
return WIFEXITED (subprocess->status) && WEXITSTATUS (subprocess->status) == 0;
|
||||
return WIFEXITED (status) && WEXITSTATUS (status) == 0;
|
||||
#else
|
||||
return subprocess->status == 0;
|
||||
return status == 0;
|
||||
#endif
|
||||
}
|
||||
|
||||
@ -1162,11 +1211,20 @@ g_subprocess_get_successful (GSubprocess *subprocess)
|
||||
gboolean
|
||||
g_subprocess_get_if_exited (GSubprocess *subprocess)
|
||||
{
|
||||
GPid pid;
|
||||
gint status G_GNUC_UNUSED;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE);
|
||||
g_return_val_if_fail (subprocess->pid == 0, FALSE);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, FALSE);
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
return WIFEXITED (subprocess->status);
|
||||
return WIFEXITED (status);
|
||||
#else
|
||||
return TRUE;
|
||||
#endif
|
||||
@ -1192,15 +1250,24 @@ g_subprocess_get_if_exited (GSubprocess *subprocess)
|
||||
gint
|
||||
g_subprocess_get_exit_status (GSubprocess *subprocess)
|
||||
{
|
||||
gint status;
|
||||
GPid pid;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), 1);
|
||||
g_return_val_if_fail (subprocess->pid == 0, 1);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, 1);
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
g_return_val_if_fail (WIFEXITED (subprocess->status), 1);
|
||||
g_return_val_if_fail (WIFEXITED (status), 1);
|
||||
|
||||
return WEXITSTATUS (subprocess->status);
|
||||
return WEXITSTATUS (status);
|
||||
#else
|
||||
return subprocess->status;
|
||||
return status;
|
||||
#endif
|
||||
}
|
||||
|
||||
@ -1222,11 +1289,20 @@ g_subprocess_get_exit_status (GSubprocess *subprocess)
|
||||
gboolean
|
||||
g_subprocess_get_if_signaled (GSubprocess *subprocess)
|
||||
{
|
||||
GPid pid;
|
||||
gint status G_GNUC_UNUSED;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE);
|
||||
g_return_val_if_fail (subprocess->pid == 0, FALSE);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, FALSE);
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
return WIFSIGNALED (subprocess->status);
|
||||
return WIFSIGNALED (status);
|
||||
#else
|
||||
return FALSE;
|
||||
#endif
|
||||
@ -1251,13 +1327,22 @@ g_subprocess_get_if_signaled (GSubprocess *subprocess)
|
||||
gint
|
||||
g_subprocess_get_term_sig (GSubprocess *subprocess)
|
||||
{
|
||||
GPid pid;
|
||||
gint status G_GNUC_UNUSED;
|
||||
|
||||
g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), 0);
|
||||
g_return_val_if_fail (subprocess->pid == 0, 0);
|
||||
|
||||
g_mutex_lock (&subprocess->pending_waits_lock);
|
||||
pid = subprocess->pid;
|
||||
status = subprocess->status;
|
||||
g_mutex_unlock (&subprocess->pending_waits_lock);
|
||||
|
||||
g_return_val_if_fail (pid == 0, 0);
|
||||
|
||||
#ifdef G_OS_UNIX
|
||||
g_return_val_if_fail (WIFSIGNALED (subprocess->status), 0);
|
||||
g_return_val_if_fail (WIFSIGNALED (status), 0);
|
||||
|
||||
return WTERMSIG (subprocess->status);
|
||||
return WTERMSIG (status);
|
||||
#else
|
||||
g_critical ("g_subprocess_get_term_sig() called on Windows, where "
|
||||
"g_subprocess_get_if_signaled() always returns FALSE...");
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user