[PATCH v4 0/7] MR871: ntdll: Fill the IOSB only inside the if (alerted) block in multiple functions.
-- v4: ntdll: Fill the IOSB in sock_transmit() only inside the "if (alerted)" block. ntdll: Combine the "if (alerted)" blocks in sock_send(). ntdll: Fill the IOSB in sock_send() only inside the "if (alerted)" block. ntdll: Combine the "if (alerted)" blocks in sock_recv(). ntdll: Fill the IOSB in sock_recv() only inside the "if (alerted)" block. ntdll: The async handle passed to set_async_direct_result() cannot be NULL. ws2_32/tests: Add more tests for iosb contents while a recv is pending. https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 7 ++++--- dlls/ws2_32/tests/sock.c | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index 21e7a50aef3..3215ddaef62 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -1466,13 +1466,14 @@ static void test_recv(void) IOCTL_AFD_RECV, ¶ms, sizeof(params) - 1, NULL, 0); ok(ret == STATUS_INVALID_PARAMETER, "got %#x\n", ret); - memset(&io, 0, sizeof(io)); + io.Status = 0xdeadbeef; + io.Information = 0xdeadbeef; memset(buffer, 0xcc, sizeof(buffer)); ret = NtDeviceIoControlFile((HANDLE)client, event, NULL, NULL, &io, IOCTL_AFD_RECV, ¶ms, sizeof(params), NULL, 0); ok(ret == STATUS_PENDING, "got %#x\n", ret); - ok(!io.Status, "got status %#lx\n", io.Status); - ok(!io.Information, "got information %#Ix\n", io.Information); + ok(io.Status == 0xdeadbeef, "got status %#lx\n", io.Status); + ok(io.Information == 0xdeadbeef, "got information %#Ix\n", io.Information); /* These structures need not remain valid. */ memset(¶ms, 0xcc, sizeof(params)); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index d4f430a29d2..adf16012951 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -12208,11 +12208,15 @@ static void test_nonblocking_async_recv(void) memset(buffer, 0, sizeof(buffer)); WSASetLastError(0xdeadbeef); + overlapped.Internal = 0xdeadbeef; + overlapped.InternalHigh = 0xdeadbeef; ret = WSARecv(client, &wsabuf, 1, NULL, &flags, &overlapped, NULL); ok(ret == -1, "got %d\n", ret); ok(WSAGetLastError() == ERROR_IO_PENDING, "got error %u\n", WSAGetLastError()); ret = WaitForSingleObject((HANDLE)client, 0); ok(ret == WAIT_TIMEOUT, "expected timeout\n"); + ok(overlapped.Internal == STATUS_PENDING, "got status %#lx\n", (NTSTATUS)overlapped.Internal); + ok(overlapped.InternalHigh == 0xdeadbeef, "got size %Iu\n", overlapped.InternalHigh); ret = send(server, "data", 4, 0); ok(ret == 4, "got %d\n", ret); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ntdll/unix/sync.c | 9 +++++---- dlls/ntdll/unix/unix_private.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 1194ee514b5..821a1f08311 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2533,21 +2533,22 @@ 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, BOOL mark_pending ) +void set_async_direct_result( HANDLE *async_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending ) { NTSTATUS ret; - if (!*optional_handle) return; + /* if we got STATUS_ALERTED, we must have a valid async handle */ + assert( *async_handle ); SERVER_START_REQ( set_async_direct_result ) { - req->handle = wine_server_obj_handle( *optional_handle ); + req->handle = wine_server_obj_handle( *async_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 ); + *async_handle = wine_server_ptr_handle( reply->handle ); } SERVER_END_REQ; diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 47f0f9c56a9..73b9ed76de0 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -278,7 +278,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, BOOL mark_pending ); +extern void set_async_direct_result( HANDLE *async_handle, NTSTATUS status, ULONG_PTR information, BOOL mark_pending ) DECLSPEC_HIDDEN; extern void dbg_init(void) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> We can only get a successful status that way. This avoids an uninitialized variable warning with gcc 12.2. --- dlls/ntdll/unix/socket.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index f8ed9f6f854..8ff58f7566e 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -23,6 +23,7 @@ #endif #include "config.h" +#include <assert.h> #include <errno.h> #include <sys/types.h> #include <sys/socket.h> @@ -879,24 +880,25 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } SERVER_END_REQ; + /* the server currently will never succeed immediately */ + assert(status == STATUS_ALERTED || status == STATUS_PENDING || NT_ERROR(status)); + alerted = status == STATUS_ALERTED; if (alerted) { status = try_recv( fd, async, &information ); if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) status = STATUS_PENDING; - } - - if (status != STATUS_PENDING) - { - if (!NT_ERROR(status) || (wait_handle && !alerted)) + if (!NT_ERROR(status) && status != STATUS_PENDING) { io->Status = status; io->Information = information; } - release_fileio( &async->io ); } + if (status != STATUS_PENDING) + release_fileio( &async->io ); + 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> There is no need to release the async_fileio structure before calling set_async_direct_result(). --- dlls/ntdll/unix/socket.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 8ff58f7566e..44694f8c404 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -852,9 +852,8 @@ static BOOL is_icmp_over_dgram( int fd ) static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc_user, IO_STATUS_BLOCK *io, int fd, struct async_recv_ioctl *async, int force_async ) { - BOOL nonblocking, alerted; - ULONG_PTR information; HANDLE wait_handle; + BOOL nonblocking; NTSTATUS status; unsigned int i; ULONG options; @@ -883,9 +882,10 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi /* the server currently will never succeed immediately */ assert(status == STATUS_ALERTED || status == STATUS_PENDING || NT_ERROR(status)); - alerted = status == STATUS_ALERTED; - if (alerted) + if (status == STATUS_ALERTED) { + ULONG_PTR information; + status = try_recv( fd, async, &information ); if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) status = STATUS_PENDING; @@ -894,12 +894,12 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi io->Status = status; io->Information = information; } + set_async_direct_result( &wait_handle, status, information, FALSE ); } if (status != STATUS_PENDING) release_fileio( &async->io ); - 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> We can only get a successful status that way. This matches sock_recv(). --- dlls/ntdll/unix/socket.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 44694f8c404..cc3c898d464 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1116,6 +1116,9 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } SERVER_END_REQ; + /* the server currently will never succeed immediately */ + assert(status == STATUS_ALERTED || status == STATUS_PENDING || NT_ERROR(status)); + if (!NT_ERROR(status) && is_icmp_over_dgram( fd )) sock_save_icmp_id( async ); @@ -1132,19 +1135,17 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi * 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)) + if (!NT_ERROR(status) && status != STATUS_PENDING) { io->Status = status; io->Information = information; } - release_fileio( &async->io ); } - else information = 0; + + if (status != STATUS_PENDING) + release_fileio( &async->io ); if (alerted) set_async_direct_result( &wait_handle, status, information, FALSE ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> There is no need to release the async_fileio structure before calling set_async_direct_result(). --- dlls/ntdll/unix/socket.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index cc3c898d464..e53ad4b76be 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1099,9 +1099,8 @@ 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 ) { - BOOL nonblocking, alerted; - ULONG_PTR information; HANDLE wait_handle; + BOOL nonblocking; NTSTATUS status; ULONG options; @@ -1122,9 +1121,10 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi if (!NT_ERROR(status) && is_icmp_over_dgram( fd )) sock_save_icmp_id( async ); - alerted = status == STATUS_ALERTED; - if (alerted) + if (status == STATUS_ALERTED) { + ULONG_PTR information; + status = try_send( fd, async ); if (status == STATUS_DEVICE_NOT_READY && (force_async || !nonblocking)) status = STATUS_PENDING; @@ -1142,12 +1142,13 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi io->Status = status; io->Information = information; } + + set_async_direct_result( &wait_handle, status, information, FALSE ); } if (status != STATUS_PENDING) release_fileio( &async->io ); - 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
From: Zebediah Figura <zfigura(a)codeweavers.com> We can only get a successful status that way. This matches sock_recv(). Also combine the if (alerted) blocks. Unlike the previous commits, this cannot be separated, as it causes warnings with some versions of gcc. --- dlls/ntdll/unix/socket.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index e53ad4b76be..b502d3a03cb 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1335,9 +1335,7 @@ 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) @@ -1389,37 +1387,38 @@ static NTSTATUS sock_transmit( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, } SERVER_END_REQ; - alerted = status == STATUS_ALERTED; - if (alerted) + /* the server currently will never succeed immediately */ + assert(status == STATUS_ALERTED || status == STATUS_PENDING || NT_ERROR(status)); + + if (status == STATUS_ALERTED) { + ULONG_PTR information; + 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) + if (!NT_ERROR(status) && status != STATUS_PENDING) { io->Status = status; io->Information = information; } - release_fileio( &async->io ); + + set_async_direct_result( &wait_handle, status, information, TRUE ); } - else information = 0; - if (alerted) + if (status != STATUS_PENDING) + release_fileio( &async->io ); + + if (!status && !(options & (FILE_SYNCHRONOUS_IO_ALERT | FILE_SYNCHRONOUS_IO_NONALERT))) { - 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; - } + /* 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/871
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125232 Your paranoid android. === debian11 (build log) === Task: Could not create the win32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the win32 report
participants (3)
-
Marvin -
Zebediah Figura -
Zebediah Figura (＠zfigura)