[PATCH 1/5] server: Cancel asyncs before closing handles when a process is destroyed.
test_exit_process_async() essentially validates this. The only reason it currently succeeds (instead of incorrectly returning ERROR_BROKEN_PIPE) is that due to the use of DuplicateHandle() the source handle is never actually closed. Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- server/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/process.c b/server/process.c index f65d7abe2af..571a80f0dc6 100644 --- a/server/process.c +++ b/server/process.c @@ -842,8 +842,8 @@ static void process_killed( struct process *process ) if (!process->is_system) close_process_desktop( process ); process->winstation = 0; process->desktop = 0; - close_process_handles( process ); cancel_process_asyncs( process ); + close_process_handles( process ); if (process->idle_event) release_object( process->idle_event ); process->idle_event = NULL; assert( !process->console ); -- 2.30.2
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- The primary motivation behind this is to support DUPLICATE_CLOSE_SOURCE with locally cached waitable handles, an element of hopefully future patches optimizing synchronization primitives, without having to replicate the fd_close_handle close prevention hack for what ends up being most types of handles in existence. Those patches are still a long way off, but since this patch set has value by itself (see e.g. [1], although it's not clear whether that reflects a real bug or is only an artifical test case) I'm submitting it now. [1] <https://www.winehq.org/pipermail/wine-devel/2021-January/179733.html> dlls/ntdll/unix/server.c | 36 ++++++++++++++++++++++++++++++++++++ server/handle.c | 3 +-- server/protocol.def | 18 ++++++++++++++++-- server/thread.c | 15 +++++++++++++++ server/trace.c | 9 +++++++++ 5 files changed, 77 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 4f149c0f644..1f8cf546977 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -580,6 +580,21 @@ static void invoke_system_apc( const apc_call_t *call, apc_result_t *result ) else result->create_thread.status = STATUS_INVALID_PARAMETER; break; } + case APC_DUP_HANDLE: + { + HANDLE dst_handle = NULL; + + result->type = call->type; + + result->dup_handle.status = NtDuplicateObject( NtCurrentProcess(), + wine_server_ptr_handle(call->dup_handle.src_handle), + wine_server_ptr_handle(call->dup_handle.dst_process), + &dst_handle, call->dup_handle.access, + call->dup_handle.attributes, call->dup_handle.options ); + result->dup_handle.handle = wine_server_obj_handle( dst_handle ); + NtClose( wine_server_ptr_handle(call->dup_handle.dst_process) ); + break; + } case APC_BREAK_PROCESS: { HANDLE handle; @@ -1679,6 +1694,27 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE { NTSTATUS ret; + if ((options & DUPLICATE_CLOSE_SOURCE) && source_process != NtCurrentProcess()) + { + apc_call_t call; + apc_result_t result; + + memset( &call, 0, sizeof(call) ); + + call.dup_handle.type = APC_DUP_HANDLE; + call.dup_handle.src_handle = wine_server_obj_handle( source ); + call.dup_handle.dst_process = wine_server_obj_handle( dest_process ); + call.dup_handle.access = access; + call.dup_handle.attributes = attributes; + call.dup_handle.options = options; + ret = server_queue_process_apc( source_process, &call, &result ); + if (ret != STATUS_SUCCESS) return ret; + + if (!result.dup_handle.status) + *dest = wine_server_ptr_handle( result.dup_handle.handle ); + return result.dup_handle.status; + } + SERVER_START_REQ( dup_handle ) { req->src_process = wine_server_obj_handle( source_process ); diff --git a/server/handle.c b/server/handle.c index 6e0848eedf0..d86f0960ccf 100644 --- a/server/handle.c +++ b/server/handle.c @@ -678,8 +678,7 @@ DECL_HANDLER(dup_handle) } /* close the handle no matter what happened */ if ((req->options & DUPLICATE_CLOSE_SOURCE) && (src != dst || req->src_handle != reply->handle)) - reply->closed = !close_handle( src, req->src_handle ); - reply->self = (src == current->process); + close_handle( src, req->src_handle ); release_object( src ); } } diff --git a/server/protocol.def b/server/protocol.def index 2bfdcca6de1..c2792e9551c 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -483,6 +483,7 @@ enum apc_type APC_MAP_VIEW, APC_UNMAP_VIEW, APC_CREATE_THREAD, + APC_DUP_HANDLE, APC_BREAK_PROCESS }; @@ -593,6 +594,15 @@ typedef union mem_size_t reserve; /* reserve size for thread stack */ mem_size_t commit; /* commit size for thread stack */ } create_thread; + struct + { + enum apc_type type; /* APC_DUP_HANDLE */ + obj_handle_t src_handle; /* src handle to duplicate */ + obj_handle_t dst_process; /* dst process handle */ + unsigned int access; /* wanted access rights */ + unsigned int attributes; /* object attributes */ + unsigned int options; /* duplicate options */ + } dup_handle; } apc_call_t; typedef union @@ -681,6 +691,12 @@ typedef union obj_handle_t handle; /* handle to new thread */ } create_thread; struct + { + enum apc_type type; /* APC_DUP_HANDLE */ + unsigned int status; /* status returned by call */ + obj_handle_t handle; /* duplicated handle in dst process */ + } dup_handle; + struct { enum apc_type type; /* APC_BREAK_PROCESS */ unsigned int status; /* status returned by call */ @@ -1117,8 +1133,6 @@ typedef struct unsigned int options; /* duplicate options */ @REPLY obj_handle_t handle; /* duplicated handle in dst process */ - int self; /* is the source the current process? */ - int closed; /* whether the source handle has been closed */ @END diff --git a/server/thread.c b/server/thread.c index e4abe4fab94..fe9f9bdec37 100644 --- a/server/thread.c +++ b/server/thread.c @@ -1756,6 +1756,21 @@ DECL_HANDLER(queue_apc) case APC_BREAK_PROCESS: process = get_process_from_handle( req->handle, PROCESS_CREATE_THREAD ); break; + case APC_DUP_HANDLE: + process = get_process_from_handle( req->handle, PROCESS_DUP_HANDLE ); + if (process && process != current->process) + { + /* duplicate the destination process handle into the target process */ + obj_handle_t handle = duplicate_handle( current->process, apc->call.dup_handle.dst_process, + process, 0, 0, DUPLICATE_SAME_ACCESS ); + if (handle) apc->call.dup_handle.dst_process = handle; + else + { + release_object( process ); + process = NULL; + } + } + break; default: set_error( STATUS_INVALID_PARAMETER ); break; diff --git a/server/trace.c b/server/trace.c index 5f906a5abfa..66a4402c0bc 100644 --- a/server/trace.c +++ b/server/trace.c @@ -235,6 +235,11 @@ static void dump_apc_call( const char *prefix, const apc_call_t *call ) dump_uint64( ",commit=", &call->create_thread.commit ); fprintf( stderr, ",flags=%x", call->create_thread.flags ); break; + case APC_DUP_HANDLE: + fprintf( stderr, "APC_DUP_HANDLE,src_handle=%04x,dst_process=%04x,access=%x,attributes=%x,options=%x", + call->dup_handle.src_handle, call->dup_handle.dst_process, call->dup_handle.access, + call->dup_handle.attributes, call->dup_handle.options ); + break; case APC_BREAK_PROCESS: fprintf( stderr, "APC_BREAK_PROCESS" ); break; @@ -318,6 +323,10 @@ static void dump_apc_result( const char *prefix, const apc_result_t *result ) get_status_name( result->create_thread.status ), result->create_thread.pid, result->create_thread.tid, result->create_thread.handle ); break; + case APC_DUP_HANDLE: + fprintf( stderr, "APC_DUP_HANDLE,status=%s,handle=%04x", + get_status_name( result->dup_handle.status ), result->dup_handle.handle ); + break; case APC_BREAK_PROCESS: fprintf( stderr, "APC_BREAK_PROCESS,status=%s", get_status_name( result->break_process.status ) ); break; -- 2.30.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=87481 Your paranoid android. === debiant2 (build log) === ../wine/dlls/ntdll/unix/server.c:1729:22: error: ‘const struct dup_handle_reply’ has no member named ‘closed’ ../wine/dlls/ntdll/unix/server.c:1729:39: error: ‘const struct dup_handle_reply’ has no member named ‘self’ Task: The win32 Wine build failed === debiant2 (build log) === ../wine/dlls/ntdll/unix/server.c:1729:22: error: ‘const struct dup_handle_reply’ has no member named ‘closed’ ../wine/dlls/ntdll/unix/server.c:1729:39: error: ‘const struct dup_handle_reply’ has no member named ‘self’ Task: The wow64 Wine build failed
If another thread creates and accesses a file between the dup_handle request and the call to remove_fd_from_cache(), the file may be allocated to the same handle number, and that thread will then receive the wrong unix fd. Avoid this race by invalidating the cache first. Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- This fixes the race mentioned here: <https://www.winehq.org/pipermail/wine-devel/2021-January/179733.html> dlls/ntdll/unix/server.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 1f8cf546977..506cc99e542 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -1693,6 +1693,7 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE ACCESS_MASK access, ULONG attributes, ULONG options ) { NTSTATUS ret; + int fd; if ((options & DUPLICATE_CLOSE_SOURCE) && source_process != NtCurrentProcess()) { @@ -1715,6 +1716,14 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE return result.dup_handle.status; } + /* always remove the cached fd; if the server request fails we'll just + * retrieve it again */ + if (options & DUPLICATE_CLOSE_SOURCE) + { + fd = remove_fd_from_cache( source ); + if (fd != -1) close( fd ); + } + SERVER_START_REQ( dup_handle ) { req->src_process = wine_server_obj_handle( source_process ); @@ -1726,11 +1735,6 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE if (!(ret = wine_server_call( req ))) { if (dest) *dest = wine_server_ptr_handle( reply->handle ); - if (reply->closed && reply->self) - { - int fd = remove_fd_from_cache( source ); - if (fd != -1) close( fd ); - } } } SERVER_END_REQ; @@ -1745,6 +1749,9 @@ NTSTATUS WINAPI NtClose( HANDLE handle ) { HANDLE port; NTSTATUS ret; + + /* always remove the cached fd; if the server request fails we'll just + * retrieve it again */ int fd = remove_fd_from_cache( handle ); SERVER_START_REQ( close_handle ) -- 2.30.2
Otherwise, it's possible for the old handle to be re-added to the cache between removing and closing it. Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- dlls/ntdll/unix/server.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 506cc99e542..7d1231bf925 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -1692,8 +1692,9 @@ NTSTATUS WINAPI DbgUiIssueRemoteBreakin( HANDLE process ) NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE dest_process, HANDLE *dest, ACCESS_MASK access, ULONG attributes, ULONG options ) { + sigset_t sigset; NTSTATUS ret; - int fd; + int fd = -1; if ((options & DUPLICATE_CLOSE_SOURCE) && source_process != NtCurrentProcess()) { @@ -1716,13 +1717,12 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE return result.dup_handle.status; } + server_enter_uninterrupted_section( &fd_cache_mutex, &sigset ); + /* always remove the cached fd; if the server request fails we'll just * retrieve it again */ if (options & DUPLICATE_CLOSE_SOURCE) - { fd = remove_fd_from_cache( source ); - if (fd != -1) close( fd ); - } SERVER_START_REQ( dup_handle ) { @@ -1738,6 +1738,10 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE } } SERVER_END_REQ; + + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + + if (fd != -1) close( fd ); return ret; } @@ -1747,12 +1751,16 @@ NTSTATUS WINAPI NtDuplicateObject( HANDLE source_process, HANDLE source, HANDLE */ NTSTATUS WINAPI NtClose( HANDLE handle ) { + sigset_t sigset; HANDLE port; NTSTATUS ret; + int fd; + + server_enter_uninterrupted_section( &fd_cache_mutex, &sigset ); /* always remove the cached fd; if the server request fails we'll just * retrieve it again */ - int fd = remove_fd_from_cache( handle ); + fd = remove_fd_from_cache( handle ); SERVER_START_REQ( close_handle ) { @@ -1760,6 +1768,9 @@ NTSTATUS WINAPI NtClose( HANDLE handle ) ret = wine_server_call( req ); } SERVER_END_REQ; + + server_leave_uninterrupted_section( &fd_cache_mutex, &sigset ); + if (fd != -1) close( fd ); if (ret != STATUS_INVALID_HANDLE || !handle) return ret; -- 2.30.2
Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- server/change.c | 1 - server/console.c | 2 +- server/file.c | 2 +- server/file.h | 1 - server/mailslot.c | 6 +++--- server/mapping.c | 2 +- server/named_pipe.c | 6 +++--- server/serial.c | 2 +- server/sock.c | 2 +- 9 files changed, 11 insertions(+), 13 deletions(-) diff --git a/server/change.c b/server/change.c index ff8c3ad1037..b02a9cd65bf 100644 --- a/server/change.c +++ b/server/change.c @@ -421,7 +421,6 @@ static int dir_close_handle( struct object *obj, struct process *process, obj_ha { struct dir *dir = (struct dir *)obj; - if (!fd_close_handle( obj, process, handle )) return 0; if (obj->handle_count == 1) release_dir_cache_entry( dir ); /* closing last handle, release cache */ return 1; /* ok to close */ } diff --git a/server/console.c b/server/console.c index 848ee6e8d98..1e6f6c0f8a3 100644 --- a/server/console.c +++ b/server/console.c @@ -168,7 +168,7 @@ static const struct object_ops console_server_ops = NULL, /* unlink_name */ console_server_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ console_server_destroy /* destroy */ }; diff --git a/server/file.c b/server/file.c index f9ce0106c3b..9a072e6c64e 100644 --- a/server/file.c +++ b/server/file.c @@ -109,7 +109,7 @@ static const struct object_ops file_ops = NULL, /* unlink_name */ file_open_file, /* open_file */ file_get_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ file_destroy /* destroy */ }; diff --git a/server/file.h b/server/file.h index fbfd1e4eec9..0fa66e5750a 100644 --- a/server/file.h +++ b/server/file.h @@ -93,7 +93,6 @@ extern int is_fd_overlapped( struct fd *fd ); extern int get_unix_fd( struct fd *fd ); extern int is_same_file_fd( struct fd *fd1, struct fd *fd2 ); extern int is_fd_removable( struct fd *fd ); -extern int fd_close_handle( struct object *obj, struct process *process, obj_handle_t handle ); extern int check_fd_events( struct fd *fd, int events ); extern void set_fd_events( struct fd *fd, int events ); extern obj_handle_t lock_fd( struct fd *fd, file_pos_t offset, file_pos_t count, int shared, int wait ); diff --git a/server/mailslot.c b/server/mailslot.c index 5597a21dd29..d4b2fd1b562 100644 --- a/server/mailslot.c +++ b/server/mailslot.c @@ -90,7 +90,7 @@ static const struct object_ops mailslot_ops = default_unlink_name, /* unlink_name */ mailslot_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ mailslot_destroy /* destroy */ }; @@ -148,7 +148,7 @@ static const struct object_ops mail_writer_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ mail_writer_destroy /* destroy */ }; @@ -240,7 +240,7 @@ static const struct object_ops mailslot_device_file_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ mailslot_device_file_destroy /* destroy */ }; diff --git a/server/mapping.c b/server/mapping.c index 13a0948a8ab..0dcaa13e91d 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -189,7 +189,7 @@ static const struct object_ops mapping_ops = default_unlink_name, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ mapping_destroy /* destroy */ }; diff --git a/server/named_pipe.c b/server/named_pipe.c index a3ce9d463f1..df8c7e3170c 100644 --- a/server/named_pipe.c +++ b/server/named_pipe.c @@ -179,7 +179,7 @@ static const struct object_ops pipe_server_ops = NULL, /* unlink_name */ pipe_server_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ pipe_server_destroy /* destroy */ }; @@ -222,7 +222,7 @@ static const struct object_ops pipe_client_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ pipe_end_destroy /* destroy */ }; @@ -299,7 +299,7 @@ static const struct object_ops named_pipe_device_file_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ named_pipe_device_file_destroy /* destroy */ }; diff --git a/server/serial.c b/server/serial.c index ba8402c5935..d3ea4cbe420 100644 --- a/server/serial.c +++ b/server/serial.c @@ -104,7 +104,7 @@ static const struct object_ops serial_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + no_close_handle, /* close_handle */ serial_destroy /* destroy */ }; diff --git a/server/sock.c b/server/sock.c index 775ec6edf32..00d5b0b9044 100644 --- a/server/sock.c +++ b/server/sock.c @@ -889,7 +889,7 @@ static int sock_close_handle( struct object *obj, struct process *process, obj_h async_terminate( req->async, STATUS_CANCELLED ); } - return fd_close_handle( obj, process, handle ); + return 1; } static void sock_destroy( struct object *obj ) -- 2.30.2
participants (2)
-
Marvin -
Zebediah Figura