[PATCH v3 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). -- v3: 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
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 | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/server/sock.c b/server/sock.c index 8a0f3198c4e..439801a8cfa 100644 --- a/server/sock.c +++ b/server/sock.c @@ -631,13 +631,27 @@ static void sock_wake_up( struct sock *sock ) } } -static inline int sock_error( struct fd *fd ) +static inline int sock_error( struct sock *sock ) { - unsigned int optval = 0; - socklen_t optlen = sizeof(optval); + int error = 0; + socklen_t len = sizeof(error); - getsockopt( get_unix_fd(fd), SOL_SOCKET, SO_ERROR, (void *) &optval, &optlen); - return optval; + getsockopt( get_unix_fd(sock->fd), SOL_SOCKET, SO_ERROR, (void *)&error, &len); + 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 error; } static void free_accept_req( void *private ) @@ -1120,9 +1134,9 @@ static void sock_poll_event( struct fd *fd, int event ) case SOCK_CONNECTING: if (event & (POLLERR|POLLHUP)) { + error = sock_error( sock ); sock->state = SOCK_UNCONNECTED; event &= ~POLLOUT; - error = sock_error( fd ); } else if (event & POLLOUT) { @@ -1133,7 +1147,7 @@ static void sock_poll_event( struct fd *fd, int event ) case SOCK_LISTENING: if (event & (POLLERR|POLLHUP)) - error = sock_error( fd ); + error = sock_error( sock ); break; case SOCK_CONNECTED: @@ -3125,7 +3139,7 @@ static void poll_socket( struct sock *poll_sock, struct async *async, int exclus { signaled = TRUE; req->sockets[i].flags = flags; - req->sockets[i].status = sock_get_ntstatus( sock_error( sock->fd ) ); + req->sockets[i].status = sock_get_ntstatus( sock_error( sock ) ); } /* 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 | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/server/sock.c b/server/sock.c index 439801a8cfa..b11ebddb51d 100644 --- a/server/sock.c +++ b/server/sock.c @@ -2797,7 +2797,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)) @@ -2806,12 +2805,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) - { - set_error( sock_get_ntstatus( errno ) ); - return; - } - + error = sock_error( sock ); if (!error) { for (i = 0; i < ARRAY_SIZE( sock->errors ); ++i) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/225
On Sat Jun 11 00:00:13 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` 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.
I've pushed version without error checking.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1898
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225
participants (3)
-
Piotr Caban -
Piotr Caban (＠piotr)
-
Zebediah Figura (＠zfigura)