From b464614bd60266eecea578921873ee7e2b19cdb9 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 20 Dec 2022 10:34:59 -0500 Subject: [PATCH 1/2] [threads] Save errno when using posix semaphores for thread transitions We already save/restore GetLastError on win32. Do it on posix platforms, too. If one thread is in a pinvoke wrapper, while another thread triggers a STW, the pinvoke wrapper will self-suspend the thread and wait for a notification to resume. Depending on the platform we can use win32 primitives, Mach semaphores or POSIX semaphores. win32 and posix can both change the value of last error (errno, respectively) while the thread is suspended. That means that code like this (generated by the LibraryImportAttribute source generator) cannot reliably retrieve the error from the last pinvoke: ```csharp __retVal = __PInvoke(__path_native, mode); // there is a pinvoke wrapper here, that transitions from GC Safe to GC Unsafe mode __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError(); ``` The solution is to explicitly preserve the value of GetLastError/errno when exiting from GC Safe. Fixes https://github.com/dotnet/runtime/issues/77364 --- src/mono/mono/utils/mono-threads.h | 33 +++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/utils/mono-threads.h b/src/mono/mono/utils/mono-threads.h index a31df21400abc0..9aecf8b78608d8 100644 --- a/src/mono/mono/utils/mono-threads.h +++ b/src/mono/mono/utils/mono-threads.h @@ -871,6 +871,17 @@ mono_win32_interrupt_wait (PVOID thread_info, HANDLE native_thread_handle, DWORD void mono_win32_abort_blocking_io_call (THREAD_INFO_TYPE *info); +#else + + +#endif + +#ifdef USE_WINDOWS_BACKEND + +/* APC calls can change GetLastError while a thread is suspended. Save/restore it when doing thread + state transitions (for example in m2n wrappers) in order to protect the result of the last + pinvoke */ + #define W32_DEFINE_LAST_ERROR_RESTORE_POINT \ const DWORD _last_error_restore_point = GetLastError (); @@ -879,11 +890,31 @@ mono_win32_abort_blocking_io_call (THREAD_INFO_TYPE *info); if (GetLastError () != _last_error_restore_point) \ mono_SetLastError (_last_error_restore_point); -#else +#elif defined(USE_WASM_BACKEND) || defined (USE_POSIX_BACKEND) + +#define W32_DEFINE_LAST_ERROR_RESTORE_POINT \ + int _last_errno_restore_point = errno; + +#define W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT \ + if (errno != _last_errno_restore_point) \ + errno = _last_errno_restore_point; + +/* Posix semaphores set errno on failure and sporadic wakeup. GC state transitions are done in n2m + * and m2n wrappers and may change the value of errno from the last pinvoke. Use these macros to + * save/restore errno when doing thread state transitions. */ + +#elif defined(USE_MACH_BACKEND) + +/* Mach semaphores don't set errno on failure. Change this to be the same as POSIX if some other primitives used + in thread state transitions pollute errno. */ #define W32_DEFINE_LAST_ERROR_RESTORE_POINT /* nothing */ #define W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT /* nothing */ +#else +#error "unknown threads backend, not sure how to save/restore last error" #endif + + #endif /* __MONO_THREADS_H__ */ From b09d29fd3a6b74d4baf6e143e80660eef09f56f1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 20 Dec 2022 10:42:43 -0500 Subject: [PATCH 2/2] rename W32_DEFINE_LAST_ERROR_RESTORE_POINT to MONO_DEFINE_LAST_ERROR_RESTORE_POINT and W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT to MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT --- src/mono/mono/utils/mono-threads-coop.c | 12 ++++++------ src/mono/mono/utils/mono-threads.c | 4 ++-- src/mono/mono/utils/mono-threads.h | 12 ++++++------ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/mono/mono/utils/mono-threads-coop.c b/src/mono/mono/utils/mono-threads-coop.c index 4ed659d66058cc..c562ada6703fe1 100644 --- a/src/mono/mono/utils/mono-threads-coop.c +++ b/src/mono/mono/utils/mono-threads-coop.c @@ -337,10 +337,10 @@ mono_threads_exit_gc_safe_region_internal (gpointer cookie, MonoStackData *stack return; #ifdef ENABLE_CHECKED_BUILD_GC - W32_DEFINE_LAST_ERROR_RESTORE_POINT; + MONO_DEFINE_LAST_ERROR_RESTORE_POINT; if (mono_check_mode_enabled (MONO_CHECK_MODE_GC)) coop_tls_pop (cookie); - W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; + MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; #endif mono_threads_exit_gc_safe_region_unbalanced_internal (cookie, stackdata); @@ -365,7 +365,7 @@ mono_threads_exit_gc_safe_region_unbalanced_internal (gpointer cookie, MonoStack /* Common to use enter/exit gc safe around OS API's affecting last error. */ /* This method can call OS API's that will reset last error on some platforms. */ /* To reduce errors, we need to restore last error before exit gc safe. */ - W32_DEFINE_LAST_ERROR_RESTORE_POINT; + MONO_DEFINE_LAST_ERROR_RESTORE_POINT; info = (MonoThreadInfo *)cookie; @@ -398,7 +398,7 @@ mono_threads_exit_gc_safe_region_unbalanced_internal (gpointer cookie, MonoStack info->user_data = NULL; } - W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; + MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; } void @@ -652,14 +652,14 @@ mono_threads_suspend_policy_init (void) // otherwise if one of the old environment variables is set, use that. // otherwise use full preemptive suspend. - W32_DEFINE_LAST_ERROR_RESTORE_POINT; + MONO_DEFINE_LAST_ERROR_RESTORE_POINT; (policy = threads_suspend_policy_getenv ()) || (policy = threads_suspend_policy_default ()) || (policy = threads_suspend_policy_getenv_compat ()) || (policy = MONO_THREADS_SUSPEND_FULL_PREEMPTIVE); - W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; + MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; g_assert (policy); mono_threads_suspend_policy_hidden_dont_modify = (char)policy; diff --git a/src/mono/mono/utils/mono-threads.c b/src/mono/mono/utils/mono-threads.c index ac6d2078bb63b8..7c2d6479d76a63 100644 --- a/src/mono/mono/utils/mono-threads.c +++ b/src/mono/mono/utils/mono-threads.c @@ -1938,7 +1938,7 @@ mono_thread_info_uninstall_interrupt (gboolean *interrupted) /* Common to uninstall interrupt handler around OS API's affecting last error. */ /* This method could call OS API's on some platforms that will reset last error so make sure to restore */ /* last error before exit. */ - W32_DEFINE_LAST_ERROR_RESTORE_POINT; + MONO_DEFINE_LAST_ERROR_RESTORE_POINT; g_assert (interrupted); *interrupted = FALSE; @@ -1961,7 +1961,7 @@ mono_thread_info_uninstall_interrupt (gboolean *interrupted) THREADS_INTERRUPT_DEBUG ("interrupt uninstall tid %p previous_token %p interrupted %s\n", mono_thread_info_get_tid (info), previous_token, *interrupted ? "TRUE" : "FALSE"); - W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; + MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT; } static MonoThreadInfoInterruptToken* diff --git a/src/mono/mono/utils/mono-threads.h b/src/mono/mono/utils/mono-threads.h index 9aecf8b78608d8..14bd12ca4c7543 100644 --- a/src/mono/mono/utils/mono-threads.h +++ b/src/mono/mono/utils/mono-threads.h @@ -882,20 +882,20 @@ mono_win32_abort_blocking_io_call (THREAD_INFO_TYPE *info); state transitions (for example in m2n wrappers) in order to protect the result of the last pinvoke */ -#define W32_DEFINE_LAST_ERROR_RESTORE_POINT \ +#define MONO_DEFINE_LAST_ERROR_RESTORE_POINT \ const DWORD _last_error_restore_point = GetLastError (); -#define W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT \ +#define MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT \ /* Only restore if changed to prevent unnecessary writes. */ \ if (GetLastError () != _last_error_restore_point) \ mono_SetLastError (_last_error_restore_point); #elif defined(USE_WASM_BACKEND) || defined (USE_POSIX_BACKEND) -#define W32_DEFINE_LAST_ERROR_RESTORE_POINT \ +#define MONO_DEFINE_LAST_ERROR_RESTORE_POINT \ int _last_errno_restore_point = errno; -#define W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT \ +#define MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT \ if (errno != _last_errno_restore_point) \ errno = _last_errno_restore_point; @@ -908,8 +908,8 @@ mono_win32_abort_blocking_io_call (THREAD_INFO_TYPE *info); /* Mach semaphores don't set errno on failure. Change this to be the same as POSIX if some other primitives used in thread state transitions pollute errno. */ -#define W32_DEFINE_LAST_ERROR_RESTORE_POINT /* nothing */ -#define W32_RESTORE_LAST_ERROR_FROM_RESTORE_POINT /* nothing */ +#define MONO_DEFINE_LAST_ERROR_RESTORE_POINT /* nothing */ +#define MONO_RESTORE_LAST_ERROR_FROM_RESTORE_POINT /* nothing */ #else #error "unknown threads backend, not sure how to save/restore last error"