[PATCH 0/6] MR8878: ntdll: Avoid deadlock on SIGPIPE while holding virtual_mutex or fd_cache_mutex.
This manifested with https://gitlab.winehq.org/wine/wine/-/merge_requests/8875, as it adds fd_cache_mutex lock on thread init to receive the new inproc sync file descriptors. The kernel32:sync test creates a thread then immediately terminates it, which causes wineserver to close its pipe while it is initializing, triggering a SIGPIPE that is receive while the thread is within the server_init_thread call and request. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 102 +++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 4b4d9d6cd7f..9964f69cc06 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -4066,57 +4066,6 @@ TEB *virtual_alloc_first_teb(void) } -/*********************************************************************** - * virtual_alloc_teb - */ -NTSTATUS virtual_alloc_teb( TEB **ret_teb ) -{ - sigset_t sigset; - TEB *teb; - void *ptr = NULL; - NTSTATUS status = STATUS_SUCCESS; - SIZE_T block_size = signal_stack_mask + 1; - - server_enter_uninterrupted_section( &virtual_mutex, &sigset ); - if (next_free_teb) - { - ptr = next_free_teb; - next_free_teb = *(void **)ptr; - memset( ptr, 0, teb_size ); - } - else - { - if (!teb_block_pos) - { - SIZE_T total = 32 * block_size; - - if ((status = NtAllocateVirtualMemory( NtCurrentProcess(), &ptr, user_space_wow_limit, - &total, MEM_RESERVE, PAGE_READWRITE ))) - { - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); - return status; - } - teb_block = ptr; - teb_block_pos = 32; - } - ptr = ((char *)teb_block + --teb_block_pos * block_size); - NtAllocateVirtualMemory( NtCurrentProcess(), (void **)&ptr, 0, &block_size, - MEM_COMMIT, PAGE_READWRITE ); - } - *ret_teb = teb = init_teb( ptr, is_wow64() ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); - - if ((status = signal_alloc_thread( teb ))) - { - server_enter_uninterrupted_section( &virtual_mutex, &sigset ); - *(void **)ptr = next_free_teb; - next_free_teb = ptr; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); - } - return status; -} - - /*********************************************************************** * virtual_free_teb */ @@ -5099,6 +5048,57 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG_PTR z } +/*********************************************************************** + * virtual_alloc_teb + */ +NTSTATUS virtual_alloc_teb( TEB **ret_teb ) +{ + sigset_t sigset; + TEB *teb; + void *ptr = NULL; + NTSTATUS status = STATUS_SUCCESS; + SIZE_T block_size = signal_stack_mask + 1; + + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); + if (next_free_teb) + { + ptr = next_free_teb; + next_free_teb = *(void **)ptr; + memset( ptr, 0, teb_size ); + } + else + { + if (!teb_block_pos) + { + SIZE_T total = 32 * block_size; + + if ((status = NtAllocateVirtualMemory( NtCurrentProcess(), &ptr, user_space_wow_limit, + &total, MEM_RESERVE, PAGE_READWRITE ))) + { + server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + return status; + } + teb_block = ptr; + teb_block_pos = 32; + } + ptr = ((char *)teb_block + --teb_block_pos * block_size); + NtAllocateVirtualMemory( NtCurrentProcess(), (void **)&ptr, 0, &block_size, + MEM_COMMIT, PAGE_READWRITE ); + } + *ret_teb = teb = init_teb( ptr, is_wow64() ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + + if ((status = signal_alloc_thread( teb ))) + { + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); + *(void **)ptr = next_free_teb; + next_free_teb = ptr; + server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + } + return status; +} + + static NTSTATUS get_extended_params( const MEM_EXTENDED_PARAMETER *parameters, ULONG count, ULONG_PTR *limit_low, ULONG_PTR *limit_high, ULONG_PTR *align, ULONG *attributes, USHORT *machine ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 9964f69cc06..278800021af 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -5070,10 +5070,10 @@ NTSTATUS virtual_alloc_teb( TEB **ret_teb ) { if (!teb_block_pos) { + UINT_PTR limit = get_zero_bits_limit( user_space_wow_limit ); SIZE_T total = 32 * block_size; - if ((status = NtAllocateVirtualMemory( NtCurrentProcess(), &ptr, user_space_wow_limit, - &total, MEM_RESERVE, PAGE_READWRITE ))) + if ((status = allocate_virtual_memory( &ptr, &total, MEM_RESERVE, PAGE_READWRITE, 0, limit, 0, 0 ))) { server_leave_uninterrupted_section( &virtual_mutex, &sigset ); return status; @@ -5082,8 +5082,7 @@ NTSTATUS virtual_alloc_teb( TEB **ret_teb ) teb_block_pos = 32; } ptr = ((char *)teb_block + --teb_block_pos * block_size); - NtAllocateVirtualMemory( NtCurrentProcess(), (void **)&ptr, 0, &block_size, - MEM_COMMIT, PAGE_READWRITE ); + allocate_virtual_memory( &ptr, &block_size, MEM_COMMIT, PAGE_READWRITE, 0, 0, 0, 0 ); } *ret_teb = teb = init_teb( ptr, is_wow64() ); server_leave_uninterrupted_section( &virtual_mutex, &sigset ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 278800021af..bf09559d755 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -167,7 +167,7 @@ static const BYTE VIRTUAL_Win32Flags[16] = }; static struct wine_rb_tree views_tree; -static pthread_mutex_t virtual_mutex; +static pthread_mutex_t virtual_mutex = PTHREAD_MUTEX_INITIALIZER; static const UINT page_shift = 12; static const UINT_PTR page_mask = 0xfff; @@ -3646,12 +3646,6 @@ void virtual_init(void) const char *preload; size_t size; int i; - pthread_mutexattr_t attr; - - pthread_mutexattr_init( &attr ); - pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ); - pthread_mutex_init( &virtual_mutex, &attr ); - pthread_mutexattr_destroy( &attr ); #ifdef __aarch64__ host_page_size = sysconf( _SC_PAGESIZE ); @@ -4879,7 +4873,6 @@ static NTSTATUS allocate_virtual_memory( void **ret, SIZE_T *size_ptr, ULONG typ unsigned int vprot; BOOL is_dos_memory = FALSE; struct file_view *view; - sigset_t sigset; SIZE_T size = *size_ptr; NTSTATUS status = STATUS_SUCCESS; @@ -4925,8 +4918,6 @@ static NTSTATUS allocate_virtual_memory( void **ret, SIZE_T *size_ptr, ULONG typ /* Reserve the memory */ - server_enter_uninterrupted_section( &virtual_mutex, &sigset ); - if ((type & MEM_RESERVE) || !base) { if (!(status = get_vprot_flags( protect, &vprot, FALSE ))) @@ -4975,8 +4966,6 @@ static NTSTATUS allocate_virtual_memory( void **ret, SIZE_T *size_ptr, ULONG typ if (!status) VIRTUAL_DEBUG_DUMP_VIEW( view ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); - if (status == STATUS_SUCCESS) { *ret = base; @@ -4998,6 +4987,8 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG_PTR z { static const ULONG type_mask = MEM_COMMIT | MEM_RESERVE | MEM_TOP_DOWN | MEM_WRITE_WATCH | MEM_RESET; ULONG_PTR limit; + NTSTATUS status; + sigset_t sigset; TRACE("%p %p %08lx %x %08x\n", process, *ret, *size_ptr, type, protect ); @@ -5013,7 +5004,6 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG_PTR z { union apc_call call; union apc_result result; - unsigned int status; memset( &call, 0, sizeof(call) ); @@ -5044,7 +5034,10 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG_PTR z else limit = 0; - return allocate_virtual_memory( ret, size_ptr, type, protect, 0, limit, 0, 0 ); + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); + status = allocate_virtual_memory( ret, size_ptr, type, protect, 0, limit, 0, 0 ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + return status; } @@ -5193,6 +5186,7 @@ NTSTATUS WINAPI NtAllocateVirtualMemoryEx( HANDLE process, PVOID *ret, SIZE_T *s ULONG attributes = 0; USHORT machine = 0; unsigned int status; + sigset_t sigset; TRACE( "%p %p %08lx %x %08x %p %u\n", process, *ret, *size_ptr, type, protect, parameters, count ); @@ -5232,8 +5226,10 @@ NTSTATUS WINAPI NtAllocateVirtualMemoryEx( HANDLE process, PVOID *ret, SIZE_T *s return result.virtual_alloc_ex.status; } - return allocate_virtual_memory( ret, size_ptr, type, protect, - limit_low, limit_high, align, attributes ); + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); + status = allocate_virtual_memory( ret, size_ptr, type, protect, limit_low, limit_high, align, attributes ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + return status; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 4 ++-- dlls/ntdll/unix/sync.c | 2 +- dlls/ntdll/unix/virtual.c | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index e8da94ebc64..349d99fefe3 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -1151,7 +1151,7 @@ int server_get_unix_fd( HANDLE handle, unsigned int wanted_access, int *unix_fd, SERVER_START_REQ( get_handle_fd ) { req->handle = wine_server_obj_handle( handle ); - if (!(ret = wine_server_call( req ))) + if (!(ret = server_call_unlocked( req ))) { if (type) *type = reply->type; if (options) *options = reply->options; @@ -1914,7 +1914,7 @@ NTSTATUS WINAPI NtClose( HANDLE handle ) SERVER_START_REQ( close_handle ) { req->handle = wine_server_obj_handle( handle ); - ret = wine_server_call( req ); + ret = server_call_unlocked( req ); } SERVER_END_REQ; diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index acdb7b3d9f8..56bfe6df70f 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -332,7 +332,7 @@ static NTSTATUS get_inproc_sync( HANDLE handle, ACCESS_MASK desired_access, stru SERVER_START_REQ( get_inproc_sync_fd ) { req->handle = wine_server_obj_handle( handle ); - if (!(ret = wine_server_call( req ))) + if (!(ret = server_call_unlocked( req ))) { obj_handle_t fd_handle; sync->fd = wine_server_receive_fd( &fd_handle ); diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index bf09559d755..dcb6d7fde6f 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -3442,7 +3442,7 @@ static NTSTATUS virtual_map_image( HANDLE mapping, void **addr_ptr, SIZE_T *size req->size = size; req->entry = image_info->entry_point; req->machine = image_info->machine; - status = wine_server_call( req ); + status = server_call_unlocked( req ); } SERVER_END_REQ; } @@ -3580,7 +3580,7 @@ static unsigned int virtual_map_section( HANDLE handle, PVOID *addr_ptr, ULONG_P req->base = wine_server_client_ptr( view->base ); req->size = size; req->start = offset.QuadPart; - res = wine_server_call( req ); + res = server_call_unlocked( req ); } SERVER_END_REQ; } @@ -3886,7 +3886,7 @@ NTSTATUS virtual_create_builtin_view( void *module, const UNICODE_STRING *nt_nam { wine_server_add_data( req, info, sizeof(*info) ); wine_server_add_data( req, nt_name->Buffer, nt_name->Length ); - status = wine_server_call( req ); + status = server_call_unlocked( req ); } SERVER_END_REQ; @@ -4952,7 +4952,7 @@ static NTSTATUS allocate_virtual_memory( void **ret, SIZE_T *size_ptr, ULONG typ req->base = wine_server_client_ptr( view->base ); req->offset = (char *)base - (char *)view->base; req->size = size; - wine_server_call( req ); + server_call_unlocked( req ); } SERVER_END_REQ; } @@ -6273,7 +6273,7 @@ static NTSTATUS unmap_view_of_section( HANDLE process, PVOID addr, ULONG flags ) SERVER_START_REQ( unmap_view ) { req->base = wine_server_client_ptr( view->base ); - status = wine_server_call( req ); + status = server_call_unlocked( req ); } SERVER_END_REQ; if (!status) @@ -6644,7 +6644,7 @@ NTSTATUS WINAPI NtAreMappedFilesTheSame(PVOID addr1, PVOID addr2) { req->base1 = wine_server_client_ptr( view1->base ); req->base2 = wine_server_client_ptr( view2->base ); - status = wine_server_call( req ); + status = server_call_unlocked( req ); } SERVER_END_REQ; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 8 ++-- dlls/ntdll/unix/signal_i386.c | 6 +-- dlls/ntdll/unix/signal_x86_64.c | 6 +-- dlls/ntdll/unix/sync.c | 2 +- dlls/ntdll/unix/unix_private.h | 2 +- dlls/ntdll/unix/virtual.c | 78 ++++++++++++++++----------------- 6 files changed, 51 insertions(+), 51 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 349d99fefe3..53308fb4c8c 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -336,7 +336,7 @@ void server_enter_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigse /*********************************************************************** * server_leave_uninterrupted_section */ -void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset ) +void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset, NTSTATUS status ) { mutex_unlock( mutex ); pthread_sigmask( SIG_SETMASK, sigset, NULL ); @@ -1172,7 +1172,7 @@ int server_get_unix_fd( HANDLE handle, unsigned int wanted_access, int *unix_fd, } SERVER_END_REQ; } - server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset, ret ); done: if (!ret && ((access & wanted_access) != wanted_access)) @@ -1856,7 +1856,7 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE } SERVER_END_REQ; - server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset, ret ); if (fd != -1) close( fd ); return ret; @@ -1918,7 +1918,7 @@ NTSTATUS WINAPI NtClose( HANDLE handle ) } SERVER_END_REQ; - server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset, ret ); if (fd != -1) close( fd ); diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index db72829bd1f..5a6a1438d80 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -2365,7 +2365,7 @@ NTSTATUS WINAPI NtSetLdtEntries( ULONG sel1, LDT_ENTRY entry1, ULONG sel2, LDT_E server_enter_uninterrupted_section( &ldt_mutex, &sigset ); if (sel1) ldt_set_entry( sel1, entry1 ); if (sel2) ldt_set_entry( sel2, entry2 ); - server_leave_uninterrupted_section( &ldt_mutex, &sigset ); + server_leave_uninterrupted_section( &ldt_mutex, &sigset, STATUS_SUCCESS ); return STATUS_SUCCESS; } @@ -2424,7 +2424,7 @@ NTSTATUS signal_alloc_thread( TEB *teb ) ldt_set_entry( (idx << 3) | 7, entry ); break; } - server_leave_uninterrupted_section( &ldt_mutex, &sigset ); + server_leave_uninterrupted_section( &ldt_mutex, &sigset, STATUS_SUCCESS ); if (idx == LDT_SIZE) return STATUS_TOO_MANY_THREADS; } thread_data->fs = (idx << 3) | 7; @@ -2449,7 +2449,7 @@ void signal_free_thread( TEB *teb ) server_enter_uninterrupted_section( &ldt_mutex, &sigset ); __wine_ldt_copy.flags[thread_data->fs >> 3] = 0; - server_leave_uninterrupted_section( &ldt_mutex, &sigset ); + server_leave_uninterrupted_section( &ldt_mutex, &sigset, STATUS_SUCCESS ); } diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index f55726801c7..040868615af 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -489,7 +489,7 @@ void set_process_instrumentation_callback( void *callback ) old = InterlockedExchangePointer( &instrumentation_callback, callback ); if (!old && callback) InterlockedExchangePointer( ptr, __wine_syscall_dispatcher_instrumentation ); else if (old && !callback) InterlockedExchangePointer( ptr, __wine_syscall_dispatcher ); - server_leave_uninterrupted_section( &instrumentation_callback_mutex, &sigset ); + server_leave_uninterrupted_section( &instrumentation_callback_mutex, &sigset, STATUS_SUCCESS ); } struct xcontext @@ -2562,7 +2562,7 @@ NTSTATUS signal_alloc_thread( TEB *teb ) ldt_set_entry( (idx << 3) | 7, entry ); break; } - server_leave_uninterrupted_section( &ldt_mutex, &sigset ); + server_leave_uninterrupted_section( &ldt_mutex, &sigset, STATUS_SUCCESS ); if (idx == LDT_SIZE) return STATUS_TOO_MANY_THREADS; thread_data->fs = (idx << 3) | 7; } @@ -2585,7 +2585,7 @@ void signal_free_thread( TEB *teb ) { server_enter_uninterrupted_section( &ldt_mutex, &sigset ); __wine_ldt_copy.flags[thread_data->fs >> 3] = 0; - server_leave_uninterrupted_section( &ldt_mutex, &sigset ); + server_leave_uninterrupted_section( &ldt_mutex, &sigset, STATUS_SUCCESS ); } } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 56bfe6df70f..22afb278302 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -343,7 +343,7 @@ static NTSTATUS get_inproc_sync( HANDLE handle, ACCESS_MASK desired_access, stru } SERVER_END_REQ; - server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset, ret ); if (ret) return ret; if ((sync->access & desired_access) != desired_access) diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 974766b97a7..bebb1889494 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -225,7 +225,7 @@ extern void start_server( BOOL debug ); extern unsigned int server_call_unlocked( void *req_ptr ); extern void server_enter_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset ); -extern void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset ); +extern void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset, NTSTATUS status ); extern unsigned int server_select( const union select_op *select_op, data_size_t size, UINT flags, timeout_t abs_timeout, struct context_data *context, struct user_apc *user_apc ); extern unsigned int server_wait( const union select_op *select_op, data_size_t size, UINT flags, diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index dcb6d7fde6f..ae356629ed7 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -826,7 +826,7 @@ void *get_builtin_so_handle( void *module ) if (ret) builtin->refcount++; break; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return ret; } @@ -858,7 +858,7 @@ static NTSTATUS get_builtin_unix_funcs( void *module, BOOL wow, const void **fun } break; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -880,7 +880,7 @@ NTSTATUS load_builtin_unixlib( void *module, const char *name ) else status = STATUS_IMAGE_ALREADY_LOADED; break; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -1420,7 +1420,7 @@ static void VIRTUAL_Dump(void) { dump_view( view ); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } #endif @@ -3456,7 +3456,7 @@ static NTSTATUS virtual_map_image( HANDLE mapping, void **addr_ptr, SIZE_T *size else delete_view( view ); done: - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); if (needs_close) close( unix_fd ); if (shared_needs_close) close( shared_fd ); return status; @@ -3595,7 +3595,7 @@ static unsigned int virtual_map_section( HANDLE handle, PVOID *addr_ptr, ULONG_P else delete_view( view ); done: - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, res ); if (needs_close) close( unix_handle ); return res; } @@ -3898,7 +3898,7 @@ NTSTATUS virtual_create_builtin_view( void *module, const UNICODE_STRING *nt_nam } else delete_view( view ); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -4101,7 +4101,7 @@ void virtual_free_teb( TEB *teb ) if (!is_win64) ptr = (char *)ptr - teb_offset; *(void **)ptr = next_free_teb; next_free_teb = ptr; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } @@ -4126,7 +4126,7 @@ NTSTATUS virtual_clear_tls_index( ULONG index ) #endif teb->TlsSlots[index] = 0; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } else { @@ -4148,7 +4148,7 @@ NTSTATUS virtual_clear_tls_index( ULONG index ) #endif if (teb->TlsExpansionSlots) teb->TlsExpansionSlots[index] = 0; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } return STATUS_SUCCESS; } @@ -4199,7 +4199,7 @@ NTSTATUS virtual_alloc_thread_stack( INITIAL_TEB *stack, ULONG_PTR limit_low, UL stack->StackBase = (char *)view->base + view->size; stack->StackLimit = (char *)view->base + (guard_page ? 2 * host_page_size : 0); done: - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -4511,7 +4511,7 @@ unsigned int virtual_locked_server_call( void *req_ptr ) if (has_write_watch) update_write_watches( addr, size, wine_server_reply_size( req )); } else memset( &req->u.reply, 0, sizeof(req->u.reply) ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, ret ); return ret; } @@ -4535,7 +4535,7 @@ ssize_t virtual_locked_read( int fd, void *addr, size_t size ) err = errno; if (has_write_watch) update_write_watches( addr, size, max( 0, ret )); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); errno = err; return ret; } @@ -4560,7 +4560,7 @@ ssize_t virtual_locked_pread( int fd, void *addr, size_t size, off_t offset ) err = errno; if (has_write_watch) update_write_watches( addr, size, max( 0, ret )); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); errno = err; return ret; } @@ -4591,7 +4591,7 @@ ssize_t virtual_locked_recvmsg( int fd, struct msghdr *hdr, int flags ) if (has_write_watch) while (i--) update_write_watches( hdr->msg_iov[i].iov_base, hdr->msg_iov[i].iov_len, 0 ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); errno = err; return ret; } @@ -4609,7 +4609,7 @@ BOOL virtual_is_valid_code_address( const void *addr, SIZE_T size ) server_enter_uninterrupted_section( &virtual_mutex, &sigset ); if ((view = find_view( addr, size ))) ret = !(view->protect & VPROT_SYSTEM); /* system views are not visible to the app */ - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return ret; } @@ -4712,7 +4712,7 @@ SIZE_T virtual_uninterrupted_read_memory( const void *addr, void *buffer, SIZE_T } } } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return bytes_read; } @@ -4738,7 +4738,7 @@ NTSTATUS virtual_uninterrupted_write_memory( void *addr, const void *buffer, SIZ memcpy( addr, buffer, size ); if (has_write_watch) update_write_watches( addr, size, size ); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return ret; } @@ -4766,7 +4766,7 @@ void virtual_set_force_exec( BOOL enable ) mprotect_range( view->base, view->size, commit, 0 ); } } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } @@ -4786,7 +4786,7 @@ void virtual_enable_write_exceptions( BOOL enable ) mprotect_range( view->base, view->size, 0, 0 ); } enable_write_exceptions = enable; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); } @@ -4952,7 +4952,7 @@ static NTSTATUS allocate_virtual_memory( void **ret, SIZE_T *size_ptr, ULONG typ req->base = wine_server_client_ptr( view->base ); req->offset = (char *)base - (char *)view->base; req->size = size; - server_call_unlocked( req ); + status = server_call_unlocked( req ); } SERVER_END_REQ; } @@ -5036,7 +5036,7 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG_PTR z server_enter_uninterrupted_section( &virtual_mutex, &sigset ); status = allocate_virtual_memory( ret, size_ptr, type, protect, 0, limit, 0, 0 ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -5068,7 +5068,7 @@ NTSTATUS virtual_alloc_teb( TEB **ret_teb ) if ((status = allocate_virtual_memory( &ptr, &total, MEM_RESERVE, PAGE_READWRITE, 0, limit, 0, 0 ))) { - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } teb_block = ptr; @@ -5078,14 +5078,14 @@ NTSTATUS virtual_alloc_teb( TEB **ret_teb ) allocate_virtual_memory( &ptr, &block_size, MEM_COMMIT, PAGE_READWRITE, 0, 0, 0, 0 ); } *ret_teb = teb = init_teb( ptr, is_wow64() ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); if ((status = signal_alloc_thread( teb ))) { server_enter_uninterrupted_section( &virtual_mutex, &sigset ); *(void **)ptr = next_free_teb; next_free_teb = ptr; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); } return status; } @@ -5228,7 +5228,7 @@ NTSTATUS WINAPI NtAllocateVirtualMemoryEx( HANDLE process, PVOID *ret, SIZE_T *s server_enter_uninterrupted_section( &virtual_mutex, &sigset ); status = allocate_virtual_memory( ret, size_ptr, type, protect, limit_low, limit_high, align, attributes ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -5320,7 +5320,7 @@ NTSTATUS WINAPI NtFreeVirtualMemory( HANDLE process, PVOID *addr_ptr, SIZE_T *si *addr_ptr = base; *size_ptr = size; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -5391,7 +5391,7 @@ NTSTATUS WINAPI NtProtectVirtualMemory( HANDLE process, PVOID *addr_ptr, SIZE_T if (!status) VIRTUAL_DEBUG_DUMP_VIEW( view ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); if (status == STATUS_SUCCESS) { @@ -5532,7 +5532,7 @@ static unsigned int fill_basic_memory_info( const void *addr, MEMORY_BASIC_INFOR else if (view->protect & (SEC_FILE | SEC_RESERVE | SEC_COMMIT)) info->Type = MEM_MAPPED; else info->Type = MEM_PRIVATE; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return STATUS_SUCCESS; } @@ -5631,7 +5631,7 @@ static unsigned int get_memory_region_info( HANDLE process, LPCVOID addr, MEMORY { if (!fake_reserved) { - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return STATUS_INVALID_ADDRESS; } info->AllocationBase = region_start; @@ -5641,7 +5641,7 @@ static unsigned int get_memory_region_info( HANDLE process, LPCVOID addr, MEMORY info->CommitSize = 0; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); if (res_len) *res_len = sizeof(*info); return STATUS_SUCCESS; @@ -5864,7 +5864,7 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, free_fill_working_set_info_data( &data ); if (ref != ref_buffer) free( ref ); - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); if (res_len) *res_len = len; @@ -6264,7 +6264,7 @@ static NTSTATUS unmap_view_of_section( HANDLE process, PVOID addr, ULONG flags ) { TRACE( "not freeing in-use builtin %p\n", view->base ); builtin->refcount--; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, STATUS_SUCCESS ); return STATUS_SUCCESS; } } @@ -6284,7 +6284,7 @@ static NTSTATUS unmap_view_of_section( HANDLE process, PVOID addr, ULONG flags ) } else FIXME( "failed to unmap %p %x\n", view->base, status ); done: - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -6447,7 +6447,7 @@ NTSTATUS WINAPI NtFlushVirtualMemory( HANDLE process, LPCVOID *addr_ptr, status = STATUS_NOT_MAPPED_DATA; #endif } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -6506,7 +6506,7 @@ NTSTATUS WINAPI NtGetWriteWatch( HANDLE process, ULONG flags, PVOID base, SIZE_T } else status = STATUS_INVALID_PARAMETER; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -6534,7 +6534,7 @@ NTSTATUS WINAPI NtResetWriteWatch( HANDLE process, PVOID base, SIZE_T size ) else status = STATUS_INVALID_PARAMETER; - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -6649,7 +6649,7 @@ NTSTATUS WINAPI NtAreMappedFilesTheSame(PVOID addr1, PVOID addr2) SERVER_END_REQ; } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, status ); return status; } @@ -6707,7 +6707,7 @@ static NTSTATUS set_dirty_state_information( ULONG_PTR count, MEMORY_RANGE_ENTRY else if (set_page_vprot_exec_write_protect( base, size )) mprotect_range( base, size, 0, 0 ); } - server_leave_uninterrupted_section( &virtual_mutex, &sigset ); + server_leave_uninterrupted_section( &virtual_mutex, &sigset, ret ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 53308fb4c8c..451bed1110b 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -235,7 +235,7 @@ static unsigned int send_request( const struct __server_request_info *req ) } } - if (errno == EPIPE) abort_thread(0); + if (errno == EPIPE) return STATUS_THREAD_IS_TERMINATING; if (errno == EFAULT) return STATUS_ACCESS_VIOLATION; server_protocol_perror( "write" ); } @@ -246,7 +246,7 @@ static unsigned int send_request( const struct __server_request_info *req ) * * Read data from the reply buffer; helper for wait_reply. */ -static void read_reply_data( void *buffer, size_t size ) +static UINT read_reply_data( void *buffer, size_t size ) { int ret; @@ -254,13 +254,13 @@ static void read_reply_data( void *buffer, size_t size ) { if ((ret = read( ntdll_get_thread_data()->reply_fd, buffer, size )) > 0) { - if (!(size -= ret)) return; + if (!(size -= ret)) return STATUS_SUCCESS; buffer = (char *)buffer + ret; continue; } if (!ret) break; if (errno == EINTR) continue; - if (errno == EPIPE) break; + if (errno == EPIPE) return STATUS_THREAD_IS_TERMINATING; server_protocol_perror("read"); } /* the server closed the connection; time to die... */ @@ -275,9 +275,10 @@ static void read_reply_data( void *buffer, size_t size ) */ static inline unsigned int wait_reply( struct __server_request_info *req ) { - read_reply_data( &req->u.reply, sizeof(req->u.reply) ); - if (req->u.reply.reply_header.reply_size) - read_reply_data( req->reply_data, req->u.reply.reply_header.reply_size ); + unsigned int ret; + if ((ret = read_reply_data( &req->u.reply, sizeof(req->u.reply) ))) return ret; + if (!req->u.reply.reply_header.reply_size) return req->u.reply.reply_header.error; + if ((ret = read_reply_data( req->reply_data, req->u.reply.reply_header.reply_size ))) return ret; return req->u.reply.reply_header.error; } @@ -307,6 +308,7 @@ unsigned int CDECL wine_server_call( void *req_ptr ) pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); ret = server_call_unlocked( req_ptr ); + if (ret == STATUS_THREAD_IS_TERMINATING) abort_thread(0); pthread_sigmask( SIG_SETMASK, &old_set, NULL ); return ret; } @@ -339,6 +341,7 @@ void server_enter_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigse void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset, NTSTATUS status ) { mutex_unlock( mutex ); + if (status == STATUS_THREAD_IS_TERMINATING) abort_thread(0); pthread_sigmask( SIG_SETMASK, sigset, NULL ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
This seems to solve the same problem as !4811, where I suggested using `raise(SIGQUIT)` instead of e.g., refactoring all uninterrupted sections. It was closed with the following comment:
From off-list conversation, this isn't going to be enough to solve the general problem of thread state becoming inconsistent when killed while in a syscall, probably mostly because of win32u problems. This needs more thought.
@zfigura Could you elaborate on what the problem was? Since we now have a (more or less) reliably reproducible case, do you think it's okay to proceed with this MR (or !4811) even if it's not a complete solution? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114520
On Tue Sep 2 13:01:10 2025 +0000, Jinoh Kang wrote:
This seems to solve the same problem as !4811, where I suggested using `raise(SIGQUIT)` instead of e.g., refactoring all uninterrupted sections. It was closed with the following comment:
From off-list conversation, this isn't going to be enough to solve the general problem of thread state becoming inconsistent when killed while in a syscall, probably mostly because of win32u problems. This needs more thought. @zfigura Could you elaborate on what the problem was? Since we now have a (more or less) reliably reproducible case, do you think it's okay to proceed with this MR (or !4811) even if it's not a complete solution? FWIW, either MR appears to solve the *specific* case where a thread is killed due to broken server pipe, if not all syscalls.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114522
Note: I suggest s/SIGPIPE/EPIPE/ to avoid confusion, since SIGPIPE is either always ignored or kills immediately (SIG_DFL), it's EPIPE that triggers the error path -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114523
I suggested using `raise(SIGQUIT)` instead of e.g., refactoring all uninterrupted sections.
Well, idk IMO it's easier to follow with an explicit return code handling rather than asynchronous signal processing, but I don't care much. I don't know what the argument was against !4811 but I think this would now be nice to have fixed or at least improved enough for the kernel32:sync test failure to avoid blocking !8875. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114524
@zfigura Could you elaborate on what the problem was? Since we now have a (more or less) reliably reproducible case, do you think it's okay to proceed with this MR (or !4811) even if it's not a complete solution?
From the conversation, I simultaneously got the impression that we don't care about NtTerminateThread() breaking things, but also that if we're going to solve this problem we need to solve it everywhere, including avoiding deadlocks or corruption in win32u. At the same time it seemed that we need a generic solution for addressing all of these deadlocks, rather than addressing specific deadlocks using specific mechanisms. I am not nearly smart enough to satisfy both of these constraints; they seem impossible to me. I haven't the faintest idea how you can possibly avoid any problems in reentrant syscalls, without dealing with them on a case by case basis.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114543
Well I'm not aiming at fixing every possible issue with NtTerminateThread, but mostly the one that got triggered in that test. If that's not a improvement then maybe we should get rid of this test because it makes it impossible to send additional fds on thread creation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114598
On Wed Sep 3 14:32:13 2025 +0000, Rémi Bernon wrote:
I suggested using `raise(SIGQUIT)` instead of e.g., refactoring all uninterrupted sections. Well, idk IMO it's easier to follow with an explicit return code handling rather than asynchronous signal processing, but I don't care much. I don't know what the argument was against !4811 but I think this would now be nice to have fixed or at least improved enough for the kernel32:sync test failure to avoid blocking !8875. I don't have strong opinions either way, just making an observation that there is a way to avoid the refactoring which I think have better chance upstream (and also, SIGPIPE/EPIPE is asynchronous in nature already so I think that doesn't make big difference)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878#note_114751
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8878
participants (3)
-
Elizabeth Figura (＠zfigura) -
Jinoh Kang (＠iamahuman)
-
Rémi Bernon