[PATCH v2 0/2] MR6335: winsock: Support IPV4 UDP broadcast on connected socket without SO_BROADCAST.
-- v2: server: Support IPV4 UDP broadcast on connected socket without SO_BROADCAST.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/ws2_32/tests/sock.c | 49 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index e8c618cc0af..33e99e7df83 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14194,6 +14194,54 @@ static void test_select_after_WSAEventSelect(void) closesocket(client); }
+static void test_broadcast(void) +{ + struct sockaddr_in bcast = {.sin_family = AF_INET, .sin_port = htons(12345), .sin_addr.s_addr = htonl(INADDR_BROADCAST)}; + struct sockaddr_in6 mcast6; + int val, ret, len; + SOCKET s; + + s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); + ok(s != INVALID_SOCKET, "got error %u.\n", WSAGetLastError()); + ret = sendto(s, "test", 4, 0, (struct sockaddr *)&bcast, sizeof(bcast)); + ok(ret == -1, "got %d, error %u.\n", ret, WSAGetLastError()); + val = 1; + ret = setsockopt(s, SOL_SOCKET, SO_BROADCAST, (char*)&val, sizeof(val)); + ok(!ret, "got %d, error %u.\n", ret, WSAGetLastError()); + ret = sendto(s, "test", 4, 0, (struct sockaddr *)&bcast, sizeof(bcast)); + ok(ret == 4, "got %d, error %u.\n", ret, WSAGetLastError()); + + val = 0; + ret = setsockopt(s, SOL_SOCKET, SO_BROADCAST, (char*)&val, sizeof(val)); + ok(!ret, "got %d, error %u.\n", ret, WSAGetLastError()); + ret = sendto(s, "test", 4, 0, (struct sockaddr *)&bcast, sizeof(bcast)); + ok(ret == -1, "got %d, error %u.\n", ret, WSAGetLastError()); + + ret = connect(s, (struct sockaddr *)&bcast, sizeof(bcast)); + todo_wine ok(!ret, "got error %u.\n", WSAGetLastError()); + val = 1; + len = sizeof(val); + ret = getsockopt(s, SOL_SOCKET, SO_BROADCAST, (char*)&val, &len); + ok(!ret, "got %d, error %u.\n", ret, WSAGetLastError()); + ok(!val, "got %d.\n", val); + ret = sendto(s, "test", 4, 0, (struct sockaddr *)&bcast, sizeof(bcast)); + ok(ret == -1, "got %d, error %u.\n", ret, WSAGetLastError()); + ret = send(s, "test", 4, 0); + todo_wine ok(ret == 4, "got %d, error %u.\n", ret, WSAGetLastError()); + closesocket(s); + + s = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP); + ok(s != INVALID_SOCKET, "got error %u.\n", WSAGetLastError()); + memset(&mcast6, 0, sizeof(mcast6)); + ret = inet_pton(AF_INET6, "ff01::1", &mcast6.sin6_addr); + ok(ret, "got error %u.\n", WSAGetLastError()); + mcast6.sin6_family = AF_INET6; + mcast6.sin6_port = htons(12345); + ret = sendto(s, "test", 4, 0, (struct sockaddr *)&mcast6, sizeof(mcast6)); + ok(ret == 4, "got %d, error %u.\n", ret, WSAGetLastError()); + closesocket(s); +} + START_TEST( sock ) { int i; @@ -14276,6 +14324,7 @@ START_TEST( sock ) test_icmp(); test_connect_udp(); test_tcp_sendto_recvfrom(); + test_broadcast();
/* There is apparently an obscure interaction between this test and * test_WSAGetOverlappedResult().
From: Paul Gofman pgofman@codeweavers.com
--- dlls/ws2_32/tests/sock.c | 4 ++-- server/sock.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 33e99e7df83..0015aae67a5 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -14218,7 +14218,7 @@ static void test_broadcast(void) ok(ret == -1, "got %d, error %u.\n", ret, WSAGetLastError());
ret = connect(s, (struct sockaddr *)&bcast, sizeof(bcast)); - todo_wine ok(!ret, "got error %u.\n", WSAGetLastError()); + ok(!ret, "got error %u.\n", WSAGetLastError()); val = 1; len = sizeof(val); ret = getsockopt(s, SOL_SOCKET, SO_BROADCAST, (char*)&val, &len); @@ -14227,7 +14227,7 @@ static void test_broadcast(void) ret = sendto(s, "test", 4, 0, (struct sockaddr *)&bcast, sizeof(bcast)); ok(ret == -1, "got %d, error %u.\n", ret, WSAGetLastError()); ret = send(s, "test", 4, 0); - todo_wine ok(ret == 4, "got %d, error %u.\n", ret, WSAGetLastError()); + ok(ret == 4, "got %d, error %u.\n", ret, WSAGetLastError()); closesocket(s);
s = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP); diff --git a/server/sock.c b/server/sock.c index 06ffd1b81f8..4d159768478 100644 --- a/server/sock.c +++ b/server/sock.c @@ -2668,6 +2668,25 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) ret = connect( unix_fd, &unix_addr.addr, unix_len ); }
+ if (ret < 0 && errno == EACCES && sock->state == SOCK_CONNECTIONLESS && unix_addr.addr.sa_family == AF_INET) + { + int broadcast, saved_errno; + socklen_t len = sizeof(broadcast); + + broadcast = 1; + getsockopt( unix_fd, SOL_SOCKET, SO_BROADCAST, &broadcast, &len ); + if (!broadcast) + { + broadcast = 1; + setsockopt( unix_fd, SOL_SOCKET, SO_BROADCAST, &broadcast, sizeof(broadcast) ); + ret = connect( unix_fd, &unix_addr.addr, unix_len ); + saved_errno = errno; + broadcast = 0; + setsockopt( unix_fd, SOL_SOCKET, SO_BROADCAST, &broadcast, sizeof(broadcast) ); + errno = saved_errno; + } + } + if (ret < 0 && errno != EINPROGRESS) { set_error( sock_get_ntstatus( errno ) );
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=147929
Your paranoid android.
=== w7u_adm (32 bit report) ===
ws2_32: sock.c:3172: Test failed: got 0, i 92.
=== debian11b (64 bit WoW report) ===
kernel32: comm.c:1586: Test failed: Unexpected time 1000, expected around 500
v2: - Remove INADDR_BROADCAST check for retrying with SO_BROADCAST.
On Thu Aug 22 15:11:42 2024 +0000, David Gow wrote:
Thanks very much! This seems to be working for Age of Empires 1 & 2 — at the very least, it resolves the issues with getting into the game lobby, and (in Age of Empires 2) starting a LAN game. Promising-looking packets are flowing. I haven't got a second copy of the game here to test an actual LAN multiplayer game fully at the moment, but this is definitely an improvement. Indeed, this works better than the [patch here](https://github.com/ValveSoftware/Proton/issues/3189#issuecomment-1823909045) did when I tested it, though that may be due to other wine or game updates since: the lobby no longer disconnects on AOE2. One minor thought about the implementation: does it need to support other broadcast addresses, not just 255.255.255.255? It's fine as-is for Age of Empires, but maybe something else cares.
Thanks! Yes, good point about generic broadcast addresses. I just removed the check for INADDR_BROADCAST, properly determining all the possible broadcast addresses is complicated. That shouldn't hurt as the condition with the present EACCES check (also received synchronously on nonblocking socket) looks already specific enough.
As a separate note, I tested locally with various broadcast masks and Windows results are mildly insane. sendto() with explicit address succeeds here on 192.168.0.255 without SO_BROADCAST set but still fails with SO_BROADCAST explicitly cleared. And then succeeds always regardless of SO_BROADCAST with 192.268.255.255 and 192.255.255.255. I think I don't want to address those specifics in the absence of anything depending on that (which is also less likely to be dependent upon looking at the inconsistency of this behaviour).
Interesting that it works even if you then disable SO_BROADCAST. I'd worry that's not reliable, but given the kernel's position towards API breakage it's probably fine.
Is there a reason to retry on EACCES, instead of just always setting SO_BROADCAST before calling connect()?
Yes, it is undocumented as far as I can tell but probably not accidental neither on Windows nor on Linux. connect() on UDP socket does something, apparently in particular pre-checking destination address.
Is there a reason to retry on EACCES, instead of just always setting SO_BROADCAST before calling connect()?
Well, I didn't check thoroughly if it is going to break anything under presumption that it might and it is cleaner to avoid such tweaks on the default path. Also, it is an extra syscall in wineserver otherwise unneeded on the common path (and avoided in the current version of the patch), I'd expect the case when it will actually trigger to be very rare.
The more-code alternative would be to move SO_BROADCAST handling to server (so we still properly report the option value) and then just set SO_BROADCAST permanently in this case. Can do but so far concluded that it is unneeded complication.
On Thu Aug 22 15:02:58 2024 +0000, Paul Gofman wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/6335/diffs?diff_id=127917&start_sha=481d0357b42472efa2a2644cb8667324b958b48d#3429c2152ff00da2c8dc7ad7d59d2987edb6f423_2672_2671)
That's right, checking if an address is a broadcast is non trivial, you would have to retrieve the local network adapters and check if the provided IP belong to it and then with the network address apply the mask and finally compare with the provided IP. And even then I'm not sure this is 100% correct.
On Thu Aug 22 17:26:34 2024 +0000, Paul Gofman wrote:
Is there a reason to retry on EACCES, instead of just always setting
SO_BROADCAST before calling connect()? Well, I didn't check thoroughly if it is going to break anything under presumption that it might and it is cleaner to avoid such tweaks on the default path. Also, it is an extra syscall in wineserver otherwise unneeded on the common path (and avoided in the current version of the patch), I'd expect the case when it will actually trigger to be very rare.
I'm not sure that either of those are really worth worrying about, but I can't say I have strong feelings about it either.
This merge request was approved by Elizabeth Figura.
-
David Aldana (＠luskaner) -
Elizabeth Figura (＠zfigura)
-
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)