[PATCH 0/3] MR225: server: Fix races related to socket errors
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). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225
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
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..c4bbd388097 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_CONNECT_ERR]; + } + 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
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 c4bbd388097..466899af064 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
[0001-tests.txt](/uploads/2142824c849655d3e830138d938a9b17/0001-tests.txt) Attaching test that demonstrates the problem. It adds sleeps to wineserver so the error is much more likely to occur. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1845
Jinoh Kang (@iamahuman) commented about server/sock.c:
+ 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_CONNECT_ERR]; Do we want the following instead?
```suggestion:-0+0 *error = sock->errors[AFD_POLL_BIT_ACCEPT]; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1853
On Fri Jun 10 16:53:21 2022 +0000, Jinoh Kang wrote:
Do we want the following instead? ```suggestion:-0+0 *error = sock->errors[AFD_POLL_BIT_ACCEPT]; ``` Yes, that was the intention, I'll send fixed version.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1856
participants (3)
-
Jinoh Kang (＠iamahuman) -
Piotr Caban
-
Piotr Caban (＠piotr)