[PATCH v2 0/5] MR6490: winsock: Avoid short writes on nonblocking sockets.
-- v2: server: Correct STATUS_NOT_FOUND return from (cancel_async). ntdll: Don't cancel background socket sends. ntdll: Locally duplicated socket fd for background send. ntdll: Avoid short writes on nonblocking sockets. ws2_32/Tests: Add tests for send buffering. https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 117 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 437ee813611..87b87727b60 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -6883,9 +6883,9 @@ static void test_write_events(struct event_test_ctx *ctx) if (!broken(1)) { - /* Windows will never send less than buffer_size bytes here, but Linux - * may do a short write. */ - while ((ret = send(server, buffer, buffer_size, 0)) > 0); + /* Windows will never send less than buffer_size bytes here. */ + while ((ret = send(server, buffer, buffer_size, 0)) > 0) + ok(ret == buffer_size, "got %d.\n", ret); ok(ret == -1, "got %d\n", ret); ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u\n", WSAGetLastError()); @@ -14263,6 +14263,116 @@ static void test_broadcast(void) closesocket(s); } +struct test_send_buffering_data +{ + int buffer_size; + int sent_size; + SOCKET server; + char *buffer; +}; + +static DWORD WINAPI test_send_buffering_thread(void *arg) +{ + struct test_send_buffering_data *d = arg; + int ret; + + d->sent_size = 0; + while ((ret = send(d->server, d->buffer, d->buffer_size, 0)) > 0) + { + todo_wine ok(ret == d->buffer_size, "got %d.\n", ret); + d->sent_size += ret; + } + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u.\n", WSAGetLastError()); + ok(d->sent_size, "got 0.\n"); + ret = CancelIoEx((HANDLE)d->server, NULL); + todo_wine ok(!ret && GetLastError() == ERROR_NOT_FOUND, "got ret %d, error %lu.\n", ret, GetLastError()); + ret = CancelIo((HANDLE)d->server); + ok(ret, "got error %lu.\n", GetLastError()); + shutdown(d->server, SD_BOTH); + closesocket(d->server); + return 0; +} + +static void test_send_buffering(void) +{ + static const char test_data[] = "abcdefg01234567"; + + struct test_send_buffering_data d; + int ret, recv_size, i; + SOCKET client; + HANDLE thread; + + d.buffer_size = 1024 * 1024 * 50; + d.buffer = malloc(d.buffer_size); + + for (i = 0; i < d.buffer_size; ++i) + d.buffer[i] = test_data[i % sizeof(test_data)]; + + tcp_socketpair(&client, &d.server); + set_blocking(client, FALSE); + set_blocking(d.server, FALSE); + + d.sent_size = 0; + while ((ret = send(d.server, d.buffer, d.buffer_size, 0)) > 0) + { + todo_wine ok(ret == d.buffer_size, "got %d.\n", ret); + d.sent_size += ret; + } + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u.\n", WSAGetLastError()); + ok(d.sent_size, "got 0.\n"); + closesocket(d.server); + + recv_size = 0; + while ((ret = recv(client, d.buffer, d.buffer_size, 0)) > 0 || WSAGetLastError() == WSAEWOULDBLOCK) + { + if (ret < 0) + continue; + for (i = 0; i < ret; ++i) + { + if (d.buffer[i] != test_data[(recv_size + i) % sizeof(test_data)]) + break; + } + ok(i == ret, "data mismatch.\n"); + recv_size += ret; + ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); + } + ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); + ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + closesocket(client); + + /* Test with the other thread which terminates before the data is actually sent. */ + for (i = 0; i < d.buffer_size; ++i) + d.buffer[i] = test_data[i % sizeof(test_data)]; + + tcp_socketpair(&client, &d.server); + set_blocking(client, FALSE); + set_blocking(d.server, FALSE); + + thread = CreateThread(NULL, 0, test_send_buffering_thread, &d, 0, NULL); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + recv_size = 0; + while ((ret = recv(client, d.buffer, d.buffer_size, 0)) > 0 || WSAGetLastError() == WSAEWOULDBLOCK) + { + if (ret < 0) + continue; + for (i = 0; i < ret; ++i) + { + if (d.buffer[i] != test_data[(recv_size + i) % sizeof(test_data)]) + break; + } + ok(i == ret, "data mismatch.\n"); + recv_size += ret; + ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); + } + ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); + ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + closesocket(client); +} + START_TEST( sock ) { int i; @@ -14346,6 +14456,7 @@ START_TEST( sock ) test_connect_udp(); test_tcp_sendto_recvfrom(); test_broadcast(); + test_send_buffering(); /* There is apparently an obscure interaction between this test and * test_WSAGetOverlappedResult(). -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 60 +++++++++++++++++++++++++++++++++++++--- dlls/ws2_32/tests/sock.c | 8 +++--- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 577b48b1336..493bf556ec1 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1134,10 +1134,62 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi /* 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; + * success, pretened we've written everything to the socket and queue writing + * remaining data. Windows never reports partial write in this case and queues + * virtually unlimited amount of data for background write in this case. */ + if (status == STATUS_DEVICE_NOT_READY && async->sent_len && async->iov_cursor < async->count) + { + struct iovec *iov = async->iov + async->iov_cursor; + SIZE_T data_size, async_size, addr_size; + struct async_send_ioctl *rem_async; + unsigned int i, iov_count; + IO_STATUS_BLOCK *rem_io; + char *p; + + TRACE( "Short write, queueing remaining data.\n" ); + data_size = 0; + iov_count = async->count - async->iov_cursor; + for (i = 0; i < iov_count; ++i) + data_size += iov[i].iov_len; + + addr_size = max( 0, async->addr_len ); + async_size = offsetof( struct async_send_ioctl, iov[1] ) + data_size + addr_size + + sizeof(IO_STATUS_BLOCK); + if (!(rem_async = (struct async_send_ioctl *)alloc_fileio( async_size, async_send_proc, handle ))) + { + status = STATUS_NO_MEMORY; + } + else + { + /* Use a local copy of socket fd so the async send works after socket handle is closed. */ + rem_async->count = 1; + p = (char *)rem_async + offsetof( struct async_send_ioctl, iov[1] ); + rem_async->iov[0].iov_base = p; + rem_async->iov[0].iov_len = data_size; + for (i = 0; i < iov_count; ++i) + { + memcpy( p, iov[i].iov_base, iov[i].iov_len ); + p += iov[i].iov_len; + } + rem_async->unix_flags = async->unix_flags; + memcpy( p, async->addr, addr_size ); + rem_async->addr = (const struct WS_sockaddr *)p; + p += addr_size; + rem_async->addr_len = async->addr_len; + rem_async->iov_cursor = 0; + rem_async->sent_len = 0; + rem_io = (IO_STATUS_BLOCK *)p; + p += sizeof(IO_STATUS_BLOCK); + status = sock_send( handle, NULL, NULL, NULL, rem_io, fd, rem_async, TRUE ); + if (status == STATUS_PENDING) status = STATUS_SUCCESS; + if (!status) + { + async->sent_len += data_size; + async->iov_cursor = async->count; + } + else ERR( "Remaining write queue failed, status %#x.\n", status ); + } + } set_async_direct_result( &wait_handle, options, io, status, async->sent_len, FALSE ); } diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 87b87727b60..deb162c6451 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14279,7 +14279,7 @@ static DWORD WINAPI test_send_buffering_thread(void *arg) d->sent_size = 0; while ((ret = send(d->server, d->buffer, d->buffer_size, 0)) > 0) { - todo_wine ok(ret == d->buffer_size, "got %d.\n", ret); + ok(ret == d->buffer_size, "got %d.\n", ret); d->sent_size += ret; } ok(ret == -1, "got %d\n", ret); @@ -14316,7 +14316,7 @@ static void test_send_buffering(void) d.sent_size = 0; while ((ret = send(d.server, d.buffer, d.buffer_size, 0)) > 0) { - todo_wine ok(ret == d.buffer_size, "got %d.\n", ret); + ok(ret == d.buffer_size, "got %d.\n", ret); d.sent_size += ret; } ok(ret == -1, "got %d\n", ret); @@ -14339,7 +14339,7 @@ static void test_send_buffering(void) ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); } ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); - ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + todo_wine ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); closesocket(client); /* Test with the other thread which terminates before the data is actually sent. */ @@ -14369,7 +14369,7 @@ static void test_send_buffering(void) ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); } ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); - ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + todo_wine ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); closesocket(client); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 11 ++++++++++- dlls/ws2_32/tests/sock.c | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 493bf556ec1..fe232ae048c 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -130,6 +130,7 @@ struct async_send_ioctl unsigned int sent_len; unsigned int count; unsigned int iov_cursor; + int fd; struct iovec iov[1]; }; @@ -1060,7 +1061,8 @@ static BOOL async_send_proc( void *user, ULONG_PTR *info, unsigned int *status ) if (*status == STATUS_ALERTED) { - if ((*status = server_get_unix_fd( async->io.handle, 0, &fd, &needs_close, NULL, NULL ))) + needs_close = FALSE; + if ((fd = async->fd) == -1 && (*status = server_get_unix_fd( async->io.handle, 0, &fd, &needs_close, NULL, NULL ))) return TRUE; *status = try_send( fd, async ); @@ -1072,6 +1074,7 @@ static BOOL async_send_proc( void *user, ULONG_PTR *info, unsigned int *status ) return FALSE; } *info = async->sent_len; + if (async->fd != -1) close( async->fd ); release_fileio( &async->io ); return TRUE; } @@ -1162,6 +1165,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi else { /* Use a local copy of socket fd so the async send works after socket handle is closed. */ + rem_async->fd = dup( fd ); rem_async->count = 1; p = (char *)rem_async + offsetof( struct async_send_ioctl, iov[1] ); rem_async->iov[0].iov_base = p; @@ -1195,7 +1199,10 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } if (status != STATUS_PENDING) + { + if (async->fd != -1) close( async->fd ); release_fileio( &async->io ); + } if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; @@ -1235,6 +1242,7 @@ static NTSTATUS sock_ioctl_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE ap async->iov[i].iov_len = buffers[i].len; } } + async->fd = -1; async->unix_flags = unix_flags; async->addr = addr; async->addr_len = addr_len; @@ -1254,6 +1262,7 @@ NTSTATUS sock_write( HANDLE handle, int fd, HANDLE event, PIO_APC_ROUTINE apc, if (!(async = (struct async_send_ioctl *)alloc_fileio( async_size, async_send_proc, handle ))) return STATUS_NO_MEMORY; + async->fd = -1; async->count = 1; async->iov[0].iov_base = (void *)buffer; async->iov[0].iov_len = length; diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index deb162c6451..1d50515ee8e 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14339,7 +14339,7 @@ static void test_send_buffering(void) ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); } ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); - todo_wine ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); closesocket(client); /* Test with the other thread which terminates before the data is actually sent. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 15 ++++++++------- dlls/ws2_32/tests/sock.c | 2 +- server/async.c | 19 ++++++++++++++++--- server/fd.c | 10 +++++----- server/file.h | 3 ++- server/protocol.def | 4 +++- server/sock.c | 12 +++++++----- 7 files changed, 42 insertions(+), 23 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index fe232ae048c..0a0e96ae526 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1105,7 +1105,7 @@ static void sock_save_icmp_id( struct async_send_ioctl *async ) } static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc_user, - IO_STATUS_BLOCK *io, int fd, struct async_send_ioctl *async, int force_async ) + IO_STATUS_BLOCK *io, int fd, struct async_send_ioctl *async, unsigned int server_flags ) { HANDLE wait_handle; BOOL nonblocking; @@ -1114,7 +1114,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi SERVER_START_REQ( send_socket ) { - req->force_async = force_async; + req->flags = server_flags; 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 ); @@ -1132,7 +1132,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi if (status == STATUS_ALERTED) { status = try_send( fd, async ); - if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) + if (status == STATUS_DEVICE_NOT_READY && ((server_flags & SERVER_SOCKET_IO_FORCE_ASYNC) || !nonblocking)) status = STATUS_PENDING; /* If we had a short write and the socket is nonblocking (and we are @@ -1184,7 +1184,8 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi rem_async->sent_len = 0; rem_io = (IO_STATUS_BLOCK *)p; p += sizeof(IO_STATUS_BLOCK); - status = sock_send( handle, NULL, NULL, NULL, rem_io, fd, rem_async, TRUE ); + status = sock_send( handle, NULL, NULL, NULL, rem_io, fd, rem_async, + SERVER_SOCKET_IO_FORCE_ASYNC | SERVER_SOCKET_IO_SYSTEM ); if (status == STATUS_PENDING) status = STATUS_SUCCESS; if (!status) { @@ -1249,7 +1250,7 @@ static NTSTATUS sock_ioctl_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE ap async->iov_cursor = 0; async->sent_len = 0; - return sock_send( handle, event, apc, apc_user, io, fd, async, force_async ); + return sock_send( handle, event, apc, apc_user, io, fd, async, force_async ? SERVER_SOCKET_IO_FORCE_ASYNC : 0 ); } @@ -1272,7 +1273,7 @@ NTSTATUS sock_write( HANDLE handle, int fd, HANDLE event, PIO_APC_ROUTINE apc, async->iov_cursor = 0; async->sent_len = 0; - return sock_send( handle, event, apc, apc_user, io, fd, async, 1 ); + return sock_send( handle, event, apc, apc_user, io, fd, async, SERVER_SOCKET_IO_FORCE_ASYNC ); } @@ -1436,7 +1437,7 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, SERVER_START_REQ( send_socket ) { - req->force_async = 1; + req->flags = SERVER_SOCKET_IO_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 ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 1d50515ee8e..30ce3db307f 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14369,7 +14369,7 @@ static void test_send_buffering(void) ok(recv_size <= d.sent_size, "got ret %d, recv_size %d, sent_size %d.\n", ret, recv_size, d.sent_size); } ok(!ret && !WSAGetLastError(), "got ret %d, error %u.\n", ret, WSAGetLastError()); - todo_wine ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); + ok(recv_size == d.sent_size, "got %d, expected %d.\n", recv_size, d.sent_size); closesocket(client); } diff --git a/server/async.c b/server/async.c index 3c9b9588c6e..26192f56ca3 100644 --- a/server/async.c +++ b/server/async.c @@ -57,6 +57,7 @@ struct async unsigned int canceled :1; /* have we already queued cancellation for this async? */ unsigned int unknown_status :1; /* initial status is not known yet */ unsigned int blocking :1; /* async is blocking */ + unsigned int no_cancel :1; /* not cancellable background async. */ struct completion *completion; /* completion associated with fd */ apc_param_t comp_key; /* completion key associated with fd */ unsigned int comp_flags; /* completion flags */ @@ -277,6 +278,7 @@ struct async *create_async( struct fd *fd, struct thread *thread, const async_da async->canceled = 0; async->unknown_status = 0; async->blocking = !is_fd_overlapped( fd ); + async->no_cancel = 0; async->completion = fd_get_completion( fd, &async->comp_key ); async->comp_flags = 0; async->completion_callback = NULL; @@ -584,7 +586,7 @@ static int cancel_async( struct process *process, struct object *obj, struct thr restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { - if (async->terminated || async->canceled) continue; + if (async->terminated || async->canceled || async->no_cancel) continue; if ((!obj || (get_fd_user( async->fd ) == obj)) && (!thread || async->thread == thread) && (!iosb || async->data.iosb == iosb)) @@ -621,7 +623,16 @@ restart: void cancel_process_asyncs( struct process *process ) { - cancel_async( process, NULL, NULL, 0 ); + struct async *async; + +restart: + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) + { + if (async->terminated || async->canceled) continue; + async->canceled = 1; + fd_cancel_async( async->fd, async ); + goto restart; + } } int async_close_obj_handle( struct object *obj, struct process *process, obj_handle_t handle ) @@ -655,6 +666,7 @@ restart: { if (async->thread != thread || async->terminated || async->canceled) continue; if (async->completion && async->data.apc_context && !async->event) continue; + if (async->no_cancel) continue; async->canceled = 1; fd_cancel_async( async->fd, async ); @@ -741,7 +753,7 @@ static struct iosb *create_iosb( const void *in_data, data_size_t in_size, data_ /* create an async associated with iosb for async-based requests * returned async must be passed to async_handoff */ -struct async *create_request_async( struct fd *fd, unsigned int comp_flags, const async_data_t *data ) +struct async *create_request_async( struct fd *fd, unsigned int comp_flags, const async_data_t *data, int no_cancel ) { struct async *async; struct iosb *iosb; @@ -760,6 +772,7 @@ struct async *create_request_async( struct fd *fd, unsigned int comp_flags, cons } async->pending = 0; async->direct_result = 1; + async->no_cancel = !!no_cancel; async->comp_flags = comp_flags; } return async; diff --git a/server/fd.c b/server/fd.c index f62e7b60efe..bed6575ab4c 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2719,7 +2719,7 @@ DECL_HANDLER(flush) if (!fd) return; - if ((async = create_request_async( fd, fd->comp_flags, &req->async ))) + if ((async = create_request_async( fd, fd->comp_flags, &req->async, 0 ))) { fd->fd_ops->flush( fd, async ); reply->event = async_handoff( async, NULL, 1 ); @@ -2748,7 +2748,7 @@ DECL_HANDLER(get_volume_info) if (!fd) return; - if ((async = create_request_async( fd, fd->comp_flags, &req->async ))) + if ((async = create_request_async( fd, fd->comp_flags, &req->async, 0 ))) { fd->fd_ops->get_volume_info( fd, async, req->info_class ); reply->wait = async_handoff( async, NULL, 1 ); @@ -2824,7 +2824,7 @@ DECL_HANDLER(read) if (!fd) return; - if ((async = create_request_async( fd, fd->comp_flags, &req->async ))) + if ((async = create_request_async( fd, fd->comp_flags, &req->async, 0 ))) { fd->fd_ops->read( fd, async, req->pos ); reply->wait = async_handoff( async, NULL, 0 ); @@ -2842,7 +2842,7 @@ DECL_HANDLER(write) if (!fd) return; - if ((async = create_request_async( fd, fd->comp_flags, &req->async ))) + if ((async = create_request_async( fd, fd->comp_flags, &req->async, 0 ))) { fd->fd_ops->write( fd, async, req->pos ); reply->wait = async_handoff( async, &reply->size, 0 ); @@ -2861,7 +2861,7 @@ DECL_HANDLER(ioctl) if (!fd) return; - if ((async = create_request_async( fd, fd->comp_flags, &req->async ))) + if ((async = create_request_async( fd, fd->comp_flags, &req->async, 0 ))) { fd->fd_ops->ioctl( fd, req->code, async ); reply->wait = async_handoff( async, NULL, 0 ); diff --git a/server/file.h b/server/file.h index 006b5a8e324..d1a33253f1c 100644 --- a/server/file.h +++ b/server/file.h @@ -250,7 +250,8 @@ typedef void (*async_completion_callback)( void *private ); extern void free_async_queue( struct async_queue *queue ); extern struct async *create_async( struct fd *fd, struct thread *thread, const async_data_t *data, struct iosb *iosb ); -extern struct async *create_request_async( struct fd *fd, unsigned int comp_flags, const async_data_t *data ); +extern struct async *create_request_async( struct fd *fd, unsigned int comp_flags, const async_data_t *data, + int no_cancel ); extern obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_blocking ); extern void queue_async( struct async_queue *queue, struct async *async ); extern void async_set_timeout( struct async *async, timeout_t timeout, unsigned int status ); diff --git a/server/protocol.def b/server/protocol.def index de3f9f1b764..e9c213d344e 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1578,13 +1578,15 @@ enum server_fd_type /* Perform a send on a socket */ @REQ(send_socket) async_data_t async; /* async I/O parameters */ - int force_async; /* Force asynchronous mode? */ + unsigned int flags; /* SERVER_SOCKET_IO_* flags */ @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 +#define SERVER_SOCKET_IO_FORCE_ASYNC 0x01 +#define SERVER_SOCKET_IO_SYSTEM 0x02 /* Get socket event flags */ @REQ(socket_get_events) diff --git a/server/sock.c b/server/sock.c index f80f833c302..d2ec882554f 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3915,7 +3915,7 @@ DECL_HANDLER(recv_socket) sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); - if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async, 0 ))) { set_error( status ); @@ -3963,6 +3963,7 @@ DECL_HANDLER(send_socket) struct async *async; struct fd *fd; int bind_errno = 0; + BOOL force_async = req->flags & SERVER_SOCKET_IO_FORCE_ASYNC; if (!sock) return; fd = sock->fd; @@ -3985,7 +3986,7 @@ DECL_HANDLER(send_socket) else if (!bind_errno) bind_errno = errno; } - if (!req->force_async && !sock->nonblocking && is_fd_overlapped( fd )) + if (!force_async && !sock->nonblocking && is_fd_overlapped( fd )) timeout = (timeout_t)sock->sndtimeo * -10000; if (bind_errno) status = sock_get_ntstatus( bind_errno ); @@ -3996,7 +3997,7 @@ DECL_HANDLER(send_socket) * asyncs will not consume all available space; if there's no space * available, the current request won't be immediately satiable. */ - if ((!req->force_async && sock->nonblocking) || check_fd_events( sock->fd, POLLOUT )) + if ((!force_async && sock->nonblocking) || check_fd_events( sock->fd, POLLOUT )) { /* Give the client opportunity to complete synchronously. * If it turns out that the I/O request is not actually immediately satiable, @@ -4021,10 +4022,11 @@ DECL_HANDLER(send_socket) } } - if (status == STATUS_PENDING && !req->force_async && sock->nonblocking) + if (status == STATUS_PENDING && !force_async && sock->nonblocking) status = STATUS_DEVICE_NOT_READY; - if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) + if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async, + req->flags & SERVER_SOCKET_IO_SYSTEM ))) { struct send_req *send_req; struct iosb *iosb = async_get_iosb( async ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 9 +++++++++ dlls/ws2_32/tests/sock.c | 2 +- server/async.c | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 697774dfa09..cdf5cdb4151 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -614,6 +614,10 @@ static void test_cancelio(void) ok(ioapc_called, "IOAPC didn't run\n"); + res = pNtCancelIoFile(hPipe, &cancel_sb); + ok(!res, "NtCancelIoFile returned %lx\n", res); + ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + CloseHandle(hPipe); if (pNtCancelIoFileEx) @@ -631,6 +635,11 @@ static void test_cancelio(void) ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); + iosb.Status = 0xdeadbeef; + res = pNtCancelIoFileEx(hPipe, NULL, &cancel_sb); + ok(res == STATUS_NOT_FOUND, "NtCancelIoFileEx returned %lx\n", res); + ok(iosb.Status == 0xdeadbeef, "Wrong iostatus %lx\n", iosb.Status); + CloseHandle(hPipe); } else diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 30ce3db307f..4fd5fc00cdd 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14286,7 +14286,7 @@ static DWORD WINAPI test_send_buffering_thread(void *arg) ok(WSAGetLastError() == WSAEWOULDBLOCK, "got error %u.\n", WSAGetLastError()); ok(d->sent_size, "got 0.\n"); ret = CancelIoEx((HANDLE)d->server, NULL); - todo_wine ok(!ret && GetLastError() == ERROR_NOT_FOUND, "got ret %d, error %lu.\n", ret, GetLastError()); + ok(!ret && GetLastError() == ERROR_NOT_FOUND, "got ret %d, error %lu.\n", ret, GetLastError()); ret = CancelIo((HANDLE)d->server); ok(ret, "got error %lu.\n", GetLastError()); shutdown(d->server, SD_BOTH); diff --git a/server/async.c b/server/async.c index 26192f56ca3..10b48693344 100644 --- a/server/async.c +++ b/server/async.c @@ -819,7 +819,7 @@ DECL_HANDLER(cancel_async) if (obj) { int count = cancel_async( current->process, obj, thread, req->iosb ); - if (!count && req->iosb) set_error( STATUS_NOT_FOUND ); + if (!count && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6490
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=148422 Your paranoid android. === w10pro64_ja (64 bit report) === ws2_32: sock.c:13198: Test failed: wait timed out sock.c:13214: Test failed: wait timed out sock.c:13227: Test failed: got size 5 sock.c:13228: Test failed: got "datad" sock.c:13062: Test failed: got size 0 sock.c:13063: Test failed: got "" sock.c:13062: Test failed: got size 0 sock.c:14286: Test failed: got error 10055. sock.c:13063: Test failed: got ""
v2: - Add a separate test (which also tests various conditions under which an async send may be cancelled); - Handle those various conditions so the background send is not cancelled; - Fix CancelIoEx() return value in case there is nothing to cancel (unrelated but trivial and avoids leaving misleading todo in test). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82183
Now that we have a flag for indicating system asyncs, I suppose we should also disable `set_fd_signaled()` for these asyncs. Might be trivial, but otherwise this subtle deviation of behavior from Windows has no FIXME and thus harder to debug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82185
Thanks, I think you might be right in principle. But trying to test that now I see differences with the socket signaled state vs what I get on Win11. E. g.: checking signaled state with WaitForSingleObject right after the first send loop in my test indeed yields WAIT_OBJECT_0 on Windows. That's not the case in Wine, but also not the case without my patches (because server returned STATUS_ALERTED for the partial send and the async hasn't been removed yet). Then, suddenly, if I add select() for write sockets *before* the send loop, the same WaitForSingleObject(d.server, 0) returns STATUS_TIMEOUT on Windows. So it probably needs much more testing around, I am not sure tweaking that set_fd_signaled() WRT the added flag in isolation without even testing that properly (and solving some orthogonal issues at the same time) makes too much sense. I don't feel too strong about that though, possibly just removing set_fd_signaled() for those background sends is a sensible thing on its own. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82195
On the closer look, the mentioned pre-existing difference is not a race even but rather server/async.c:async_set_result() doesn't call set_fd_signaled if async has an event so it is never going to be signaled now after doing such sends in test, regardless of short write handling. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82197
In general, if there's no event, the file handle itself is used as an "event". That is, it's designaled when an async I/O begins [queue_async()], and signaled when it completes [async_set_result()]. I don't remember if the handle is supposed to be designaled for a synchronous call or one that doesn't return STATUS_PENDING. I would expect however that if background data is sent behind the scenes, the file handle is not going to be designaled when we start to send that data, or signaled when it's done, so I would assert that the right thing to do is indeed to skip set_fd_signaled().
* If there is racing send(). It may actually interpose the data in between the short written part and the queued one. That will be wrong, although not worse what is happening now: if now short read happens, the racing data may be interposed the same way, regardless of whether the app is going to handle short write or not. This may be fixed with some added synchronization but so far seems unnecessary;
We indeed can't protect against races with another I/O. Actually protecting against that would be hard; we'd either need to always copy (I think this can be dismissed as untenable) or extend the async infrastructure to allow an async to be "completed" but actually still processing. This honestly may be another good argument that async is two different things awkwardly rolled into one (namely, a way to coordinate client and server I/O, and the server's representation of an IRP). Alternatively we just ignore it for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82199
Elizabeth Figura (@zfigura) commented about dlls/ntdll/unix/socket.c:
+ rem_async->iov[0].iov_len = data_size; + for (i = 0; i < iov_count; ++i) + { + memcpy( p, iov[i].iov_base, iov[i].iov_len ); + p += iov[i].iov_len; + } + rem_async->unix_flags = async->unix_flags; + memcpy( p, async->addr, addr_size ); + rem_async->addr = (const struct WS_sockaddr *)p; + p += addr_size; + rem_async->addr_len = async->addr_len; + rem_async->iov_cursor = 0; + rem_async->sent_len = 0; + rem_io = (IO_STATUS_BLOCK *)p; + p += sizeof(IO_STATUS_BLOCK); + status = sock_send( handle, NULL, NULL, NULL, rem_io, fd, rem_async, TRUE ); This needs handling of wow64 IOSB like sync_ioctl().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82200
On Fri Sep 13 22:28:53 2024 +0000, Elizabeth Figura wrote:
This needs handling of wow64 IOSB like sync_ioctl(). Eh, but it copies not from user data but from already prepared async structure. Or am I missing something?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82201
On Fri Sep 13 22:34:10 2024 +0000, Paul Gofman wrote:
Eh, but it copies not from user data but from already prepared async structure. Or am I missing something? And the IO_STATUS_BLOCK is not relayed anywhere to the app / caller.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82202
On Fri Sep 13 22:36:58 2024 +0000, Paul Gofman wrote:
And the IO_STATUS_BLOCK is not relayed anywhere to the app / caller. Neither is the sync_ioctl() IOSB. The problem is that for user I/O the 32-bit iosb will always exist, so our I/O infrastructure assumes it will and tries to write to it. Making it not do that is a few orders of magnitude harder than just supplying the dummy iosb.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82203
On Fri Sep 13 22:39:18 2024 +0000, Elizabeth Figura wrote:
Neither is the sync_ioctl() IOSB. The problem is that for user I/O the 32-bit iosb will always exist, so our I/O infrastructure assumes it will and tries to write to it. Making it not do that is a few orders of magnitude harder than just supplying the dummy iosb. I see, thanks, I will update that now (along with avoiding set_fd_cancel, probably then renaming no_cancel to is_system in async structure.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6490#note_82204
participants (5)
-
Elizabeth Figura (＠zfigura) -
Jinoh Kang (＠iamahuman)
-
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)