[PATCH 2/2] ntdll: Don't set socket IO status after queuing async.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Thu Dec 16 13:43:01 CST 2021
On 12/16/21 12:43, Paul Gofman wrote:
> Signed-off-by: Paul Gofman <pgofman at codeweavers.com>
> ---
> Currently NtDeviceIoControlFile() always sets the io comletion status after sock_ioctl().
> This may happen after the application has received and processed the completion already and
> rightfully freed or reused IOSB (OVERLAPPED) pointer and result in data corruption.
> Fixes random crashes in Forza Horizon 4 when connected to online.
>
> dlls/ntdll/unix/file.c | 1 +
> dlls/ntdll/unix/socket.c | 52 ++++++++++++++++++++++++----------------
> 2 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c
> index 51c92df57e3..07bfa6c22c4 100644
> --- a/dlls/ntdll/unix/file.c
> +++ b/dlls/ntdll/unix/file.c
> @@ -5712,6 +5712,7 @@ NTSTATUS WINAPI NtDeviceIoControlFile( HANDLE handle, HANDLE event, PIO_APC_ROUT
> case FILE_DEVICE_BEEP:
> case FILE_DEVICE_NETWORK:
> status = sock_ioctl( handle, event, apc, apc_context, io, code, in_buffer, in_size, out_buffer, out_size );
> + if (status != STATUS_NOT_SUPPORTED && status != STATUS_BAD_DEVICE_TYPE) return status;
> break;
> case FILE_DEVICE_DISK:
> case FILE_DEVICE_CD_ROM:
I think other types of ioctls deserve a similar treatment (although
perhaps that should wait until after code freeze?)
This patch also misses the following socket ioctls:
* IOCTL_AFD_WINE_TRANSMIT
* IOCTL_AFD_WINE_COMPLETE_ASYNC
* IOCTL_AFD_WINE_FIONREAD
* IOCTL_AFD_WINE_SIOCATMARK
* IOCTL_AFD_WINE_GET_INTERFACE_LIST
* IOCTL_AFD_WINE_KEEPALIVE_VALS
> diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
> index 6937a84df85..94e1e9efcfe 100644
> --- a/dlls/ntdll/unix/socket.c
> +++ b/dlls/ntdll/unix/socket.c
> @@ -1162,7 +1162,11 @@ static NTSTATUS do_getsockopt( HANDLE handle, IO_STATUS_BLOCK *io, int level,
> ret = getsockopt( fd, level, option, out_buffer, &len );
> if (needs_close) close( fd );
> if (ret) return sock_errno_to_status( errno );
> - if (io) io->Information = len;
> + if (io)
> + {
> + io->Status = STATUS_SUCCESS;
> + io->Information = len;
> + }
> return STATUS_SUCCESS;
> }
>
> @@ -1179,7 +1183,9 @@ static NTSTATUS do_setsockopt( HANDLE handle, IO_STATUS_BLOCK *io, int level,
>
> ret = setsockopt( fd, level, option, optval, optlen );
> if (needs_close) close( fd );
> - return ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + if (ret) return sock_errno_to_status( errno );
> + if (io) io->Status = STATUS_SUCCESS;
> + return STATUS_SUCCESS;
> }
>
>
> @@ -1308,9 +1314,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (params.msg_flags & AFD_MSG_WAITALL)
> FIXME( "MSG_WAITALL is not supported\n" );
>
> - status = sock_recv( handle, event, apc, apc_user, io, fd, params.buffers, params.count, NULL,
> + return sock_recv( handle, event, apc, apc_user, io, fd, params.buffers, params.count, NULL,
> NULL, NULL, NULL, unix_flags, !!(params.recv_flags & AFD_RECV_FORCE_ASYNC) );
> - break;
> }
>
> case IOCTL_AFD_WINE_RECVMSG:
This leaks "fd".
> @@ -1335,11 +1340,10 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (*ws_flags & WS_MSG_WAITALL)
> FIXME( "MSG_WAITALL is not supported\n" );
>
> - status = sock_recv( handle, event, apc, apc_user, io, fd, u64_to_user_ptr(params->buffers_ptr),
> + return sock_recv( handle, event, apc, apc_user, io, fd, u64_to_user_ptr(params->buffers_ptr),
> params->count, u64_to_user_ptr(params->control_ptr),
> u64_to_user_ptr(params->addr_ptr), u64_to_user_ptr(params->addr_len_ptr),
> ws_flags, unix_flags, params->force_async );
> - break;
> }
>
> case IOCTL_AFD_WINE_SENDMSG:
Same here.
> @@ -1363,9 +1367,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (params->ws_flags & ~(WS_MSG_OOB | WS_MSG_PARTIAL))
> FIXME( "unknown flags %#x\n", params->ws_flags );
>
> - status = sock_send( handle, event, apc, apc_user, io, fd, u64_to_user_ptr( params->buffers_ptr ), params->count,
> + return sock_send( handle, event, apc, apc_user, io, fd, u64_to_user_ptr( params->buffers_ptr ), params->count,
> u64_to_user_ptr( params->addr_ptr ), params->addr_len, unix_flags, params->force_async );
> - break;
> }
>
> case IOCTL_AFD_WINE_TRANSMIT:
Same here.
> @@ -1666,7 +1669,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> io->Information = sizeof(*ws_linger);
> }
>
> - return ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + status = ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + break;
> }
>
> case IOCTL_AFD_WINE_SET_SO_LINGER:
And this might close "fd" twice.
> @@ -1695,7 +1699,6 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> case IOCTL_AFD_WINE_SET_SO_REUSEADDR:
> {
> int fd, needs_close = FALSE;
> - NTSTATUS status;
> int ret;
>
> if ((status = server_get_unix_fd( handle, 0, &fd, &needs_close, NULL, NULL )))
> @@ -1706,7 +1709,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (!ret) ret = setsockopt( fd, SOL_SOCKET, SO_REUSEPORT, in_buffer, in_size );
> #endif
> if (needs_close) close( fd );
> - return ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + status = ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + break;
> }
>
> case IOCTL_AFD_WINE_SET_IP_ADD_MEMBERSHIP:
Same here.
> @@ -1748,7 +1752,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (needs_close) close( fd );
> if (ret) return sock_errno_to_status( errno );
> io->Information = len;
> - return STATUS_SUCCESS;
> + status = STATUS_SUCCESS;
> + break;
> }
>
> case IOCTL_AFD_WINE_SET_IP_DONTFRAGMENT:
Same here.
> @@ -1766,7 +1771,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
>
> if (!once++)
> FIXME( "IP_DONTFRAGMENT is not supported on this platform\n" );
> - return STATUS_SUCCESS; /* fake success */
> + status = STATUS_SUCCESS; /* fake success */
> + break;
> }
> #endif
>
> @@ -1954,7 +1960,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (needs_close) close( fd );
> if (ret) return sock_errno_to_status( errno );
> io->Information = len;
> - return STATUS_SUCCESS;
> + status = STATUS_SUCCESS;
> + break;
> }
>
> case IOCTL_AFD_WINE_SET_IPV6_DONTFRAG:
Same here.
> @@ -2093,7 +2100,6 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> int fd, needs_close = FALSE;
> union unix_sockaddr addr;
> socklen_t len = sizeof(addr);
> - NTSTATUS status;
> int ret;
>
> if ((status = server_get_unix_fd( handle, 0, &fd, &needs_close, NULL, NULL )))
> @@ -2103,12 +2109,14 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> {
> /* changing IPV6_V6ONLY succeeds on an unbound IPv4 socket */
> WARN( "ignoring IPV6_V6ONLY on an unbound IPv4 socket\n" );
> - return STATUS_SUCCESS;
> + status = STATUS_SUCCESS;
> + break;
> }
>
> ret = setsockopt( fd, IPPROTO_IPV6, IPV6_V6ONLY, in_buffer, in_size );
> if (needs_close) close( fd );
> - return ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + status = ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
> + break;
> }
>
> #ifdef SOL_IPX
Same here.
> @@ -2132,7 +2140,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> if (needs_close) close( fd );
> if (ret) return sock_errno_to_status( errno );
> *(DWORD *)out_buffer = value.ipx_pt;
> - return STATUS_SUCCESS;
> + status = STATUS_SUCCESS;
> + break;
> }
>
> case IOCTL_AFD_WINE_SET_IPX_PTYPE:
Same here.
> @@ -2154,7 +2163,6 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> socklen_t len = sizeof(buffer);
> DEVICELIST *ws_list = out_buffer;
> int fd, needs_close = FALSE;
> - NTSTATUS status;
> unsigned int i;
> int ret;
>
> @@ -2184,7 +2192,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> ws_dev->irdaDeviceHints2 = unix_dev->hints[1];
> ws_dev->irdaCharSet = unix_dev->charset;
> }
> - return STATUS_SUCCESS;
> + status = STATUS_SUCCESS;
> + break;
> }
> #endif
>
Same here.
> @@ -2212,5 +2221,8 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
> }
>
> if (needs_close) close( fd );
> +
> + if (status != STATUS_PENDING && !NT_ERROR(status)) io->Status = status;
> +
> return status;
> }
>
More information about the wine-devel
mailing list