[PATCH v2 1/2] ws2_32: Validate pointers in select().

[Torge Matthies]

The documentation says that SOCKET_ERROR is returned and the error is
set to WSAEFAULT if any of the input pointers point to unmapped memory.

Signed-off-by: Torge Matthies 
---
 dlls/ws2_32/socket.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index b3dab22ae6e..7b7a985c40d 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -2384,6 +2384,14 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr,

TRACE( "read %p, write %p, except %p, timeout %p\n", read_ptr, write_ptr, except_ptr, timeout );

+    if ((read_ptr && IsBadWritePtr(read_ptr, sizeof(*read_ptr)))
+        || (write_ptr && IsBadWritePtr(write_ptr, sizeof(*write_ptr)))
+        || (except_ptr && IsBadWritePtr(except_ptr, sizeof(*except_ptr))))
+    {
+        SetLastError( WSAEFAULT );
+        return -1;
+    }
+
     FD_ZERO( &read );
     FD_ZERO( &write );
     FD_ZERO( &except );
@@ -2393,11 +2401,6 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr,

if (!(sync_event = get_sync_event())) return -1;

-    if (timeout)
-        params->timeout = timeout->tv_sec * -10000000 + timeout->tv_usec * -10;
-    else
-        params->timeout = TIMEOUT_INFINITE;
-
     for (i = 0; i < read.fd_count; ++i)
     {
         params->sockets[params->count].socket = read.fd_array[i];
@@ -2428,6 +2431,18 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr,
         return -1;
     }

+    if (timeout)
+    {
+        if (IsBadReadPtr(timeout, sizeof(*timeout)))
+        {
+            SetLastError( WSAEFAULT );
+            return -1;
+        }
+        params->timeout = timeout->tv_sec * -10000000 + timeout->tv_usec * -10;
+    }
+    else
+        params->timeout = TIMEOUT_INFINITE;
+
     params_size = offsetof( struct afd_poll_params, sockets[params->count] );

status = NtDeviceIoControlFile( (HANDLE)poll_socket, sync_event, NULL, NULL, &io,

[PATCH v2 2/2] ws2_32/tests: Add tests for passing invalid pointers to select().

[Torge Matthies]

Signed-off-by: Torge Matthies 
---
 dlls/ws2_32/tests/sock.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 2bb219d7c0e..989b7c3db57 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -3030,6 +3030,46 @@ static void test_errors(void)

ret = select(1, NULL, &set, NULL, &timeval);
         ok( (ret == 0), "expected 0 (timeout), got: %d\n", ret );
+
+        FD_ZERO( &set );
+        FD_SET( sock, &set );
+        ret = select(1, NULL, (fd_set *)0x1, NULL, &timeval);
+        ok( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got: %d\n", ret );
+        if (ret == SOCKET_ERROR)
+        {
+            err = WSAGetLastError();
+            ok( (err == WSAEFAULT), "expected WSAEFAULT, got: %d\n", err );
+        }
+
+        FD_ZERO( &set );
+        FD_SET( sock, &set );
+        ret = select(1, NULL, &set, NULL, (TIMEVAL *)0x1);
+        ok( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got: %d\n", ret );
+        if (ret == SOCKET_ERROR)
+        {
+            err = WSAGetLastError();
+            ok( (err == WSAEFAULT), "expected WSAEFAULT, got: %d\n", err );
+        }
+    }
+
+    {
+        fd_set set = {0};
+
+        ret = select(1, NULL, &set, NULL, (TIMEVAL *)0x1);
+        ok( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got: %d\n", ret );
+        if (ret == SOCKET_ERROR)
+        {
+            err = WSAGetLastError();
+            ok( (err == WSAEINVAL), "expected WSAEINVAL, got: %d\n", err );
+        }
+    }
+
+    ret = select(1, NULL, NULL, NULL, (TIMEVAL *)0x1);
+    ok( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got: %d\n", ret );
+    if (ret == SOCKET_ERROR)
+    {
+        err = WSAGetLastError();
+        ok( (err == WSAEINVAL), "expected WSAEINVAL, got: %d\n", err );
     }

ret = closesocket(sock);

[PATCH v2 2/2] ws2_32/tests: Add tests for passing invalid pointers to select().

[Marvin]

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=100251

Your paranoid android.

=== debiant2 (32 bit German report) ===

ws2_32:
sock.c:1135: Test failed: wait failed, error 258

=== debiant2 (32 bit French report) ===

ws2_32:
sock.c:1723: Test failed: bind(s1) failed error: 10048
sock.c:1777: Test failed: bind() failed error: 10013
sock.c:2014: Test failed: bind failed, error 10048
sock: Timeout

=== debiant2 (32 bit Hebrew:Israel report) ===

ws2_32:
sock.c:1723: Test failed: bind(s1) failed error: 10048
sock.c:1777: Test failed: bind() failed error: 10013
sock.c:2014: Test failed: bind failed, error 10048
sock: Timeout

=== debiant2 (32 bit Hindi:India report) ===

ws2_32:
sock.c:1723: Test failed: bind(s1) failed error: 10048
sock.c:1777: Test failed: bind() failed error: 10013
sock.c:2014: Test failed: bind failed, error 10048
sock: Timeout

=== debiant2 (32 bit Japanese:Japan report) ===

ws2_32:
sock.c:1723: Test failed: bind(s1) failed error: 10048
sock.c:1777: Test failed: bind() failed error: 10013
sock.c:2014: Test failed: bind failed, error 10048
sock: Timeout

=== debiant2 (32 bit Chinese:China report) ===

ws2_32:
sock.c:1723: Test failed: bind(s1) failed error: 10048
sock.c:1777: Test failed: bind() failed error: 10013
sock.c:2014: Test failed: bind failed, error 10048
sock: Timeout

=== debiant2 (64 bit WoW report) ===

ws2_32:
sock.c:5204: Test failed: expected timeout

[Marvin]

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=100250

Your paranoid android.

=== debiant2 (32 bit report) ===

ws2_32:
sock.c:5164: Test failed: expected timeout

[Alexandre Julliard]

Torge Matthies  writes:
> The documentation says that SOCKET_ERROR is returned and the error is
> set to WSAEFAULT if any of the input pointers point to unmapped memory.

Do you have an application that does this?

[Torge Matthies]

A crash in osu! was reported on Discord that segfaulted in this
function. I don't know if there is any underlying bug and if adding
these patches just hides a bug, but the current behavior of select() of
just crashing is definitely wrong.

On Mon, 18 Oct 2021 at 17:07, Alexandre Julliard  wrote:
> Torge Matthies  writes:
> > The documentation says that SOCKET_ERROR is returned and the error is
> > set to WSAEFAULT if any of the input pointers point to unmapped memory.
>
> Do you have an application that does this?
>
> --
> Alexandre Julliard
> [e-mail]

[Alexandre Julliard]

Torge Matthies  writes:
> A crash in osu! was reported on Discord that segfaulted in this
> function. I don't know if there is any underlying bug and if adding
> these patches just hides a bug, but the current behavior of select() of
> just crashing is definitely wrong.

Maybe, but we don't want to add such pointer checks all over the
place. If there's an actual app that requires this, it should be done
with an exception handler. But first it should be confirmed that it's
not a Wine bug that's causing the invalid pointer.

[Torge Matthies]

The IsBad{Read,Write}Ptr implementation looked too complex to copy it
around everywhere. And idk how to debug a crash that I've only seen in
a .NET backtrace once.

Feel free to change the patches to how you would do it.
I have attached the crash message from osu!.

On Tue, 19 Oct 2021 at 19:42, Alexandre Julliard  wrote:
> Torge Matthies  writes:
> > A crash in osu! was reported on Discord that segfaulted in this
> > function. I don't know if there is any underlying bug and if adding
> > these patches just hides a bug, but the current behavior of select() of
> > just crashing is definitely wrong.
>
> Maybe, but we don't want to add such pointer checks all over the
> place. If there's an actual app that requires this, it should be done
> with an exception handler. But first it should be confirmed that it's
> not a Wine bug that's causing the invalid pointer.
>
> --
> Alexandre Julliard
> [e-mail]