signals: acquire loader lock during stackwalk on Win32 (#59840)

Lock the Windows loader lock in jl_with_stackwalk_lock to prevent
certain deadlocks when unwinding the stack, similar to how macOS
acquires the keymgr lock in jl_lock_profile_mach.

The loader lock prevents the loader from modifying internal data
structures while we're walking the stack, which could otherwise cause
crashes or hangs during backtrace collection.

Fixes some of #59650

🤖 Generated with Claude Code

Author: vtjnash

src/julia_internal.h

@@ -240,7 +240,6 @@ JL_DLLEXPORT int jl_lock_profile(void) JL_NOTSAFEPOINT JL_NOTSAFEPOINT_ENTER;
 JL_DLLEXPORT void jl_unlock_profile(void) JL_NOTSAFEPOINT JL_NOTSAFEPOINT_LEAVE;
 JL_DLLEXPORT int jl_lock_profile_wr(void) JL_NOTSAFEPOINT JL_NOTSAFEPOINT_ENTER;
 JL_DLLEXPORT void jl_unlock_profile_wr(void) JL_NOTSAFEPOINT JL_NOTSAFEPOINT_LEAVE;
-void jl_with_stackwalk_lock(void (*f)(void*) JL_NOTSAFEPOINT, void *ctx) JL_NOTSAFEPOINT;
 
 arraylist_t *jl_get_all_tasks_arraylist(void) JL_NOTSAFEPOINT;
 typedef struct {
@@ -1533,8 +1532,8 @@ void jl_fprint_bt_entry_codeloc(ios_t *s, jl_bt_element_t *bt_data) JL_NOTSAFEPO
 #ifdef _OS_WINDOWS_
 JL_DLLEXPORT void jl_refresh_dbg_module_list(void);
 #endif
-int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx) JL_NOTSAFEPOINT;
 void jl_thread_resume(int tid) JL_NOTSAFEPOINT;
+int jl_thread_suspend(int16_t tid, bt_context_t *ctx) JL_NOTSAFEPOINT;
 
 // *to is NULL or malloc'd pointer, from is allowed to be NULL
 STATIC_INLINE char *jl_copy_str(char **to, const char *from) JL_NOTSAFEPOINT

src/signals-mach.c

@@ -527,7 +527,7 @@ static int jl_thread_suspend_and_get_state2(int tid, host_thread_state_t *ctx) J
     return 1;
 }
 
-int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx)
+static int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx)
 {
     (void)timeout;
     host_thread_state_t state;
@@ -712,6 +712,14 @@ static void jl_unlock_profile_mach(int dlsymlock, int keymgr_locked)
     jl_unlock_profile();
 }
 
+int jl_thread_suspend(int16_t tid, bt_context_t *ctx)
+{
+    int lockret = jl_lock_profile_mach(1);
+    int success = jl_thread_suspend_and_get_state(tid, 1, ctx);
+    jl_unlock_profile_mach(1, lockret);
+    return success;
+}
+
 void jl_with_stackwalk_lock(void (*f)(void*), void *ctx)
 {
     int lockret = jl_lock_profile_mach(1);

src/signals-unix.c

@@ -303,41 +303,34 @@ int exc_reg_is_write_fault(uintptr_t esr) {
 }
 #endif
 
+static int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx);
+
 #if defined(HAVE_MACH)
 #include "signals-mach.c"
 #else
 #include <poll.h>
 #include <sys/eventfd.h>
 #include <link.h>
 
-#ifndef _OS_FREEBSD_
 typedef struct {
-    void (*f)(void*) JL_NOTSAFEPOINT;
-    void *ctx;
-} callback_t;
+    int16_t tid;
+    bt_context_t *ctx;
+    int success;
+} callback_data_t;
 static int with_dl_iterate_phdr_lock(struct dl_phdr_info *info, size_t size, void *data)
 {
     jl_lock_profile();
-    callback_t *callback = (callback_t*)data;
-    callback->f(callback->ctx);
+    callback_data_t *cb_data = (callback_data_t*)data;
+    cb_data->success = jl_thread_suspend_and_get_state(cb_data->tid, 1, cb_data->ctx);
     jl_unlock_profile();
     return 1; // only call this once
 }
-#endif
 
-void jl_with_stackwalk_lock(void (*f)(void*), void *ctx)
+int jl_thread_suspend(int16_t tid, bt_context_t *ctx)
 {
-#ifndef _OS_FREEBSD_
-    callback_t callback = {f, ctx};
-    dl_iterate_phdr(with_dl_iterate_phdr_lock, &callback);
-#else
-    // FreeBSD makes the questionable decisions to use a terrible implementation of a spin
-    // lock and to block all signals while a lock is held. However, that also means it is
-    // not currently vulnerable to this libunwind bug that other platforms can encounter.
-    jl_lock_profile();
-    f(ctx);
-    jl_unlock_profile();
-#endif
+    callback_data_t cb_data = {tid, ctx, 0};
+    dl_iterate_phdr(with_dl_iterate_phdr_lock, &cb_data);
+    return cb_data.success;
 }
 
 #if defined(_OS_LINUX_) && (defined(_CPU_X86_64_) || defined(_CPU_X86_))
@@ -458,7 +451,7 @@ static int exit_signal_cond = -1;
 static int signal_caught_cond = -1;
 static int signals_inflight = 0;
 
-int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx)
+static int jl_thread_suspend_and_get_state(int tid, int timeout, bt_context_t *ctx)
 {
     int err;
     pthread_mutex_lock(&in_signal_lock);
@@ -845,15 +838,15 @@ void trigger_profile_peek(void)
 
 static jl_bt_element_t signal_bt_data[JL_MAX_BT_SIZE + 1];
 static size_t signal_bt_size = 0;
-static void do_critical_profile(void *ctx)
+static void do_critical_profile(void)
 {
     bt_context_t signal_context;
     // sample each thread, round-robin style in reverse order
     // (so that thread zero gets notified last)
     int nthreads = jl_atomic_load_acquire(&jl_n_threads);
     for (int i = nthreads; i-- > 0; ) {
         // notify thread to stop
-        if (!jl_thread_suspend_and_get_state(i, 1, &signal_context))
+        if (!jl_thread_suspend(i, &signal_context))
             continue;
 
         // do backtrace on thread contexts for critical signals
@@ -866,7 +859,7 @@ static void do_critical_profile(void *ctx)
     }
 }
 
-static void do_profile(void *ctx)
+static void do_profile(void)
 {
     bt_context_t signal_context;
     int nthreads = jl_atomic_load_acquire(&jl_n_threads);
@@ -883,7 +876,7 @@ static void do_profile(void *ctx)
             return;
         }
         // notify thread to stop
-        if (!jl_thread_suspend_and_get_state(tid, 1, &signal_context))
+        if (!jl_thread_suspend(tid, &signal_context))
             return;
         // unwinding can fail, so keep track of the current state
         // and restore from the SEGV handler if anything happens.
@@ -1062,15 +1055,15 @@ static void *signal_listener(void *arg)
         signal_bt_size = 0;
 #if !defined(JL_DISABLE_LIBUNWIND)
         if (critical) {
-            jl_with_stackwalk_lock(do_critical_profile, NULL);
+            do_critical_profile();
         }
         else if (profile) {
             if (profile_all_tasks) {
                 // Don't take the stackwalk lock here since it's already taken in `jl_rec_backtrace`
                 jl_profile_task();
             }
             else {
-                jl_with_stackwalk_lock(do_profile, NULL);
+                do_profile();
             }
         }
 #ifndef HAVE_MACH

src/signals-win.c

@@ -4,6 +4,11 @@
 // Note that this file is `#include`d by "signal-handling.c"
 #include <mmsystem.h> // hidden by LEAN_AND_MEAN
 
+// Loader lock functions from ntdll
+// See https://devblogs.microsoft.com/oldnewthing/20140808-00/?p=293
+extern NTSTATUS NTAPI LdrLockLoaderLock(ULONG Flags, ULONG *State, ULONG_PTR *Cookie);
+extern NTSTATUS NTAPI LdrUnlockLoaderLock(ULONG Flags, ULONG_PTR Cookie);
+
 static const size_t sig_stack_size = 131072; // 128k reserved for backtrace_fiber for stack overflow handling
 
 // Copied from MINGW_FLOAT_H which may not be found due to a collision with the builtin gcc float.h
@@ -439,26 +444,19 @@ void jl_thread_resume(int tid)
     }
 }
 
-void jl_lock_stackwalk(void)
+int jl_thread_suspend(int16_t tid, bt_context_t *ctx)
 {
     uv_mutex_lock(&jl_in_stackwalk);
     jl_lock_profile();
-}
-
-void jl_unlock_stackwalk(void)
-{
+    ULONG_PTR lock_cookie = 0;
+    LdrLockLoaderLock(0x1, NULL, &lock_cookie);
+    int success = jl_thread_suspend_and_get_state(tid, 0, ctx);
+    LdrUnlockLoaderLock(0x1, lock_cookie);
     jl_unlock_profile();
     uv_mutex_unlock(&jl_in_stackwalk);
+    return success;
 }
 
-void jl_with_stackwalk_lock(void (*f)(void*), void *ctx)
-{
-    jl_lock_stackwalk();
-    f(ctx);
-    jl_unlock_stackwalk();
-}
-
-
 static DWORD WINAPI profile_bt( LPVOID lparam )
 {
     // Note: illegal to use jl_* functions from this thread except for profiling-specific functions
@@ -477,17 +475,15 @@ static DWORD WINAPI profile_bt( LPVOID lparam )
             }
             else {
                 // TODO: bring this up to parity with other OS by adding loop over tid here
-                jl_lock_stackwalk();
-                CONTEXT ctxThread;
-                if (!jl_thread_suspend_and_get_state(0, 0, &ctxThread)) {
-                    jl_unlock_stackwalk();
+                bt_context_t c;
+                if (!jl_thread_suspend(0, &c)) {
                     fputs("failed to suspend main thread. aborting profiling.", stderr);
                     jl_profile_stop_timer();
                     break;
                 }
                 // Get backtrace data
                 profile_bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)profile_bt_data_prof + profile_bt_size_cur,
-                        profile_bt_size_max - profile_bt_size_cur - 1, &ctxThread, NULL);
+                        profile_bt_size_max - profile_bt_size_cur - 1, &c, NULL);
 
                 jl_ptls_t ptls = jl_atomic_load_relaxed(&jl_all_tls_states)[0]; // given only profiling hMainThread
 
@@ -507,7 +503,6 @@ static DWORD WINAPI profile_bt( LPVOID lparam )
                 // Mark the end of this block with two 0's
                 profile_bt_data_prof[profile_bt_size_cur++].uintptr = 0;
                 profile_bt_data_prof[profile_bt_size_cur++].uintptr = 0;
-                jl_unlock_stackwalk();
                 jl_thread_resume(0);
                 jl_check_profile_autostop();
             }

src/stackwalk.c

@@ -1257,26 +1257,13 @@ return 0;
 #endif
 }
 
-typedef struct {
-    int16_t old;
-    bt_context_t *c;
-    int success;
-} suspend_t;
-static void suspend(void *ctx)
-{
-    suspend_t *suspenddata = (suspend_t*)ctx;
-    suspenddata->success = jl_thread_suspend_and_get_state(suspenddata->old, 1, suspenddata->c);
-}
-
 JL_DLLEXPORT size_t jl_try_record_thread_backtrace(jl_ptls_t ptls2, jl_bt_element_t *bt_data, size_t max_bt_size) JL_NOTSAFEPOINT
 {
     int16_t tid = ptls2->tid;
     jl_task_t *t = NULL;
     bt_context_t *context = NULL;
     bt_context_t c;
-    suspend_t suspenddata = {tid, &c};
-    jl_with_stackwalk_lock(suspend, &suspenddata);
-    if (!suspenddata.success) {
+    if (!jl_thread_suspend(tid, &c)) {
         return 0;
     }
     // thread is stopped, safe to read the task it was running before we stopped it
@@ -1309,9 +1296,7 @@ JL_DLLEXPORT jl_record_backtrace_result_t jl_record_backtrace(jl_task_t *t, jl_b
     int16_t old;
     for (old = -1; !jl_atomic_cmpswap(&t->tid, &old, tid) && old != tid; old = -1) {
         // if this task is already running somewhere, we need to stop the thread it is running on and query its state
-        suspend_t suspenddata = {old, &c};
-        jl_with_stackwalk_lock(suspend, &suspenddata);
-        if (!suspenddata.success) {
+        if (!jl_thread_suspend(old, &c)) {
             if (jl_atomic_load_relaxed(&t->tid) != old)
                 continue;
             return result;