From 4788cbc7860434f656a2ef83d3ff985369ba78f5 Mon Sep 17 00:00:00 2001 From: Ryan Lucia Date: Fri, 31 May 2019 14:39:22 -0400 Subject: [PATCH] [coop] Add GC safe transitions around blocking OS API calls (mono/mono#14612) If this was not possible due to them being called during runtime initialization, a short comment was left noting the specific reason why. Fixes mono/mono#14377 This includes a couple other random fixes I noticed on the way, most notably replacing SEE_MASK_FLAG_DDEWAIT with SEE_MASK_FLAG_NOASYNC as the former is deprecated. Commit migrated from https://github.com/mono/mono/commit/5d088cf0de7f3e50e3547dba361af4401e938dd4 --- src/mono/mono/metadata/coree.c | 1 + src/mono/mono/metadata/file-mmap-posix.c | 22 ++++++++++- src/mono/mono/metadata/file-mmap-windows.c | 58 +++++++++++++++++++++++++---- src/mono/mono/metadata/w32process-unix.c | 8 +++- src/mono/mono/metadata/w32process-win32.c | 22 +++++++++-- src/mono/mono/metadata/w32semaphore-unix.c | 3 ++ src/mono/mono/metadata/w32semaphore-win32.c | 10 ++++- src/mono/mono/utils/mono-dl.c | 1 + src/mono/mono/utils/mono-mmap.c | 3 ++ 9 files changed, 112 insertions(+), 16 deletions(-) diff --git a/src/mono/mono/metadata/coree.c b/src/mono/mono/metadata/coree.c index 861c01d..0f9a8ad 100644 --- a/src/mono/mono/metadata/coree.c +++ b/src/mono/mono/metadata/coree.c @@ -928,6 +928,7 @@ mono_load_coree (const char* exe_file_name) if (coree_module_handle) return; + // No GC safe transition because this is called early in driver.c if (!init_from_coree && exe_file_name) mono_coree_set_act_ctx (exe_file_name); diff --git a/src/mono/mono/metadata/file-mmap-posix.c b/src/mono/mono/metadata/file-mmap-posix.c index ac701c3..31adaa5 100644 --- a/src/mono/mono/metadata/file-mmap-posix.c +++ b/src/mono/mono/metadata/file-mmap-posix.c @@ -192,7 +192,7 @@ access_mode_to_unix (int access) } static int -acess_to_mmap_flags (int access) +access_to_mmap_flags (int access) { switch (access) { case MMAP_FILE_ACCESS_READ_WRITE: @@ -236,10 +236,12 @@ open_file_map (const char *c_path, int input_fd, int mode, gint64 *capacity, int MmapHandle *handle = NULL; int result, fd; + MONO_ENTER_GC_SAFE; if (c_path) result = stat (c_path, &buf); else result = fstat (input_fd, &buf); + MONO_EXIT_GC_SAFE; if (mode == FILE_MODE_TRUNCATE || mode == FILE_MODE_APPEND || mode == FILE_MODE_OPEN) { if (result == -1) { //XXX translate errno? @@ -275,10 +277,12 @@ open_file_map (const char *c_path, int input_fd, int mode, gint64 *capacity, int } } + MONO_ENTER_GC_SAFE; if (c_path) //FIXME use io portability? fd = open (c_path, file_mode_to_unix (mode) | access_mode_to_unix (access), DEFAULT_FILEMODE); else fd = dup (input_fd); + MONO_EXIT_GC_SAFE; if (fd == -1) { //XXX translate errno? *ioerror = COULD_NOT_OPEN; @@ -353,13 +357,17 @@ open_memory_map (const char *c_mapName, int mode, gint64 *capacity, int access, strcpy (file_name, tmp_dir); strcat (file_name, MONO_ANON_FILE_TEMPLATE); + MONO_ENTER_GC_SAFE; fd = mkstemp (file_name); + MONO_EXIT_GC_SAFE; if (fd == -1) { *ioerror = COULD_NOT_MAP_MEMORY; goto done; } + MONO_ENTER_GC_SAFE; unlink (file_name); + MONO_EXIT_GC_SAFE; #ifdef HAVE_FTRUNCATE unused = ftruncate (fd, (off_t)*capacity); #endif @@ -467,7 +475,9 @@ mono_mmap_close (void *mmap_handle, MonoError *error) g_hash_table_remove (named_regions, handle->name); g_free (handle->name); + MONO_ENTER_GC_SAFE; close (handle->fd); + MONO_EXIT_GC_SAFE; g_free (handle); } named_regions_unlock (); @@ -480,7 +490,9 @@ mono_mmap_configure_inheritability (void *mmap_handle, gint32 inheritability, Mo int fd, flags; fd = h->fd; + MONO_ENTER_GC_SAFE; flags = fcntl (fd, F_GETFD, 0); + MONO_EXIT_GC_SAFE; if (inheritability) flags &= ~FD_CLOEXEC; else @@ -495,7 +507,9 @@ mono_mmap_flush (void *mmap_handle, MonoError *error) #ifdef HAVE_MSYNC if (h) + MONO_ENTER_GC_SAFE; msync (h->address, h->length, MS_SYNC); + MONO_EXIT_GC_SAFE; #endif } @@ -525,8 +539,10 @@ mono_mmap_map (void *handle, gint64 offset, gint64 *size, int access, void **mma mmap_offset = align_down_to_page_size (offset); eff_size += (offset - mmap_offset); + MONO_ENTER_GC_SAFE; //FIXME translate some interesting errno values - res.address = mono_file_map ((size_t)eff_size, acess_to_mmap_flags (access), fh->fd, mmap_offset, &res.free_handle); + res.address = mono_file_map ((size_t)eff_size, access_to_mmap_flags (access), fh->fd, mmap_offset, &res.free_handle); + MONO_EXIT_GC_SAFE; res.length = eff_size; if (res.address) { @@ -544,7 +560,9 @@ mono_mmap_unmap (void *mmap_handle, MonoError *error) int res = 0; MmapInstance *h = (MmapInstance *)mmap_handle; + MONO_ENTER_GC_SAFE; res = mono_file_unmap (h->address, h->free_handle); + MONO_EXIT_GC_SAFE; g_free (h); return res == 0; diff --git a/src/mono/mono/metadata/file-mmap-windows.c b/src/mono/mono/metadata/file-mmap-windows.c index 108731b..792bbf8 100644 --- a/src/mono/mono/metadata/file-mmap-windows.c +++ b/src/mono/mono/metadata/file-mmap-windows.c @@ -157,7 +157,11 @@ open_handle (void *handle, const gunichar2 *mapName, gint mapName_length, int mo } } else { FILE_STANDARD_INFO info; - if (!GetFileInformationByHandleEx (handle, FileStandardInfo, &info, sizeof (FILE_STANDARD_INFO))) { + gboolean getinfo_success; + MONO_ENTER_GC_SAFE; + getinfo_success = GetFileInformationByHandleEx (handle, FileStandardInfo, &info, sizeof (FILE_STANDARD_INFO)); + MONO_EXIT_GC_SAFE; + if (!getinfo_success) { *ioerror = convert_win32_error (GetLastError (), COULD_NOT_OPEN); return NULL; } @@ -175,16 +179,22 @@ open_handle (void *handle, const gunichar2 *mapName, gint mapName_length, int mo HANDLE result = NULL; if (mode == FILE_MODE_CREATE_NEW || handle != INVALID_HANDLE_VALUE) { + MONO_ENTER_GC_SAFE; result = CreateFileMappingW (handle, NULL, get_page_access (access) | options, (DWORD)(((guint64)*capacity) >> 32), (DWORD)*capacity, mapName); + MONO_EXIT_GC_SAFE; if (result && GetLastError () == ERROR_ALREADY_EXISTS) { + MONO_ENTER_GC_SAFE; CloseHandle (result); + MONO_EXIT_GC_SAFE; result = NULL; *ioerror = FILE_ALREADY_EXISTS; } else if (!result && GetLastError () != NO_ERROR) { *ioerror = convert_win32_error (GetLastError (), COULD_NOT_OPEN); } } else if (mode == FILE_MODE_OPEN || mode == FILE_MODE_OPEN_OR_CREATE && access == MMAP_FILE_ACCESS_WRITE) { + MONO_ENTER_GC_SAFE; result = OpenFileMappingW (get_file_map_access (access), FALSE, mapName); + MONO_EXIT_GC_SAFE; if (!result) { if (mode == FILE_MODE_OPEN_OR_CREATE && GetLastError () == ERROR_FILE_NOT_FOUND) { *ioerror = INVALID_FILE_MODE; @@ -210,14 +220,18 @@ open_handle (void *handle, const gunichar2 *mapName, gint mapName_length, int mo guint32 waitSleep = 0; while (waitRetries > 0) { + MONO_ENTER_GC_SAFE; result = CreateFileMappingW (handle, NULL, get_page_access (access) | options, (DWORD)(((guint64)*capacity) >> 32), (DWORD)*capacity, mapName); + MONO_EXIT_GC_SAFE; if (result) break; if (GetLastError() != ERROR_ACCESS_DENIED) { *ioerror = convert_win32_error (GetLastError (), COULD_NOT_OPEN); break; } + MONO_ENTER_GC_SAFE; result = OpenFileMappingW (get_file_map_access (access), FALSE, mapName); + MONO_EXIT_GC_SAFE; if (result) break; if (GetLastError () != ERROR_FILE_NOT_FOUND) { @@ -253,12 +267,17 @@ mono_mmap_open_file (const gunichar2 *path, gint path_length, int mode, const gu if (path) { WIN32_FILE_ATTRIBUTE_DATA file_attrs; - gboolean existed = GetFileAttributesExW (path, GetFileExInfoStandard, &file_attrs); + gboolean existed; + MONO_ENTER_GC_SAFE; + existed = GetFileAttributesExW (path, GetFileExInfoStandard, &file_attrs); + MONO_EXIT_GC_SAFE; if (!existed && mode == FILE_MODE_CREATE_NEW && *capacity == 0) { *ioerror = CAPACITY_SMALLER_THAN_FILE_SIZE; goto done; } + MONO_ENTER_GC_SAFE; hFile = CreateFileW (path, get_file_access (access), FILE_SHARE_READ, NULL, mode, FILE_ATTRIBUTE_NORMAL, NULL); + MONO_EXIT_GC_SAFE; if (hFile == INVALID_HANDLE_VALUE) { *ioerror = convert_win32_error (GetLastError (), COULD_NOT_OPEN); goto done; @@ -272,10 +291,12 @@ mono_mmap_open_file (const gunichar2 *path, gint path_length, int mode, const gu result = open_handle (hFile, mapName, mapName_length, mode, capacity, access, options, ioerror, error); done: + MONO_ENTER_GC_SAFE; if (hFile != INVALID_HANDLE_VALUE) CloseHandle (hFile); if (!result && delete_on_error) DeleteFileW (path); + MONO_EXIT_GC_SAFE; return result; } @@ -292,7 +313,9 @@ void mono_mmap_close (void *mmap_handle, MonoError *error) { g_assert (mmap_handle); + MONO_ENTER_GC_SAFE; CloseHandle (mmap_handle); + MONO_EXIT_GC_SAFE; } void @@ -310,9 +333,14 @@ mono_mmap_flush (void *mmap_handle, MonoError *error) g_assert (mmap_handle); MmapInstance *h = (MmapInstance *)mmap_handle; - if (FlushViewOfFile (h->address, h->length)) + gboolean flush_success; + MONO_ENTER_GC_SAFE; + flush_success = FlushViewOfFile (h->address, h->length); + MONO_EXIT_GC_SAFE; + if (flush_success) return; + // This replicates how CoreFX does MemoryMappedView.Flush (). // It is a known issue within the NTFS transaction log system that @@ -331,7 +359,10 @@ mono_mmap_flush (void *mmap_handle, MonoError *error) mono_thread_info_sleep (pause, NULL); for (int r = 0; r < MAX_FLUSH_RETIRES_PER_WAIT; r++) { - if (FlushViewOfFile (h->address, h->length)) + MONO_ENTER_GC_SAFE; + flush_success = FlushViewOfFile (h->address, h->length); + MONO_EXIT_GC_SAFE; + if (flush_success) return; if (GetLastError () != ERROR_LOCK_VIOLATION) @@ -365,13 +396,18 @@ mono_mmap_map (void *handle, gint64 offset, gint64 *size, int access, void **mma return CAPACITY_LARGER_THAN_LOGICAL_ADDRESS_SPACE; #endif - void *address = MapViewOfFile (handle, get_file_map_access (access), (DWORD) (newOffset >> 32), (DWORD) newOffset, (SIZE_T) nativeSize); + void *address; + MONO_ENTER_GC_SAFE; + address = MapViewOfFile (handle, get_file_map_access (access), (DWORD) (newOffset >> 32), (DWORD) newOffset, (SIZE_T) nativeSize); + MONO_EXIT_GC_SAFE; if (!address) return convert_win32_error (GetLastError (), COULD_NOT_MAP_MEMORY); // Query the view for its size and allocation type MEMORY_BASIC_INFORMATION viewInfo; + MONO_ENTER_GC_SAFE; VirtualQuery (address, &viewInfo, sizeof (MEMORY_BASIC_INFORMATION)); + MONO_EXIT_GC_SAFE; guint64 viewSize = (guint64) viewInfo.RegionSize; // Allocate the pages if we were using the MemoryMappedFileOptions.DelayAllocatePages option @@ -385,12 +421,17 @@ mono_mmap_map (void *handle, gint64 offset, gint64 *size, int access, void **mma // and size of the region of pages with matching attributes starting from base address. // VirtualQueryEx: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366907(v=vs.85).aspx if (((viewInfo.State & MEM_RESERVE) != 0) || viewSize < (guint64) nativeSize) { - void *tempAddress = VirtualAlloc (address, nativeSize != 0 ? nativeSize : viewSize, MEM_COMMIT, get_page_access (access)); + void *tempAddress; + MONO_ENTER_GC_SAFE; + tempAddress = VirtualAlloc (address, nativeSize != 0 ? nativeSize : viewSize, MEM_COMMIT, get_page_access (access)); + MONO_EXIT_GC_SAFE; if (!tempAddress) { return convert_win32_error (GetLastError (), COULD_NOT_MAP_MEMORY); } // again query the view for its new size + MONO_ENTER_GC_SAFE; VirtualQuery (address, &viewInfo, sizeof (MEMORY_BASIC_INFORMATION)); + MONO_EXIT_GC_SAFE; viewSize = (guint64) viewInfo.RegionSize; } @@ -413,7 +454,10 @@ mono_mmap_unmap (void *mmap_handle, MonoError *error) MmapInstance *h = (MmapInstance *) mmap_handle; - gboolean result = UnmapViewOfFile (h->address); + gboolean result; + MONO_ENTER_GC_SAFE; + result = UnmapViewOfFile (h->address); + MONO_EXIT_GC_SAFE; g_free (h); return result; diff --git a/src/mono/mono/metadata/w32process-unix.c b/src/mono/mono/metadata/w32process-unix.c index b937b33..d0aba50 100644 --- a/src/mono/mono/metadata/w32process-unix.c +++ b/src/mono/mono/metadata/w32process-unix.c @@ -2044,6 +2044,7 @@ ves_icall_System_Diagnostics_Process_ShellExecuteEx_internal (MonoW32ProcessStar * On Linux, try: xdg-open, the FreeDesktop standard way of doing it, * if that fails, try to use gnome-open, then kfmclient */ + MONO_ENTER_GC_SAFE; handler = g_find_program_in_path ("xdg-open"); if (handler != NULL) handler_needswait = TRUE; @@ -2054,7 +2055,6 @@ ves_icall_System_Diagnostics_Process_ShellExecuteEx_internal (MonoW32ProcessStar if (handler == NULL){ handler_utf16 = (gunichar2 *) -1; ret = FALSE; - goto done; } else { /* kfmclient needs exec argument */ char *old = handler; @@ -2064,6 +2064,10 @@ ves_icall_System_Diagnostics_Process_ShellExecuteEx_internal (MonoW32ProcessStar } } } + MONO_EXIT_GC_SAFE; + if (ret == FALSE){ + goto done; + } #endif handler_utf16 = g_utf8_to_utf16 (handler, -1, NULL, NULL, NULL); g_free (handler); @@ -2219,7 +2223,9 @@ ves_icall_System_Diagnostics_Process_GetProcesses_internal (void) gpointer *pidarray; int i, count; + MONO_ENTER_GC_SAFE; pidarray = mono_process_list (&count); + MONO_EXIT_GC_SAFE; if (!pidarray) { mono_error_set_not_supported (error, "This system does not support EnumProcesses"); mono_error_set_pending_exception (error); diff --git a/src/mono/mono/metadata/w32process-win32.c b/src/mono/mono/metadata/w32process-win32.c index fed5159..31b3a5d 100644 --- a/src/mono/mono/metadata/w32process-win32.c +++ b/src/mono/mono/metadata/w32process-win32.c @@ -81,7 +81,7 @@ ves_icall_System_Diagnostics_Process_ShellExecuteEx_internal (MonoW32ProcessStar gboolean ret; shellex.cbSize = sizeof(SHELLEXECUTEINFO); - shellex.fMask = (gulong)(SEE_MASK_FLAG_DDEWAIT | SEE_MASK_NOCLOSEPROCESS | SEE_MASK_UNICODE); + shellex.fMask = (gulong)(SEE_MASK_NOASYNC | SEE_MASK_NOCLOSEPROCESS | SEE_MASK_UNICODE); shellex.nShow = (gulong)MONO_HANDLE_GETVAL (proc_start_info, window_style); shellex.nShow = (gulong)((shellex.nShow == 0) ? 1 : (shellex.nShow == 1 ? 0 : shellex.nShow)); @@ -99,7 +99,10 @@ ves_icall_System_Diagnostics_Process_ShellExecuteEx_internal (MonoW32ProcessStar else shellex.fMask = (gulong)(shellex.fMask | SEE_MASK_FLAG_NO_UI); + MONO_ENTER_GC_SAFE; ret = ShellExecuteEx (&shellex); + MONO_EXIT_GC_SAFE; + if (ret == FALSE) { process_info->pid = -GetLastError (); } else { @@ -138,6 +141,7 @@ mono_process_create_process (MonoCreateProcessCoop *coop, MonoW32ProcessInfo *mo gchandle_t cmd_gchandle = 0; gunichar2 *cmd_chars = MONO_HANDLE_IS_NULL (cmd) ? NULL : mono_string_handle_pin_chars (cmd, &cmd_gchandle); + MONO_ENTER_GC_SAFE; if (coop->username) { guint32 logon_flags = mono_process_info->load_user_profile ? LOGON_WITH_PROFILE : 0; @@ -161,6 +165,7 @@ mono_process_create_process (MonoCreateProcessCoop *coop, MonoW32ProcessInfo *mo start_info, process_info); } + MONO_EXIT_GC_SAFE; mono_gchandle_free_internal (cmd_gchandle); @@ -204,7 +209,7 @@ process_complete_path (const gunichar2 *appname, gchar **completed) char *utf8app; char *utf8appmemory = NULL; char *found = NULL; - gboolean result; + gboolean result, file_test_result; utf8appmemory = g_utf16_to_utf8 (appname, -1, NULL, NULL, NULL); utf8app = process_unquote_application_name (utf8appmemory); @@ -215,13 +220,18 @@ process_complete_path (const gunichar2 *appname, gchar **completed) goto exit; } - if (g_file_test (utf8app, G_FILE_TEST_IS_EXECUTABLE) && !g_file_test (utf8app, G_FILE_TEST_IS_DIR)) { + MONO_ENTER_GC_SAFE; + file_test_result = g_file_test (utf8app, G_FILE_TEST_IS_EXECUTABLE) && !g_file_test (utf8app, G_FILE_TEST_IS_DIR); + MONO_EXIT_GC_SAFE; + if (file_test_result) { *completed = process_quote_path (utf8app); result = TRUE; goto exit; } + MONO_ENTER_GC_SAFE; found = g_find_program_in_path (utf8app); + MONO_EXIT_GC_SAFE; if (found == NULL) { *completed = NULL; result = FALSE; @@ -358,7 +368,11 @@ exit: static gboolean mono_process_win_enum_processes (DWORD *pids, DWORD count, DWORD *needed) { - return !!EnumProcesses (pids, count, needed); + gboolean success; + MONO_ENTER_GC_SAFE; + success = EnumProcesses (pids, count, needed); + MONO_EXIT_GC_SAFE; + return success; } #endif /* G_HAVE_API_SUPPORT(HAVE_CLASSIC_WINAPI_SUPPORT) */ diff --git a/src/mono/mono/metadata/w32semaphore-unix.c b/src/mono/mono/metadata/w32semaphore-unix.c index 86562dc..6a851fc 100644 --- a/src/mono/mono/metadata/w32semaphore-unix.c +++ b/src/mono/mono/metadata/w32semaphore-unix.c @@ -233,6 +233,9 @@ exit: return handle; } +// These functions appear to be using coop-aware locking functions, and so this file does not include explicit +// GC-safe transitions like its corresponding Windows version + gpointer ves_icall_System_Threading_Semaphore_CreateSemaphore_icall (gint32 initialCount, gint32 maximumCount, const gunichar2 *name, gint32 name_length, gint32 *win32error, MonoError *error) diff --git a/src/mono/mono/metadata/w32semaphore-win32.c b/src/mono/mono/metadata/w32semaphore-win32.c index d3ca09f..a6d3f2b 100644 --- a/src/mono/mono/metadata/w32semaphore-win32.c +++ b/src/mono/mono/metadata/w32semaphore-win32.c @@ -25,7 +25,10 @@ gpointer ves_icall_System_Threading_Semaphore_CreateSemaphore_icall (gint32 initialCount, gint32 maximumCount, const gunichar2 *name, gint32 name_length, gint32 *win32error, MonoError *error) { - HANDLE sem = CreateSemaphoreW (NULL, initialCount, maximumCount, name); + HANDLE sem; + MONO_ENTER_GC_SAFE; + sem = CreateSemaphoreW (NULL, initialCount, maximumCount, name); + MONO_EXIT_GC_SAFE; *win32error = GetLastError (); return sem; } @@ -41,7 +44,10 @@ gpointer ves_icall_System_Threading_Semaphore_OpenSemaphore_icall (const gunichar2 *name, gint32 name_length, gint32 rights, gint32 *win32error, MonoError *error) { - HANDLE sem = OpenSemaphoreW (rights, FALSE, name); + HANDLE sem; + MONO_ENTER_GC_SAFE; + sem = OpenSemaphoreW (rights, FALSE, name); + MONO_EXIT_GC_SAFE; *win32error = GetLastError (); return sem; } diff --git a/src/mono/mono/utils/mono-dl.c b/src/mono/mono/utils/mono-dl.c index eddb0c4..a5440c5 100644 --- a/src/mono/mono/utils/mono-dl.c +++ b/src/mono/mono/utils/mono-dl.c @@ -150,6 +150,7 @@ mono_dl_open (const char *name, int flags, char **error_msg) } module->main_module = name == NULL? TRUE: FALSE; + // No GC safe transition because this is called early in main.c lib = mono_dl_open_file (name, lflags); if (!lib) { diff --git a/src/mono/mono/utils/mono-mmap.c b/src/mono/mono/utils/mono-mmap.c index 0b4765f..b6931bc 100644 --- a/src/mono/mono/utils/mono-mmap.c +++ b/src/mono/mono/utils/mono-mmap.c @@ -383,6 +383,7 @@ mono_file_map (size_t length, int flags, int fd, guint64 offset, void **ret_hand return NULL; #endif + // No GC safe transition because this is called early in main.c BEGIN_CRITICAL_SECTION; ptr = mmap (0, length, prot, mflags, fd, offset); END_CRITICAL_SECTION; @@ -412,6 +413,7 @@ mono_file_unmap (void *addr, void *handle) { int res; + // No GC safe transition because this is called early in driver.c via mono_debug_init (with a few layers of indirection) BEGIN_CRITICAL_SECTION; res = munmap (addr, (size_t)handle); END_CRITICAL_SECTION; @@ -453,6 +455,7 @@ mono_mprotect (void *addr, size_t length, int flags) #endif #endif } + // No GC safe transition because this is called early in mini_init via mono_arch_init (with a few layers of indirection) return mprotect (addr, length, prot); } -- 2.7.4