From: Dmitry Timoshkov dmitry@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@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);
``` 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.
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().
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.
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.
This merge request was closed by Dmitry Timoshkov.
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.
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Elizabeth Figura (＠zfigura)