[PATCH v8 0/4] MR10232: ntdll: Avoid exposing non-user rsp in normal syscall return path
When rsp is outside the kernel stack, the SIGUSR1 handler considers the context "out of syscall" and reports the real register values to wineserver. rsp pointing into the syscall frame is used for iret returns. Commit 0a5f7a710365 moved this earlier in the return path, creating a window in the regular direct return path where rsp points neither to the kernel stack, nor to the expected user stack. When a thread is interrupted by SIGUSR1 within this window, this exposes a bogus stack pointer to GetThreadContext(), breaking the garbage collector in Unity applications. Fix the regression by first switching to the real user stack (needed so we don't break things by touching other registers), then, only if an iret path is coming up, switching to the syscall frame pointer. This fixes the regression introduced by 0a5f7a710365, since GetThreadContext() will now observe the proper stack pointer during regular syscall returns. Further work would be needed to solve other corner cases and ensure that the register context returned by GetThreadContext() behaves exactly as on Windows (with no kernel-mode context info ever returned), but that is a larger/more invasive change. Fixes: 0a5f7a710365 ("ntdll: Switch to the user stack before restoring the %fs register.") Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59333 Signed-off-by: Hoshino Lina lina@lina.yt -- v8: gitlab: Run ntdll tests with win64 architecture. ntdll: Abort wow64 tests early if wow64 is unavailable ntdll/tests: Add a test for bogus SP values from GetThreadContext(). ntdll: Avoid exposing non-user rsp in normal syscall return path https://gitlab.winehq.org/wine/wine/-/merge_requests/10232
From: Hoshino Lina <lina@lina.yt> When rsp is outside the kernel stack, the SIGUSR1 handler considers the context "out of syscall" and reports the real register values to wineserver. rsp pointing into the syscall frame is used for iret returns. Commit 0a5f7a710365 moved this earlier in the return path, creating a window in the regular direct return path where rsp points neither to the kernel stack, nor to the expected user stack. When a thread is interrupted by SIGUSR1 within this window, this exposes a bogus stack pointer to GetThreadContext(), breaking the garbage collector in Unity applications. Fix the regression by first switching to the real user stack (needed so we don't break things by touching other registers), and only switching to the syscall frame pointer in the iret paths that require it. This fixes the regression introduced by 0a5f7a710365, since GetThreadContext() will now observe the proper stack pointer during regular syscall returns. Further work would be needed to solve other corner cases and ensure that the register context returned by GetThreadContext() behaves exactly as on Windows (with no kernel-mode context info ever returned), but that is a larger/more invasive change. Suggested by: Etaash Mathamsetty <etaash.mathamsetty@gmail.com> Fixes: 0a5f7a710365 ("ntdll: Switch to the user stack before restoring the %fs register.") Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59333 Signed-off-by: Hoshino Lina <lina@lina.yt> --- dlls/ntdll/unix/signal_x86_64.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 0c62e8a1ad4..dd3ac47bf81 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -3172,7 +3172,8 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, /* push rbp-based kernel stack cfi */ __ASM_CFI(".cfi_remember_state\n\t") __ASM_CFI_CFA_IS_AT2(rcx, 0xa8, 0x01) /* frame->syscall_cfa */ - "leaq 0x70(%rcx),%rsp\n\t" /* %rsp > frame means no longer inside syscall */ + /* switch to user stack */ + "movq 0x88(%rcx),%rsp\n\t" #ifdef __linux__ "movw 0x338(%r13),%dx\n" /* amd64_thread_data()->fs */ "testw %dx,%dx\n\t" @@ -3228,8 +3229,6 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "3:\ttestl $0x3,%edx\n\t" /* CONTEXT_CONTROL | CONTEXT_INTEGER */ "jnz 1f\n\t" - /* switch to user stack */ - "movq 0x88(%rcx),%rsp\n\t" /* push rcx-based kernel stack cfi */ __ASM_CFI(".cfi_remember_state\n\t") __ASM_CFI(".cfi_def_cfa %rsp, 0\n\t") @@ -3254,7 +3253,8 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, /* pop rcx-based kernel stack cfi */ __ASM_CFI(".cfi_restore_state\n") - "1:\ttestl $0x2,%edx\n\t" /* CONTEXT_INTEGER */ + "1:\tleaq 0x70(%rcx),%rsp\n\t" /* %rsp > frame means no longer inside syscall */ + "testl $0x2,%edx\n\t" /* CONTEXT_INTEGER */ "jnz 1f\n\t" /* CONTEXT_CONTROL */ "movq (%rsp),%rcx\n\t" /* frame->rip */ @@ -3280,6 +3280,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "movq (%r10),%r10\n\t" "test %r10,%r10\n\t" "jz 3b\n\t" + "leaq 0x70(%rcx),%rsp\n\t" /* %rsp > frame means no longer inside syscall */ "testl $0x2,%edx\n\t" /* CONTEXT_INTEGER */ "jnz 1b\n\t" "xchgq %r10,(%rsp)\n\t" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10232
From: Hoshino Lina <lina@lina.yt> --- dlls/ntdll/tests/Makefile.in | 1 + dlls/ntdll/tests/threadctx.c | 148 +++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 dlls/ntdll/tests/threadctx.c diff --git a/dlls/ntdll/tests/Makefile.in b/dlls/ntdll/tests/Makefile.in index 42d663777ea..177b9c7e35c 100644 --- a/dlls/ntdll/tests/Makefile.in +++ b/dlls/ntdll/tests/Makefile.in @@ -25,6 +25,7 @@ SOURCES = \ testdll.c \ testdll.spec \ thread.c \ + threadctx.c \ threadpool.c \ time.c \ unwind.c \ diff --git a/dlls/ntdll/tests/threadctx.c b/dlls/ntdll/tests/threadctx.c new file mode 100644 index 00000000000..1d738a0a717 --- /dev/null +++ b/dlls/ntdll/tests/threadctx.c @@ -0,0 +1,148 @@ +/* + * Unit test suite for ntdll thread context behavior + * + * Copyright 2026 Hoshino Lina + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + * + */ + +#include <stdarg.h> +#include <stdbool.h> + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" +#include "wine/test.h" + +// 110 second max runtime, to avoid winetest timeouts +#define MAX_RUNTIME 110 +#define THREADS 50 +#define STACK_SIZE 0x10000 +#define LOOPS 2000 + +#ifdef __x86_64__ +#define CTX_SP Rsp +#define CTX_IP Rip +#endif +#ifdef __i386__ +#define CTX_SP Esp +#define CTX_IP Eip +#endif +#ifdef __arm__ +#define CTX_SP Sp +#define CTX_IP Pc +#endif +#ifdef __aarch64__ +#define CTX_SP Sp +#define CTX_IP Pc +#endif + +static volatile bool looping = true; + +struct thread { + DWORD tid; + bool stopped; + HANDLE hnd; + CONTEXT ctx; + DWORD64 stack_lo; + DWORD64 stack_hi; +}; + +struct thread t[THREADS]; + +DWORD WINAPI thread_func(LPVOID lpParameter) +{ + struct thread *self = lpParameter; + + ULONG_PTR lo, hi; + GetCurrentThreadStackLimits(&lo, &hi); + + self->stack_lo = (DWORD64)lo; + self->stack_hi = (DWORD64)hi; + + while (looping) + Sleep(0); + + return 0; +} + +static void test_context_sp(void) +{ + int loop; + bool failed = false; + DWORD timeout = GetTickCount() + MAX_RUNTIME * 1000; + + for (int i = 0; i < THREADS; i++) { + t[i].hnd = CreateThread(0, STACK_SIZE, thread_func, (LPVOID)&t[i], 0, + &t[i].tid); + ok(!!t[i].hnd, "Failed to create thread"); + if (!t[i].hnd) { + looping = false; + failed = true; + break; + } + } + + Sleep(100); + + trace("Starting %d loops of thread context fetching\n", LOOPS); + + for (loop = 0; !failed && loop < LOOPS && GetTickCount() < timeout; loop++) { + for (int i = 0; i < THREADS; i++) { + if (SuspendThread(t[i].hnd) != (DWORD)-1) { + t[i].ctx.ContextFlags = CONTEXT_INTEGER | CONTEXT_CONTROL; + + if (GetThreadContext(t[i].hnd, &t[i].ctx)) { + bool in_range = t[i].ctx.CTX_SP > t[i].stack_lo && + t[i].ctx.CTX_SP <= t[i].stack_hi; + ok(in_range, + "[%d/%d] SP=0x%llx [%llx..%llx] " + "RIP=0x%llx", + loop, LOOPS, (long long)t[i].ctx.CTX_SP, + (long long)t[i].stack_lo, (long long)t[i].stack_hi, + (long long)t[i].ctx.CTX_IP); + + if (!in_range) + failed = true; + + t[i].stopped = true; + } else { + ResumeThread(t[i].hnd); + } + } + } + + for (int i = 0; i < THREADS; i++) { + if (t[i].stopped) + ResumeThread(t[i].hnd); + + t[i].stopped = false; + } + } + + trace("Completed %d/%d loops\n", loop, LOOPS); + + looping = false; + + for (int i = 0; i < THREADS; i++) + WaitForSingleObject(t[i].hnd, INFINITE); +} + +START_TEST(threadctx) { + test_context_sp(); +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10232
From: Hoshino Lina <lina@lina.yt> If the tests are run with a pure wine64 build, then CreateProcessA will fail. Just stop the test early instead of carrying on failing every single check after that. --- dlls/ntdll/tests/exception.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index dfc1581492e..4c7543ae691 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -4579,6 +4579,8 @@ static void test_wow64_context(void) sprintf( cmdline, "\"%s\" /c for /l %%n in () do @echo >nul", appname ); r = CreateProcessA( appname, cmdline, NULL, NULL, FALSE, CREATE_SUSPENDED, NULL, NULL, &si, &pi); ok( r, "failed to start %s err %lu\n", appname, GetLastError() ); + if (!r) + return; ret = pRtlWow64GetThreadContext( pi.hThread, &ctx ); ok(ret == STATUS_SUCCESS, "got %#lx\n", ret); @@ -9100,6 +9102,8 @@ static void test_debug_registers_wow64(void) si.cb = sizeof(si); bret = CreateProcessA(cmdline, NULL, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); ok(bret, "CreateProcessA failed\n"); + if (!bret) + return; bret = pIsWow64Process(pi.hProcess, &is_wow64); ok(bret && is_wow64, "expected Wow64 process\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10232
From: Hoshino Lina <lina@lina.yt> --- tools/gitlab/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/gitlab/test.yml b/tools/gitlab/test.yml index db77ad5111d..70d5e0afbbb 100644 --- a/tools/gitlab/test.yml +++ b/tools/gitlab/test.yml @@ -62,7 +62,7 @@ test-linux-64: extends: .wine-test variables: - INCLUDE_TESTS: "dinput ntoskrnl.exe" + INCLUDE_TESTS: "dinput ntdll ntoskrnl.exe" rules: - if: $CI_PIPELINE_SOURCE == 'merge_request_event' needs: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10232
@etaash.mathamsetty That's what I was going to try next and indeed that fixes the regression and works better. I've pushed this for now and I'll unmark as draft because I think this is an improvement (and it no longer regresses other ntdll tests). I credited you as `Suggested by`, I hope that's ok! I've also added another commit to stop some ntdll:exception test cases from breaking really badly on a non-wow64 (pure 64bit) build of Wine. They still fail but this makes them not hang/crash/cause a stream of failures. That's mostly just to make local testing with a 64bit build easier when other tests are the target. I'm going to try to implement a more robust fix using the "deferred signal" approach next, but perhaps this is worth merging in the interim since it's less intrusive and it fixes a real regression? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10232#note_133206
Second attempt in !10419. That one defers SIGUSR1 during the syscall entry path to after the context is saved. After that point SIGUSR1 is processed immediately, but also, if the interruption happened during the syscall exit path, the context restore is restarted (which allows any changes to the context to happen correctly). This does not fix the broader issue of other signals during kernel-mode code though. The reason why I'm processing the SIGUSR1 immediately is that I assume syscalls can block, and blocking SIGUSR1 possibly indefinitely is presumably a bad idea. So this MR strictly attempts to fix the "broken contexts" issue, not other bugs. But as far as I can tell, it should be a comprehensive fix for this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10232#note_133212
participants (2)
-
Hoshino Lina -
Hoshino Lina (＠hoshinolina)