[PATCH v2 0/3] MR225: server: Fix races related to socket errors

[Piotr Caban (＠piotr)]

Calling ioctl(..., SO_ERROR, ...) clears pending socket error. Because of that we may clear the error in poll_socket or IOCTL_AFD_GET_SO_ERROR. This leads to failures during Battle.net installation (it's a race that I can only reproduce on slow machines).

--
  v2: server: Don't reset socket error in IOCTL_AFD_GET_SO_ERROR.
      server: Don't reset socket error in poll_socket.
      server: Always return Win32 error code from IOCTL_AFD_GET_SO_ERROR.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/225

[PATCH v2 1/3] server: Always return Win32 error code from IOCTL_AFD_GET_SO_ERROR.

[Piotr Caban]

From: Piotr Caban <piotr(a)codeweavers.com>

Signed-off-by: Piotr Caban <piotr(a)codeweavers.com>
---
 server/sock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/server/sock.c b/server/sock.c
index 77a88a7fcf7..8a0f3198c4e 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -2804,12 +2804,13 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
             {
                 if (sock->errors[i])
                 {
-                    error = sock_get_error( sock->errors[i] );
+                    error = sock->errors[i];
                     break;
                 }
             }
         }
 
+        error = sock_get_error( error );
         set_reply_data( &error, sizeof(error) );
         return;
     }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/225

[PATCH v2 2/3] server: Don't reset socket error in poll_socket.

[Piotr Caban]

From: Piotr Caban <piotr(a)codeweavers.com>

Otherwise socket error may be cleared in poll_socket causing ioctl SO_ERROR calls to return no error.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51433
Signed-off-by: Piotr Caban <piotr(a)codeweavers.com>
---
 server/sock.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/server/sock.c b/server/sock.c
index 8a0f3198c4e..7334300e33a 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -631,13 +631,28 @@ static void sock_wake_up( struct sock *sock )
     }
 }
 
-static inline int sock_error( struct fd *fd )
+static inline int sock_error( struct sock *sock, int *error )
 {
-    unsigned int optval = 0;
-    socklen_t optlen = sizeof(optval);
+    socklen_t len = sizeof(*error);
 
-    getsockopt( get_unix_fd(fd), SOL_SOCKET, SO_ERROR, (void *) &optval, &optlen);
-    return optval;
+    if (getsockopt( get_unix_fd(sock->fd), SOL_SOCKET, SO_ERROR, (void *)error, &len) < 0)
+        return -1;
+
+    if (sock->state == SOCK_CONNECTING)
+    {
+        if (*error)
+            sock->errors[AFD_POLL_BIT_CONNECT_ERR] = *error;
+        else
+            *error = sock->errors[AFD_POLL_BIT_CONNECT_ERR];
+    }
+    else if (sock->state == SOCK_LISTENING)
+    {
+        if (*error)
+            sock->errors[AFD_POLL_BIT_ACCEPT] = *error;
+        else
+            *error = sock->errors[AFD_POLL_BIT_ACCEPT];
+    }
+    return 0;
 }
 
 static void free_accept_req( void *private )
@@ -1120,9 +1135,9 @@ static void sock_poll_event( struct fd *fd, int event )
     case SOCK_CONNECTING:
         if (event & (POLLERR|POLLHUP))
         {
+            sock_error( sock, &error );
             sock->state = SOCK_UNCONNECTED;
             event &= ~POLLOUT;
-            error = sock_error( fd );
         }
         else if (event & POLLOUT)
         {
@@ -1133,7 +1148,7 @@ static void sock_poll_event( struct fd *fd, int event )
 
     case SOCK_LISTENING:
         if (event & (POLLERR|POLLHUP))
-            error = sock_error( fd );
+            sock_error( sock, &error );
         break;
 
     case SOCK_CONNECTED:
@@ -3123,9 +3138,12 @@ static void poll_socket( struct sock *poll_sock, struct async *async, int exclus
 
         if (flags)
         {
+            int error = 0;
+
             signaled = TRUE;
             req->sockets[i].flags = flags;
-            req->sockets[i].status = sock_get_ntstatus( sock_error( sock->fd ) );
+            sock_error( sock, &error );
+            req->sockets[i].status = sock_get_ntstatus( error );
         }
 
         /* FIXME: do other error conditions deserve a similar treatment? */
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/225

[PATCH v2 2/3] server: Don't reset socket error in poll_socket.

[Zebediah Figura]

On 6/10/22 12:33, Piotr Caban wrote:
> -static inline int sock_error( struct fd *fd )
> +static inline int sock_error( struct sock *sock, int *error )
>   {
> -    unsigned int optval = 0;
> -    socklen_t optlen = sizeof(optval);
> +    socklen_t len = sizeof(*error);
>
> -    getsockopt( get_unix_fd(fd), SOL_SOCKET, SO_ERROR, (void *) &optval, &optlen);
> -    return optval;
> +    if (getsockopt( get_unix_fd(sock->fd), SOL_SOCKET, SO_ERROR, (void *)error, &len) < 0)
> +        return -1;

I'd personally just throw away the getsockopt() return value check 
instead; it should never fail.

[PATCH v2 3/3] server: Don't reset socket error in IOCTL_AFD_GET_SO_ERROR.

[Piotr Caban]

From: Piotr Caban <piotr(a)codeweavers.com>

Signed-off-by: Piotr Caban <piotr(a)codeweavers.com>
---
 server/sock.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/server/sock.c b/server/sock.c
index 7334300e33a..0bcad4c65bd 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -2798,7 +2798,6 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
     case IOCTL_AFD_WINE_GET_SO_ERROR:
     {
         int error;
-        socklen_t len = sizeof(error);
         unsigned int i;
 
         if (get_reply_max_size() < sizeof(error))
@@ -2807,7 +2806,7 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
             return;
         }
 
-        if (getsockopt( unix_fd, SOL_SOCKET, SO_ERROR, (char *)&error, &len ) < 0)
+        if (sock_error( sock, &error ) < 0)
         {
             set_error( sock_get_ntstatus( errno ) );
             return;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/225