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.
-- v4: ntdll/tests: Add tests for setting context on unsuspended thread. server: Store both contexts in pending context object.
From: Jacek Caban jacek@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..e4a58a0ef86 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 (ctx_count == 2 && (is_pending || 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( ctx, &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 );
From: Jacek Caban jacek@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(); }
On Thu Aug 24 20:45:55 2023 +0000, Jinoh Kang wrote:
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`.
Good catch, fixed in the new version.
On Thu Aug 24 16:28:23 2023 +0000, Jinoh Kang wrote:
Shouldn't we have already initialized `thread->context->regs[CTX_WOW]->machine` to `thread->process->machine` beforehand?
It will not be initialized for a pending context (that was not yet modified). For example, `stop_thread` earlier in this function may create such context.
I've run the tests locally without "server: Store both contexts in pending context object.", but the tests still pass. Maybe you have missed something?
On Fri Aug 25 15:02:29 2023 +0000, Jinoh Kang wrote:
I've run the tests locally without "server: Store both contexts in pending context object.", but the tests still pass. Maybe you have missed something? **EDIT**: Actually I'll try again with new-style wow64 enabled.
Yeah, the problem is specific to the new wow64 mode, here is what I get without the fix:
``` thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 thread.c:259: Test failed: IP = 004AECF0, expected 01450000 thread.c:268: Test failed: IP = 004AECF0, expected 01450004 0020:thread: 89 tests executed (0 marked as todo, 0 as flaky, 20 failures), 0 skipped. ```
This causes regression in 64-bit ntdll:exception on new-style wow64:
```diff --- OLD 2023-08-26 07:05:32.639000000 +0900 +++ NEW 2023-08-26 07:07:53.909000000 +0900 @@ -15,6 +15,16 @@ exception.c:3627: exception: c0000008 flags:0 addr:0000000000AB0010 context: Rip:0000000000AB0010 exception.c:3740: Test marked todo: Frame handler called exception.c:3742: Test marked todo: UnhandledExceptionFilter wasn't called +exception.c:8328: Test failed: expected 0x12340000, got 0 +exception.c:8329: Test failed: expected 0x12340001, got 0 +exception.c:8330: Test failed: expected 0x12340002, got 0 +exception.c:8331: Test failed: expected 0x12340003, got 0 +exception.c:8332: Test failed: expected 0x155, got 0 +exception.c:8341: Test failed: expected 0x12340000, got 0 +exception.c:8342: Test failed: expected 0x12340001, got 0 +exception.c:8343: Test failed: expected 0x12340002, got 0 +exception.c:8344: Test failed: expected 0x12340003, got 0 +exception.c:8345: Test failed: expected 0x155, got 0 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 @@ -221,9 +231,10 @@ exception.c:4467: Test failed: cs64: wrong fs 0063 / 0000 exception.c:4468: Test failed: cs64: wrong gs 002b / 0000 exception.c:4516: Test failed: cs64: ecx set to 00000000 +exception.c:4520: Test failed: cs64: rcx set to 00007FFFFE1FF820 exception.c:4388: in 32-bit mode 0023 -exception.c:4439: Test failed: cs64: ecx set to 00000003 -exception.c:4453: Test failed: cs64: rcx set to 0FEDCBA987654321 +exception.c:4439: Test failed: cs64: ecx set to 7bc6f610 +exception.c:4453: Test failed: cs64: rcx set to 000000007BC6F610 exception.c:4608: rec 000000000021F260, context 000000000021ED70. exception.c:4609: context->Rip 0xab0025, context->Rsp 0x21f488, ContextFlags 0x10005f. exception.c:4566: handler context->Rip 0xab0025, codemem 0000000000AB0000. @@ -362,4 +373,4 @@ exception.c:8746: vect. handler 80000003 addr:0000000000AB0001 018c:exception: 7 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0020:exception: Silenced 0 todos, 0 skips and 14 traces. -0020:exception: 4955 tests executed (129 marked as todo, 0 as flaky, 12 failures), 0 skipped. +0020:exception: 4931 tests executed (129 marked as todo, 0 as flaky, 23 failures), 0 skipped. ```
Jinoh Kang (@iamahuman) commented about server/thread.c:
if (thread->context->status != STATUS_PENDING)
/* 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 */ {
ctx = thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE;context = &contexts[ctx];
context_t *ctx = &thread->context->regs[CTX_NATIVE];copy_context( ctx, &contexts[CTX_NATIVE], native_flags );ctx->flags |= native_flags; }
else ctx = CTX_PENDING;
if (is_pending || !thread->context->regs[CTX_WOW].machine)
If `ctx_count == 1`, we shall still fully set the native context.
```suggestion:-0+0 if (ctx_count == 1 || is_pending || !thread->context->regs[CTX_WOW].machine) ```
But the code complexity is getting out of hand.
We're needlessly writing `regs[CTX_NATIVE]` twice even as we got rid of `CTX_PENDING` (which made it necessary in the first place). These two native context writes can be merged by tweaking the `native_flags` computation so that it takes both if condition into account.
Since the copied native context flags and wow context flags are mutually exclusive, the wow context flags computation can stay as-is.
On Fri Aug 25 22:26:34 2023 +0000, Jinoh Kang wrote:
If `ctx_count == 1`, we shall still fully set the native context.
if (ctx_count == 1 || is_pending || !thread->context->regs[CTX_WOW].machine)But the code complexity is getting out of hand. We're needlessly writing `regs[CTX_NATIVE]` twice even as we got rid of `CTX_PENDING` (which made it necessary in the first place). These two native context writes can be merged by tweaking the `native_flags` computation so that it takes both if condition into account.
Actually `ctx_count == 2` only if `thread->context->regs[CTX_WOW].machine` is set (otherwise the parameter validation fails), so you can replace the `thread->context->regs[CTX_WOW].machine` checks with `ctx_count == 2` entirely.
Something like this should be more readable:
```c if (thread->context && !get_error()) { const unsigned int native_flags = contexts[CTX_NATIVE].flags & req->native_flags; const int is_pending = thread->context->status == STATUS_PENDING; const int selected_ctx = ctx_count == 2 && thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE; unsigned int i;
for (i = 0; i < ctx_count; i++) { context_t *dest_context = &thread->context->regs[i]; unsigned int new_flags = 0;
/* some regs are always set from the native context */ if (i == CTX_NATIVE) new_flags |= native_flags;
/* 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. */ if (is_pending || i == selected_ctx) new_flags |= contexts[i].flags & ~native_flags;
if (i == CTX_WOW && !dest_context->machine) { if (!is_pending) continue; /* If pending, lazily initialize machine */ dest_context->machine = thread->process->machine; }
copy_context( dest_context, &contexts[i], new_flags ); dest_context->flags |= flags; } } ```
-
Jacek Caban -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)