[PATCH v3 0/2] MR3566: server: Store both contexts in pending context object.
And discard irrelevant parts in select request. We currently store something in CTX_PENDING and ignore it later in select request if we're currently running in wow context. -- v3: ntdll/tests: Add tests for setting context on unsuspended thread. server: Store both contexts in pending context object. https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
From: Jacek Caban <jacek(a)codeweavers.com> And discard irrelevant parts in select request. --- server/thread.c | 69 ++++++++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/server/thread.c b/server/thread.c index 3e35b27f694..c48e7176928 100644 --- a/server/thread.c +++ b/server/thread.c @@ -119,11 +119,10 @@ struct context { struct object obj; /* object header */ unsigned int status; /* status of the context */ - context_t regs[3]; /* context data */ + context_t regs[2]; /* context data */ }; #define CTX_NATIVE 0 /* context for native machine */ #define CTX_WOW 1 /* context if thread is inside WoW */ -#define CTX_PENDING 2 /* pending native context when we don't know whether thread is inside WoW */ /* flags for registers that always need to be set from the server side */ static const unsigned int system_flags = SERVER_CTX_DEBUG_REGISTERS; @@ -292,7 +291,6 @@ static struct context *create_thread_context( struct thread *thread ) context->status = STATUS_PENDING; memset( &context->regs, 0, sizeof(context->regs) ); context->regs[CTX_NATIVE].machine = native_machine; - context->regs[CTX_PENDING].machine = native_machine; return context; } @@ -1591,8 +1589,6 @@ DECL_HANDLER(select) const context_t *native_context = (const context_t *)((const char *)(result + 1) + req->size); const context_t *wow_context = (ctx_count > 1) ? native_context + 1 : NULL; - if (current->context && current->context->status != STATUS_PENDING) goto invalid_param; - if (native_context->machine == native_machine) { if (wow_context && wow_context->machine != current->process->machine) goto invalid_param; @@ -1605,7 +1601,14 @@ DECL_HANDLER(select) } else goto invalid_param; - if (!current->context && !(current->context = create_thread_context( current ))) return; + if ((ctx = current->context)) + { + if (ctx->status != STATUS_PENDING) goto invalid_param; + /* if context was modified in pending state, discard irrelevant changes */ + if (wow_context) ctx->regs[CTX_NATIVE].flags &= ~ctx->regs[CTX_WOW].flags; + else ctx->regs[CTX_WOW].flags = ctx->regs[CTX_WOW].machine = 0; + } + else if (!(current->context = create_thread_context( current ))) return; ctx = current->context; if (native_context) @@ -1618,13 +1621,6 @@ DECL_HANDLER(select) ctx->regs[CTX_WOW].machine = current->process->machine; copy_context( &ctx->regs[CTX_WOW], wow_context, wow_context->flags & ~ctx->regs[CTX_WOW].flags ); } - else if (ctx->regs[CTX_PENDING].flags) - { - unsigned int flags = ctx->regs[CTX_PENDING].flags & ~ctx->regs[CTX_NATIVE].flags; - copy_context( &ctx->regs[CTX_NATIVE], &ctx->regs[CTX_PENDING], flags ); - ctx->regs[CTX_NATIVE].flags |= flags; - } - ctx->regs[CTX_PENDING].flags = 0; ctx->status = STATUS_SUCCESS; current->suspend_cookie = req->cookie; wake_up( &ctx->obj, 0 ); @@ -1931,36 +1927,39 @@ DECL_HANDLER(set_thread_context) set_error( STATUS_INVALID_PARAMETER ); else if (thread->state != TERMINATED) { - unsigned int ctx = CTX_NATIVE; - const context_t *context = &contexts[CTX_NATIVE]; - unsigned int flags = system_flags & context->flags; - unsigned int native_flags = context->flags & req->native_flags; + unsigned int flags = system_flags & contexts[CTX_NATIVE].flags; if (thread != current) stop_thread( thread ); - else if (flags) set_thread_context( thread, context, flags ); + else if (flags) set_thread_context( thread, &contexts[CTX_NATIVE], flags ); + if (thread->context && !get_error()) { - if (ctx_count == 2) + /* If context is in a pending state, we don't know if we will use WoW or native + * context, so store both and discard irrevelant one in select request. */ + const int is_pending = thread->context->status == STATUS_PENDING; + unsigned int native_flags = contexts[CTX_NATIVE].flags & req->native_flags; + + if (native_flags) /* some regs are always set from the native context */ { - /* If the target thread doesn't have a WoW context, set native instead. - * If we don't know yet whether we have a WoW context, store native context - * in CTX_PENDING and update when the target thread sends its context(s). */ - if (thread->context->status != STATUS_PENDING) - { - ctx = thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE; - context = &contexts[ctx]; - } - else ctx = CTX_PENDING; + context_t *ctx = &thread->context->regs[CTX_NATIVE]; + copy_context( ctx, &contexts[CTX_NATIVE], native_flags ); + ctx->flags |= native_flags; + } + if (is_pending || !thread->context->regs[CTX_WOW].machine) + { + context_t *ctx = &thread->context->regs[CTX_NATIVE]; + flags = contexts[CTX_NATIVE].flags & ~native_flags; + copy_context( ctx, &contexts[CTX_NATIVE], flags ); + ctx->flags |= flags; } - flags = context->flags; - if (native_flags && ctx != CTX_NATIVE) /* some regs are always set from the native context */ + if (is_pending || (ctx_count == 2 && thread->context->regs[CTX_WOW].machine)) { - copy_context( &thread->context->regs[CTX_NATIVE], &contexts[CTX_NATIVE], native_flags ); - thread->context->regs[CTX_NATIVE].flags |= native_flags; - flags &= ~native_flags; + context_t *ctx = &thread->context->regs[CTX_WOW]; + flags = contexts[CTX_WOW].flags & ~native_flags; + ctx->machine = contexts[CTX_WOW].machine; + copy_context( &thread->context->regs[CTX_WOW], &contexts[CTX_WOW], flags ); + ctx->flags |= flags; } - copy_context( &thread->context->regs[ctx], context, flags ); - thread->context->regs[ctx].flags |= flags; } } else set_error( STATUS_UNSUCCESSFUL ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
From: Jacek Caban <jacek(a)codeweavers.com> Based on patch by Evan Tang. --- dlls/ntdll/tests/thread.c | 85 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/dlls/ntdll/tests/thread.c b/dlls/ntdll/tests/thread.c index 56fb078babc..1ec2410409a 100644 --- a/dlls/ntdll/tests/thread.c +++ b/dlls/ntdll/tests/thread.c @@ -197,6 +197,90 @@ static void test_errno(void) ok( val == 0xbeef, "wrong value %x\n", val ); } +#if defined(__i386__) +# define IP_REG(ctx) ctx.Eip +#elif defined(__x86_64__) +# define IP_REG(ctx) ctx.Rip +#endif + +#ifdef IP_REG +static volatile int exit_ip_test; +static WINAPI DWORD ip_test_thread_proc( void *param ) +{ + SetEvent( param ); + while (!exit_ip_test); + return ERROR_SUCCESS; +} +#endif + +static void test_set_thread_context_ip(void) +{ +#ifdef IP_REG + UINT_PTR old_ip, target; + HANDLE thread, event; + char *target_ptr; + CONTEXT ctx; + DWORD res; + int i; + + /* jmp to self at offset 0 and 4 */ + static const char target_code[] = {0xeb, 0xfe, 0x90, 0x90, 0xeb, 0xfe}; + + target_ptr = VirtualAlloc( NULL, 65536, MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE ); + memcpy( target_ptr, target_code, sizeof(target_code) ); + target = (UINT_PTR)target_ptr; + + event = CreateEventW( NULL, TRUE, FALSE, NULL ); + thread = CreateThread( NULL, 65536, ip_test_thread_proc, event, 0, NULL ); + ok( thread != NULL, "Failed to create thread: %lx\n", GetLastError() ); + res = WaitForSingleObject( event, 1000 ); + ok( !res, "wait returned: %ld\n", res ); + CloseHandle( event ); + + memset( &ctx, 0, sizeof(ctx) ); + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + old_ip = IP_REG(ctx); + + IP_REG(ctx) = target; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError() ); + + for (i = 0; i < 10; i++) + { + IP_REG(ctx) = target; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError() ); + + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + ok( IP_REG(ctx) == target, "IP = %p, expected %p\n", (void *)IP_REG(ctx), target_ptr ); + + IP_REG(ctx) = target + 4; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError()) ; + + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + ok( IP_REG(ctx) == target + 4, "IP = %p, expected %p\n", (void *)IP_REG(ctx), target_ptr + 4 ); + } + + exit_ip_test = 1; + ctx.ContextFlags = CONTEXT_ALL; + IP_REG(ctx) = old_ip; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to restore thread context: %lx\n", GetLastError() ); + + res = WaitForSingleObject( thread, 1000 ); + ok( !res, "wait returned: %ld\n", res ); + + VirtualFree( target_ptr, 0, MEM_RELEASE ); +#endif +} + START_TEST(thread) { init_function_pointers(); @@ -204,4 +288,5 @@ START_TEST(thread) test_dbg_hidden_thread_creation(); test_unique_teb(); test_errno(); + test_set_thread_context_ip(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
On Tue Aug 22 16:24:17 2023 +0000, Jacek Caban wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/3566/diffs?diff_id=64126&start_sha=2f71153b8e209aaccd00f8e155342e6454f2d0cb#e54a93e094a94cfc85d5c061decb6b27dbd311f1_1935_1933) Good catch, thanks. It should be fixed in the new version. I also removed a duplicated PENDING check.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43050
Jinoh Kang (@iamahuman) commented about server/thread.c:
+ context_t *ctx = &thread->context->regs[CTX_NATIVE]; + flags = contexts[CTX_NATIVE].flags & ~native_flags; + copy_context( ctx, &contexts[CTX_NATIVE], flags ); + ctx->flags |= flags; } - flags = context->flags; - if (native_flags && ctx != CTX_NATIVE) /* some regs are always set from the native context */ + if (is_pending || (ctx_count == 2 && thread->context->regs[CTX_WOW].machine)) { - copy_context( &thread->context->regs[CTX_NATIVE], &contexts[CTX_NATIVE], native_flags ); - thread->context->regs[CTX_NATIVE].flags |= native_flags; - flags &= ~native_flags; + context_t *ctx = &thread->context->regs[CTX_WOW]; + flags = contexts[CTX_WOW].flags & ~native_flags; + ctx->machine = contexts[CTX_WOW].machine; + copy_context( &thread->context->regs[CTX_WOW], &contexts[CTX_WOW], flags ); This does not use the `ctx` alias, inconsistent with the native case above.
copy_context( ctx, &contexts[CTX_WOW], flags );
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43294
Jinoh Kang (@iamahuman) commented about server/thread.c:
+ if (is_pending || !thread->context->regs[CTX_WOW].machine) + { + context_t *ctx = &thread->context->regs[CTX_NATIVE]; + flags = contexts[CTX_NATIVE].flags & ~native_flags; + copy_context( ctx, &contexts[CTX_NATIVE], flags ); + ctx->flags |= flags; } - flags = context->flags; - if (native_flags && ctx != CTX_NATIVE) /* some regs are always set from the native context */ + if (is_pending || (ctx_count == 2 && thread->context->regs[CTX_WOW].machine)) { - copy_context( &thread->context->regs[CTX_NATIVE], &contexts[CTX_NATIVE], native_flags ); - thread->context->regs[CTX_NATIVE].flags |= native_flags; - flags &= ~native_flags; + context_t *ctx = &thread->context->regs[CTX_WOW]; + flags = contexts[CTX_WOW].flags & ~native_flags; If `ctx_count == 1`, then `contexts[CTX_WOW]` does not exist.
This happens when a 64-bit (native) process calls `NtSetContextThread` on a 32-bit (WoW64) thread whose context is still pending. In this case, `is_pending` will be true but `ctx_count == 1`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43295
Jinoh Kang (@iamahuman) commented about server/thread.c:
+ { + context_t *ctx = &thread->context->regs[CTX_NATIVE]; + flags = contexts[CTX_NATIVE].flags & ~native_flags; + copy_context( ctx, &contexts[CTX_NATIVE], flags ); + ctx->flags |= flags; } - flags = context->flags; - if (native_flags && ctx != CTX_NATIVE) /* some regs are always set from the native context */ + if (is_pending || (ctx_count == 2 && thread->context->regs[CTX_WOW].machine)) { - copy_context( &thread->context->regs[CTX_NATIVE], &contexts[CTX_NATIVE], native_flags ); - thread->context->regs[CTX_NATIVE].flags |= native_flags; - flags &= ~native_flags; + context_t *ctx = &thread->context->regs[CTX_WOW]; + flags = contexts[CTX_WOW].flags & ~native_flags; + ctx->machine = contexts[CTX_WOW].machine; Shouldn't we have already initialized `thread->context->regs[CTX_WOW]->machine` to `thread->process->machine` beforehand?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43297
participants (3)
-
Jacek Caban -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)