This fixes the send part of the issue #52401 [1]: "Improper synchronization in sock_recv/sock_send leads to arbitrary reordering of completion of I/O requests", which was initially reported (outside Bugzilla) by Dongwan Kim [2]. Changelog: v1 -> v2: - drop patch "server: Compact struct set_async_direct_result_reply." - add async and fd arguments to async_initial_status_callback - recv_socket_initial_callback: pass OOB flag via private instead of implementing two separate functions for OOB and non-OOB - detect short write in send_socket_initial_callback - preserve the behaviour of returning success if we had a short write and the socket is nonblocking and force_async is unset v2 -> v3: - fix typo in comment v3 -> v4: - fix typo in commit message - address feedback - drop patch "server: Defer postprocessing until after setting initial status in recv_socket handler." [1] https://bugs.winehq.org/show_bug.cgi?id=52401 [2] https://www.winehq.org/pipermail/wine-devel/2021-May/186454.html Jinoh Kang (8): server: Actually set initial status in set_async_direct_result handler. server: Ensure initial status is set in async_set_result(). server: Add async_initial_status_callback. server: Defer postprocessing until after setting initial status in send_socket handler. server: Add mark_pending field to set_async_direct_result request. server: Attempt to complete I/O request immediately in send_socket. ntdll: Don't call try_send before server call in sock_send. server: Replace redundant send_socket status fields with force_async boolean field. dlls/ntdll/unix/socket.c | 102 +++++++++++++++++++++++++-------- dlls/ntdll/unix/sync.c | 9 +-- dlls/ntdll/unix/unix_private.h | 2 +- dlls/ws2_32/tests/sock.c | 14 ++--- server/async.c | 60 ++++++++++++++----- server/file.h | 2 + server/protocol.def | 6 +- server/sock.c | 92 ++++++++++++++++------------- 8 files changed, 194 insertions(+), 93 deletions(-) Interdiff: diff --git a/server/async.c b/server/async.c index ded4f8392d2..90fede50371 100644 --- a/server/async.c +++ b/server/async.c @@ -326,12 +326,8 @@ void async_set_initial_status_callback( struct async *async, async_initial_statu * the initial status may be STATUS_PENDING */ void async_set_initial_status( struct async *async, unsigned int status ) { - assert( async->unknown_status ); - if (!async->terminated) - { - set_async_initial_status( async, status ); - async->unknown_status = 0; - } + set_async_initial_status( async, status ); + async->unknown_status = 0; } void set_async_pending( struct async *async ) @@ -509,6 +505,8 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota assert( async->terminated ); /* it must have been woken up if we get a result */ + if (async->unknown_status) async_set_initial_status( async, status ); + if (async->alerted && status == STATUS_PENDING) /* restart it */ { async->terminated = 0; @@ -797,11 +795,6 @@ DECL_HANDLER(set_async_direct_result) return; } - /* Set iosb information field for async_initial_status_callback */ - async->iosb->result = information; - set_async_initial_status( async, status ); - async->unknown_status = 0; - if (status == STATUS_PENDING) { async->direct_result = 0; @@ -812,6 +805,9 @@ DECL_HANDLER(set_async_direct_result) async->pending = 1; } + /* Set iosb information field for async_initial_status_callback */ + async->iosb->result = information; + /* if the I/O has completed successfully (or unsuccessfully, and * async->pending is set), the client would have already set the IOSB. * therefore, we can do async_set_result() directly and let the client skip diff --git a/server/sock.c b/server/sock.c index 1112708369d..c742bd687a1 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3393,16 +3393,6 @@ struct object *create_socket_device( struct object *root, const struct unicode_s return create_named_object( root, &socket_device_ops, name, attr, sd ); } -static void recv_socket_initial_callback( void *private, struct async *async, struct fd *fd, unsigned int status ) -{ - struct sock *sock = get_fd_user( fd ); - int is_oob = (int)(long)private; - - sock->pending_events &= ~(is_oob ? AFD_POLL_OOB : AFD_POLL_READ); - sock->reported_events &= ~(is_oob ? AFD_POLL_OOB : AFD_POLL_READ); - sock_reselect( sock ); -} - DECL_HANDLER(recv_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); @@ -3440,18 +3430,22 @@ DECL_HANDLER(recv_socket) if (status == STATUS_PENDING && !req->force_async && sock->nonblocking) status = STATUS_DEVICE_NOT_READY; + sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); + sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { set_error( status ); - async_set_initial_status_callback( async, recv_socket_initial_callback, (void *)(long)req->oob ); - if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT ); if (status == STATUS_PENDING || status == STATUS_ALERTED) queue_async( &sock->read_q, async ); + /* always reselect; we changed reported_events above */ + sock_reselect( sock ); + reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); reply->nonblocking = sock->nonblocking; -- 2.34.1