[PATCH v3 0/2] MR3186: ntdll/unix: Skip address conversion for TCP sockets in try_send().
On Windows, `sendto()` ignores its destination parameters when provided with a connection-based socket (currently only SOCK_STREAM), even if they contain invalid data. This patch implements this behavior. -- v3: ws2_32/tests: Add test for sendto() and recvfrom() on TCP sockets. https://gitlab.winehq.org/wine/wine/-/merge_requests/3186
From: Ally Sommers <dropbear.sh(a)gmail.com> --- dlls/ntdll/unix/socket.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 4e706323a0a..bc5bc84fdf8 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -985,10 +985,14 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async ) union unix_sockaddr unix_addr; struct msghdr hdr; int attempt = 0; + int sock_type; + socklen_t len = sizeof(sock_type); ssize_t ret; + getsockopt(fd, SOL_SOCKET, SO_TYPE, &sock_type, &len); + memset( &hdr, 0, sizeof(hdr) ); - if (async->addr) + if (async->addr && sock_type != SOCK_STREAM) { hdr.msg_name = &unix_addr; hdr.msg_namelen = sockaddr_to_unix( async->addr, async->addr_len, &unix_addr ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3186
From: Ally Sommers <dropbear.sh(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 56 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index cff4acb0b3c..c3373d872a8 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -13785,6 +13785,61 @@ static void test_connect_udp(void) closesocket(client); } +static void test_tcp_sendto_recvfrom(void) +{ + SOCKET listener, client, server = 0; + SOCKADDR_IN addr = { AF_INET, SERVERPORT }; + SOCKADDR_IN bad_addr, bad_addr_copy; + const char serverMsg[] = "ws2_32/TCP socket test"; + char clientBuf[sizeof(serverMsg)] = { 0 }; + ULONG zero = 0; + ULONG one = 1; + int to_len = sizeof(bad_addr); + int ret; + + inet_pton(AF_INET, SERVERIP, &addr.sin_addr); + + listener = socket(AF_INET, SOCK_STREAM, 0); + ok(listener != INVALID_SOCKET, "Failed to create TCP socket: %d\n", WSAGetLastError()); + ret = bind(listener, (SOCKADDR *)&addr, sizeof(addr)); + ok(!ret, "Failed to bind TCP socket to %s: %d\n", SERVERIP, WSAGetLastError()); + ret = listen(listener, 0); + ok(!ret, "Failed to listen on TCP socket: %d\n", WSAGetLastError()); + + client = socket(AF_INET, SOCK_STREAM, 0); + ok(client != INVALID_SOCKET, "Failed to create TCP socket: %d\n", WSAGetLastError()); + ret = ioctlsocket(client, FIONBIO, &one); + ok(!ret, "Could not set TCP socket to nonblocking: %d; skipping connection\n", WSAGetLastError()); + if (!ret) + { + ret = connect(client, (SOCKADDR *)&addr, sizeof(addr)); + ok(ret == SOCKET_ERROR && GetLastError() == WSAEWOULDBLOCK, + "Incorrect error when connecting to TCP socket: %lu\n", + GetLastError()); + server = accept(listener, NULL, NULL); + ok(server != INVALID_SOCKET, "Could not accept TCP socket connection: %lu\n", + GetLastError()); + ret = ioctlsocket(client, FIONBIO, &zero); + ok(!ret, "Could not set TCP socket to blocking: %lun", GetLastError()); + } + + // Fill the address with invalid data + for (int i = 0; i < sizeof(bad_addr); i++) + ((char *)&bad_addr)[i] = sizeof(bad_addr) - i; + memcpy(&bad_addr_copy, &bad_addr, sizeof(bad_addr_copy)); + + ret = sendto(server, serverMsg, sizeof(serverMsg), 0, (SOCKADDR *)&bad_addr, sizeof(bad_addr)); + ok(ret == sizeof(serverMsg), "Incorrect return value from sendto: %d (%d)\n", ret, WSAGetLastError()); + ret = recvfrom(client, clientBuf, sizeof(clientBuf), 0, (SOCKADDR *)&bad_addr, &to_len); + ok(!memcmp(&bad_addr, &bad_addr_copy, sizeof(bad_addr)), "Provided address modified by recvfrom\n"); + ok(ret == sizeof(serverMsg), "Incorrect return value from recvfrom: %d (%d)\n", ret, WSAGetLastError()); + ok(!memcmp(serverMsg, clientBuf, sizeof(serverMsg)), "Data mismatch over TCP socket\n"); + + closesocket(listener); + closesocket(client); + closesocket(server); +} + START_TEST( sock ) { int i; @@ -13866,6 +13921,7 @@ START_TEST( sock ) test_tcp_reset(); test_icmp(); test_connect_udp(); + test_tcp_sendto_recvfrom(); /* this is an io heavy test, do it at the end so the kernel doesn't start dropping packets */ test_send(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3186
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134273 Your paranoid android. === w10pro64_zh_CN (64 bit report) === ws2_32: sock.c:7798: Test failed: wrong count 0 sock.c:7807: Test failed: wrong count 1
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
+ client = socket(AF_INET, SOCK_STREAM, 0); + ok(client != INVALID_SOCKET, "Failed to create TCP socket: %d\n", WSAGetLastError()); + ret = ioctlsocket(client, FIONBIO, &one); + ok(!ret, "Could not set TCP socket to nonblocking: %d; skipping connection\n", WSAGetLastError()); + if (!ret) + { + ret = connect(client, (SOCKADDR *)&addr, sizeof(addr)); + ok(ret == SOCKET_ERROR && GetLastError() == WSAEWOULDBLOCK, + "Incorrect error when connecting to TCP socket: %lu\n", + GetLastError()); + server = accept(listener, NULL, NULL); + ok(server != INVALID_SOCKET, "Could not accept TCP socket connection: %lu\n", + GetLastError()); + ret = ioctlsocket(client, FIONBIO, &zero); + ok(!ret, "Could not set TCP socket to blocking: %lun", GetLastError()); + } We have a tcp_socketpair() helper that does all of this for you.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3186#note_37238
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
+ if (!ret) + { + ret = connect(client, (SOCKADDR *)&addr, sizeof(addr)); + ok(ret == SOCKET_ERROR && GetLastError() == WSAEWOULDBLOCK, + "Incorrect error when connecting to TCP socket: %lu\n", + GetLastError()); + server = accept(listener, NULL, NULL); + ok(server != INVALID_SOCKET, "Could not accept TCP socket connection: %lu\n", + GetLastError()); + ret = ioctlsocket(client, FIONBIO, &zero); + ok(!ret, "Could not set TCP socket to blocking: %lun", GetLastError()); + } + + // Fill the address with invalid data + for (int i = 0; i < sizeof(bad_addr); i++) + ((char *)&bad_addr)[i] = sizeof(bad_addr) - i; This works, but something like memset(&bad_addr, 0xcc, sizeof(bad_addr); would probably be simpler.
(We also avoid C++ comments, which in this case doesn't seem that necessary anyway, and I think there's still some compiler that chokes on the loop declaration. Although at this point I'm not convinced that's not a -std=c89 problem...?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3186#note_37239
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
+ server = accept(listener, NULL, NULL); + ok(server != INVALID_SOCKET, "Could not accept TCP socket connection: %lu\n", + GetLastError()); + ret = ioctlsocket(client, FIONBIO, &zero); + ok(!ret, "Could not set TCP socket to blocking: %lun", GetLastError()); + } + + // Fill the address with invalid data + for (int i = 0; i < sizeof(bad_addr); i++) + ((char *)&bad_addr)[i] = sizeof(bad_addr) - i; + memcpy(&bad_addr_copy, &bad_addr, sizeof(bad_addr_copy)); + + ret = sendto(server, serverMsg, sizeof(serverMsg), 0, (SOCKADDR *)&bad_addr, sizeof(bad_addr)); + ok(ret == sizeof(serverMsg), "Incorrect return value from sendto: %d (%d)\n", ret, WSAGetLastError()); + ret = recvfrom(client, clientBuf, sizeof(clientBuf), 0, (SOCKADDR *)&bad_addr, &to_len); + ok(!memcmp(&bad_addr, &bad_addr_copy, sizeof(bad_addr)), "Provided address modified by recvfrom\n"); Can you please also check to_len here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3186#note_37240
Nitpick, but the subject of patch 1/2 at least is slightly inaccurate—this is about STREAM sockets, not (just) TCP. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3186#note_37241
participants (4)
-
Ally Sommers -
Ally Sommers (＠dropbear)
-
Marvin -
Zebediah Figura (＠zfigura)