[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. https://gitlab.winehq.org/wine/wine/-/merge_requests/6335
From: Paul Gofman <pgofman(a)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(). -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6335
From: Paul Gofman <pgofman(a)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 ) ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6335
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79608
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). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79609
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79619
Is there a reason to retry on EACCES, instead of just always setting SO_BROADCAST before calling connect()? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79620
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79621
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79622
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79623
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79626
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335#note_79627
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6335
participants (5)
-
David Aldana (＠luskaner) -
Elizabeth Figura (＠zfigura)
-
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)