[PATCH v2 0/8] Fix sock_send reordering issue (#52401)
Jinoh Kang
jinoh.kang.kr at gmail.com
Mon Feb 21 11:10:32 CST 2022
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
[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: Add async_initial_status_callback.
server: Defer postprocessing until after setting initial status in
recv_socket handler.
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 | 52 +++++++++++++---
server/file.h | 2 +
server/protocol.def | 6 +-
server/sock.c | 110 +++++++++++++++++++--------------
8 files changed, 204 insertions(+), 93 deletions(-)
Interdiff:
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
index ba6d65244ec..11ce652fed5 100644
--- a/dlls/ntdll/unix/socket.c
+++ b/dlls/ntdll/unix/socket.c
@@ -874,6 +874,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
NTSTATUS status;
unsigned int i;
ULONG options;
+ data_size_t data_size;
BOOL nonblocking, alerted;
async_size = offsetof( struct async_send_ioctl, iov[count] );
@@ -908,8 +909,21 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
async->iov_cursor = 0;
async->sent_len = 0;
+ data_size = 0;
+ for (i = 0; i < count; ++i)
+ {
+ SIZE_T len = async->iov[i].iov_len;
+ if (len > (SIZE_T)(data_size_t)-1 || (data_size_t)(data_size + len) < data_size)
+ {
+ release_fileio( &async->io );
+ return STATUS_NO_MEMORY;
+ }
+ data_size += len;
+ }
+
SERVER_START_REQ( send_socket )
{
+ req->data_size = data_size;
req->force_async = force_async;
req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
status = wine_server_call( req );
@@ -925,6 +939,13 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
status = try_send( fd, async );
if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking))
status = STATUS_PENDING;
+
+ /* If we had a short write and the socket is nonblocking (and we are
+ * not trying to force the operation to be asynchronous), return
+ * success. Windows actually refuses to send any data in this case,
+ * and returns EWOULDBLOCK, but we have no way of doing that. */
+ if (status == STATUS_DEVICE_NOT_READY && async->sent_len)
+ status = STATUS_SUCCESS;
}
if (status != STATUS_PENDING)
@@ -1106,6 +1127,7 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc,
SERVER_START_REQ( send_socket )
{
+ req->data_size = async->head_len + async->file_len + async->tail_len;
req->force_async = 1;
req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
status = wine_server_call( req );
diff --git a/server/async.c b/server/async.c
index 39e65d65ad3..ded4f8392d2 100644
--- a/server/async.c
+++ b/server/async.c
@@ -146,7 +146,7 @@ static void set_async_initial_status( struct async *async, unsigned int status )
async->initial_status = status;
if (async->initial_status_callback)
{
- async->initial_status_callback(async->initial_status_callback_private, status);
+ async->initial_status_callback( async->initial_status_callback_private, async, async->fd, status );
async->initial_status_callback = NULL;
}
}
@@ -783,9 +783,13 @@ DECL_HANDLER(set_async_direct_result)
{
struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
unsigned int status = req->status;
+ apc_param_t information = req->information;
if (!async) return;
+ /* we only return an async handle for asyncs created via create_request_async() */
+ assert( async->iosb );
+
if (!async->unknown_status || !async->terminated || !async->alerted)
{
set_error( STATUS_INVALID_PARAMETER );
@@ -793,6 +797,8 @@ 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;
@@ -811,7 +817,7 @@ DECL_HANDLER(set_async_direct_result)
* therefore, we can do async_set_result() directly and let the client skip
* waiting on wait_handle.
*/
- async_set_result( &async->obj, status, req->information );
+ async_set_result( &async->obj, status, information );
/* close wait handle here to avoid extra server round trip, if the I/O
* either has completed, or is pending and not blocking.
diff --git a/server/file.h b/server/file.h
index 1927374a164..893d4ac6411 100644
--- a/server/file.h
+++ b/server/file.h
@@ -219,7 +219,7 @@ extern struct object *create_serial( struct fd *fd );
/* async I/O functions */
typedef void (*async_completion_callback)( void *private );
-typedef void (*async_initial_status_callback)( void *private, unsigned int status );
+typedef void (*async_initial_status_callback)( void *private, struct async *async, struct fd *fd, unsigned int status );
extern void free_async_queue( struct async_queue *queue );
extern struct async *create_async( struct fd *fd, struct thread *thread, const async_data_t *data, struct iosb *iosb );
diff --git a/server/protocol.def b/server/protocol.def
index c2b6735b659..5d8038d58b9 100644
--- a/server/protocol.def
+++ b/server/protocol.def
@@ -1461,6 +1461,7 @@ enum server_fd_type
/* Perform a send on a socket */
@REQ(send_socket)
+ data_size_t data_size; /* total number of bytes to send */
async_data_t async; /* async I/O parameters */
int force_async; /* Force asynchronous mode? */
@REPLY
@@ -2166,8 +2167,8 @@ enum message_type
/* Notify direct completion of async and close the wait handle if not blocking */
@REQ(set_async_direct_result)
obj_handle_t handle; /* wait handle */
- unsigned int status; /* completion status */
apc_param_t information; /* IO_STATUS_BLOCK Information */
+ unsigned int status; /* completion status */
int mark_pending; /* set async to pending before completion? */
@REPLY
obj_handle_t handle; /* wait handle, or NULL if closed */
diff --git a/server/sock.c b/server/sock.c
index 5fa909f3fbc..2706c2d20ed 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -3393,22 +3393,14 @@ 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, unsigned int status )
+static void recv_socket_initial_callback( void *private, struct async *async, struct fd *fd, unsigned int status )
{
- struct sock *sock = (struct sock *)private;
- sock->pending_events &= ~AFD_POLL_READ;
- sock->reported_events &= ~AFD_POLL_READ;
- sock_reselect( sock );
- release_object( sock );
-}
+ struct sock *sock = get_fd_user( fd );
+ int is_oob = (int)(long)private;
-static void recv_socket_initial_callback_oob( void *private, unsigned int status )
-{
- struct sock *sock = (struct sock *)private;
- sock->pending_events &= ~AFD_POLL_OOB;
- sock->reported_events &= ~AFD_POLL_OOB;
+ 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 );
- release_object( sock );
}
DECL_HANDLER(recv_socket)
@@ -3452,10 +3444,7 @@ DECL_HANDLER(recv_socket)
{
set_error( status );
- async_set_initial_status_callback( async,
- req->oob ? recv_socket_initial_callback_oob
- : recv_socket_initial_callback,
- grab_object( sock ));
+ async_set_initial_status_callback( async, recv_socket_initial_callback, (void *)(long)req->oob );
if (timeout)
async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
@@ -3471,9 +3460,17 @@ DECL_HANDLER(recv_socket)
release_object( sock );
}
-static void send_socket_initial_callback( void *private, unsigned int status )
+static void send_socket_initial_callback( void *private, struct async *async, struct fd *fd, unsigned int status )
{
- struct sock *sock = (struct sock *)private;
+ struct sock *sock = get_fd_user( fd );
+ struct iosb *iosb;
+ int is_short_write = 0;
+
+ if ((iosb = async_get_iosb( async )))
+ {
+ is_short_write = iosb->result < (unsigned long)private;
+ release_object( iosb );
+ }
if (sock->type == WS_SOCK_DGRAM)
{
@@ -3481,20 +3478,21 @@ static void send_socket_initial_callback( void *private, unsigned int status )
union unix_sockaddr unix_addr;
socklen_t unix_len = sizeof(unix_addr);
- if (!sock->bound && !getsockname( get_unix_fd( sock->fd ), &unix_addr.addr, &unix_len ))
+ if (!sock->bound && !getsockname( get_unix_fd( fd ), &unix_addr.addr, &unix_len ))
sock->addr_len = sockaddr_from_unix( &unix_addr, &sock->addr.addr, sizeof(sock->addr) );
sock->bound = 1;
}
- if (status != STATUS_SUCCESS)
+ if (status != STATUS_SUCCESS || is_short_write)
{
- /* send() calls only clear and reselect events if unsuccessful. */
+ /* send() calls only clear and reselect events if unsuccessful.
+ * Also treat short writes as being unsuccessful.
+ */
sock->pending_events &= ~AFD_POLL_WRITE;
sock->reported_events &= ~AFD_POLL_WRITE;
}
sock_reselect( sock );
- release_object( sock );
}
DECL_HANDLER(send_socket)
@@ -3538,7 +3536,7 @@ DECL_HANDLER(send_socket)
{
set_error( status );
- async_set_initial_status_callback( async, send_socket_initial_callback, grab_object( sock ));
+ async_set_initial_status_callback( async, send_socket_initial_callback, (void *)(unsigned long)req->data_size );
if (timeout)
async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
--
2.34.1
More information about the wine-devel
mailing list