[PATCH 0/1] MR7845: ws2_32: Remove local socket cache check from send().
From: Dmitry Timoshkov <dmitry(a)baikal.ru> recv() doesn't have it, and the whole idea of local socket cache seems a little bit fragile, especially wrt duplicated handles. Wine-Bug: http://bugs.winehq.org/show_bug.cgi?id=58122 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/ws2_32/socket.c | 6 ------ dlls/ws2_32/tests/sock.c | 6 ++++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 3a412b00c1e..f27dfa12302 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1026,12 +1026,6 @@ static int WS2_sendto( SOCKET s, WSABUF *buffers, DWORD buffer_count, DWORD *ret "addr_len %d, overlapped %p, completion %p\n", s, buffers, buffer_count, flags, addr, addr_len, overlapped, completion ); - if (!socket_list_find( s )) - { - SetLastError( WSAENOTSOCK ); - return -1; - } - if (!overlapped && !ret_size) { SetLastError( WSAEFAULT ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index f8d86950f3c..cdf3cb9d89d 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -7564,6 +7564,7 @@ static void test_WSASendMsg(void) ret = pWSASendMsg(INVALID_SOCKET, &msg, 0, NULL, NULL, NULL); ok(ret == SOCKET_ERROR, "WSASendMsg should have failed\n"); err = WSAGetLastError(); + todo_wine ok(err == WSAENOTSOCK, "expected 10038, got %ld instead\n", err); WSASetLastError(0xdeadbeef); @@ -7723,8 +7724,9 @@ static void test_WSASendTo(void) WSASetLastError(12345); ret = WSASendTo(INVALID_SOCKET, &data_buf, 1, NULL, 0, (struct sockaddr*)&addr, sizeof(addr), NULL, NULL); - ok(ret == SOCKET_ERROR && WSAGetLastError() == WSAENOTSOCK, - "WSASendTo() failed: %d/%d\n", ret, WSAGetLastError()); + ok(ret == SOCKET_ERROR, "WSASendTo() should fail\n"); + todo_wine + ok(WSAGetLastError() == WSAENOTSOCK, "got %d\n", WSAGetLastError()); len = sizeof(ret_addr); ret = getsockname(s, (struct sockaddr *)&ret_addr, &len); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7845
``` recv() doesn't have it, and the whole idea of local socket cache seems a little bit fragile, especially wrt duplicated handles. ``` It's certainly fragile, but it's how native behaves. It's not hard to demonstrate that native has a process-local list that it refers to in some cases, and that this indeed breaks when you use DuplicateHandle() directly. In fact, as far as Microsoft is concerned, you're broadly not supposed to do that; you're supposed to use WSADuplicateSocketW() and WSASocket() on the other end. I believe that this is also what SO_UPDATE_ACCEPT_CONTEXT and SO_UPDATE_CONNECT_CONTEXT are for in some sense—I don't think everything works without them, although I'm less sure about that one. I don't think this patch should be committed as long as it breaks todos. It's possible that only some calls in the send() family should check the socket list, and we should move the socket_list_find() call to those specific callers. Or possibly this one is only here because of an error condition ordering problem, which may be trickier to address. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845#note_101307
On Thu Apr 17 19:59:06 2025 +0000, Elizabeth Figura wrote:
``` recv() doesn't have it, and the whole idea of local socket cache seems a little bit fragile, especially wrt duplicated handles. ``` It's certainly fragile, but it's how native behaves. It's not hard to demonstrate that native has a process-local list that it refers to in some cases, and that this indeed breaks when you use DuplicateHandle() directly. In fact, as far as Microsoft is concerned, you're broadly not supposed to do that; you're supposed to use WSADuplicateSocketW() and WSASocket() on the other end. I believe that this is also what SO_UPDATE_ACCEPT_CONTEXT and SO_UPDATE_CONNECT_CONTEXT are for in some sense—I don't think everything works without them, although I'm less sure about that one. I don't think this patch should be committed as long as it breaks todos. It's possible that only some calls in the send() family should check the socket list, and we should move the socket_list_find() call to those specific callers. Or possibly this one is only here because of an error condition ordering problem, which may be trickier to address.
The patch doesn't really break the todos, although it may look like it. Rather the affected tests are broken because it seems that they will fail even when called with a valid socket handle, what will break them is the next condition if (!overlapped && !ret_size) { SetLastError( WSAEFAULT ); return -1; } So, it seems that it's better to set wrong last error value than completely fail send(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845#note_101308
On Thu Apr 17 19:59:06 2025 +0000, Dmitry Timoshkov wrote:
The patch doesn't really break the todos, although it may look like it. Rather the affected tests are broken because it seems that they will fail even when called with a valid socket handle, what will break them is the next condition if (!overlapped && !ret_size) { SetLastError( WSAEFAULT ); return -1; } So, it seems that it's better to set wrong last error value than completely fail send(). Poor phrasing on my part, sorry. My point is that better yet would be to match native behaviour, and not add any new todos.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845#note_101335
On Thu Apr 17 22:34:34 2025 +0000, Elizabeth Figura wrote:
Poor phrasing on my part, sorry. My point is that better yet would be to match native behaviour, and not add any new todos. Also, it should be easy to add a new test that actually replicates what the application is doing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845#note_101336
This merge request was closed by Dmitry Timoshkov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845
I have no idea what the application is doing, that was a pure guess based on the description in the bug report. If this MR is not an acceptable fix and not an improvement then probably it's best to just close it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7845#note_101368
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Elizabeth Figura (＠zfigura)