This fixes the send part of the issue #52401 [1]: "Improper synchronization in sock_recv/sock_send leads to arbitrary reordering of completion of I/O requests", which was initially reported (outside Bugzilla) by Dongwan Kim [2].
Changelog: v1 -> v2: - drop patch "server: Compact struct set_async_direct_result_reply." - add async and fd arguments to async_initial_status_callback - recv_socket_initial_callback: pass OOB flag via private instead of implementing two separate functions for OOB and non-OOB - detect short write in send_socket_initial_callback - preserve the behaviour of returning success if we had a short write and the socket is nonblocking and force_async is unset v2 -> v3: - fix typo in comment v3 -> v4: - fix typo in commit message - address feedback - drop patch "server: Defer postprocessing until after setting initial status in recv_socket handler." v4 -> v5: - drop patch "server: Add async_initial_status_callback." - drop patch "server: Defer postprocessing until after setting initial status in send_socket handler." - add patch "server: Ensure completion_callback is called before async is destroyed." - add patch "ws2_32/tests: Continue sending remaining data on short write in test_write_events." - add patch "server: Defer clearing events until async is either pending or completed in send_socket handler." - add patch "server: Ensure datagram sockets are bound in send_socket." v5 -> v6: - centralise status code computation in one place, even for bind errno translation - fix erroneous subject (s/either pending or completed/completed/) v6 -> v7: - drop patch "server: Ensure completion_callback is called before async is destroyed." - add patch "server: Ensure async completion callback is called on synchronous failure." - add patch "server: Allow async completion callback to retrieve status on synchronous failure."
Supersedes 227636 - 227644. Supersedes 228988 - 228998. Supersedes 230246 - 230256.
[1] https://bugs.winehq.org/show_bug.cgi?id=52401 [2] https://www.winehq.org/pipermail/wine-devel/2021-May/186454.html
Jinoh Kang (11): server: Actually set initial status in set_async_direct_result handler. server: Ensure initial status is set in async_set_result(). server: Ensure async completion callback is called on synchronous failure. server: Allow async completion callback to retrieve status on synchronous failure. ws2_32/tests: Continue sending remaining data on short write in test_write_events. server: Defer clearing events until async is completed in send_socket handler. server: Add mark_pending field to set_async_direct_result request. server: Attempt to complete I/O request immediately in send_socket. server: Ensure datagram sockets are bound in send_socket. ntdll: Don't call try_send before server call in sock_send. server: Replace redundant send_socket status fields with force_async boolean field.
dlls/ntdll/unix/socket.c | 91 ++++++++++++------ dlls/ntdll/unix/sync.c | 9 +- dlls/ntdll/unix/unix_private.h | 2 +- dlls/ws2_32/tests/sock.c | 23 ++--- server/async.c | 37 +++++--- server/protocol.def | 5 +- server/sock.c | 164 +++++++++++++++++++++++---------- 7 files changed, 220 insertions(+), 111 deletions(-)
Commit 15483b1a126 (server: Allow calling async_handoff() with status code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result handler which calls async_set_initial_status().
However, the async_set_initial_status() call does nothing since async->terminated is set, leaving the async in a confusing state (unknown_status = 1 but pending/completed).
So far, this issue is unlikely to have been a problem in practice for the following reasons:
1. async_set_initial_status() would have unset unknown_status, but it remains set instead. This is usually not a problem, since unknown_status is usually ever read by code paths effectively unreachable for non-device (e.g. socket) asyncs.
It would still potentially allow set_async_direct_result to be called multiple times, but it wouldn't actually happen in practice unless something goes wrong.
2. async_set_initial_status() would have set initial_status; however, it is left with the default value STATUS_PENDING. If the actual status is something other than that, the handler closes the wait handle and async_satisfied (the only real consumer of initial_status) would never be called anyway.
For reasons above, this issue is not effectively observable or testable. Nonetheless, the current code does leave the async object in an inconsistent state.
Fix this by removing the !async->terminated check in async_set_initial_status().
Also, remove assert( async->unknown_status ). The client can now trigger the assert() by calling set_async_direct_result on a device async, thereby causing async_set_initial_status() to be called twice.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes v3 -> v4: reuse async_set_initial_status() per feedback v4 -> v5: - fix typo in commit message (s/realconsumer/real consumer/) - remove assert( async->unknown_status ) in async_set_initial_status() v5 -> v6: no changes v6 -> v7: no changes
server/async.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/server/async.c b/server/async.c index 7d0ff10f7a4..6d663a6a553 100644 --- a/server/async.c +++ b/server/async.c @@ -302,12 +302,8 @@ struct async *create_async( struct fd *fd, struct thread *thread, const async_da * the initial status may be STATUS_PENDING */ void async_set_initial_status( struct async *async, unsigned int status ) { - assert( async->unknown_status ); - if (!async->terminated) - { - async->initial_status = status; - async->unknown_status = 0; - } + async->initial_status = status; + async->unknown_status = 0; }
void set_async_pending( struct async *async )
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
Shift the resposibility of setting initial status from set_async_direct_result request handler to async_set_result().
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v3 -> v4: new patch v4 -> v5: adjust for previous patch change v5 -> v6: no changes v6 -> v7: no changes
server/async.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/async.c b/server/async.c index 6d663a6a553..d49fb8b7c04 100644 --- a/server/async.c +++ b/server/async.c @@ -481,6 +481,8 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota
assert( async->terminated ); /* it must have been woken up if we get a result */
+ if (async->unknown_status) async_set_initial_status( async, status ); + if (async->alerted && status == STATUS_PENDING) /* restart it */ { async->terminated = 0; @@ -765,8 +767,6 @@ DECL_HANDLER(set_async_direct_result) return; }
- async_set_initial_status( async, status ); - if (status == STATUS_PENDING) { async->direct_result = 0;
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
async_set_result() is the only way the async completion callback can be called. If the async were a request async and the I/O failed even before it was marked as pending, async_handoff() simply discards the async without either calling async_set_result() or directly invoking the completion callback. This leads to memory leak.
Fix this by calling the completion_callback on synchronous failure path if it was not already called, setting it to NULL afterwards.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v6 -> v7: new patch
server/async.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/server/async.c b/server/async.c index d49fb8b7c04..182344923d4 100644 --- a/server/async.c +++ b/server/async.c @@ -322,6 +322,13 @@ void async_wake_obj( struct async *async ) } }
+static void async_call_completion_callback( struct async *async ) +{ + if (async->completion_callback) + async->completion_callback( async->completion_callback_private ); + async->completion_callback = NULL; +} + /* return async object status and wait handle to client */ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_blocking ) { @@ -363,6 +370,8 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
if (!async->pending && NT_ERROR( get_error() )) { + async_call_completion_callback( async ); + close_handle( async->thread->process, async->wait_handle ); async->wait_handle = 0; return 0; @@ -528,9 +537,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota wake_up( &async->obj, 0 ); }
- if (async->completion_callback) - async->completion_callback( async->completion_callback_private ); - async->completion_callback = NULL; + async_call_completion_callback( async );
if (async->queue) {
On 3/19/22 17:28, Jinoh Kang wrote:
async_set_result() is the only way the async completion callback can be called. If the async were a request async and the I/O failed even before it was marked as pending, async_handoff() simply discards the async without either calling async_set_result() or directly invoking the completion callback. This leads to memory leak.
Fix this by calling the completion_callback on synchronous failure path if it was not already called, setting it to NULL afterwards.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
I'm going to nitpick this, because I think clarity is very important here.
The assumption that's currently made about completion callbacks is that they're only set if the async is actually queued, and that assumption holds for all of the places that currently use them.
That could be reasonably changed, I think (although I'm not sure it strictly needs to be—we could duplicate clearing events, I think either approach is reasonable) but it does mean that this description isn't really accurate.
This also makes async_handoff() behaviour more consistent with async_set_result() for handling I/O failures.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v6 -> v7: new patch
server/async.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/server/async.c b/server/async.c index 182344923d4..6905058fa69 100644 --- a/server/async.c +++ b/server/async.c @@ -370,6 +370,7 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
if (!async->pending && NT_ERROR( get_error() )) { + async->iosb->status = get_error(); async_call_completion_callback( async );
close_handle( async->thread->process, async->wait_handle );
Today, we assert that a short write clears the FD_WRITE event bit; however, short writes can never happen on Windows in the first place. We're testing for a property of a socket behaviour that does not exist on Windows, but currently happens to be exhibited by Wine.
Ignore short writes, and continue sending until it fails with EWOULDBLOCK. This way, the test won't care whether or not a short write clears FD_WRITE. This allows us some flexibility in implementation of send().
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v4 -> v5: new patch v5 -> v6: no changes v6 -> v7: no changes
dlls/ws2_32/tests/sock.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 6cb8ff0a7a8..cc74e301f60 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -5666,14 +5666,11 @@ static void test_write_events(struct event_test_ctx *ctx)
if (!broken(1)) { - while ((ret = send(server, buffer, buffer_size, 0)) == buffer_size); /* Windows will never send less than buffer_size bytes here, but Linux * may do a short write. */ - todo_wine_if (ret > 0) - { - ok(ret == -1, "got %d\n", ret); - ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); - } + while ((ret = send(server, buffer, buffer_size, 0)) > 0); + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError());
while (recv(client, buffer, buffer_size, 0) > 0); ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError());
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
This allows the initial I/O to be performed after the send_socket handler is called.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v4 -> v5: new patch v5 -> v6: - fix erroneous subject (s/either pending or completed/completed/) v6 -> v7: no changes
server/sock.c | 56 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 14 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 91f98556552..0441ce2bbfa 100644 --- a/server/sock.c +++ b/server/sock.c @@ -153,6 +153,12 @@ struct connect_req unsigned int addr_len, send_len, send_cursor; };
+struct send_req +{ + struct iosb *iosb; + struct sock *sock; +}; + enum connection_state { SOCK_LISTENING, @@ -3454,6 +3460,26 @@ DECL_HANDLER(recv_socket) release_object( sock ); }
+static void send_socket_completion_callback( void *private ) +{ + struct send_req *send_req = private; + struct iosb *iosb = send_req->iosb; + struct sock *sock = send_req->sock; + + if (iosb->status != STATUS_SUCCESS) + { + /* send() calls only clear and reselect events if unsuccessful. */ + sock->pending_events &= ~AFD_POLL_WRITE; + sock->reported_events &= ~AFD_POLL_WRITE; + } + + sock_reselect( sock ); + + release_object( iosb ); + release_object( sock ); + free( send_req ); +} + DECL_HANDLER(send_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); @@ -3476,13 +3502,6 @@ DECL_HANDLER(send_socket) sock->bound = 1; }
- if (status != STATUS_SUCCESS) - { - /* send() calls only clear and reselect events if unsuccessful. */ - sock->pending_events &= ~AFD_POLL_WRITE; - sock->reported_events &= ~AFD_POLL_WRITE; - } - /* If we had a short write and the socket is nonblocking (and the client is * not trying to force the operation to be asynchronous), return success. * Windows actually refuses to send any data in this case, and returns @@ -3510,22 +3529,31 @@ DECL_HANDLER(send_socket)
if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { - if (status == STATUS_SUCCESS) + struct send_req *send_req; + struct iosb *iosb = async_get_iosb( async ); + + if ((send_req = mem_alloc( sizeof(*send_req) ))) { - struct iosb *iosb = async_get_iosb( async ); - iosb->result = req->total; - release_object( iosb ); + send_req->iosb = (struct iosb *)grab_object( iosb ); + send_req->sock = (struct sock *)grab_object( sock ); + async_set_completion_callback( async, send_socket_completion_callback, send_req ); } + else if (status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) + status = STATUS_NO_MEMORY; + + if (status == STATUS_SUCCESS) iosb->result = req->total; + release_object( iosb ); + set_error( status );
if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
if (status == STATUS_PENDING) + { queue_async( &sock->write_q, async ); - - /* always reselect; we changed reported_events above */ - sock_reselect( sock ); + sock_reselect( sock ); + }
reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd );
On 3/19/22 17:28, Jinoh Kang wrote:> +static void send_socket_completion_callback( void *private )
+{
- struct send_req *send_req = private;
- struct iosb *iosb = send_req->iosb;
- struct sock *sock = send_req->sock;
- if (iosb->status != STATUS_SUCCESS)
- {
/* send() calls only clear and reselect events if unsuccessful. */sock->pending_events &= ~AFD_POLL_WRITE;sock->reported_events &= ~AFD_POLL_WRITE;- }
- sock_reselect( sock );
I'd move this inside the prior if block. The point of reselecting here is that our events just changed, and that might trigger us to need reselect for POLLOUT.
- release_object( iosb );
- release_object( sock );
- free( send_req );
+}
The client can set mark_pending to indicate that the full-blown I/O completion mechanism shall be triggered (asynchronous completion) even if the status indicates failure.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes v3 -> v4: no changes v4 -> v5: no changes v5 -> v6: no changes v6 -> v7: no changes
dlls/ntdll/unix/socket.c | 2 +- dlls/ntdll/unix/sync.c | 9 +++++---- dlls/ntdll/unix/unix_private.h | 2 +- server/async.c | 11 ++++++++--- server/protocol.def | 1 + 5 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 23059e3cff8..2f8bd6e62bf 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -767,7 +767,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi release_fileio( &async->io ); }
- if (alerted) set_async_direct_result( &wait_handle, status, information ); + if (alerted) set_async_direct_result( &wait_handle, status, information, FALSE ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index ee25dfd0099..0786454dad2 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2514,7 +2514,7 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG /* Notify direct completion of async and close the wait handle if it is no longer needed. * This function is a no-op (returns status as-is) if the supplied handle is NULL. */ -void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information ) +void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending ) { NTSTATUS ret;
@@ -2522,9 +2522,10 @@ void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PT
SERVER_START_REQ( set_async_direct_result ) { - req->handle = wine_server_obj_handle( *optional_handle ); - req->status = status; - req->information = information; + req->handle = wine_server_obj_handle( *optional_handle ); + req->status = status; + req->information = information; + req->mark_pending = mark_pending; ret = wine_server_call( req ); if (ret == STATUS_SUCCESS) *optional_handle = wine_server_ptr_handle( reply->handle ); diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index b1595c792c0..b1a834efba5 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -274,7 +274,7 @@ extern NTSTATUS get_device_info( int fd, struct _FILE_FS_DEVICE_INFORMATION *inf extern void init_files(void) DECLSPEC_HIDDEN; extern void init_cpu_info(void) DECLSPEC_HIDDEN; extern void add_completion( HANDLE handle, ULONG_PTR value, NTSTATUS status, ULONG info, BOOL async ) DECLSPEC_HIDDEN; -extern void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information ); +extern void set_async_direct_result( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending );
extern void dbg_init(void) DECLSPEC_HIDDEN;
diff --git a/server/async.c b/server/async.c index 6905058fa69..a4fbeab555e 100644 --- a/server/async.c +++ b/server/async.c @@ -780,10 +780,15 @@ DECL_HANDLER(set_async_direct_result) async->direct_result = 0; async->pending = 1; } + else if (req->mark_pending) + { + async->pending = 1; + }
- /* if the I/O has completed successfully, the client would have already - * set the IOSB. therefore, we can skip waiting on wait_handle and do - * async_set_result() directly. + /* if the I/O has completed successfully (or unsuccessfully, and + * async->pending is set), the client would have already set the IOSB. + * therefore, we can do async_set_result() directly and let the client skip + * waiting on wait_handle. */ async_set_result( &async->obj, status, req->information );
diff --git a/server/protocol.def b/server/protocol.def index 9d90544fa41..66c6c97b1e0 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2168,6 +2168,7 @@ enum message_type obj_handle_t handle; /* wait handle */ apc_param_t information; /* IO_STATUS_BLOCK Information */ unsigned int status; /* completion status */ + int mark_pending; /* set async to pending before completion? */ @REPLY obj_handle_t handle; /* wait handle, or NULL if closed */ @END
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=110795
Your paranoid android.
=== debian11 (32 bit WoW report) ===
ntdll: change.c:242: Test failed: should be ready change.c:248: Test failed: action wrong change.c:278: Test failed: should be ready change.c:281: Test failed: info not set change.c:294: Test failed: status set too soon change.c:295: Test failed: info set too soon
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
Make send_socket alert the async immediately if poll() call detects that there are incoming data in the socket, bypassing the wineserver's main polling loop.
For sock_transmit, we always mark the async as pending and set the IOSB (unless async allocation has failed).
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - retain the behaviour of returning success if we had a short write and the socket is nonblocking and force_async is unset v2 -> v3: fix typo in comment v3 -> v4: no changes v4 -> v5: no changes v5 -> v6: no changes v6 -> v7: no changes
dlls/ntdll/unix/socket.c | 72 ++++++++++++++++++++++++++++++++++------ server/protocol.def | 1 + server/sock.c | 22 +++++++++++- 3 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 2f8bd6e62bf..62920a7e557 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -868,11 +868,13 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi const struct WS_sockaddr *addr, unsigned int addr_len, int unix_flags, int force_async ) { struct async_send_ioctl *async; + ULONG_PTR information; HANDLE wait_handle; DWORD async_size; NTSTATUS status; unsigned int i; ULONG options; + BOOL nonblocking, alerted;
async_size = offsetof( struct async_send_ioctl, iov[count] );
@@ -925,16 +927,38 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; - if ((!NT_ERROR(status) || wait_handle) && status != STATUS_PENDING) + nonblocking = reply->nonblocking; + } + SERVER_END_REQ; + + alerted = status == STATUS_ALERTED; + if (alerted) + { + status = try_send( fd, async ); + if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) + status = STATUS_PENDING; + + /* If we had a short write and the socket is nonblocking (and we are + * not trying to force the operation to be asynchronous), return + * success. Windows actually refuses to send any data in this case, + * and returns EWOULDBLOCK, but we have no way of doing that. */ + if (status == STATUS_DEVICE_NOT_READY && async->sent_len) + status = STATUS_SUCCESS; + } + + if (status != STATUS_PENDING) + { + information = async->sent_len; + if (!NT_ERROR(status) || (wait_handle && !alerted)) { io->Status = status; - io->Information = async->sent_len; + io->Information = information; } + release_fileio( &async->io ); } - SERVER_END_REQ; - - if (status != STATUS_PENDING) release_fileio( &async->io ); + else information = 0;
+ if (alerted) set_async_direct_result( &wait_handle, status, information, FALSE ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } @@ -1055,7 +1079,9 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, socklen_t addr_len; HANDLE wait_handle; NTSTATUS status; + ULONG_PTR information; ULONG options; + BOOL alerted;
addr_len = sizeof(addr); if (getpeername( fd, &addr.addr, &addr_len ) != 0) @@ -1105,16 +1131,40 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; - /* In theory we'd fill the iosb here, as above in sock_send(), but it's - * actually currently impossible to get STATUS_SUCCESS. The server will - * either return STATUS_PENDING or an error code, and in neither case - * should the iosb be filled. */ - if (!status) FIXME( "Unhandled success status." ); } SERVER_END_REQ;
- if (status != STATUS_PENDING) release_fileio( &async->io ); + alerted = status == STATUS_ALERTED; + if (alerted) + { + status = try_transmit( fd, file_fd, async ); + if (status == STATUS_DEVICE_NOT_READY) + status = STATUS_PENDING; + }
+ if (status != STATUS_PENDING) + { + information = async->head_cursor + async->file_cursor + async->tail_cursor; + if (!NT_ERROR(status) || wait_handle) + { + io->Status = status; + io->Information = information; + } + release_fileio( &async->io ); + } + else information = 0; + + if (alerted) + { + set_async_direct_result( &wait_handle, status, information, TRUE ); + if (!(options & (FILE_SYNCHRONOUS_IO_ALERT | FILE_SYNCHRONOUS_IO_NONALERT))) + { + /* Pretend we always do async I/O. The client can always retrieve + * the actual I/O status via the IO_STATUS_BLOCK. + */ + status = STATUS_PENDING; + } + } if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; } diff --git a/server/protocol.def b/server/protocol.def index 66c6c97b1e0..81b44aefd7c 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1467,6 +1467,7 @@ enum server_fd_type @REPLY obj_handle_t wait; /* handle to wait on for blocking send */ unsigned int options; /* device open options */ + int nonblocking; /* is socket non-blocking? */ @END
diff --git a/server/sock.c b/server/sock.c index 0441ce2bbfa..9170f8fe352 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3527,6 +3527,25 @@ DECL_HANDLER(send_socket) if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->wr_shutdown) status = STATUS_PIPE_DISCONNECTED;
+ if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &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 + * available, the current request won't be immediately satiable. + */ + struct pollfd pollfd; + pollfd.fd = get_unix_fd( sock->fd ); + pollfd.events = POLLOUT; + pollfd.revents = 0; + if (poll(&pollfd, 1, 0) >= 0 && pollfd.revents) + { + /* Give the client opportunity to complete synchronously. + * If it turns out that the I/O request is not actually immediately satiable, + * the client may then choose to re-queue the async (with STATUS_PENDING). */ + status = STATUS_ALERTED; + } + } + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { struct send_req *send_req; @@ -3549,7 +3568,7 @@ DECL_HANDLER(send_socket) if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
- if (status == STATUS_PENDING) + if (status == STATUS_PENDING || status == STATUS_ALERTED) { queue_async( &sock->write_q, async ); sock_reselect( sock ); @@ -3557,6 +3576,7 @@ DECL_HANDLER(send_socket)
reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); + reply->nonblocking = sock->nonblocking; release_object( async ); } release_object( sock );
If the type of the socket is SOCK_DGRAM, it shall always be bound to an address if we ever attempt to send datagrams through the socket, whether the attempt succeeds or not.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v4 -> v5: new patch v5 -> v6: no changes v6 -> v7: no changes
server/sock.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 9170f8fe352..2091384e4a4 100644 --- a/server/sock.c +++ b/server/sock.c @@ -471,6 +471,32 @@ static socklen_t sockaddr_to_unix( const struct WS_sockaddr *wsaddr, int wsaddrl } }
+static socklen_t get_unix_sockaddr_any( union unix_sockaddr *uaddr, int ws_family ) +{ + memset( uaddr, 0, sizeof(*uaddr) ); + switch (ws_family) + { + case WS_AF_INET: + uaddr->in.sin_family = AF_INET; + return sizeof(uaddr->in); + case WS_AF_INET6: + uaddr->in6.sin6_family = AF_INET6; + return sizeof(uaddr->in6); +#ifdef HAS_IPX + case WS_AF_IPX: + uaddr->ipx.sipx_family = AF_IPX; + return sizeof(uaddr->ipx); +#endif +#ifdef HAS_IRDA + case WS_AF_IRDA: + uaddr->irda.sir_family = AF_IRDA; + return sizeof(uaddr->irda); +#endif + default: + return 0; + } +} + /* some events are generated at the same time but must be sent in a particular * order (e.g. CONNECT must be sent before READ) */ static const enum afd_poll_bit event_bitorder[] = @@ -3491,15 +3517,24 @@ DECL_HANDLER(send_socket) if (!sock) return; fd = sock->fd;
- if (sock->type == WS_SOCK_DGRAM) + if (sock->type == WS_SOCK_DGRAM && !sock->bound) { - /* sendto() and sendmsg() implicitly binds a socket */ union unix_sockaddr unix_addr; - socklen_t unix_len = sizeof(unix_addr); + socklen_t unix_len; + int bind_errno = 0; + int unix_fd = get_unix_fd( fd );
- if (!sock->bound && !getsockname( get_unix_fd( fd ), &unix_addr.addr, &unix_len )) + unix_len = get_unix_sockaddr_any( &unix_addr, sock->family ); + if (bind( unix_fd, &unix_addr.addr, unix_len ) < 0) + bind_errno = errno; + + if (getsockname( unix_fd, &unix_addr.addr, &unix_len ) >= 0) + { sock->addr_len = sockaddr_from_unix( &unix_addr, &sock->addr.addr, sizeof(sock->addr) ); - sock->bound = 1; + sock->bound = 1; + } + else if (status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) + status = sock_get_ntstatus( bind_errno ? bind_errno : errno ); }
/* If we had a short write and the socket is nonblocking (and the client is
Otherwise, try_send() call from sock_send() may race against try_send() call from async_send_proc(), shuffling the packet order.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52401 Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: no changes v2 -> v3: no changes v3 -> v4: no changes v4 -> v5: no changes v5 -> v6: no changes v6 -> v7: no changes
dlls/ntdll/unix/socket.c | 13 ++----------- dlls/ws2_32/tests/sock.c | 14 +++++++------- 2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 62920a7e557..8e5a1825181 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -908,21 +908,12 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi async->iov_cursor = 0; async->sent_len = 0;
- status = try_send( fd, async ); - - if (status != STATUS_SUCCESS && status != STATUS_DEVICE_NOT_READY) - { - release_fileio( &async->io ); - return status; - } - - if (status == STATUS_DEVICE_NOT_READY && force_async) - status = STATUS_PENDING; + status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
SERVER_START_REQ( send_socket ) { req->status = status; - req->total = async->sent_len; + req->total = 0; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index cc74e301f60..d501930b32d 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -8010,7 +8010,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -8057,7 +8057,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(server, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); @@ -8107,7 +8107,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -8125,7 +8125,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = send(server, "test", 5, 0); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); @@ -8256,7 +8256,7 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = sendto(client, "test", 5, 0, (struct sockaddr *)&server_addr, sizeof(server_addr)); ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
closesocket(client); closesocket(server); @@ -8337,7 +8337,7 @@ static void test_DisconnectEx(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "expected failure\n"); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret); @@ -8391,7 +8391,7 @@ static void test_DisconnectEx(void) WSASetLastError(0xdeadbeef); ret = send(client, "test", 5, 0); ok(ret == -1, "expected failure\n"); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
ret = recv(server, buffer, sizeof(buffer), 0); ok(!ret, "got %d\n", ret);
The 'status' field of send_socket_request is always either STATUS_PENDING or STATUS_DEVICE_NOT_READY, and the 'total' field is always zero.
Replace the 'status' field with 'force_async' boolean field, and get rid of the 'total' field entirely.
Also, clean up the send_socket handler code a bit.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com ---
Notes: v1 -> v2: - adjust patch for new field "data_size" in send_socket_request v2 -> v3: no changes v3 -> v4: no changes v4 -> v5: no changes v5 -> v6: - centralise status code computation in one place, even for bind errno translation v6 -> v7: no changes
dlls/ntdll/unix/socket.c | 8 ++------ server/protocol.def | 3 +-- server/sock.c | 41 +++++++++++----------------------------- 3 files changed, 14 insertions(+), 38 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 8e5a1825181..351028d4983 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -908,12 +908,9 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi async->iov_cursor = 0; async->sent_len = 0;
- status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY; - SERVER_START_REQ( send_socket ) { - req->status = status; - req->total = 0; + req->force_async = force_async; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); @@ -1116,8 +1113,7 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc,
SERVER_START_REQ( send_socket ) { - req->status = STATUS_PENDING; - req->total = 0; + req->force_async = 1; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); diff --git a/server/protocol.def b/server/protocol.def index 81b44aefd7c..d9bed6855e9 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1462,8 +1462,7 @@ enum server_fd_type /* Perform a send on a socket */ @REQ(send_socket) async_data_t async; /* async I/O parameters */ - unsigned int status; /* status of initial call */ - unsigned int total; /* number of bytes already sent */ + int force_async; /* Force asynchronous mode? */ @REPLY obj_handle_t wait; /* handle to wait on for blocking send */ unsigned int options; /* device open options */ diff --git a/server/sock.c b/server/sock.c index 2091384e4a4..07522b16ff7 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3509,10 +3509,11 @@ static void send_socket_completion_callback( void *private ) DECL_HANDLER(send_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); - unsigned int status = req->status; + unsigned int status = STATUS_PENDING; timeout_t timeout = 0; struct async *async; struct fd *fd; + int bind_errno = 0;
if (!sock) return; fd = sock->fd; @@ -3521,7 +3522,6 @@ DECL_HANDLER(send_socket) { union unix_sockaddr unix_addr; socklen_t unix_len; - int bind_errno = 0; int unix_fd = get_unix_fd( fd );
unix_len = get_unix_sockaddr_any( &unix_addr, sock->family ); @@ -3533,36 +3533,15 @@ DECL_HANDLER(send_socket) sock->addr_len = sockaddr_from_unix( &unix_addr, &sock->addr.addr, sizeof(sock->addr) ); sock->bound = 1; } - else if (status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) - status = sock_get_ntstatus( bind_errno ? bind_errno : errno ); + else if (!bind_errno) bind_errno = errno; }
- /* If we had a short write and the socket is nonblocking (and the client is - * not trying to force the operation to be asynchronous), return success. - * Windows actually refuses to send any data in this case, and returns - * EWOULDBLOCK, but we have no way of doing that. */ - if (status == STATUS_DEVICE_NOT_READY && req->total && sock->nonblocking) - status = STATUS_SUCCESS; + if (!req->force_async && !sock->nonblocking && is_fd_overlapped( fd )) + timeout = (timeout_t)sock->sndtimeo * -10000;
- /* send() returned EWOULDBLOCK or a short write, i.e. cannot send all data yet */ - if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking) - { - /* Set a timeout on the async if necessary. - * - * We want to do this *only* if the client gave us STATUS_DEVICE_NOT_READY. - * If the client gave us STATUS_PENDING, it expects the async to always - * block (it was triggered by WSASend*() with a valid OVERLAPPED - * structure) and for the timeout not to be respected. */ - if (is_fd_overlapped( fd )) - timeout = (timeout_t)sock->sndtimeo * -10000; - - status = STATUS_PENDING; - } - - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->wr_shutdown) - status = STATUS_PIPE_DISCONNECTED; - - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->write_q )) + 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 )) { /* 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 @@ -3581,6 +3560,9 @@ DECL_HANDLER(send_socket) } }
+ if (status == STATUS_PENDING && !req->force_async && sock->nonblocking) + status = STATUS_DEVICE_NOT_READY; + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { struct send_req *send_req; @@ -3595,7 +3577,6 @@ DECL_HANDLER(send_socket) else if (status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) status = STATUS_NO_MEMORY;
- if (status == STATUS_SUCCESS) iosb->result = req->total; release_object( iosb );
set_error( status );
-
Jinoh Kang -
Marvin -
Zebediah Figura