[PATCH v2 0/2] MR10894: ws2_32: Return WSAENOTSOCK instead of WSAENOTSUPP from select().
That helps War Thunder which otherwise may complain that select() returned 10045 in a message box when going in game. -- v2: ws2_32: Return WSAENOTSOCK instead of WSAENOTSUPP from select() when trying to select on non-socket handle. ws2_32/tests: Test select() with valid but non-socket handle. https://gitlab.winehq.org/wine/wine/-/merge_requests/10894
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/ws2_32/tests/sock.c | 62 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index d861f303367..a8ad034530c 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -4360,6 +4360,8 @@ static void test_select(void) unsigned int apc_count; unsigned int maxfd, i; char *page_pair; + char path[MAX_PATH]; + HANDLE file, hdup; fdRead = socket(AF_INET, SOCK_STREAM, 0); ok( (fdRead != INVALID_SOCKET), "socket failed unexpectedly: %d\n", WSAGetLastError() ); @@ -4459,8 +4461,68 @@ static void test_select(void) maxfd = fdRead; if(fdWrite > maxfd) maxfd = fdWrite; + GetSystemWindowsDirectoryA(path, ARRAY_SIZE(path)); + strcat(path, "\\system.ini"); + + file = CreateFileA(path, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_ALWAYS, 0x0, NULL); + ok(file != INVALID_HANDLE_VALUE, "failed to open file, error %lu\n", GetLastError()); + + if ((SOCKET)file > maxfd) maxfd = (SOCKET)file; + ret = DuplicateHandle(GetCurrentProcess(), (HANDLE)fdRead, GetCurrentProcess(), &hdup, 0, FALSE, DUPLICATE_SAME_ACCESS); + ok(ret, "got %d.\n", ret); + + /* Test with valid but non socket handle which also supports some ioctls. */ + FD_ZERO_ALL(); + FD_SET((SOCKET)file, &readfds); + SetLastError(0); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok ( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got %i\n", ret); + todo_wine ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); + ok ( FD_ISSET((SOCKET)file, &readfds), "FD should be set\n"); + + FD_ZERO(&readfds); + FD_SET(fdRead, &readfds); + FD_SET(fdRead, &exceptfds); + FD_SET((SOCKET)file, &writefds); + SetLastError(0); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok ( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got %i\n", ret); + ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); + + FD_ZERO_ALL(); + FD_SET(fdRead, &readfds); + FD_SET(fdWrite, &writefds); + FD_SET((SOCKET)file, &exceptfds); + SetLastError(0); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok ( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got %i\n", ret); + todo_wine ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); + + /* Test with duplicated handle of valid socket. */ + FD_ZERO_ALL(); + FD_SET((SOCKET)hdup, &readfds); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok( !ret, "select returned %d\n", ret ); + FD_ZERO(&readfds); FD_SET(fdRead, &readfds); + FD_SET(fdRead, &exceptfds); + FD_SET((SOCKET)hdup, &writefds); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok( ret == 1, "select returned %d\n", ret ); + + FD_ZERO_ALL(); + FD_SET(fdRead, &readfds); + FD_SET(fdWrite, &writefds); + FD_SET((SOCKET)hdup, &exceptfds); + ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); + ok( ret == 1, "select returned %d\n", ret ); + + CloseHandle(hdup); + CloseHandle(file); + + FD_ZERO_ALL(); + FD_SET(fdRead, &readfds); apc_count = 0; ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count); ok(ret, "QueueUserAPC returned %d\n", ret); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10894
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/ws2_32/socket.c | 1 + dlls/ws2_32/tests/sock.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index f5580cab4e3..b2601c6e4af 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -3022,6 +3022,7 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr, status = NtDeviceIoControlFile( (HANDLE)poll_socket, sync_event, NULL, NULL, &io, IOCTL_AFD_POLL, params, params_size, params, params_size ); + if (status == STATUS_NOT_SUPPORTED) status = STATUS_INVALID_HANDLE; if (status == STATUS_PENDING) { if (wait_event_alertable( sync_event ) == WAIT_FAILED) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index a8ad034530c..e67e50907b0 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -4477,7 +4477,7 @@ static void test_select(void) SetLastError(0); ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); ok ( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got %i\n", ret); - todo_wine ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); + ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); ok ( FD_ISSET((SOCKET)file, &readfds), "FD should be set\n"); FD_ZERO(&readfds); @@ -4496,7 +4496,7 @@ static void test_select(void) SetLastError(0); ret = select(maxfd + 1, &readfds, &writefds, &exceptfds, &select_timeout); ok ( (ret == SOCKET_ERROR), "expected SOCKET_ERROR, got %i\n", ret); - todo_wine ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); + ok ( WSAGetLastError() == WSAENOTSOCK, "expected WSAENOTSOCK, got %i\n", WSAGetLastError()); /* Test with duplicated handle of valid socket. */ FD_ZERO_ALL(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10894
v2: * add tests with duplicated valid socket. The tests show that select still succeeds this way, so we shouldn't check sockets with socket_list_find() in select(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_139862
This merge request was approved by Erich Hoover. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894
Hmm... I'm a little concerned, from the CW bug tracker I gather that this is a handle that was closed and has now been reallocated to something else. That means it could be anything; what if it returns something other than STATUS_NOT_SUPPORTED? STATUS_INVALID_DEVICE_REQUEST seems likely. I think native does this a different way, which I'm investigating... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140067
Hm, yes, it seems possible. We could of course do is_valid_socket() on request socket (I all the other sockets being polled are handled inside the ioctl handling and that part is responsible for returning proper status in this case). But IMO it is better to avoid an extra ioctl and check status instead, maybe add STATUS_INVALID_DEVICE_REQUEST to the same check? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140068
FWIW as far as this game is concerned, I just checked that setting the unknown status (like 0xdeadbeef) which results in WSAEINVAL is also fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140069
STATUS_NOT_SUPPORTED is different from STATUS_INVALID_DEVICE_REQUEST in a sense that it is explicitly converted to WSAENOTSUP. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140070
Oh another way ofc is that maybe we should not extract request socket from select sockets at all and use some pre-created special AFD socket. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140072
Thinking of, that looks better and will potentially avoid a broader class of problems. Should I? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140073
Oh another way ofc is that maybe we should not extract request socket from select sockets at all and use some pre-created special AFD socket.
To summarize some off-list discussion, this is roughly what native seems to do, at least for select(). But WSAPoll() behaves differently; you can mix valid and invalid sockets and it'll return success. There's also some other odd subtle differences in the existing WSAPoll() tests. Getting this right seems like a rabbit hole and I'm not convinced it's worthwhile. This merge request is probably fine enough. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894#note_140740
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10894
participants (4)
-
Elizabeth Figura (＠zfigura) -
Erich Hoover (＠ehoover)
-
Paul Gofman
-
Paul Gofman (＠gofman)