[PATCH 0/2] MR4967: winsock: Stop racings send forcing async results and reporting socket as not write ready in select().
When two sends are performed in parallel on a socket the send which came second to server's (socked_send) will always considered blocking if the first send hasn't performed set_async_direct_result() in client's ntdll yet. Similarly, if select() is coming shortly after send() it may report socket as not write ready. We don't care for avoiding a race here because if those two sends or send / select are not synced by app it is unlikely possible for an app to depend on which gets processed first, and the one which was called earlier in wall time now also is not guaranteed to be actually processed first. The patches fix Tekken 8 which gets frequently disconnected from multiplayer matches. The game treats WSAEWOULDBLOCK from send() on its non-blocking UDP socket, as well as the same socket not reported as write ready from select as a disconnection condition. Those conditions normally (outside of actual network problem / contention) only happen due to the races described above (while the game performs send() concurrently from different threads or is racing send with select). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 42 ++++++++++++++++++++++++++++++++++++++++ server/async.c | 10 ++++++++++ server/file.h | 1 + server/sock.c | 2 +- 4 files changed, 54 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index b43d7d61867..d37c25f9225 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -3106,6 +3106,29 @@ static test_setup tests [] = } }; +struct send_udp_thread_param +{ + int sock; + HANDLE start_event; +}; + +static DWORD WINAPI send_udp_thread( void *param ) +{ + struct send_udp_thread_param *p = param; + static char buf[256]; + unsigned int i; + int ret; + + WaitForSingleObject( p->start_event, INFINITE ); + for (i = 0; i < 256; ++i) + { + ret = send( p->sock, buf, sizeof(buf), 0 ); + ok( ret == sizeof(buf), "got %d, error %u, i %u.\n", ret, WSAGetLastError(), i ); + } + + return 0; +} + static void test_UDP(void) { /* This function tests UDP sendto() and recvfrom(). UDP is unreliable, so it is @@ -3117,6 +3140,9 @@ static void test_UDP(void) int ss, i, n_recv, n_sent, ret; struct sockaddr_in addr; int sock; + struct send_udp_thread_param udp_thread_param; + HANDLE thread; + memset (buf,0,sizeof(buf)); for ( i = NUM_UDP_PEERS - 1; i >= 0; i-- ) { @@ -3174,6 +3200,22 @@ static void test_UDP(void) ok( ret == sizeof(buf), "got %d, error %u.\n", ret, WSAGetLastError() ); } + /* Test sending packets in parallel (mostly a regression test for Wine async handling race conditions). */ + set_blocking( sock, FALSE ); + + udp_thread_param.sock = sock; + udp_thread_param.start_event = CreateEventW( NULL, FALSE, FALSE, NULL ); + thread = CreateThread( NULL, 0, send_udp_thread, &udp_thread_param, 0, NULL ); + SetEvent( udp_thread_param.start_event ); + for (i = 0; i < 256; ++i) + { + ret = send( sock, buf, sizeof(buf), 0 ); + ok( ret == sizeof(buf), "got %d, error %u, i %u.\n", ret, WSAGetLastError(), i ); + } + WaitForSingleObject( thread, INFINITE ); + CloseHandle( thread ); + CloseHandle( udp_thread_param.start_event ); + closesocket(sock); } diff --git a/server/async.c b/server/async.c index 9cb251df5ce..80129ac0ac3 100644 --- a/server/async.c +++ b/server/async.c @@ -551,6 +551,16 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota } } +int async_queue_has_waiting_asyncs( struct async_queue *queue ) +{ + struct async *async; + + LIST_FOR_EACH_ENTRY( async, &queue->queue, struct async, queue_entry ) + if (!async->unknown_status) return 1; + + return 0; +} + /* check if an async operation is waiting to be alerted */ int async_waiting( struct async_queue *queue ) { diff --git a/server/file.h b/server/file.h index 39a833cd105..7f2d1637863 100644 --- a/server/file.h +++ b/server/file.h @@ -237,6 +237,7 @@ extern void set_async_pending( struct async *async ); extern void async_set_initial_status( struct async *async, unsigned int status ); extern void async_wake_obj( struct async *async ); extern int async_waiting( struct async_queue *queue ); +extern int async_queue_has_waiting_asyncs( struct async_queue *queue ); extern void async_terminate( struct async *async, unsigned int status ); extern void async_request_complete( struct async *async, unsigned int status, data_size_t result, data_size_t out_size, void *out_data ); diff --git a/server/sock.c b/server/sock.c index c34fd3eb5eb..a884179c604 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3945,7 +3945,7 @@ DECL_HANDLER(send_socket) if (bind_errno) status = sock_get_ntstatus( bind_errno ); else if (sock->wr_shutdown) status = STATUS_PIPE_DISCONNECTED; - else if (!async_queued( &sock->write_q )) + else if (!async_queue_has_waiting_asyncs( &sock->write_q )) { /* If write_q is not empty, we cannot really tell if the already queued * asyncs will not consume all available space; if there's no space -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4967
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 12 ++++++++++++ server/sock.c | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index d37c25f9225..b0287fdd004 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -3115,13 +3115,19 @@ struct send_udp_thread_param static DWORD WINAPI send_udp_thread( void *param ) { struct send_udp_thread_param *p = param; + static const TIMEVAL timeout_zero = {0}; static char buf[256]; + fd_set writefds; unsigned int i; int ret; WaitForSingleObject( p->start_event, INFINITE ); for (i = 0; i < 256; ++i) { + FD_ZERO(&writefds); + FD_SET(p->sock, &writefds); + ret = select( 1, NULL, &writefds, NULL, &timeout_zero ); + ok( ret == 1, "got %d, i %u.\n", ret, i ); ret = send( p->sock, buf, sizeof(buf), 0 ); ok( ret == sizeof(buf), "got %d, error %u, i %u.\n", ret, WSAGetLastError(), i ); } @@ -3135,6 +3141,7 @@ static void test_UDP(void) possible that this test fails due to dropped packets. */ /* peer 0 receives data from all other peers */ + static const TIMEVAL timeout_zero = {0}; struct sock_info peer[NUM_UDP_PEERS]; char buf[16]; int ss, i, n_recv, n_sent, ret; @@ -3142,6 +3149,7 @@ static void test_UDP(void) int sock; struct send_udp_thread_param udp_thread_param; HANDLE thread; + fd_set writefds; memset (buf,0,sizeof(buf)); @@ -3211,6 +3219,10 @@ static void test_UDP(void) { ret = send( sock, buf, sizeof(buf), 0 ); ok( ret == sizeof(buf), "got %d, error %u, i %u.\n", ret, WSAGetLastError(), i ); + FD_ZERO(&writefds); + FD_SET(sock, &writefds); + ret = select( 1, NULL, &writefds, NULL, &timeout_zero ); + ok( ret == 1, "got %d, i %u.\n", ret, i ); } WaitForSingleObject( thread, INFINITE ); CloseHandle( thread ); diff --git a/server/sock.c b/server/sock.c index a884179c604..3ad1ce39194 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1231,7 +1231,7 @@ static int sock_dispatch_asyncs( struct sock *sock, int event, int error ) event &= ~(POLLIN | POLLPRI); } - if ((event & POLLOUT) && async_queued( &sock->write_q )) + if ((event & POLLOUT) && async_queue_has_waiting_asyncs( &sock->write_q )) { if (async_waiting( &sock->write_q )) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4967
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142516 Your paranoid android. === w1064_2qxl (64 bit report) === ws2_32: sock.c:13124: Test failed: wait timed out sock.c:13137: Test failed: got size 5 sock.c:13138: Test failed: got "datad" sock.c:12972: Test failed: got size 0 sock.c:12973: Test failed: got "" === w1064_adm (64 bit report) === ws2_32: sock.c:13108: Test failed: wait timed out sock.c:13124: Test failed: wait timed out sock.c:13137: Test failed: got size 5 sock.c:13138: Test failed: got "datad" sock.c:12972: Test failed: got size 0 sock.c:12972: Test failed: got size 0 sock.c:12973: Test failed: got "" sock.c:12973: Test failed: got ""
When two sends are performed in parallel on a socket the send which came second to server's (socked_send) will always considered blocking if the first send hasn't performed set_async_direct_result() in client's ntdll yet. Similarly, if select() is coming shortly after send() it may report socket as not write ready. We don't care for avoiding a race here because if those two sends or send / select are not synced by app it is unlikely possible for an app to depend on which gets processed first, and the one which was called earlier in wall time now also is not guaranteed to be actually processed first.
Yeah. I'd put it in even stronger terms, actually. The race that's being fixed here is due to the gap between send_socket and set_async_direct_result, which simply isn't a gap that exists on Windows. It all happens inside of NtDeviceIoControlFile(); there are no synchronization points that exist. I wish dearly that there were a way to introduce a synchronous model for server calls. Asyncs are far, far too complex, and the price of that complexity is quite evident to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967#note_59883
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967
Jinoh Kang (@iamahuman) commented about server/sock.c:
if (bind_errno) status = sock_get_ntstatus( bind_errno ); else if (sock->wr_shutdown) status = STATUS_PIPE_DISCONNECTED; - else if (!async_queued( &sock->write_q )) + else if (!async_queue_has_waiting_asyncs( &sock->write_q ))
I wonder if we just want to special-case UDP (and other unordered-packet transports) here instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967#note_60032
On Mon Feb 5 15:38:23 2024 +0000, Jinoh Kang wrote:
I wonder if we just want to special-case UDP (and other unordered-packet transports) here instead. Actually, I can think of a scenario where the short pulse of "unknown_status" becomes problematic even for in-sequence transports.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967#note_60047
Ideally I suppose most other uses of `async_queued` could be substituted with `async_queue_has_waiting_asyncs` for consistency as well, but I'm not sure exactly which. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967#note_60049
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4967
participants (5)
-
Jinoh Kang (＠iamahuman) -
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)