[PATCH v3 0/4] MR1088: ntdll: Avoid calling pthread_exit on thread exit.
Using a dedicated exit jmpbuf and removing the need for assembly routines. When Wine handles an exception in unix code, we return to user mode by jumping to the last syscall frame. This can leave some pthread cancel cleanups registered, in the pthread internal linked list, and at the same time later overwrite the stack frame they were registered for. In the same way, jumping to the exit frame on thread exit or abort, can also leave some cleanup handlers registered for invalid stack frames. Depending on the implementation, calling pthread_exit will cause all the registered pthread cleanup handlers to be called, possibly jumping back to now overwritten stack frames and causing segmentation faults. Exiting a pthread normally, by returning from its procedure, or calling exit(0) for the main thread doesn't run pthread_exit and doesn't call cleanup handlers, avoiding that situation. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 ### Additional note: For robustness, we should probably try to execute these cleanup handlers when unwinding the stack frames, as we would otherwise leave pthread objects in a potential problematic state (like a mutex locked, etc). It is however hard to do so when the handlers are registered from some C code: pthread C implementation is done by calling some internal pthread functions to register the handlers, and they aren't registered as standard unwind handlers. Only pthread_cancel and pthread_exit can unwind and call / unregister the C handlers, but interrupting that procedure, for instance calling setjmp / longjmp from withing our own handler isn't supported.
From C++ code, pthread cleanup handlers are registered through C++ class constructors / destructors, and it would then be possible to partially unwind and call them at the same time.
-- v3: ntdll: Remove unnecessary signal_start_thread register spilling. ntdll: Remove unnecessary arch specific exit frame pointer. ntdll: Avoid calling pthread_exit on thread exit. ntdll: Create a pthread for the main thread. https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
From: Rémi Bernon <rbernon(a)codeweavers.com> So that we can safely call pthread_exit from the unix main thread, and wait for all other threads to exit. The last win32 thread will actually terminate the process directly by calling _exit. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 --- dlls/ntdll/unix/loader.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c index c21d32ea811..f3daa14f7f0 100644 --- a/dlls/ntdll/unix/loader.c +++ b/dlls/ntdll/unix/loader.c @@ -2148,10 +2148,7 @@ static struct unix_funcs unix_funcs = }; -/*********************************************************************** - * start_main_thread - */ -static void start_main_thread(void) +static void *main_thread( void *arg ) { SYSTEM_SERVICE_TABLE syscall_table = { (ULONG_PTR *)syscalls, NULL, ARRAY_SIZE(syscalls), syscall_args }; NTSTATUS status; @@ -2185,6 +2182,20 @@ static void start_main_thread(void) NtTerminateProcess( GetCurrentProcess(), status ); } server_init_process_done(); + return 0; +} + +/*********************************************************************** + * start_main_thread + */ +static void start_main_thread(void) +{ + pthread_t thread; + void *ret; + + pthread_create( &thread, NULL, main_thread, NULL ); + pthread_join( thread, &ret ); + pthread_exit( ret ); } #ifdef __ANDROID__ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
From: Rémi Bernon <rbernon(a)codeweavers.com> Using a dedicated exit jmpbuf and removing the need for assembly routines. When Wine handles an exception in unix code, we return to user mode by jumping to the last syscall frame. This can leave some pthread cancel cleanups registered, in the pthread internal linked list, and at the same time later overwrite the stack frame they were registered for. In the same way, jumping to the exit frame on thread exit or abort, can also leave some cleanup handlers registered for invalid stack frames. Depending on the implementation, calling pthread_exit will cause all the registered pthread cleanup handlers to be called, possibly jumping back to now overwritten stack frames and causing segmentation faults. Exiting a pthread normally, by returning from its procedure doesn't run pthread_exit and doesn't call cleanup handlers, avoiding that situation. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 --- dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/signal_arm.c | 13 ------------ dlls/ntdll/unix/signal_arm64.c | 12 ----------- dlls/ntdll/unix/signal_i386.c | 25 ----------------------- dlls/ntdll/unix/signal_x86_64.c | 21 -------------------- dlls/ntdll/unix/thread.c | 35 +++++++++++++++++++++++++-------- dlls/ntdll/unix/unix_private.h | 4 +++- 7 files changed, 31 insertions(+), 81 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 77e8d5c7566..98afc42ad27 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -1581,7 +1581,7 @@ void server_init_process_done(void) SERVER_END_REQ; assert( !status ); - signal_start_thread( entry, peb, suspend, NtCurrentTeb() ); + thread_start( entry, peb, suspend, NtCurrentTeb() ); } diff --git a/dlls/ntdll/unix/signal_arm.c b/dlls/ntdll/unix/signal_arm.c index 5d1478a1ff4..ab00a67d436 100644 --- a/dlls/ntdll/unix/signal_arm.c +++ b/dlls/ntdll/unix/signal_arm.c @@ -1183,19 +1183,6 @@ __ASM_GLOBAL_FUNC( signal_start_thread, "bl " __ASM_NAME("call_init_thunk") ) -/*********************************************************************** - * signal_exit_thread - */ -__ASM_GLOBAL_FUNC( signal_exit_thread, - "ldr r3, [r2, #0x1d4]\n\t" /* arm_thread_data()->exit_frame */ - "mov ip, #0\n\t" - "str ip, [r2, #0x1d4]\n\t" - "cmp r3, ip\n\t" - "it ne\n\t" - "movne sp, r3\n\t" - "blx r1" ) - - /*********************************************************************** * __wine_syscall_dispatcher */ diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index 4e552e0f10a..b24dbc2d8a4 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -1253,18 +1253,6 @@ __ASM_GLOBAL_FUNC( signal_start_thread, "1:\tmov sp, x8\n\t" "bl " __ASM_NAME("call_init_thunk") ) -/*********************************************************************** - * signal_exit_thread - */ -__ASM_GLOBAL_FUNC( signal_exit_thread, - "stp x29, x30, [sp,#-16]!\n\t" - "ldr x3, [x2, #0x2f0]\n\t" /* arm64_thread_data()->exit_frame */ - "str xzr, [x2, #0x2f0]\n\t" - "cbz x3, 1f\n\t" - "mov sp, x3\n" - "1:\tldp x29, x30, [sp], #16\n\t" - "br x1" ) - /*********************************************************************** * __wine_syscall_dispatcher diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index 2dfce706394..0eecb5fb57e 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -2468,31 +2468,6 @@ __ASM_GLOBAL_FUNC( signal_start_thread, "call " __ASM_NAME("call_init_thunk") ) -/*********************************************************************** - * signal_exit_thread - */ -__ASM_GLOBAL_FUNC( signal_exit_thread, - "movl 8(%esp),%ecx\n\t" - "movl 12(%esp),%esi\n\t" - "xorl %edx,%edx\n\t" - /* fetch exit frame */ - "xchgl %edx,0x1f4(%esi)\n\t" /* x86_thread_data()->exit_frame */ - "testl %edx,%edx\n\t" - "jnz 1f\n\t" - "jmp *%ecx\n\t" - /* switch to exit frame stack */ - "1:\tmovl 4(%esp),%eax\n\t" - "movl %edx,%ebp\n\t" - __ASM_CFI(".cfi_def_cfa %ebp,4\n\t") - __ASM_CFI(".cfi_rel_offset %ebp,0\n\t") - __ASM_CFI(".cfi_rel_offset %ebx,-4\n\t") - __ASM_CFI(".cfi_rel_offset %esi,-8\n\t") - __ASM_CFI(".cfi_rel_offset %edi,-12\n\t") - "leal -20(%ebp),%esp\n\t" - "pushl %eax\n\t" - "call *%ecx" ) - - /*********************************************************************** * __wine_syscall_dispatcher */ diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 88c517daecf..fc73925e819 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -3324,27 +3324,6 @@ __ASM_GLOBAL_FUNC( signal_start_thread, "call " __ASM_NAME("call_init_thunk")) -/*********************************************************************** - * signal_exit_thread - */ -__ASM_GLOBAL_FUNC( signal_exit_thread, - /* fetch exit frame */ - "xorl %ecx,%ecx\n\t" - "xchgq %rcx,0x320(%rdx)\n\t" /* amd64_thread_data()->exit_frame */ - "testq %rcx,%rcx\n\t" - "jnz 1f\n\t" - "jmp *%rsi\n" - /* switch to exit frame stack */ - "1:\tmovq %rcx,%rsp\n\t" - __ASM_CFI(".cfi_adjust_cfa_offset 56\n\t") - __ASM_CFI(".cfi_rel_offset %rbp,48\n\t") - __ASM_CFI(".cfi_rel_offset %rbx,40\n\t") - __ASM_CFI(".cfi_rel_offset %r12,32\n\t") - __ASM_CFI(".cfi_rel_offset %r13,24\n\t") - __ASM_CFI(".cfi_rel_offset %r14,16\n\t") - __ASM_CFI(".cfi_rel_offset %r15,8\n\t") - "call *%rsi" ) - /*********************************************************************** * __wine_syscall_dispatcher */ diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index bb8c1c2936b..ff994ea1503 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -1050,24 +1050,43 @@ static void pthread_exit_wrapper( int status ) close( ntdll_get_thread_data()->wait_fd[1] ); close( ntdll_get_thread_data()->reply_fd ); close( ntdll_get_thread_data()->request_fd ); - pthread_exit( UIntToPtr(status) ); +} + +void DECLSPEC_NORETURN thread_exit( int status, void (*func)(int), TEB *teb ) +{ + struct ntdll_thread_data *thread_data = (struct ntdll_thread_data *)&teb->GdiTebBatch; + + func(status); + __wine_longjmp( thread_data->exit_buf, 1 ); } /*********************************************************************** - * start_thread + * pthread_start_wrapper * * Startup routine for a newly created thread. */ -static void start_thread( TEB *teb ) +static void *pthread_start_wrapper( void *arg ) { + TEB *teb = arg; struct ntdll_thread_data *thread_data = (struct ntdll_thread_data *)&teb->GdiTebBatch; BOOL suspend; thread_data->pthread_id = pthread_self(); signal_init_thread( teb ); server_init_thread( thread_data->start, &suspend ); - signal_start_thread( thread_data->start, thread_data->param, suspend, teb ); + thread_start( thread_data->start, thread_data->param, suspend, teb ); + return NULL; +} + +void thread_start( PRTL_THREAD_START_ROUTINE entry, void *arg, + BOOL suspend, TEB *teb ) +{ + struct ntdll_thread_data *thread_data = (struct ntdll_thread_data *)&teb->GdiTebBatch; + __wine_jmp_buf exit_buf; + + if (!__wine_setjmpex( (thread_data->exit_buf = &exit_buf), NULL )) + signal_start_thread( entry, arg, suspend, teb ); } @@ -1345,7 +1364,7 @@ NTSTATUS WINAPI NtCreateThreadEx( HANDLE *handle, ACCESS_MASK access, OBJECT_ATT pthread_attr_setguardsize( &pthread_attr, 0 ); pthread_attr_setscope( &pthread_attr, PTHREAD_SCOPE_SYSTEM ); /* force creating a kernel thread */ InterlockedIncrement( &nb_threads ); - if (pthread_create( &pthread_id, &pthread_attr, (void * (*)(void *))start_thread, teb )) + if (pthread_create( &pthread_id, &pthread_attr, pthread_start_wrapper, teb )) { InterlockedDecrement( &nb_threads ); virtual_free_teb( teb ); @@ -1373,7 +1392,7 @@ void abort_thread( int status ) { pthread_sigmask( SIG_BLOCK, &server_block_set, NULL ); if (InterlockedDecrement( &nb_threads ) <= 0) abort_process( status ); - signal_exit_thread( status, pthread_exit_wrapper, NtCurrentTeb() ); + thread_exit( status, pthread_exit_wrapper, NtCurrentTeb() ); } @@ -1406,7 +1425,7 @@ static DECLSPEC_NORETURN void exit_thread( int status ) virtual_free_teb( teb ); } } - signal_exit_thread( status, pthread_exit_wrapper, NtCurrentTeb() ); + thread_exit( status, pthread_exit_wrapper, NtCurrentTeb() ); } @@ -1416,7 +1435,7 @@ static DECLSPEC_NORETURN void exit_thread( int status ) void exit_process( int status ) { pthread_sigmask( SIG_BLOCK, &server_block_set, NULL ); - signal_exit_thread( get_unix_exit_code( status ), process_exit_wrapper, NtCurrentTeb() ); + thread_exit( get_unix_exit_code( status ), process_exit_wrapper, NtCurrentTeb() ); } diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 47f0f9c56a9..77d5feb7661 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -61,6 +61,7 @@ struct ntdll_thread_data PRTL_THREAD_START_ROUTINE start; /* thread entry point */ void *param; /* thread entry point parameter */ void *jmp_buf; /* setjmp buffer for exception handling */ + void *exit_buf; /* setjmp buffer for thread exit */ }; C_ASSERT( sizeof(struct ntdll_thread_data) <= sizeof(((TEB *)0)->GdiTebBatch) ); @@ -237,7 +238,8 @@ extern void signal_init_thread( TEB *teb ) DECLSPEC_HIDDEN; extern void signal_init_process(void) DECLSPEC_HIDDEN; extern void DECLSPEC_NORETURN signal_start_thread( PRTL_THREAD_START_ROUTINE entry, void *arg, BOOL suspend, TEB *teb ) DECLSPEC_HIDDEN; -extern void DECLSPEC_NORETURN signal_exit_thread( int status, void (*func)(int), TEB *teb ) DECLSPEC_HIDDEN; +extern void thread_start( PRTL_THREAD_START_ROUTINE entry, void *arg, + BOOL suspend, TEB *teb ) DECLSPEC_HIDDEN; extern SYSTEM_SERVICE_TABLE KeServiceDescriptorTable[4] DECLSPEC_HIDDEN; extern void __wine_syscall_dispatcher(void) DECLSPEC_HIDDEN; extern void WINAPI DECLSPEC_NORETURN __wine_syscall_dispatcher_return( void *frame, ULONG_PTR retval ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 --- dlls/ntdll/unix/signal_arm.c | 14 +++++--------- dlls/ntdll/unix/signal_arm64.c | 15 +++++---------- dlls/ntdll/unix/signal_i386.c | 14 +++++--------- dlls/ntdll/unix/signal_x86_64.c | 20 ++++++++------------ 4 files changed, 23 insertions(+), 40 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm.c b/dlls/ntdll/unix/signal_arm.c index ab00a67d436..d5905622d1c 100644 --- a/dlls/ntdll/unix/signal_arm.c +++ b/dlls/ntdll/unix/signal_arm.c @@ -202,13 +202,11 @@ C_ASSERT( sizeof( struct syscall_frame ) == 0x160); struct arm_thread_data { - void *exit_frame; /* 1d4 exit frame pointer */ - struct syscall_frame *syscall_frame; /* 1d8 frame pointer on syscall entry */ + struct syscall_frame *syscall_frame; /* 1d4 frame pointer on syscall entry */ }; C_ASSERT( sizeof(struct arm_thread_data) <= sizeof(((struct ntdll_thread_data *)0)->cpu_data) ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm_thread_data, exit_frame ) == 0x1d4 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm_thread_data, syscall_frame ) == 0x1d8 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm_thread_data, syscall_frame ) == 0x1d4 ); static inline struct arm_thread_data *arm_thread_data(void) { @@ -1172,13 +1170,11 @@ void DECLSPEC_HIDDEN call_init_thunk( LPTHREAD_START_ROUTINE entry, void *arg, B */ __ASM_GLOBAL_FUNC( signal_start_thread, "push {r4-r12,lr}\n\t" - /* store exit frame */ - "str sp, [r3, #0x1d4]\n\t" /* arm_thread_data()->exit_frame */ /* set syscall frame */ - "ldr r6, [r3, #0x1d8]\n\t" /* arm_thread_data()->syscall_frame */ + "ldr r6, [r3, #0x1d4]\n\t" /* arm_thread_data()->syscall_frame */ "cbnz r6, 1f\n\t" "sub r6, sp, #0x160\n\t" /* sizeof(struct syscall_frame) */ - "str r6, [r3, #0x1d8]\n\t" /* arm_thread_data()->syscall_frame */ + "str r6, [r3, #0x1d4]\n\t" /* arm_thread_data()->syscall_frame */ "1:\tmov sp, r6\n\t" "bl " __ASM_NAME("call_init_thunk") ) @@ -1188,7 +1184,7 @@ __ASM_GLOBAL_FUNC( signal_start_thread, */ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "mrc p15, 0, r1, c13, c0, 2\n\t" /* NtCurrentTeb() */ - "ldr r1, [r1, #0x1d8]\n\t" /* arm_thread_data()->syscall_frame */ + "ldr r1, [r1, #0x1d4]\n\t" /* arm_thread_data()->syscall_frame */ "add r0, r1, #0x10\n\t" "stm r0, {r4-r12,lr}\n\t" "add r2, sp, #0x10\n\t" diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index b24dbc2d8a4..a9c3a1b3844 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -150,13 +150,11 @@ C_ASSERT( sizeof( struct syscall_frame ) == 0x330 ); struct arm64_thread_data { - void *exit_frame; /* 02f0 exit frame pointer */ - struct syscall_frame *syscall_frame; /* 02f8 frame pointer on syscall entry */ + struct syscall_frame *syscall_frame; /* 02f0 frame pointer on syscall entry */ }; C_ASSERT( sizeof(struct arm64_thread_data) <= sizeof(((struct ntdll_thread_data *)0)->cpu_data) ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm64_thread_data, exit_frame ) == 0x2f0 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm64_thread_data, syscall_frame ) == 0x2f8 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct arm64_thread_data, syscall_frame ) == 0x2f0 ); static inline struct arm64_thread_data *arm64_thread_data(void) { @@ -1242,14 +1240,11 @@ void DECLSPEC_HIDDEN call_init_thunk( LPTHREAD_START_ROUTINE entry, void *arg, B */ __ASM_GLOBAL_FUNC( signal_start_thread, "stp x29, x30, [sp,#-16]!\n\t" - /* store exit frame */ - "mov x29, sp\n\t" - "str x29, [x3, #0x2f0]\n\t" /* arm64_thread_data()->exit_frame */ /* set syscall frame */ - "ldr x8, [x3, #0x2f8]\n\t" /* arm64_thread_data()->syscall_frame */ + "ldr x8, [x3, #0x2f0]\n\t" /* arm64_thread_data()->syscall_frame */ "cbnz x8, 1f\n\t" "sub x8, sp, #0x330\n\t" /* sizeof(struct syscall_frame) */ - "str x8, [x3, #0x2f8]\n\t" /* arm64_thread_data()->syscall_frame */ + "str x8, [x3, #0x2f0]\n\t" /* arm64_thread_data()->syscall_frame */ "1:\tmov sp, x8\n\t" "bl " __ASM_NAME("call_init_thunk") ) @@ -1274,7 +1269,7 @@ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "ldr x30, [sp, #80]\n\t" "ldp x0, x1, [sp], #96\n\t" - "ldr x10, [x18, #0x2f8]\n\t" /* arm64_thread_data()->syscall_frame */ + "ldr x10, [x18, #0x2f0]\n\t" /* arm64_thread_data()->syscall_frame */ "stp x18, x19, [x10, #0x90]\n\t" "stp x20, x21, [x10, #0xa0]\n\t" "stp x22, x23, [x10, #0xb0]\n\t" diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index 0eecb5fb57e..c9e2684bde6 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -477,14 +477,12 @@ struct x86_thread_data DWORD dr3; /* 1e8 */ DWORD dr6; /* 1ec */ DWORD dr7; /* 1f0 */ - void *exit_frame; /* 1f4 exit frame pointer */ - struct syscall_frame *syscall_frame; /* 1f8 frame pointer on syscall entry */ + struct syscall_frame *syscall_frame; /* 1f4 frame pointer on syscall entry */ }; C_ASSERT( sizeof(struct x86_thread_data) <= sizeof(((struct ntdll_thread_data *)0)->cpu_data) ); C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct x86_thread_data, gs ) == 0x1d8 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct x86_thread_data, exit_frame ) == 0x1f4 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct x86_thread_data, syscall_frame ) == 0x1f8 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct x86_thread_data, syscall_frame ) == 0x1f4 ); /* flags to control the behavior of the syscall dispatcher */ #define SYSCALL_HAVE_XSAVE 1 @@ -2450,16 +2448,14 @@ __ASM_GLOBAL_FUNC( signal_start_thread, __ASM_CFI(".cfi_rel_offset %esi,-8\n\t") "pushl %edi\n\t" __ASM_CFI(".cfi_rel_offset %edi,-12\n\t") - /* store exit frame */ "movl 20(%ebp),%ecx\n\t" /* teb */ - "movl %ebp,0x1f4(%ecx)\n\t" /* x86_thread_data()->exit_frame */ /* set syscall frame */ - "movl 0x1f8(%ecx),%eax\n\t" /* x86_thread_data()->syscall_frame */ + "movl 0x1f4(%ecx),%eax\n\t" /* x86_thread_data()->syscall_frame */ "orl %eax,%eax\n\t" "jnz 1f\n\t" "leal -0x380(%esp),%eax\n\t" /* sizeof(struct syscall_frame) */ "andl $~63,%eax\n\t" - "movl %eax,0x1f8(%ecx)\n" /* x86_thread_data()->syscall_frame */ + "movl %eax,0x1f4(%ecx)\n" /* x86_thread_data()->syscall_frame */ "1:\tmovl %eax,%esp\n\t" "pushl %ecx\n\t" /* teb */ "pushl 16(%ebp)\n\t" /* suspend */ @@ -2472,7 +2468,7 @@ __ASM_GLOBAL_FUNC( signal_start_thread, * __wine_syscall_dispatcher */ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, - "movl %fs:0x1f8,%ecx\n\t" /* x86_thread_data()->syscall_frame */ + "movl %fs:0x1f4,%ecx\n\t" /* x86_thread_data()->syscall_frame */ "movw $0,0x02(%ecx)\n\t" /* frame->restore_flags */ "popl 0x08(%ecx)\n\t" /* frame->eip */ "pushfl\n\t" diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index fc73925e819..48091d45a8f 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -418,16 +418,14 @@ struct amd64_thread_data DWORD_PTR dr3; /* 0308 */ DWORD_PTR dr6; /* 0310 */ DWORD_PTR dr7; /* 0318 */ - void *exit_frame; /* 0320 exit frame pointer */ - struct syscall_frame *syscall_frame; /* 0328 syscall frame pointer */ - void *pthread_teb; /* 0330 thread data for pthread */ - DWORD fs; /* 0338 WOW TEB selector */ + struct syscall_frame *syscall_frame; /* 0320 syscall frame pointer */ + void *pthread_teb; /* 0328 thread data for pthread */ + DWORD fs; /* 0330 WOW TEB selector */ }; C_ASSERT( sizeof(struct amd64_thread_data) <= sizeof(((struct ntdll_thread_data *)0)->cpu_data) ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, exit_frame ) == 0x320 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, syscall_frame ) == 0x328 ); -C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, pthread_teb ) == 0x330 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, syscall_frame ) == 0x320 ); +C_ASSERT( offsetof( TEB, GdiTebBatch ) + offsetof( struct amd64_thread_data, pthread_teb ) == 0x328 ); static inline struct amd64_thread_data *amd64_thread_data(void) { @@ -3311,15 +3309,13 @@ __ASM_GLOBAL_FUNC( signal_start_thread, __ASM_CFI(".cfi_rel_offset %r14,16\n\t") "movq %r15,8(%rsp)\n\t" __ASM_CFI(".cfi_rel_offset %r15,8\n\t") - /* store exit frame */ - "movq %rsp,0x320(%rcx)\n\t" /* amd64_thread_data()->exit_frame */ /* set syscall frame */ - "movq 0x328(%rcx),%rax\n\t" /* amd64_thread_data()->syscall_frame */ + "movq 0x320(%rcx),%rax\n\t" /* amd64_thread_data()->syscall_frame */ "orq %rax,%rax\n\t" "jnz 1f\n\t" "leaq -0x400(%rsp),%rax\n\t" /* sizeof(struct syscall_frame) */ "andq $~63,%rax\n\t" - "movq %rax,0x328(%rcx)\n" /* amd64_thread_data()->syscall_frame */ + "movq %rax,0x320(%rcx)\n" /* amd64_thread_data()->syscall_frame */ "1:\tmovq %rax,%rsp\n\t" "call " __ASM_NAME("call_init_thunk")) @@ -3329,7 +3325,7 @@ __ASM_GLOBAL_FUNC( signal_start_thread, */ __ASM_GLOBAL_FUNC( __wine_syscall_dispatcher, "movq %gs:0x30,%rcx\n\t" - "movq 0x328(%rcx),%rcx\n\t" /* amd64_thread_data()->syscall_frame */ + "movq 0x320(%rcx),%rcx\n\t" /* amd64_thread_data()->syscall_frame */ "popq 0x70(%rcx)\n\t" /* frame->rip */ "pushfq\n\t" "popq 0x80(%rcx)\n\t" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 --- dlls/ntdll/unix/signal_i386.c | 6 ------ dlls/ntdll/unix/signal_x86_64.c | 16 ---------------- 2 files changed, 22 deletions(-) diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index c9e2684bde6..07f90a853d2 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -2442,12 +2442,6 @@ __ASM_GLOBAL_FUNC( signal_start_thread, __ASM_CFI(".cfi_rel_offset %ebp,0\n\t") "movl %esp,%ebp\n\t" __ASM_CFI(".cfi_def_cfa_register %ebp\n\t") - "pushl %ebx\n\t" - __ASM_CFI(".cfi_rel_offset %ebx,-4\n\t") - "pushl %esi\n\t" - __ASM_CFI(".cfi_rel_offset %esi,-8\n\t") - "pushl %edi\n\t" - __ASM_CFI(".cfi_rel_offset %edi,-12\n\t") "movl 20(%ebp),%ecx\n\t" /* teb */ /* set syscall frame */ "movl 0x1f4(%ecx),%eax\n\t" /* x86_thread_data()->syscall_frame */ diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 48091d45a8f..f14b4f96886 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -3293,22 +3293,6 @@ void DECLSPEC_HIDDEN call_init_thunk( LPTHREAD_START_ROUTINE entry, void *arg, B * signal_start_thread */ __ASM_GLOBAL_FUNC( signal_start_thread, - "subq $56,%rsp\n\t" - __ASM_SEH(".seh_stackalloc 56\n\t") - __ASM_SEH(".seh_endprologue\n\t") - __ASM_CFI(".cfi_adjust_cfa_offset 56\n\t") - "movq %rbp,48(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %rbp,48\n\t") - "movq %rbx,40(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %rbx,40\n\t") - "movq %r12,32(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %r12,32\n\t") - "movq %r13,24(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %r13,24\n\t") - "movq %r14,16(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %r14,16\n\t") - "movq %r15,8(%rsp)\n\t" - __ASM_CFI(".cfi_rel_offset %r15,8\n\t") /* set syscall frame */ "movq 0x320(%rcx),%rax\n\t" /* amd64_thread_data()->syscall_frame */ "orq %rax,%rax\n\t" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125093 Your paranoid android. === debian11 (32 bit report) === Report validation errors: kernel32:sync has no test summary line (early exit of the main process?) === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723.
On Tue Oct 18 13:11:45 2022 +0000, Rémi Bernon wrote:
I guess another way would be to not use the main unix thread, and spawn a new one to be the main win32 thread. We could then safely call `pthread_exit` from the main unix thread, which would wait for all pthreads to exit. So I'm doing that now, I think it's actually better, and it would probably let us allocate that thread stack beforehand and maybe treat it a bit more like the other threads. I've kept the initialization process in that thread for now though, as I'm not sure at all what kind of expectations we have here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11148
On Tue Oct 18 17:35:22 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125093 Your paranoid android. === debian11 (32 bit report) === Report validation errors: kernel32:sync has no test summary line (early exit of the main process?) === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24723. ``` Even though the pipeline succeeded that `kernel32:sync` error looks weird.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11185
What I would try to do (and maybe one day I'll have time to actually try), instead of sidestepping pthreads calling cleanup handlers, would be to arrange things so that pthreads can actually call cleanup handlers and they do they right thing. I still have to think this more, but I guess it should be attainable if we arrange the syscall stack so that each userspace entry/exit pair looks like a usual frame (maybe the task of cleaning it up could be delegated to a cleanup handler as well, if it is beneficial). I think your proposal of avoid using the initial thread and only run Windows code in threads created by us would make everything easier. For one thing all threads would have the same stack layout (i.e., they would have a stack allocated and decided by us even from the point of view of pthreads; now, instead, for the main thread we are pivoting to a different stack without telling pthreads, which is why we need `exit_frame` I think). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11317
On Wed Oct 19 12:08:10 2022 +0000, Giovanni Mascellani wrote:
What I would try to do (and maybe one day I'll have time to actually try), instead of sidestepping pthreads calling cleanup handlers, would be to arrange things so that pthreads can actually call cleanup handlers and they do they right thing. I still have to think this more, but I guess it should be attainable if we arrange the syscall stack so that each userspace entry/exit pair looks like a usual frame (maybe the task of cleaning it up could be delegated to a cleanup handler as well, if it is beneficial). I think your proposal of avoid using the initial thread and only run Windows code in threads created by us would make everything easier. For one thing all threads would have the same stack layout (i.e., they would have a stack allocated and decided by us even from the point of view of pthreads; now, instead, for the main thread we are pivoting to a different stack without telling pthreads, which is why we need `exit_frame` I think). We could maybe arrange the syscall unwind information so that `pthread_exit` would skip the PE frames, but `pthread_exit` on normal thread exit is only one side of the problem.
The other side is when exception happen in unix code, in which case we jump back to user code while leaving pthread cleanup handlers in place. If that happened, and even if everything else runs fine, calling pthread_exit on thread exit becomes dangerous. To address that scenario we would have to partially unwind the stacks when returning to user mode, and call any pushed handler. This isn't possible with the C cleanup handlers ABI, which are only registered to some internal pthread structures. It would be possible if the code was in C++, where the handlers are registered as unwinding personality functions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11318
On Wed Oct 19 12:08:10 2022 +0000, Rémi Bernon wrote:
We could maybe arrange the syscall unwind information so that `pthread_exit` would skip the PE frames, but `pthread_exit` on normal thread exit is only one side of the problem. The other side is when exception happen in unix code, in which case we jump back to user code while leaving pthread cleanup handlers in place. If that happened, and even if everything else runs fine, calling pthread_exit on thread exit becomes dangerous. To address that scenario we would have to partially unwind the stacks when returning to user mode, and call any pushed handler. This isn't possible with the C cleanup handlers ABI, which are only registered to some internal pthread structures. It would be possible if the code was in C++, where the handlers are registered as unwinding personality functions. For the record, we discussed a bit more about that and it ends up that a C code compiled with `-fexceptions` also registers its pthread cleanup handlers into the personality routines.
It would then eventually be possible to partially unwind stacks, and correctly execute and unregister the pthread handlers using standard stack unwinding logic (through libunwind or other), were we building everything with `-fexceptions`. There would still be potential issues when interacting with third-party C libraries built without that flag. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11377
On Wed Oct 19 17:00:00 2022 +0000, Rémi Bernon wrote:
For the record, we discussed a bit more about that and it ends up that a C code compiled with `-fexceptions` also registers its pthread cleanup handlers into the personality routines. It would then eventually be possible to partially unwind stacks, and correctly execute and unregister the pthread handlers using standard stack unwinding logic (through libunwind or other), were we building everything with `-fexceptions`. There would still be potential issues when interacting with third-party C libraries built without that flag. BTW, I doubt many libraries really use cleanup handlers, except glibc which already uses `-fexceptions`. So there's hope that's not a problem in practice.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11471
This will conflict with !1065, I'll rebase it later (after looking at the kernel32 failure). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_11509
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
Closing for now, I think it'd be nice to avoid crashing, but the proper fix would be to unwind unix side frames. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1088#note_13851
participants (3)
-
Giovanni Mascellani (＠giomasce) -
Marvin -
Rémi Bernon