From 2dc3a6f0c80e5a8f786369eee0c45bfe19b55f4f Mon Sep 17 00:00:00 2001 From: Marco Trevisan Date: Thu, 13 Feb 2025 22:17:48 +0000 Subject: [PATCH] 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 #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 #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 --- gio/gsubprocess.c | 151 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 118 insertions(+), 33 deletions(-) diff --git a/gio/gsubprocess.c b/gio/gsubprocess.c index 204b0a606..1972fd2c0 100644 --- a/gio/gsubprocess.c +++ b/gio/gsubprocess.c @@ -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_unlock (&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...");