From aca2261cda91cae685cd0aad1558c3da7818e5c3 Mon Sep 17 00:00:00 2001 From: Justine Tunney Date: Fri, 13 Oct 2023 13:56:14 -0700 Subject: [PATCH] Don't preempt WIN32 libraries This change refactors our POSIX signals emulation for Windows so that it performs some additional safety checks before calling SetThreadContext() which needs to be locked and must never ever interrupt Microsoft's code. Kudos to the the Go developers for figuring out how to do this properly. --- libc/calls/park.c | 3 ++ libc/calls/readwrite-nt.c | 5 +-- libc/calls/sig.c | 69 +++++++++++++++++++++++++------------- libc/sock/winsockblock.c | 5 +-- test/libc/calls/sig_test.c | 7 ++-- 5 files changed, 60 insertions(+), 29 deletions(-) diff --git a/libc/calls/park.c b/libc/calls/park.c index ec3df7c94c1..06f1f4233fe 100644 --- a/libc/calls/park.c +++ b/libc/calls/park.c @@ -48,8 +48,11 @@ static textwindows int _park_thread(uint32_t msdelay, sigset_t waitmask, if ((rc = _check_cancel()) != -1 && (rc = _check_signal(restartable)) != -1) { unassert((wi = WaitForSingleObject(sem, msdelay)) != -1u); if (wi != kNtWaitTimeout) { + _check_signal(false); rc = eintr(); _check_cancel(); + } else if ((rc = _check_signal(restartable))) { + _check_cancel(); } } __sig_finishwait(om); diff --git a/libc/calls/readwrite-nt.c b/libc/calls/readwrite-nt.c index bd6bc46bf48..9f6e4884f26 100644 --- a/libc/calls/readwrite-nt.c +++ b/libc/calls/readwrite-nt.c @@ -193,8 +193,9 @@ sys_readwrite_nt(int fd, void *data, size_t size, ssize_t offset, // check and see if it was pthread_cancel() which committed the deed // in which case _check_cancel() can acknowledge the cancelation now // it's also fine to do nothing here; punt to next cancelation point - if (GetLastError() == kNtErrorOperationAborted && _check_cancel() == -1) { - return ecanceled(); + if (GetLastError() == kNtErrorOperationAborted) { + if (_check_cancel() == -1) return ecanceled(); + if (!eintered && _check_signal(false)) return eintr(); } // if we chose to process a pending signal earlier then we preserve diff --git a/libc/calls/sig.c b/libc/calls/sig.c index 7650e8f48e2..c59d7dbd07a 100644 --- a/libc/calls/sig.c +++ b/libc/calls/sig.c @@ -128,8 +128,7 @@ static textwindows bool __sig_start(struct PosixThread *pt, int sig, return false; } if (pt->tib->tib_sigmask & (1ull << (sig - 1))) { - STRACE("tid %d masked %G delivering to tib_sigpending", _pthread_tid(pt), - sig); + STRACE("enqueing %G on %d", sig, _pthread_tid(pt)); pt->tib->tib_sigpending |= 1ull << (sig - 1); return false; } @@ -137,10 +136,6 @@ static textwindows bool __sig_start(struct PosixThread *pt, int sig, STRACE("terminating on %G due to no handler", sig); __sig_terminate(sig); } - if (*flags & SA_RESETHAND) { - STRACE("resetting %G handler", sig); - __sighandrvas[sig] = (int32_t)(intptr_t)SIG_DFL; - } return true; } @@ -153,6 +148,10 @@ textwindows int __sig_raise(int sig, int sic) { struct PosixThread *pt = _pthread_self(); ucontext_t ctx = {.uc_sigmask = pt->tib->tib_sigmask}; if (!__sig_start(pt, sig, &rva, &flags)) return 0; + if (flags & SA_RESETHAND) { + STRACE("resetting %G handler", sig); + __sighandrvas[sig] = (int32_t)(intptr_t)SIG_DFL; + } siginfo_t si = {.si_signo = sig, .si_code = sic}; struct NtContext nc; nc.ContextFlags = kNtContextFull; @@ -232,30 +231,53 @@ static textwindows wontreturn void __sig_tramp(struct SignalFrame *sf) { __sig_restore(&sf->ctx); } -static textwindows int __sig_killer(struct PosixThread *pt, int sig, int sic) { - uintptr_t th; +static int __sig_killer(struct PosixThread *pt, int sig, int sic) { + + // prepare for signal unsigned rva, flags; - if (!__sig_start(pt, sig, &rva, &flags)) return 0; - th = _pthread_syshand(pt); - uint32_t old_suspend_count; - old_suspend_count = SuspendThread(th); - if (old_suspend_count == -1u) { + if (!__sig_start(pt, sig, &rva, &flags)) { + return 0; + } + + // take control of thread + // suspending the thread happens asynchronously + // however getting the context blocks until it's frozen + static pthread_spinlock_t killer_lock; + pthread_spin_lock(&killer_lock); + uintptr_t th = _pthread_syshand(pt); + if (SuspendThread(th) == -1u) { STRACE("SuspendThread failed w/ %d", GetLastError()); + pthread_spin_unlock(&killer_lock); return ESRCH; } - if (old_suspend_count) { - STRACE("kill contention of %u on tid %d", old_suspend_count, - _pthread_tid(pt)); - pt->tib->tib_sigpending |= 1ull << (sig - 1); - ResumeThread(th); - return 0; - } struct NtContext nc; nc.ContextFlags = kNtContextFull; if (!GetThreadContext(th, &nc)) { STRACE("GetThreadContext failed w/ %d", GetLastError()); + ResumeThread(th); + pthread_spin_unlock(&killer_lock); return ESRCH; } + pthread_spin_unlock(&killer_lock); + + // we can't preempt threads that are running in win32 code + if ((pt->tib->tib_sigmask & (1ull << (sig - 1))) || + !((uintptr_t)__executable_start <= nc.Rip && + nc.Rip < (uintptr_t)__privileged_start)) { + STRACE("enqueing %G on %d", sig, _pthread_tid(pt)); + pt->tib->tib_sigpending |= 1ull << (sig - 1); + ResumeThread(th); + __sig_cancel(pt, sig, flags); + return 0; + } + + // we're committed to delivering this signal now + if (flags & SA_RESETHAND) { + STRACE("resetting %G handler", sig); + __sighandrvas[sig] = (int32_t)(intptr_t)SIG_DFL; + } + + // inject trampoline function call into thread uintptr_t sp; if (__sig_should_use_altstack(flags, pt->tib)) { sp = (uintptr_t)pt->tib->tib_sigstack_addr + pt->tib->tib_sigstack_size; @@ -307,7 +329,8 @@ textwindows void __sig_generate(int sig, int sic) { _pthread_lock(); for (e = dll_first(_pthread_list); e; e = dll_next(_pthread_list, e)) { pt = POSIXTHREAD_CONTAINER(e); - if (atomic_load_explicit(&pt->pt_status, memory_order_acquire) < + if (pt != _pthread_self() && + atomic_load_explicit(&pt->pt_status, memory_order_acquire) < kPosixThreadTerminated && !(pt->tib->tib_sigmask & (1ull << (sig - 1)))) { mark = pt; @@ -315,14 +338,14 @@ textwindows void __sig_generate(int sig, int sic) { } } _pthread_unlock(); - ALLOW_SIGNALS; if (mark) { STRACE("generating %G by killing %d", sig, _pthread_tid(mark)); - __sig_kill(mark, sig, sic); + __sig_killer(mark, sig, sic); } else { STRACE("all threads block %G so adding to pending signals of process", sig); __sig.pending |= 1ull << (sig - 1); } + ALLOW_SIGNALS; } static int __sig_crash_sig(struct NtExceptionPointers *ep, int *code) { diff --git a/libc/sock/winsockblock.c b/libc/sock/winsockblock.c index 04f36e9f901..9a0f18661ac 100644 --- a/libc/sock/winsockblock.c +++ b/libc/sock/winsockblock.c @@ -140,8 +140,9 @@ __winsock_block(int64_t handle, uint32_t flags, bool nonblock, if (eagained) { return eagain(); } - if (WSAGetLastError() == kNtErrorOperationAborted && _check_cancel()) { - return ecanceled(); + if (GetLastError() == kNtErrorOperationAborted) { + if (_check_cancel() == -1) return ecanceled(); + if (!eintered && _check_signal(false)) return eintr(); } if (eintered) { return eintr(); diff --git a/test/libc/calls/sig_test.c b/test/libc/calls/sig_test.c index d1a0f024122..8fe87fb5cce 100644 --- a/test/libc/calls/sig_test.c +++ b/test/libc/calls/sig_test.c @@ -19,6 +19,7 @@ #include "libc/sysv/consts/sig.h" #include "libc/atomic.h" #include "libc/calls/calls.h" +#include "libc/calls/internal.h" #include "libc/calls/struct/sigaction.h" #include "libc/dce.h" #include "libc/errno.h" @@ -26,6 +27,7 @@ #include "libc/nt/enum/context.h" #include "libc/nt/struct/context.h" #include "libc/nt/thread.h" +#include "libc/runtime/clktck.h" #include "libc/sock/struct/pollfd.h" #include "libc/sysv/consts/poll.h" #include "libc/testlib/testlib.h" @@ -64,6 +66,7 @@ TEST(SetThreadContext, test) { if (!IsWindows()) return; ASSERT_EQ(0, pthread_create(&th, 0, Worker, 0)); while (!ready) donothing; + usleep(1000); int64_t hand = _pthread_syshand((struct PosixThread *)th); ASSERT_EQ(0, SuspendThread(hand)); struct NtContext nc; @@ -101,9 +104,9 @@ TEST(poll, interrupt) { signal(SIGUSR1, OnSig); ASSERT_SYS(0, 0, pipe(pfds)); ASSERT_EQ(0, pthread_create(&th, 0, Worker2, 0)); - for (int i = 0; i < 100; ++i) { + for (int i = 0; i < 20; ++i) { ASSERT_EQ(0, pthread_kill(th, SIGUSR1)); - usleep(1000); + usleep(1e6 / CLK_TCK * 2); } isdone = true; ASSERT_EQ(0, pthread_kill(th, SIGUSR1));