From 6d88df8d824a0813093ec0e7ef586cd62920466a Mon Sep 17 00:00:00 2001 From: Ludovic Henry Date: Wed, 28 Jun 2017 18:42:35 -0400 Subject: [PATCH] [w32handle] Replace mono_w32handle_{ref,unref} by mono_w32handle_{duplicate, close} (#5106) * [w32handle] Fix handle dumped ref * [w32handle] Replace exposing mono_w32handle_{ref,unref} by mono_w32handle_{duplicate,close}, it is closer to what the win32 API exposes. * [w32handle] Remove unrefing of handles on shutdown If one of the handle is a file handle for example, then w32file has already be cleaned up, so we cannot guarantee that the handle we are going to close is not going to call into w32file code. --- mono/metadata/w32file-unix.c | 4 +- mono/metadata/w32file.c | 3 +- mono/metadata/w32handle-namespace.c | 4 +- mono/metadata/w32handle.c | 108 +++++++++++++++------------- mono/metadata/w32handle.h | 9 +-- mono/metadata/w32mutex-unix.c | 6 +- mono/metadata/w32process-unix.c | 10 ++- 7 files changed, 73 insertions(+), 71 deletions(-) diff --git a/mono/metadata/w32file-unix.c b/mono/metadata/w32file-unix.c index 64e46c2d588..d87335f2f0d 100644 --- a/mono/metadata/w32file-unix.c +++ b/mono/metadata/w32file-unix.c @@ -3624,7 +3624,7 @@ mono_w32file_find_close (gpointer handle) mono_w32handle_unlock_handle (handle); MONO_ENTER_GC_SAFE; - mono_w32handle_unref (handle); + mono_w32handle_close (handle); MONO_EXIT_GC_SAFE; return(TRUE); @@ -3995,7 +3995,7 @@ mono_w32file_create_pipe (gpointer *readpipe, gpointer *writepipe, guint32 size) g_warning ("%s: error creating pipe write handle", __func__); MONO_ENTER_GC_SAFE; - mono_w32handle_unref (read_handle); + mono_w32handle_close (read_handle); close (filedes[0]); close (filedes[1]); diff --git a/mono/metadata/w32file.c b/mono/metadata/w32file.c index 919965c1639..296a84d6a47 100644 --- a/mono/metadata/w32file.c +++ b/mono/metadata/w32file.c @@ -1073,8 +1073,7 @@ ves_icall_System_IO_MonoIO_DuplicateHandle (HANDLE source_process_handle, HANDLE ret=DuplicateHandle (source_process_handle, source_handle, target_process_handle, target_handle, access, inherit, options); MONO_EXIT_GC_SAFE; #else - mono_w32handle_ref (source_handle); - *target_handle = source_handle; + *target_handle = mono_w32handle_duplicate (source_handle); ret = TRUE; #endif diff --git a/mono/metadata/w32handle-namespace.c b/mono/metadata/w32handle-namespace.c index 6975c48934b..f45d2efae53 100644 --- a/mono/metadata/w32handle-namespace.c +++ b/mono/metadata/w32handle-namespace.c @@ -91,9 +91,7 @@ mono_w32handle_namespace_search_handle_callback (gpointer handle, gpointer data, __func__, handle); /* we do not want the handle to be destroyed before we return it */ - mono_w32handle_ref (handle); - - search_data->ret = handle; + search_data->ret = mono_w32handle_duplicate (handle); } return TRUE; diff --git a/mono/metadata/w32handle.c b/mono/metadata/w32handle.c index 0dcfb6f70b4..02aa4db8263 100644 --- a/mono/metadata/w32handle.c +++ b/mono/metadata/w32handle.c @@ -210,6 +210,12 @@ mono_w32handle_unlock_signal_mutex (void) mono_os_mutex_unlock (&global_signal_mutex); } +static void +mono_w32handle_ref (gpointer handle); + +static void +mono_w32handle_unref (gpointer handle); + void mono_w32handle_lock_handle (gpointer handle) { @@ -297,29 +303,11 @@ mono_w32handle_init (void) void mono_w32handle_cleanup (void) { - int i, j, k; + int i; g_assert (!shutting_down); shutting_down = TRUE; - /* Every shared handle we were using ought really to be closed - * by now, but to make sure just blow them all away. The - * exiting finalizer thread in particular races us to the - * program exit and doesn't always win, so it can be left - * cluttering up the shared file. Anything else left over is - * really a bug. - */ - for(i = SLOT_INDEX (0); private_handles[i] != NULL; i++) { - for(j = SLOT_OFFSET (0); j < HANDLE_PER_SLOT; j++) { - MonoW32HandleBase *handle_data = &private_handles[i][j]; - gpointer handle = GINT_TO_POINTER (i*HANDLE_PER_SLOT+j); - - for(k = handle_data->ref; k > 0; k--) { - mono_w32handle_unref (handle); - } - } - } - for (i = 0; i < SLOT_MAX; ++i) g_free (private_handles [i]); } @@ -490,12 +478,44 @@ gpointer mono_w32handle_new_fd (MonoW32HandleType type, int fd, return(GUINT_TO_POINTER(fd)); } +static gboolean +mono_w32handle_ref_core (gpointer handle, MonoW32HandleBase *handle_data); + +static gboolean +mono_w32handle_unref_core (gpointer handle, MonoW32HandleBase *handle_data); + +static void +w32handle_destroy (gpointer handle); + +gpointer +mono_w32handle_duplicate (gpointer handle) +{ + MonoW32HandleBase *handle_data; + + if (handle == INVALID_HANDLE_VALUE) + return handle; + if (!mono_w32handle_lookup_data (handle, &handle_data)) + return INVALID_HANDLE_VALUE; + if (handle == (gpointer) 0 && handle_data->type != MONO_W32HANDLE_CONSOLE) + return handle; + + if (!mono_w32handle_ref_core (handle, handle_data)) + g_error ("%s: failed to ref handle %p", __func__, handle); + + return handle; +} + gboolean mono_w32handle_close (gpointer handle) { + MonoW32HandleBase *handle_data; + gboolean destroy; + if (handle == INVALID_HANDLE_VALUE) return FALSE; - if (handle == (gpointer) 0 && mono_w32handle_get_type (handle) != MONO_W32HANDLE_CONSOLE) { + if (!mono_w32handle_lookup_data (handle, &handle_data)) + return FALSE; + if (handle == (gpointer) 0 && handle_data->type != MONO_W32HANDLE_CONSOLE) { /* Problem: because we map file descriptors to the * same-numbered handle we can't tell the difference * between a bogus handle and the handle to stdin. @@ -504,7 +524,10 @@ mono_w32handle_close (gpointer handle) return FALSE; } - mono_w32handle_unref (handle); + destroy = mono_w32handle_unref_core (handle, handle_data); + if (destroy) + w32handle_destroy (handle); + return TRUE; } @@ -529,15 +552,6 @@ mono_w32handle_lookup (gpointer handle, MonoW32HandleType type, return(TRUE); } -static gboolean -mono_w32handle_ref_core (gpointer handle, MonoW32HandleBase *handle_data); - -static gboolean -mono_w32handle_unref_core (gpointer handle, MonoW32HandleBase *handle_data); - -static void -w32handle_destroy (gpointer handle); - void mono_w32handle_foreach (gboolean (*on_each)(gpointer handle, gpointer data, gpointer user_data), gpointer user_data) { @@ -644,14 +658,13 @@ mono_w32handle_unref_core (gpointer handle, MonoW32HandleBase *handle_data) return new == 0; } -void mono_w32handle_ref (gpointer handle) +static void +mono_w32handle_ref (gpointer handle) { MonoW32HandleBase *handle_data; - if (!mono_w32handle_lookup_data (handle, &handle_data)) { - mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_W32HANDLE, "%s: failed to ref handle %p, unknown handle", __func__, handle); - return; - } + if (!mono_w32handle_lookup_data (handle, &handle_data)) + g_error ("%s: failed to ref handle %p, unknown handle", __func__, handle); if (!mono_w32handle_ref_core (handle, handle_data)) g_error ("%s: failed to ref handle %p", __func__, handle); @@ -703,17 +716,14 @@ w32handle_destroy (gpointer handle) } /* The handle must not be locked on entry to this function */ -void +static void mono_w32handle_unref (gpointer handle) { MonoW32HandleBase *handle_data; gboolean destroy; - if (!mono_w32handle_lookup_data (handle, &handle_data)) { - mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_W32HANDLE, "%s: failed to unref handle %p, unknown handle", - __func__, handle); - return; - } + if (!mono_w32handle_lookup_data (handle, &handle_data)) + g_error ("%s: failed to unref handle %p, unknown handle", __func__, handle); destroy = mono_w32handle_unref_core (handle, handle_data); if (destroy) @@ -1077,13 +1087,14 @@ signal_handle_and_unref (gpointer handle) mono_os_cond_broadcast (cond); mono_os_mutex_unlock (mutex); - mono_w32handle_unref (handle); + mono_w32handle_close (handle); } static int mono_w32handle_timedwait_signal_handle (gpointer handle, guint32 timeout, gboolean poll, gboolean *alerted) { MonoW32HandleBase *handle_data; + gpointer handle_duplicate; int res; if (!mono_w32handle_lookup_data (handle, &handle_data)) @@ -1096,10 +1107,11 @@ mono_w32handle_timedwait_signal_handle (gpointer handle, guint32 timeout, gboole *alerted = FALSE; if (alerted) { - mono_thread_info_install_interrupt (signal_handle_and_unref, handle, alerted); - if (*alerted) + mono_thread_info_install_interrupt (signal_handle_and_unref, handle_duplicate = mono_w32handle_duplicate (handle), alerted); + if (*alerted) { + mono_w32handle_close (handle_duplicate); return 0; - mono_w32handle_ref (handle); + } } res = mono_w32handle_timedwait_signal_naked (&handle_data->signal_cond, &handle_data->signal_mutex, timeout, poll, alerted); @@ -1107,8 +1119,8 @@ mono_w32handle_timedwait_signal_handle (gpointer handle, guint32 timeout, gboole if (alerted) { mono_thread_info_uninstall_interrupt (alerted); if (!*alerted) { - /* if it is alerted, then the handle is unref in the interrupt callback */ - mono_w32handle_unref (handle); + /* if it is alerted, then the handle_duplicate is closed in the interrupt callback */ + mono_w32handle_close (handle_duplicate); } } @@ -1124,7 +1136,7 @@ dump_callback (gpointer handle, gpointer handle_specific, gpointer user_data) g_error ("cannot dump unknown handle %p", handle); g_print ("%p [%7s] signalled: %5s ref: %3d ", - handle, mono_w32handle_ops_typename (handle_data->type), handle_data->signalled ? "true" : "false", handle_data->ref); + handle, mono_w32handle_ops_typename (handle_data->type), handle_data->signalled ? "true" : "false", handle_data->ref - 1 /* foreach increase ref by 1 */); mono_w32handle_ops_details (handle_data->type, handle_data->specific); g_print ("\n"); diff --git a/mono/metadata/w32handle.h b/mono/metadata/w32handle.h index 029f24dad82..b7df9a8810e 100644 --- a/mono/metadata/w32handle.h +++ b/mono/metadata/w32handle.h @@ -116,6 +116,9 @@ mono_w32handle_new (MonoW32HandleType type, gpointer handle_specific); gpointer mono_w32handle_new_fd (MonoW32HandleType type, int fd, gpointer handle_specific); +gpointer +mono_w32handle_duplicate (gpointer handle); + gboolean mono_w32handle_close (gpointer handle); @@ -134,12 +137,6 @@ mono_w32handle_foreach (gboolean (*on_each)(gpointer handle, gpointer data, gpoi void mono_w32handle_dump (void); -void -mono_w32handle_ref (gpointer handle); - -void -mono_w32handle_unref (gpointer handle); - void mono_w32handle_register_capabilities (MonoW32HandleType type, MonoW32HandleCapability caps); diff --git a/mono/metadata/w32mutex-unix.c b/mono/metadata/w32mutex-unix.c index 189b808ba22..ab2ef003ddf 100644 --- a/mono/metadata/w32mutex-unix.c +++ b/mono/metadata/w32mutex-unix.c @@ -38,8 +38,6 @@ mono_w32mutex_open (const gchar* utf8_name, gint32 right G_GNUC_UNUSED, gint32 * static void thread_own_mutex (MonoInternalThread *internal, gpointer handle) { - mono_w32handle_ref (handle); - /* if we are not on the current thread, there is a * race condition when allocating internal->owned_mutexes */ g_assert (mono_thread_internal_is_current (internal)); @@ -47,7 +45,7 @@ thread_own_mutex (MonoInternalThread *internal, gpointer handle) if (!internal->owned_mutexes) internal->owned_mutexes = g_ptr_array_new (); - g_ptr_array_add (internal->owned_mutexes, handle); + g_ptr_array_add (internal->owned_mutexes, mono_w32handle_duplicate (handle)); } static void @@ -61,7 +59,7 @@ thread_disown_mutex (MonoInternalThread *internal, gpointer handle) removed = g_ptr_array_remove (internal->owned_mutexes, handle); g_assert (removed); - mono_w32handle_unref (handle); + mono_w32handle_close (handle); } static void diff --git a/mono/metadata/w32process-unix.c b/mono/metadata/w32process-unix.c index 559939c9b61..972942af2b6 100644 --- a/mono/metadata/w32process-unix.c +++ b/mono/metadata/w32process-unix.c @@ -736,7 +736,7 @@ processes_cleanup (void) if (process->signalled && process->handle) { /* This process has exited and we need to remove the artifical ref * on the handle */ - mono_w32handle_unref (process->handle); + mono_w32handle_close (process->handle); process->handle = NULL; } } @@ -862,7 +862,7 @@ mono_w32process_init (void) process_set_name (&process_handle); current_process = mono_w32handle_new (MONO_W32HANDLE_PROCESS, &process_handle); - g_assert (current_process); + g_assert (current_process != INVALID_HANDLE_VALUE); mono_os_mutex_init (&processes_mutex); } @@ -965,8 +965,7 @@ get_process_foreach_callback (gpointer handle, gpointer handle_specific, gpointe if (mono_w32handle_issignalled (handle)) return FALSE; - mono_w32handle_ref (handle); - foreach_data->handle = handle; + foreach_data->handle = mono_w32handle_duplicate (handle); return TRUE; } @@ -2019,8 +2018,7 @@ process_create (const gunichar2 *appname, const gunichar2 *cmdline, /* Keep the process handle artificially alive until the process * exits so that the information in the handle isn't lost. */ - mono_w32handle_ref (handle); - process->handle = handle; + process->handle = mono_w32handle_duplicate (handle); mono_os_mutex_lock (&processes_mutex); process->next = processes; -- 2.25.1