[PATCH v4 0/8] Fix sock_send reordering issue (#52401)

3 Mar 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
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

    

Jinoh Kang

Marvin

tags

participants (2)