[PATCH v3] ws2_32: Check if waiting asyncs exist in WSARecv/WSASend
WSARecv call should not intercept message for the waiting overlapped WSARecv. To do that, We should know if waiting asyncs exist before trying ws2_recv/ws2_send. Signed-off-by: Dongwan Kim <kdw6485(a)gmail.com> --- dlls/ws2_32/socket.c | 31 +++++++++++++++++++++++++++++-- include/wine/server_protocol.h | 20 +++++++++++++++++++- server/protocol.def | 8 ++++++++ server/request.h | 8 ++++++++ server/sock.c | 26 ++++++++++++++++++++++++++ server/trace.c | 12 ++++++++++++ 6 files changed, 102 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 3a8bfa0ce41..cf76d96894c 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -2016,6 +2016,23 @@ static void WINAPI ws2_async_apc( void *arg, IO_STATUS_BLOCK *iosb, ULONG reserv release_async_io( &wsa->io ); } +/* query if the socket has pending asyncs */ +static int WS_QueryAsyncWaiting( SOCKET sock, int type ) +{ + int result=0; + + SERVER_START_REQ( query_async_waiting ) + { + req->handle = wine_server_obj_handle( SOCKET2HANDLE(sock) ); + req->type = type; + wine_server_call(req); + result = reply->state; + } + SERVER_END_REQ; + return result; + +} + /*********************************************************************** * WS2_recv (INTERNAL) * @@ -5049,7 +5066,12 @@ static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount, } flags = convert_flags(dwFlags); - n = WS2_send( fd, wsa, flags ); + if(WS_QueryAsyncWaiting(s , ASYNC_TYPE_WRITE)) + { + n = -1; errno = EAGAIN; + } + else + n = WS2_send( fd, wsa, flags ); if (n == -1 && errno != EAGAIN) { err = wsaErrno(); @@ -6129,7 +6151,12 @@ static int WS2_recv_base( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount, flags = convert_flags(wsa->flags); for (;;) { - n = WS2_recv( fd, wsa, flags ); + if(WS_QueryAsyncWaiting(s, ASYNC_TYPE_READ)) + { + n = -1; errno = EAGAIN; + } + else + n = WS2_recv( fd, wsa, flags ); if (n == -1) { /* Unix-like systems return EINVAL when attempting to read OOB data from diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index 9fe826918fa..8bdae3c386e 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -5403,6 +5403,21 @@ struct get_next_thread_reply char __pad_12[4]; }; +struct query_async_waiting_request +{ + struct request_header __header; + obj_handle_t handle; + int type; + char __pad_20[4]; + +}; +struct query_async_waiting_reply +{ + struct reply_header __header; + int state; + char __pad_12[4]; +}; + enum request { @@ -5680,6 +5695,7 @@ enum request REQ_suspend_process, REQ_resume_process, REQ_get_next_thread, + REQ_query_async_waiting, REQ_NB_REQUESTS }; @@ -5961,6 +5977,7 @@ union generic_request struct suspend_process_request suspend_process_request; struct resume_process_request resume_process_request; struct get_next_thread_request get_next_thread_request; + struct query_async_waiting_request query_async_waiting_request; }; union generic_reply { @@ -6240,11 +6257,12 @@ union generic_reply struct suspend_process_reply suspend_process_reply; struct resume_process_reply resume_process_reply; struct get_next_thread_reply get_next_thread_reply; + struct query_async_waiting_reply query_async_waiting_reply; }; /* ### protocol_version begin ### */ -#define SERVER_PROTOCOL_VERSION 700 +#define SERVER_PROTOCOL_VERSION 701 /* ### protocol_version end ### */ diff --git a/server/protocol.def b/server/protocol.def index eaffa886f21..72a05a0cd58 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3711,3 +3711,11 @@ struct handle_info @REPLY obj_handle_t handle; /* next thread handle */ @END + +/* Query if there are waiting asyncs of the socket */ +(a)REQ(query_async_waiting) + obj_handle_t handle; /* socket handle */ + int type; /* async type */ +(a)REPLY + int state; /* waiting async exists */ +(a)END diff --git a/server/request.h b/server/request.h index ec5595bedf2..450667e9cf3 100644 --- a/server/request.h +++ b/server/request.h @@ -393,6 +393,7 @@ DECL_HANDLER(terminate_job); DECL_HANDLER(suspend_process); DECL_HANDLER(resume_process); DECL_HANDLER(get_next_thread); +DECL_HANDLER(query_async_waiting); #ifdef WANT_REQUEST_HANDLERS @@ -673,6 +674,7 @@ static const req_handler req_handlers[REQ_NB_REQUESTS] = (req_handler)req_suspend_process, (req_handler)req_resume_process, (req_handler)req_get_next_thread, + (req_handler)req_query_async_waiting, }; C_ASSERT( sizeof(abstime_t) == 8 ); @@ -2236,6 +2238,12 @@ C_ASSERT( FIELD_OFFSET(struct get_next_thread_request, flags) == 28 ); C_ASSERT( sizeof(struct get_next_thread_request) == 32 ); C_ASSERT( FIELD_OFFSET(struct get_next_thread_reply, handle) == 8 ); C_ASSERT( sizeof(struct get_next_thread_reply) == 16 ); +C_ASSERT( FIELD_OFFSET(struct query_async_waiting_request, handle) == 12 ); +C_ASSERT( FIELD_OFFSET(struct query_async_waiting_request, type) == 16 ); +C_ASSERT( sizeof(struct query_async_waiting_request) == 24 ); +C_ASSERT( FIELD_OFFSET(struct query_async_waiting_reply, state) == 8 ); +C_ASSERT( sizeof(struct query_async_waiting_reply) == 16 ); + #endif /* WANT_REQUEST_HANDLERS */ diff --git a/server/sock.c b/server/sock.c index 6891b5cbc57..147cb106bb0 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1890,3 +1890,29 @@ DECL_HANDLER(get_socket_info) release_object( &sock->obj ); } + +DECL_HANDLER( query_async_waiting ) +{ + struct sock *sock; + struct async *async; + reply->state = 0; + + if (!(sock = (struct sock *)get_handle_obj( current->process, req->handle, + FILE_READ_ATTRIBUTES, &sock_ops))) return; + if (get_unix_fd( sock->fd ) == -1) return; + + if (is_fd_overlapped( sock->fd )) + { + if(req->type == ASYNC_TYPE_READ && ( async = find_pending_async(&sock->read_q) )) { + reply->state = 1; + release_object(async); + } + if(req->type == ASYNC_TYPE_WRITE && ( async = find_pending_async(&sock->write_q )) ) + { + reply->state = 1; + release_object(async); + } + } + release_object( &sock->obj ); + +} diff --git a/server/trace.c b/server/trace.c index e40f0769a35..f522980c6da 100644 --- a/server/trace.c +++ b/server/trace.c @@ -4478,6 +4478,15 @@ static void dump_get_next_thread_reply( const struct get_next_thread_reply *req { fprintf( stderr, " handle=%04x", req->handle ); } +static void dump_query_async_waiting_request( const struct query_async_waiting_request *req ) +{ + fprintf( stderr, " handle=%04x", req->handle ); + fprintf( stderr, ", type=%d", req->type ); +} +static void dump_query_async_waiting_reply( const struct query_async_waiting_reply *req ) +{ + fprintf( stderr, " state=%d", req->state ); +} static const dump_func req_dumpers[REQ_NB_REQUESTS] = { (dump_func)dump_new_process_request, @@ -4754,6 +4763,7 @@ static const dump_func req_dumpers[REQ_NB_REQUESTS] = { (dump_func)dump_suspend_process_request, (dump_func)dump_resume_process_request, (dump_func)dump_get_next_thread_request, + (dump_func)dump_query_async_waiting_request, }; static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { @@ -5031,6 +5041,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { NULL, NULL, (dump_func)dump_get_next_thread_reply, + (dump_func)dump_query_async_waiting_reply, }; static const char * const req_names[REQ_NB_REQUESTS] = { @@ -5308,6 +5319,7 @@ static const char * const req_names[REQ_NB_REQUESTS] = { "suspend_process", "resume_process", "get_next_thread", + "query_async_waiting", }; static const struct -- 2.30.2
On 5/11/21 10:03 PM, Dongwan Kim wrote:
WSARecv call should not intercept message for the waiting overlapped WSARecv. To do that, We should know if waiting asyncs exist before trying ws2_recv/ws2_send.
Hello, thanks for the patch! I don't think this is quite the right way to handle this architecturally. However, more importantly, this introduces extra overhead into what can already be a rather slow operation. Hence two questions: (1) Is there an application that actually breaks because of this? (If not, I think it's probably not worth doing.) (2) Can you write a test case—ideally to be submitted to Wine's test suite along with the patch, but failing that a standalone program—to demonstrate that this really is true?
Thanks for your clear review. (1) I found this misbehavior from KakaoTalk which is the major messenger app in South Korea. Login rarely fails when I execute the app in wine. Also I made simple server/client programs that the message ordering always fails. (2) I made the simplified test case based on the programs I mentioned, and I will submit it in next revision.
On 5/18/21 09:03, Dongwan Kim wrote:
Thanks for your clear review.
(1) I found this misbehavior from KakaoTalk which is the major messenger app in South Korea. Login rarely fails when I execute the app in wine.
Also I made simple server/client programs that the message ordering always fails.
(2) I made the simplified test case based on the programs I mentioned, and I will submit it in next revision.
As Zebediah noted, this will have a noticable performance impact (additional server call) for all the socket send and receive operation, which is very unfortunate, to say the least. Is it maybe possible to introduce something like a shared memory section to store the dynamic state for the state that can't be cached locally (the question is also for Zebediah)? Maybe it will be easier to do once the winsock IO move to ntdll is complete?
On 5/18/21 4:19 AM, Paul Gofman wrote:
On 5/18/21 09:03, Dongwan Kim wrote:
Thanks for your clear review.
(1) I found this misbehavior from KakaoTalk which is the major messenger app in South Korea. Login rarely fails when I execute the app in wine.
Also I made simple server/client programs that the message ordering always fails.
(2) I made the simplified test case based on the programs I mentioned, and I will submit it in next revision.
As Zebediah noted, this will have a noticable performance impact (additional server call) for all the socket send and receive operation, which is very unfortunate, to say the least. Is it maybe possible to introduce something like a shared memory section to store the dynamic state for the state that can't be cached locally (the question is also for Zebediah)? Maybe it will be easier to do once the winsock IO move to ntdll is complete?
I have suspicions that native ws2_32 stores some state on the client side, probably in shared memory, but I'm not sure. I think that shared memory will definitely make things uglier, though. For what it's worth, currently most instances of recv() take two server calls (or more) anyway. The only case that doesn't is when data is already available and the operation is not overlapped. I believe it should be possible to add protection against incorrect ordering in an architecturally sensible way and without adding an extra server call for any other case.
participants (3)
-
Dongwan Kim -
Paul Gofman -
Zebediah Figura (she/her)