[PATCH 0/6] MR1111: winsock: Always use Unix SO_REUSEADDR on TCP sockets.

[Paul Gofman (＠gofman)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50955

The problem which this patchset aims to solve is (sometimes) huge timeout on TCP listening port availability on Linux after the listening socket was closed one or another way. By default, the listening port will also be blocked for the whole time accepted socket waits through lengthy timeouts (sometimes; most surely when the listening process with an active connection was force killed, but not limited to this condition).

BSD SO_REUSEADDR socket option is aimed mainly to avoid that extra wait on binding already closed listening socket address. From [1]: "Indicates that the rules used in validating addresses supplied in a bind(2) call should allow reuse of local addresses. For AF_INET sockets this means that a socket              may bind, except when there is an active listening socket bound to the address.".

Unix SO_REUSEADDR does not really allow reusing address in the Winsock sense. It will just allow to ditch the timeout (which is always the case on Windows without any specific options). Unfortunately it is not the only effect of the option. It still won't allow listening on the address simultaneously (unlike Winsock SO_REUSEADDR which allows simultaneous listening), or binding to an address which is being listened. But it will allow to bind different sockets for the same address which is not the Winsock behaviour when Winsock SO_REUSEADDR is set.

So the patchset enables SO_REUSEADDR on every TCP socket and introduces the bound address tracking which will allow to return an error from bind() when needed.

Not related to this patchset, but Winsock SO_REUSEADDR is somewhat closer to BSD SO_REUSEPORT, although is different in a way that _REUSEPORT will load balance connections between listeners while with Winsock _REUSEADDR the connections will always go to the first listener.

I hope that the bound addresses tracking introduced in these patches may be reused in the future. E. g., maybe it might be helpful on the way of implementing the todos introduced by my extended tests (those todos are not related to this patchset and exist both with and without it).

1. https://man7.org/linux/man-pages/man7/socket.7.html

[PATCH 4/6] ntdll: Move SO_REUSEADDR handling to server.

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntdll/unix/socket.c | 21 --------------------
 server/sock.c            | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
index f8ed9f6f854..33e72462aa9 100644
--- a/dlls/ntdll/unix/socket.c
+++ b/dlls/ntdll/unix/socket.c
@@ -1974,27 +1974,6 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc
         case IOCTL_AFD_WINE_SET_SO_OOBINLINE:
             return do_setsockopt( handle, io, SOL_SOCKET, SO_OOBINLINE, in_buffer, in_size );

-        case IOCTL_AFD_WINE_GET_SO_REUSEADDR:
-            return do_getsockopt( handle, io, SOL_SOCKET, SO_REUSEADDR, out_buffer, out_size );
-
-        /* BSD socket SO_REUSEADDR is not 100% compatible to winsock semantics;
-         * however, using it the BSD way fixes bug 8513 and seems to be what
-         * most programmers assume, anyway */
-        case IOCTL_AFD_WINE_SET_SO_REUSEADDR:
-        {
-            int ret;
-
-            if ((status = server_get_unix_fd( handle, 0, &fd, &needs_close, NULL, NULL )))
-                return status;
-
-            ret = setsockopt( fd, SOL_SOCKET, SO_REUSEADDR, in_buffer, in_size );
-#ifdef __APPLE__
-            if (!ret) ret = setsockopt( fd, SOL_SOCKET, SO_REUSEPORT, in_buffer, in_size );
-#endif
-            status = ret ? sock_errno_to_status( errno ) : STATUS_SUCCESS;
-            break;
-        }
-
         case IOCTL_AFD_WINE_SET_IP_ADD_MEMBERSHIP:
             return do_setsockopt( handle, io, IPPROTO_IP, IP_ADD_MEMBERSHIP, in_buffer, in_size );

diff --git a/server/sock.c b/server/sock.c
index 4e57d6774a6..87a536bb363 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -2905,6 +2905,29 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
         return;
     }

+    /* BSD socket SO_REUSEADDR is not 100% compatible to winsock semantics;
+     * however, using it the BSD way fixes bug 8513 and seems to be what
+     * most programmers assume, anyway */
+    case IOCTL_AFD_WINE_SET_SO_REUSEADDR:
+    {
+        int reuse, ret;
+
+        if (get_req_data_size() < sizeof(reuse))
+        {
+            set_error( STATUS_BUFFER_TOO_SMALL );
+            return;
+        }
+
+        reuse = *(int *)get_req_data();
+        ret = setsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse) );
+#ifdef __APPLE__
+        if (!ret) ret = setsockopt( unix_fd, SOL_SOCKET, SO_REUSEPORT, &reuse, sizeof(reuse) );
+#endif
+        if (ret)
+            set_error( sock_get_ntstatus( errno ) );
+        return;
+    }
+
     case IOCTL_AFD_WINE_GET_SO_SNDBUF:
     {
         int sndbuf = sock->sndbuf;
@@ -2992,6 +3015,24 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
         return;
     }

+    case IOCTL_AFD_WINE_GET_SO_REUSEADDR:
+    {
+        int reuse;
+        socklen_t len = sizeof(reuse);
+
+        if (!get_reply_max_size())
+        {
+            set_error( STATUS_BUFFER_TOO_SMALL );
+            return;
+        }
+
+        if (!getsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, &reuse, &len ))
+            set_reply_data( &reuse, min( sizeof(reuse), get_reply_max_size() ));
+        else
+            set_error( sock_get_ntstatus( errno ) );
+        return;
+    }
+
     case IOCTL_AFD_POLL:
     {
         if (get_reply_max_size() < get_req_data_size())

[PATCH 3/6] ws2_32/tests: Also test TCP6 in test_so_reuseaddr().

[Paul Gofman]

From: Paul Gofman

---
 dlls/ws2_32/tests/sock.c | 186 ++++++++++++++++++++++-----------------
 1 file changed, 105 insertions(+), 81 deletions(-)

diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 05ad45c6cdb..4f04a0c607d 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -2015,119 +2015,143 @@ static void test_set_getsockopt(void)

static void test_so_reuseaddr(void)
 {
-    struct sockaddr_in saddr;
+    static struct sockaddr_in6 saddr_in6_any, saddr_in6_loopback;
+    static struct sockaddr_in saddr_in_any, saddr_in_loopback;
+
+    static const struct
+    {
+        int domain;
+        struct sockaddr *addr_any;
+        struct sockaddr *addr_loopback;
+        socklen_t addrlen;
+    }
+    tests[] =
+    {
+        { AF_INET, (struct sockaddr *)&saddr_in_any, (struct sockaddr *)&saddr_in_loopback, sizeof(saddr_in_any) },
+        { AF_INET6, (struct sockaddr *)&saddr_in6_any, (struct sockaddr *)&saddr_in6_loopback, sizeof(saddr_in6_any) },
+    };
     unsigned int rc, reuse;
+    struct sockaddr saddr;
     SOCKET s1, s2, s3, s4;
+    unsigned int i;
     int size;

-    saddr.sin_family      = AF_INET;
-    saddr.sin_port        = htons(SERVERPORT+1);
-    saddr.sin_addr.s_addr = inet_addr("127.0.0.1");
+    saddr_in_any.sin_family = AF_INET;
+    saddr_in_any.sin_port = htons(SERVERPORT + 1);
+    saddr_in_any.sin_addr.s_addr = htonl(INADDR_ANY);
+    saddr_in_loopback = saddr_in_any;
+    saddr_in_loopback.sin_addr.s_addr = htonl(INADDR_LOOPBACK);

-    s1=socket(AF_INET, SOCK_STREAM, 0);
-    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+    saddr_in6_any.sin6_family = AF_INET6;
+    saddr_in6_any.sin6_port = htons(SERVERPORT + 1);
+    memset( &saddr_in6_any.sin6_addr, 0, sizeof(saddr_in6_any.sin6_addr) );
+    saddr_in6_loopback = saddr_in6_any;
+    inet_pton(AF_INET6, "::1", &saddr_in6_loopback.sin6_addr);

-    reuse = 1;
-    rc = setsockopt(s1, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+    for (i = 0; i < ARRAY_SIZE(tests); ++i)
+    {
+        winetest_push_context("test %u", i);
+        s1 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    rc = bind(s1, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        reuse = 1;
+        rc = setsockopt(s1, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    s2 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        rc = bind(s1, tests[i].addr_loopback, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    reuse=0x1234;
-    size=sizeof(reuse);
-    rc=getsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, &size);
-    ok(!rc && !reuse,"got rc %d, reuse %d.\n", rc, reuse);
+        s2 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(rc == SOCKET_ERROR, "got rc %d, error %d.\n", rc, WSAGetLastError());
+        reuse = 0x1234;
+        size = sizeof(reuse);
+        rc = getsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, &size);
+        ok(!rc && !reuse,"got rc %d, reuse %d.\n", rc, reuse);

-    reuse = 1;
-    rc = setsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = bind(s2, tests[i].addr_loopback, tests[i].addrlen);
+        ok(rc == SOCKET_ERROR, "got rc %d, error %d.\n", rc, WSAGetLastError());

-    rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        reuse = 1;
+        rc = setsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    s3 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        rc = bind(s2, tests[i].addr_loopback, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    /* Test if we can really connect to one of them. */
-    rc = listen(s1, 1);
-    ok(!rc, "got error %d.\n", WSAGetLastError());
-    rc = listen(s2, 1);
-    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
-    rc = connect(s3, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        s3 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    /* The connection is delivered to the first socket. */
-    size = sizeof(saddr);
-    s4 = accept(s1, (struct sockaddr*)&saddr, &size);
-    saddr.sin_port = htons(SERVERPORT + 1);
-    ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        /* Test if we can really connect to one of them. */
+        rc = listen(s1, 1);
+        ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = listen(s2, 1);
+        todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = connect(s3, tests[i].addr_loopback, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

+        /* The connection is delivered to the first socket. */
+        size = tests[i].addrlen;
+        s4 = accept(s1, &saddr, &size);
+        ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    closesocket(s1);
-    closesocket(s2);
-    closesocket(s3);
-    closesocket(s4);
+        closesocket(s1);
+        closesocket(s2);
+        closesocket(s3);
+        closesocket(s4);

-    /* Test binding and listening on INADDR_ANY together with 127.0.0.1. */
-    s1 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        /* Test binding and listening on INADDR_ANY together with 127.0.0.1. */
+        s1 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    saddr.sin_addr.s_addr = htonl(INADDR_ANY);
-    rc = bind(s1, (struct sockaddr *)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = bind(s1, tests[i].addr_any, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    rc = listen(s1, 1);
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = listen(s1, 1);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    s2 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        s2 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    saddr.sin_addr.s_addr = inet_addr("127.0.0.1");
-    rc = bind(s2, (struct sockaddr *)&saddr, sizeof(saddr));
-    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = bind(s2, tests[i].addr_loopback, tests[i].addrlen);
+        todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());

-    rc = listen(s2, 1);
-    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = listen(s2, 1);
+        todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());

-    s3 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        s3 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    rc = connect(s3, (struct sockaddr *)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = connect(s3, tests[i].addr_loopback, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    size = sizeof(saddr);
-    s4 = accept(s2, (struct sockaddr *)&saddr, &size);
-    saddr.sin_port        = htons(SERVERPORT + 1);
-    todo_wine ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        size = tests[i].addrlen;
+        s4 = accept(s2, &saddr, &size);
+        todo_wine ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    closesocket(s1);
-    closesocket(s2);
-    closesocket(s3);
-    closesocket(s4);
+        closesocket(s1);
+        closesocket(s2);
+        closesocket(s3);
+        closesocket(s4);

-    /* Test binding to INADDR_ANY on two sockets. */
-    s1 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        /* Test binding to INADDR_ANY on two sockets. */
+        s1 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    saddr.sin_addr.s_addr = htonl(INADDR_ANY);
-    rc = bind(s1, (struct sockaddr *)&saddr, sizeof(saddr));
-    ok(!rc, "got error %d.\n", WSAGetLastError());
+        rc = bind(s1, tests[i].addr_any, tests[i].addrlen);
+        ok(!rc, "got error %d.\n", WSAGetLastError());

-    s2 = socket(AF_INET, SOCK_STREAM, 0);
-    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+        s2 = socket(tests[i].domain, SOCK_STREAM, 0);
+        ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-    rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(rc == SOCKET_ERROR && WSAGetLastError() == WSAEADDRINUSE, "got rc %d, error %d.\n", rc, WSAGetLastError());
+        rc = bind(s2, tests[i].addr_any, tests[i].addrlen);
+        ok(rc == SOCKET_ERROR && WSAGetLastError() == WSAEADDRINUSE, "got rc %d, error %d.\n", rc, WSAGetLastError());

-    closesocket(s1);
-    closesocket(s2);
+        closesocket(s1);
+        closesocket(s2);
+
+        winetest_pop_context();
+    }
 }

#define IP_PKTINFO_LEN (sizeof(WSACMSGHDR) + WSA_CMSG_ALIGN(sizeof(struct in_pktinfo)))

[PATCH 2/6] ws2_32/tests: Add tests for reusing address without SO_REUSEADDR.

[Paul Gofman]

From: Paul Gofman

---
 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 62ace3018be..05ad45c6cdb 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -2066,12 +2066,68 @@ static void test_so_reuseaddr(void)
     /* The connection is delivered to the first socket. */
     size = sizeof(saddr);
     s4 = accept(s1, (struct sockaddr*)&saddr, &size);
+    saddr.sin_port = htons(SERVERPORT + 1);
     ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

+
     closesocket(s1);
     closesocket(s2);
     closesocket(s3);
     closesocket(s4);
+
+    /* Test binding and listening on INADDR_ANY together with 127.0.0.1. */
+    s1 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    saddr.sin_addr.s_addr = htonl(INADDR_ANY);
+    rc = bind(s1, (struct sockaddr *)&saddr, sizeof(saddr));
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    rc = listen(s1, 1);
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    s2 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    saddr.sin_addr.s_addr = inet_addr("127.0.0.1");
+    rc = bind(s2, (struct sockaddr *)&saddr, sizeof(saddr));
+    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    rc = listen(s2, 1);
+    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    s3 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    rc = connect(s3, (struct sockaddr *)&saddr, sizeof(saddr));
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    size = sizeof(saddr);
+    s4 = accept(s2, (struct sockaddr *)&saddr, &size);
+    saddr.sin_port        = htons(SERVERPORT + 1);
+    todo_wine ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    closesocket(s1);
+    closesocket(s2);
+    closesocket(s3);
+    closesocket(s4);
+
+    /* Test binding to INADDR_ANY on two sockets. */
+    s1 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    saddr.sin_addr.s_addr = htonl(INADDR_ANY);
+    rc = bind(s1, (struct sockaddr *)&saddr, sizeof(saddr));
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    s2 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
+    ok(rc == SOCKET_ERROR && WSAGetLastError() == WSAEADDRINUSE, "got rc %d, error %d.\n", rc, WSAGetLastError());
+
+    closesocket(s1);
+    closesocket(s2);
 }

#define IP_PKTINFO_LEN (sizeof(WSACMSGHDR) + WSA_CMSG_ALIGN(sizeof(struct in_pktinfo)))

[PATCH 1/6] ws2_32/tests: Make test_so_reuseaddr() more conclusive.

[Paul Gofman]

From: Paul Gofman

---
 dlls/ws2_32/tests/sock.c | 80 +++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 47 deletions(-)

diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index d4f430a29d2..62ace3018be 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -2016,76 +2016,62 @@ static void test_set_getsockopt(void)
 static void test_so_reuseaddr(void)
 {
     struct sockaddr_in saddr;
-    SOCKET s1,s2;
-    unsigned int rc,reuse;
+    unsigned int rc, reuse;
+    SOCKET s1, s2, s3, s4;
     int size;
-    DWORD err;

saddr.sin_family      = AF_INET;
     saddr.sin_port        = htons(SERVERPORT+1);
     saddr.sin_addr.s_addr = inet_addr("127.0.0.1");

s1=socket(AF_INET, SOCK_STREAM, 0);
-    ok(s1!=INVALID_SOCKET, "socket() failed error: %d\n", WSAGetLastError());
+    ok(s1 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());
+
+    reuse = 1;
+    rc = setsockopt(s1, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
     rc = bind(s1, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(rc!=SOCKET_ERROR, "bind(s1) failed error: %d\n", WSAGetLastError());
+    ok(!rc, "got error %d.\n", WSAGetLastError());

-    s2=socket(AF_INET, SOCK_STREAM, 0);
-    ok(s2!=INVALID_SOCKET, "socket() failed error: %d\n", WSAGetLastError());
+    s2 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s2 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

reuse=0x1234;
     size=sizeof(reuse);
-    rc=getsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, &size );
-    ok(rc==0 && reuse==0,"wrong result in getsockopt(SO_REUSEADDR): rc=%d reuse=%d\n",rc,reuse);
+    rc=getsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, &size);
+    ok(!rc && !reuse,"got rc %d, reuse %d.\n", rc, reuse);

rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-    ok(rc==SOCKET_ERROR, "bind() succeeded\n");
+    ok(rc == SOCKET_ERROR, "got rc %d, error %d.\n", rc, WSAGetLastError());

reuse = 1;
     rc = setsockopt(s2, SOL_SOCKET, SO_REUSEADDR, (char*)&reuse, sizeof(reuse));
-    ok(rc==0, "setsockopt() failed error: %d\n", WSAGetLastError());
+    ok(!rc, "got error %d.\n", WSAGetLastError());

-    /* On Win2k3 and above, all SO_REUSEADDR seems to do is to allow binding to
-     * a port immediately after closing another socket on that port, so
-     * basically following the BSD socket semantics here. */
     rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-    if(rc==0)
-    {
-        int s3=socket(AF_INET, SOCK_STREAM, 0), s4;
-
-        /* If we could bind again in the same port this is Windows version <= XP.
-         * Lets test if we can really connect to one of them. */
-        set_blocking(s1, FALSE);
-        set_blocking(s2, FALSE);
-        rc = listen(s1, 1);
-        ok(!rc, "listen() failed with error: %d\n", WSAGetLastError());
-        rc = listen(s2, 1);
-        ok(!rc, "listen() failed with error: %d\n", WSAGetLastError());
-        rc = connect(s3, (struct sockaddr*)&saddr, sizeof(saddr));
-        ok(!rc, "connecting to accepting socket failed %d\n", WSAGetLastError());
-
-        /* the delivery of the connection is random so we need to try on both sockets */
-        size = sizeof(saddr);
-        s4 = accept(s1, (struct sockaddr*)&saddr, &size);
-        if(s4 == INVALID_SOCKET)
-            s4 = accept(s2, (struct sockaddr*)&saddr, &size);
-        ok(s4 != INVALID_SOCKET, "none of the listening sockets could get the connection\n");
+    ok(!rc, "got error %d.\n", WSAGetLastError());

-        closesocket(s1);
-        closesocket(s3);
-        closesocket(s4);
-    }
-    else
-    {
-        err = WSAGetLastError();
-        ok(err==WSAEACCES, "expected 10013, got %ld\n", err);
+    s3 = socket(AF_INET, SOCK_STREAM, 0);
+    ok(s3 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

-        closesocket(s1);
-        rc = bind(s2, (struct sockaddr*)&saddr, sizeof(saddr));
-        ok(rc==0, "bind() failed error: %d\n", WSAGetLastError());
-    }
+    /* Test if we can really connect to one of them. */
+    rc = listen(s1, 1);
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+    rc = listen(s2, 1);
+    todo_wine ok(!rc, "got error %d.\n", WSAGetLastError());
+    rc = connect(s3, (struct sockaddr*)&saddr, sizeof(saddr));
+    ok(!rc, "got error %d.\n", WSAGetLastError());
+
+    /* The connection is delivered to the first socket. */
+    size = sizeof(saddr);
+    s4 = accept(s1, (struct sockaddr*)&saddr, &size);
+    ok(s4 != INVALID_SOCKET, "got error %d.\n", WSAGetLastError());

+    closesocket(s1);
     closesocket(s2);
+    closesocket(s3);
+    closesocket(s4);
 }

#define IP_PKTINFO_LEN (sizeof(WSACMSGHDR) + WSA_CMSG_ALIGN(sizeof(struct in_pktinfo)))

[PATCH 5/6] server: Track SO_REUSEADDR value.

[Paul Gofman]

From: Paul Gofman

---
 server/sock.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/server/sock.c b/server/sock.c
index 87a536bb363..7c5f0185fd5 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -233,6 +233,7 @@ struct sock
     unsigned int        nonblocking : 1; /* is the socket nonblocking? */
     unsigned int        bound : 1;   /* is the socket bound? */
     unsigned int        reset : 1;   /* did we get a TCP reset? */
+    unsigned int        reuseaddr : 1; /* winsock SO_REUSEADDR option value */
 };

static void sock_dump( struct object *obj, int verbose );
@@ -1545,6 +1546,7 @@ static struct sock *create_socket(void)
     sock->nonblocking = 0;
     sock->bound = 0;
     sock->reset = 0;
+    sock->reuseaddr = 0;
     sock->rcvbuf = 0;
     sock->sndbuf = 0;
     sock->rcvtimeo = 0;
@@ -2720,14 +2722,8 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )

if (bind( unix_fd, &bind_addr.addr, unix_len ) < 0)
         {
-            if (errno == EADDRINUSE)
-            {
-                int reuse;
-                socklen_t len = sizeof(reuse);
-
-                if (!getsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, (char *)&reuse, &len ) && reuse)
-                    errno = EACCES;
-            }
+            if (errno == EADDRINUSE && sock->reuseaddr)
+                errno = EACCES;

set_error( sock_get_ntstatus( errno ) );
             return;
@@ -2925,6 +2921,8 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
 #endif
         if (ret)
             set_error( sock_get_ntstatus( errno ) );
+        else
+            sock->reuseaddr = !!reuse;
         return;
     }

@@ -3018,7 +3016,6 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
     case IOCTL_AFD_WINE_GET_SO_REUSEADDR:
     {
         int reuse;
-        socklen_t len = sizeof(reuse);

if (!get_reply_max_size())
         {
@@ -3026,10 +3023,8 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
             return;
         }

-        if (!getsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, &reuse, &len ))
-            set_reply_data( &reuse, min( sizeof(reuse), get_reply_max_size() ));
-        else
-            set_error( sock_get_ntstatus( errno ) );
+        reuse = sock->reuseaddr;
+        set_reply_data( &reuse, min( sizeof(reuse), get_reply_max_size() ));
         return;
     }

[PATCH 6/6] server: Set Unix SO_REUSEADDR on all the TCP sockets.

[Paul Gofman]

From: Paul Gofman

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50955
---
 server/sock.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 199 insertions(+), 17 deletions(-)

diff --git a/server/sock.c b/server/sock.c
index 7c5f0185fd5..716c3630e57 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -91,6 +91,7 @@
 #include "wsipx.h"
 #include "af_irda.h"
 #include "wine/afd.h"
+#include "wine/rbtree.h"

#include "process.h"
 #include "file.h"
@@ -114,6 +115,19 @@ union win_sockaddr
     SOCKADDR_IRDA irda;
 };

+union unix_sockaddr
+{
+    struct sockaddr addr;
+    struct sockaddr_in in;
+    struct sockaddr_in6 in6;
+#ifdef HAS_IPX
+    struct sockaddr_ipx ipx;
+#endif
+#ifdef HAS_IRDA
+    struct sockaddr_irda irda;
+#endif
+};
+
 static struct list poll_list = LIST_INIT( poll_list );

struct poll_req
@@ -169,6 +183,13 @@ enum connection_state
     SOCK_CONNECTIONLESS,
 };

+struct bound_addr
+{
+    struct rb_entry entry;
+    union unix_sockaddr addr;
+    int reuse_count;
+};
+
 #define MAX_ICMP_HISTORY_LENGTH 8

struct sock
@@ -224,6 +245,7 @@ struct sock
         unsigned short icmp_seq;
     }
     icmp_fixup_data[MAX_ICMP_HISTORY_LENGTH]; /* Sent ICMP packets history used to fixup reply id. */
+    struct bound_addr  *bound_addr[2]; /* Links to the entries in bound addresses tree. */
     unsigned int        icmp_fixup_data_len;  /* Sent ICMP packets history length. */
     unsigned int        rd_shutdown : 1; /* is the read end shut down? */
     unsigned int        wr_shutdown : 1; /* is the write end shut down? */
@@ -236,6 +258,151 @@ struct sock
     unsigned int        reuseaddr : 1; /* winsock SO_REUSEADDR option value */
 };

+static int is_tcp_socket( struct sock *sock )
+{
+    return sock->type == WS_SOCK_STREAM && (sock->family == WS_AF_INET || sock->family == WS_AF_INET6);
+}
+
+static int addr_compare( const void *key, const struct wine_rb_entry *entry )
+{
+    const struct bound_addr *bound_addr = RB_ENTRY_VALUE(entry, struct bound_addr, entry);
+    const union unix_sockaddr *addr = key;
+
+    if (addr->addr.sa_family != bound_addr->addr.addr.sa_family)
+        return addr->addr.sa_family < bound_addr->addr.addr.sa_family ? -1 : 1;
+
+    if (addr->addr.sa_family == AF_INET)
+    {
+        if (addr->in.sin_port != bound_addr->addr.in.sin_port)
+            return addr->in.sin_port < bound_addr->addr.in.sin_port ? -1 : 1;
+        if (addr->in.sin_addr.s_addr == bound_addr->addr.in.sin_addr.s_addr)
+            return 0;
+        return addr->in.sin_addr.s_addr < bound_addr->addr.in.sin_addr.s_addr ? -1 : 1;
+    }
+
+    assert( addr->addr.sa_family == AF_INET6 );
+    if (addr->in6.sin6_port != bound_addr->addr.in6.sin6_port)
+        return addr->in6.sin6_port < bound_addr->addr.in6.sin6_port ? -1 : 1;
+    return memcmp( &addr->in6.sin6_addr, &bound_addr->addr.in6.sin6_addr, sizeof(addr->in6.sin6_addr) );
+}
+
+static int ipv4addr_from_v6( union unix_sockaddr *v4addr, const struct sockaddr_in6 *in6 )
+{
+    v4addr->in.sin_family = AF_INET;
+    v4addr->in.sin_port = in6->sin6_port;
+    if (IN6_IS_ADDR_UNSPECIFIED(&in6->sin6_addr))
+    {
+        v4addr->in.sin_addr.s_addr = INADDR_ANY;
+        return 1;
+    }
+    if (IN6_IS_ADDR_LOOPBACK(&in6->sin6_addr))
+    {
+        v4addr->in.sin_addr.s_addr = INADDR_LOOPBACK;
+        return 1;
+    }
+    if (IN6_IS_ADDR_V4COMPAT(&in6->sin6_addr) || IN6_IS_ADDR_V4MAPPED(&in6->sin6_addr))
+    {
+        memcpy( &v4addr->in.sin_addr.s_addr, &in6->sin6_addr.s6_addr[12], sizeof(v4addr->in.sin_addr.s_addr) );
+        return 1;
+    }
+    return 0;
+}
+
+static struct rb_tree bound_addresses_tree = { addr_compare };
+
+static int is_blocking_addr( struct sock *sock, const union unix_sockaddr *addr )
+{
+    if (!is_tcp_socket( sock )) return 0;
+
+    if (sock->family == WS_AF_INET)
+    {
+        if (addr->addr.sa_family != AF_INET) return 0;
+        if (!addr->in.sin_port) return 0;
+        return 1;
+    }
+    if (addr->addr.sa_family != AF_INET6) return 0;
+    if (!addr->in6.sin6_port) return 0;
+    return 1;
+}
+
+static int check_addr_usage( struct sock *sock, const union unix_sockaddr *addr, int *v6only )
+{
+    struct bound_addr *bound_addr;
+    union unix_sockaddr v4addr;
+    struct rb_entry *entry;
+
+    *v6only = 1;
+
+#ifdef IPV6_V6ONLY
+    if (sock->family == WS_AF_INET6)
+    {
+        socklen_t len = sizeof(*v6only);
+
+        getsockopt( get_unix_fd(sock->fd), IPPROTO_IPV6, IPV6_V6ONLY, v6only, &len );
+    }
+#endif
+
+    if (!is_blocking_addr( sock, addr )) return 0;
+
+    if ((entry = rb_get( &bound_addresses_tree, addr )))
+    {
+        bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, entry);
+        if (bound_addr->reuse_count == -1 || !sock->reuseaddr) return 1;
+    }
+
+    if (sock->family != WS_AF_INET6 || *v6only) return 0;
+    if (!ipv4addr_from_v6( &v4addr, &addr->in6 )) return 0;
+    if ((entry = rb_get( &bound_addresses_tree, &v4addr )))
+    {
+        bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, entry);
+        if (bound_addr->reuse_count == -1 || !sock->reuseaddr) return 1;
+    }
+    return 0;
+}
+
+static struct bound_addr *register_bound_address( struct sock *sock, const union unix_sockaddr *addr )
+{
+    struct bound_addr *bound_addr;
+
+    if (!(bound_addr = malloc( sizeof(*bound_addr) )))
+        fatal_error( "out of memory\n" );
+
+    if (rb_put( &bound_addresses_tree, addr, &bound_addr->entry ))
+    {
+        free( bound_addr );
+        bound_addr = WINE_RB_ENTRY_VALUE(rb_get( &bound_addresses_tree, addr ), struct bound_addr, entry);
+        if (bound_addr->reuse_count == -1)
+        {
+            if (debug_level)
+                fprintf( stderr, "register_bound_address: address being updated is already exclusively bound\n" );
+            return NULL;
+        }
+        ++bound_addr->reuse_count;
+    }
+    else
+    {
+        bound_addr->addr = *addr;
+        bound_addr->reuse_count = sock->reuseaddr ? 1 : -1;
+    }
+    return bound_addr;
+}
+
+static void update_addr_usage( struct sock *sock, const union unix_sockaddr *addr, int v6only )
+{
+    union unix_sockaddr v4addr;
+
+    assert( !sock->bound_addr[0] && !sock->bound_addr[1] );
+
+    if (!is_blocking_addr( sock, addr )) return;
+
+    sock->bound_addr[0] = register_bound_address( sock, addr );
+
+    if (sock->family != WS_AF_INET6 || v6only) return;
+    if (!ipv4addr_from_v6( &v4addr, &addr->in6 )) return;
+
+    sock->bound_addr[1] = register_bound_address( sock, &v4addr );
+}
+
 static void sock_dump( struct object *obj, int verbose );
 static struct fd *sock_get_fd( struct object *obj );
 static int sock_close_handle( struct object *obj, struct process *process, obj_handle_t handle );
@@ -297,19 +464,6 @@ static const struct fd_ops sock_fd_ops =
     sock_reselect_async           /* reselect_async */
 };

-union unix_sockaddr
-{
-    struct sockaddr addr;
-    struct sockaddr_in in;
-    struct sockaddr_in6 in6;
-#ifdef HAS_IPX
-    struct sockaddr_ipx ipx;
-#endif
-#ifdef HAS_IRDA
-    struct sockaddr_irda irda;
-#endif
-};
-
 static int sockaddr_from_unix( const union unix_sockaddr *uaddr, struct WS_sockaddr *wsaddr, socklen_t wsaddrlen )
 {
     memset( wsaddr, 0, wsaddrlen );
@@ -1493,11 +1647,21 @@ static int sock_close_handle( struct object *obj, struct process *process, obj_h
 static void sock_destroy( struct object *obj )
 {
     struct sock *sock = (struct sock *)obj;
+    unsigned int i;

assert( obj->ops == &sock_ops );

/* FIXME: special socket shutdown stuff? */

+    for (i = 0; i < 2; ++i)
+    {
+        if (sock->bound_addr[i] && --sock->bound_addr[i]->reuse_count <= 0)
+        {
+            rb_remove( &bound_addresses_tree, &sock->bound_addr[i]->entry );
+            free( sock->bound_addr[i] );
+        }
+    }
+
     if ( sock->deferred )
         release_object( sock->deferred );

@@ -1552,6 +1716,7 @@ static struct sock *create_socket(void)
     sock->rcvtimeo = 0;
     sock->sndtimeo = 0;
     sock->icmp_fixup_data_len = 0;
+    sock->bound_addr[0] = sock->bound_addr[1] = NULL;
     init_async_queue( &sock->read_q );
     init_async_queue( &sock->write_q );
     init_async_queue( &sock->ifchange_q );
@@ -1740,6 +1905,13 @@ static int init_socket( struct sock *sock, int family, int type, int protocol )
     sock->type   = type;
     sock->family = family;

+    if (is_tcp_socket( sock ))
+    {
+        int reuse = 1;
+
+        setsockopt( sockfd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse) );
+    }
+
     if (sock->fd)
     {
         options = get_fd_options( sock->fd );
@@ -2671,6 +2843,7 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
         union unix_sockaddr unix_addr, bind_addr;
         data_size_t in_size;
         socklen_t unix_len;
+        int v6only;

/* the ioctl is METHOD_NEITHER, so ntdll gives us the output buffer as
          * input */
@@ -2720,6 +2893,12 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )

set_async_pending( async );

+        if (check_addr_usage( sock, &bind_addr, &v6only ))
+        {
+            set_error( sock->reuseaddr ? STATUS_ACCESS_DENIED : STATUS_SHARING_VIOLATION );
+            return;
+        }
+
         if (bind( unix_fd, &bind_addr.addr, unix_len ) < 0)
         {
             if (errno == EADDRINUSE && sock->reuseaddr)
@@ -2741,6 +2920,8 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
             sock->addr_len = sockaddr_from_unix( &bind_addr, &sock->addr.addr, sizeof(sock->addr) );
         }

+        update_addr_usage( sock, &bind_addr, v6only );
+
         if (get_reply_max_size() >= sock->addr_len)
             set_reply_data( &sock->addr, sock->addr_len );
         return;
@@ -2901,9 +3082,7 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
         return;
     }

-    /* BSD socket SO_REUSEADDR is not 100% compatible to winsock semantics;
-     * however, using it the BSD way fixes bug 8513 and seems to be what
-     * most programmers assume, anyway */
+    /* BSD socket SO_REUSEADDR is not compatible with winsock semantics. */
     case IOCTL_AFD_WINE_SET_SO_REUSEADDR:
     {
         int reuse, ret;
@@ -2915,7 +3094,10 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async )
         }

reuse = *(int *)get_req_data();
-        ret = setsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse) );
+        if (is_tcp_socket( sock ))
+            ret = 0;
+        else
+            ret = setsockopt( unix_fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse) );
 #ifdef __APPLE__
         if (!ret) ret = setsockopt( unix_fd, SOL_SOCKET, SO_REUSEPORT, &reuse, sizeof(reuse) );
 #endif

[PATCH 6/6] server: Set Unix SO_REUSEADDR on all the TCP sockets.

[Marvin]

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 full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=125167

Your paranoid android.

=== build (build log) ===

error: patch failed: dlls/ws2_32/tests/sock.c:2015
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: dlls/ws2_32/tests/sock.c:2015
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: dlls/ws2_32/tests/sock.c:2015
Task: Patch failed to apply

[PATCH 6/6] server: Set Unix SO_REUSEADDR on all the TCP sockets.

[Paul Gofman]

This is probably a Testbot error. The patch diff Testbot shows has some 
chunk which does not belong to my patch:

```
From: Giovanni Mascellani

Signed-off-by: Giovanni Mascellani 
---
  libs/vkd3d-shader/hlsl_sm4.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
index aa89b6a0..4118ce88 100644
--- a/libs/vkd3d-shader/hlsl_sm4.c
+++ b/libs/vkd3d-shader/hlsl_sm4.c
@@ -1637,6 +1637,11 @@ static void write_sm4_expr(struct hlsl_ctx *ctx,
              write_sm4_unary_op(buffer, VKD3D_SM4_OP_ROUND_NI, 
&expr->node, arg1, 0);
              break;

+        case HLSL_OP1_FRACT:
+            assert(type_is_float(dst_type));
+            write_sm4_unary_op(buffer, VKD3D_SM4_OP_FRC, &expr->node, 
arg1, 0);
+            break;
+
          case HLSL_OP1_LOG2:
              assert(type_is_float(dst_type));
              write_sm4_unary_op(buffer, VKD3D_SM4_OP_LOG, &expr->node, 
arg1, 0);
...

On 10/19/22 14:45, Marvin wrote:
> 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 full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=125167
>
> Your paranoid android.
>
> === build (build log) ===
>
> error: patch failed: dlls/ws2_32/tests/sock.c:2015
> Task: Patch failed to apply
>
> === debian11 (build log) ===
>
> error: patch failed: dlls/ws2_32/tests/sock.c:2015
> Task: Patch failed to apply
>
> === debian11 (build log) ===
>
> error: patch failed: dlls/ws2_32/tests/sock.c:2015
> Task: Patch failed to apply

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
> -
>
> +static struct rb_tree bound_addresses_tree = { addr_compare };
> -
>
> +static int is_blocking_addr( struct sock *sock, const union unix_sockaddr *addr )
> +{
> - if (!is_tcp_socket( sock )) return 0;
> -
> - if (sock->family == WS_AF_INET)
> - {
> -
>    if (addr->addr.sa_family != AF_INET) return 0;
> -
>    if (!addr->in.sin_port) return 0;
> -
>    return 1;
> - }
> - if (addr->addr.sa_family != AF_INET6) return 0;
> - if (!addr->in6.sin6_port) return 0;
> - return 1;

I think it'd be clearer if you had an "else if (sock->family == WS_AF_INET6)" branch, even though it's more lines of code.

Also, do we really need those sa_family checks? It skips the address lookup, but that doesn't seem necessary, and it's a bit distracting.

[Paul Gofman]

On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> Zebediah Figura (@zfigura) commented about server/sock.c:
> > -
> >
> > +static struct rb_tree bound_addresses_tree = { addr_compare };
> > -
> >
> > +static int is_blocking_addr( struct sock *sock, const union unix_sockaddr *addr )
> > +{
> > - if (!is_tcp_socket( sock )) return 0;
> > -
> > - if (sock->family == WS_AF_INET)
> > - {
> > -
> >    if (addr->addr.sa_family != AF_INET) return 0;
> > -
> >    if (!addr->in.sin_port) return 0;
> > -
> >    return 1;
> > - }
> > - if (addr->addr.sa_family != AF_INET6) return 0;
> > - if (!addr->in6.sin6_port) return 0;
> > - return 1;
>
> I think it'd be clearer if you had an "else if (sock->family == WS_AF_INET6)" branch, even though it's more lines of code.
>
> Also, do we really need those sa_family checks? It skips the address lookup, but that doesn't seem necessary, and it's a bit distracting.

The check is here because in theory an app may supply the wrong address 
type for socket. In this case, if I don't skip the lookup here and 
happen to find the actually supplied address already bound we will 
return the wrong error code (that should fail in native bind() with a 
different error). I don't think such specific and weird condition 
warrants any code complication, but it also looks weird to search for a 
non matching address type for socket, so maybe we could keep the check 
in place?

[Zebediah Figura]

On 10/25/22 20:18, Paul Gofman wrote:
> On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > -
> > >
> > > +static struct rb_tree bound_addresses_tree = { addr_compare };
> > > -
> > >
> > > +static int is_blocking_addr( struct sock *sock, const union 
> > > unix_sockaddr *addr )
> > > +{
> > > +    if (!is_tcp_socket( sock )) return 0;
> > > -
> > >
> > > +    if (sock->family == WS_AF_INET)
> > > +    {
> > > +        if (addr->addr.sa_family != AF_INET) return 0;
> > > +        if (!addr->in.sin_port) return 0;
> > > +        return 1;
> > > +    }
> > > +    if (addr->addr.sa_family != AF_INET6) return 0;
> > > +    if (!addr->in6.sin6_port) return 0;
> > > +    return 1;
> >
> > I think it'd be clearer if you had an "else if (sock->family == 
> > WS_AF_INET6)" branch, even though it's more lines of code.
> >
> > Also, do we really need those sa_family checks? It skips the address 
> > lookup, but that doesn't seem necessary, and it's a bit distracting.
>
> The check is here because in theory an app may supply the wrong address 
> type for socket. In this case, if I don't skip the lookup here and 
> happen to find the actually supplied address already bound we will 
> return the wrong error code (that should fail in native bind() with a 
> different error). I don't think such specific and weird condition 
> warrants any code complication, but it also looks weird to search for a 
> non matching address type for socket, so maybe we could keep the check 
> in place?

Oh, right, I can't think. I guess in that case the question is the 
opposite—can we just check sa_family here and *not* sock->family?

[Paul Gofman]

On 10/26/22 16:04, Zebediah Figura wrote:
> On 10/25/22 20:18, Paul Gofman wrote:
> > On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > > -
> > > >
> > > > +static struct rb_tree bound_addresses_tree = { addr_compare };
> > > > -
> > > >
> > > > +static int is_blocking_addr( struct sock *sock, const union 
> > > > unix_sockaddr *addr )
> > > > +{
> > > > +    if (!is_tcp_socket( sock )) return 0;
> > > > -
> > > >
> > > > +    if (sock->family == WS_AF_INET)
> > > > +    {
> > > > +        if (addr->addr.sa_family != AF_INET) return 0;
> > > > +        if (!addr->in.sin_port) return 0;
> > > > +        return 1;
> > > > +    }
> > > > +    if (addr->addr.sa_family != AF_INET6) return 0;
> > > > +    if (!addr->in6.sin6_port) return 0;
> > > > +    return 1;
> > >
> > > I think it'd be clearer if you had an "else if (sock->family == 
> > > WS_AF_INET6)" branch, even though it's more lines of code.
> > >
> > > Also, do we really need those sa_family checks? It skips the address 
> > > lookup, but that doesn't seem necessary, and it's a bit distracting.
> >
> > The check is here because in theory an app may supply the wrong 
> > address type for socket. In this case, if I don't skip the lookup 
> > here and happen to find the actually supplied address already bound 
> > we will return the wrong error code (that should fail in native 
> > bind() with a different error). I don't think such specific and weird 
> > condition warrants any code complication, but it also looks weird to 
> > search for a non matching address type for socket, so maybe we could 
> > keep the check in place?
>
> Oh, right, I can't think. I guess in that case the question is the 
> opposite—can we just check sa_family here and *not* sock->family?

If I am not missing anything, this is another aspect. Checking just 
sa_family would avoid weird access for ipv4 as ipv6 and vice versa from 
addr, but still family not matching between socket and addr will result 
in a bind error later and we ideally don't want to return wrong error 
code here if that doesn't cost too much. I redid the function in the 
patch update and now I believe it is much more readable, do you think it 
is still too messy?

[Paul Gofman]

On 10/26/22 16:08, Paul Gofman wrote:
> On 10/26/22 16:04, Zebediah Figura wrote:
> > On 10/25/22 20:18, Paul Gofman wrote:
> > > On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > > > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > > > -
> > > > >
> > > > > +static struct rb_tree bound_addresses_tree = { addr_compare };
> > > > > -
> > > > >
> > > > > +static int is_blocking_addr( struct sock *sock, const union 
> > > > > unix_sockaddr *addr )
> > > > > +{
> > > > > +    if (!is_tcp_socket( sock )) return 0;
> > > > > -
> > > > >
> > > > > +    if (sock->family == WS_AF_INET)
> > > > > +    {
> > > > > +        if (addr->addr.sa_family != AF_INET) return 0;
> > > > > +        if (!addr->in.sin_port) return 0;
> > > > > +        return 1;
> > > > > +    }
> > > > > +    if (addr->addr.sa_family != AF_INET6) return 0;
> > > > > +    if (!addr->in6.sin6_port) return 0;
> > > > > +    return 1;
> > > >
> > > > I think it'd be clearer if you had an "else if (sock->family == 
> > > > WS_AF_INET6)" branch, even though it's more lines of code.
> > > >
> > > > Also, do we really need those sa_family checks? It skips the 
> > > > address lookup, but that doesn't seem necessary, and it's a bit 
> > > > distracting.
> > >
> > > The check is here because in theory an app may supply the wrong 
> > > address type for socket. In this case, if I don't skip the lookup 
> > > here and happen to find the actually supplied address already bound 
> > > we will return the wrong error code (that should fail in native 
> > > bind() with a different error). I don't think such specific and 
> > > weird condition warrants any code complication, but it also looks 
> > > weird to search for a non matching address type for socket, so maybe 
> > > we could keep the check in place?
> >
> > Oh, right, I can't think. I guess in that case the question is the 
> > opposite—can we just check sa_family here and *not* sock->family?
>
> If I am not missing anything, this is another aspect. Checking just 
> sa_family would avoid weird access for ipv4 as ipv6 and vice versa 
> from addr, but still family not matching between socket and addr will 
> result in a bind error later and we ideally don't want to return wrong 
> error code here if that doesn't cost too much. I redid the function in 
> the patch update and now I believe it is much more readable, do you 
> think it is still too messy?

There might be a question is why I am not validating anything else in 
the address then, but the address added to the tree is always validated 
through native bind() / getsockname(), so if we check that family 
matches an otherwise invalid address just can't be found in the tree, so 
this family part is special.

if address type is correct and expected

[Zebediah Figura]

On 10/26/22 16:16, Paul Gofman wrote:
> On 10/26/22 16:08, Paul Gofman wrote:
> > On 10/26/22 16:04, Zebediah Figura wrote:
> > > On 10/25/22 20:18, Paul Gofman wrote:
> > > > On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > > > > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > > > > -
> > > > > >
> > > > > > +static struct rb_tree bound_addresses_tree = { addr_compare };
> > > > > > -
> > > > > >
> > > > > > +static int is_blocking_addr( struct sock *sock, const union 
> > > > > > unix_sockaddr *addr )
> > > > > > +{
> > > > > > +    if (!is_tcp_socket( sock )) return 0;
> > > > > > -
> > > > > >
> > > > > > +    if (sock->family == WS_AF_INET)
> > > > > > +    {
> > > > > > +        if (addr->addr.sa_family != AF_INET) return 0;
> > > > > > +        if (!addr->in.sin_port) return 0;
> > > > > > +        return 1;
> > > > > > +    }
> > > > > > +    if (addr->addr.sa_family != AF_INET6) return 0;
> > > > > > +    if (!addr->in6.sin6_port) return 0;
> > > > > > +    return 1;
> > > > >
> > > > > I think it'd be clearer if you had an "else if (sock->family == 
> > > > > WS_AF_INET6)" branch, even though it's more lines of code.
> > > > >
> > > > > Also, do we really need those sa_family checks? It skips the 
> > > > > address lookup, but that doesn't seem necessary, and it's a bit 
> > > > > distracting.
> > > >
> > > > The check is here because in theory an app may supply the wrong 
> > > > address type for socket. In this case, if I don't skip the lookup 
> > > > here and happen to find the actually supplied address already bound 
> > > > we will return the wrong error code (that should fail in native 
> > > > bind() with a different error). I don't think such specific and 
> > > > weird condition warrants any code complication, but it also looks 
> > > > weird to search for a non matching address type for socket, so maybe 
> > > > we could keep the check in place?
> > >
> > > Oh, right, I can't think. I guess in that case the question is the 
> > > opposite—can we just check sa_family here and *not* sock->family?
> >
> > If I am not missing anything, this is another aspect. Checking just 
> > sa_family would avoid weird access for ipv4 as ipv6 and vice versa 
> > from addr, but still family not matching between socket and addr will 
> > result in a bind error later and we ideally don't want to return wrong 
> > error code here if that doesn't cost too much. I redid the function in 
> > the patch update and now I believe it is much more readable, do you 
> > think it is still too messy?
>
> There might be a question is why I am not validating anything else in 
> the address then, but the address added to the tree is always validated 
> through native bind() / getsockname(), so if we check that family 
> matches an otherwise invalid address just can't be found in the tree, so 
> this family part is special.
>
> if address type is correct and expected

Less a matter of readability than just being confusing, i.e. "why is 
this here"? I think in this case it's probably a small enough deal that 
it's not worth worrying about, though.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
> - if (IN6_IS_ADDR_LOOPBACK(&in6->sin6_addr))
> - {
> -
>    v4addr->in.sin_addr.s_addr = INADDR_LOOPBACK;
> -
>    return 1;
> - }
> - if (IN6_IS_ADDR_V4COMPAT(&in6->sin6_addr) || IN6_IS_ADDR_V4MAPPED(&in6->sin6_addr))
> - {
> -
>    memcpy( &v4addr->in.sin_addr.s_addr, &in6->sin6_addr.s6_addr[12], sizeof(v4addr->in.sin_addr.s_addr) );
> -
>    return 1;
> - }
> - return 0;
>
> +}
> -
>
> +static struct rb_tree bound_addresses_tree = { addr_compare };
> -
>
> +static int is_blocking_addr( struct sock *sock, const union unix_sockaddr *addr )

Naming here is awkward, mostly because "blocking" usually means something else when we're talking about sockets. I don't have much better, though. Maybe "addr_can_conflict"?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
>      set_async_pending( async );
>
> -
>    if (check_addr_usage( sock, &bind_addr, &v6only ))

Does "v6only" need to be an out parameter? It seems like it'd make more sense to just calculate it in the caller and then pass it as an in parameter like with update_addr_usage(), but maybe I'm missing something.

[Paul Gofman]

On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> Zebediah Figura (@zfigura) commented about server/sock.c:
> >       set_async_pending( async );
> >
> > -
> >    if (check_addr_usage( sock, &bind_addr, &v6only ))
>
> Does "v6only" need to be an out parameter? It seems like it'd make more sense to just calculate it in the caller and then pass it as an in parameter like with update_addr_usage(), but maybe I'm missing something.

Yes, sure, I can just getsockopt() at the caller and pass it to both 
functions by value.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
> - struct bound_addr *bound_addr;
> -
> - if (!(bound_addr = malloc( sizeof(*bound_addr) )))
> -
>    fatal_error( "out of memory\n" );
> -
> - if (rb_put( &bound_addresses_tree, addr, &bound_addr->entry ))
> - {
> -
>    free( bound_addr );
> -
>    bound_addr = WINE_RB_ENTRY_VALUE(rb_get( &bound_addresses_tree, addr ), struct bound_addr, entry);
> -
>    if (bound_addr->reuse_count == -1)
> -
>    {
> -
>        if (debug_level)
> -
>            fprintf( stderr, "register_bound_address: address being updated is already exclusively bound\n" );
> -
>        return NULL;
> -
>    }
> -
>    ++bound_addr->reuse_count;

Under what circumstances can this happen? Should this be an assert() instead?

(I know that we don't want to bring down the server, but I think it's better to assert than go ahead with something that really shouldn't be the case. Alexandre might disagree with me, though.)

[Paul Gofman]

On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> Zebediah Figura (@zfigura) commented about server/sock.c:
> > - struct bound_addr *bound_addr;
> > -
> > - if (!(bound_addr = malloc( sizeof(*bound_addr) )))
> > -
> >    fatal_error( "out of memory\n" );
> > -
> > - if (rb_put( &bound_addresses_tree, addr, &bound_addr->entry ))
> > - {
> > -
> >    free( bound_addr );
> > -
> >    bound_addr = WINE_RB_ENTRY_VALUE(rb_get( &bound_addresses_tree, addr ), struct bound_addr, entry);
> > -
> >    if (bound_addr->reuse_count == -1)
> > -
> >    {
> > -
> >        if (debug_level)
> > -
> >            fprintf( stderr, "register_bound_address: address being updated is already exclusively bound\n" );
> > -
> >        return NULL;
> > -
> >    }
> > -
> >    ++bound_addr->reuse_count;
>
> Under what circumstances can this happen? Should this be an assert() instead?
>
> (I know that we don't want to bring down the server, but I think it's better to assert than go ahead with something that really shouldn't be the case. Alexandre might disagree with me, though.)

Well, I think that normally it should not happen. But note that we (have 
to) block the address we get after getting the socket name after bind. 
Unfortunately I can't say for sure if it ever can change the address 
from, e. g., ANY to some specific, maybe it can at some specific 
configuration, and I guess such case would be very hard to handle, while 
maybe not strictly necessary. So I'd really prefer to just let it 
proceed with bind in such case and fail later on listen.

[Zebediah Figura]

On 10/25/22 19:14, Paul Gofman wrote:
> On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > +    struct bound_addr *bound_addr;
> > > -
> > >
> > > +    if (!(bound_addr = malloc( sizeof(*bound_addr) )))
> > > +        fatal_error( "out of memory\n" );
> > > -
> > >
> > > +    if (rb_put( &bound_addresses_tree, addr, &bound_addr->entry ))
> > > +    {
> > > +        free( bound_addr );
> > > +        bound_addr = WINE_RB_ENTRY_VALUE(rb_get( 
> > > &bound_addresses_tree, addr ), struct bound_addr, entry);
> > > +        if (bound_addr->reuse_count == -1)
> > > +        {
> > > +            if (debug_level)
> > > +                fprintf( stderr, "register_bound_address: address 
> > > being updated is already exclusively bound\n" );
> > > +            return NULL;
> > > +        }
> > > +        ++bound_addr->reuse_count;
> >
> > Under what circumstances can this happen? Should this be an assert() 
> > instead?
> >
> > (I know that we don't want to bring down the server, but I think it's 
> > better to assert than go ahead with something that really shouldn't be 
> > the case. Alexandre might disagree with me, though.)
>
> Well, I think that normally it should not happen. But note that we (have 
> to) block the address we get after getting the socket name after bind. 
> Unfortunately I can't say for sure if it ever can change the address 
> from, e. g., ANY to some specific, maybe it can at some specific 
> configuration, and I guess such case would be very hard to handle, while 
> maybe not strictly necessary. So I'd really prefer to just let it 
> proceed with bind in such case and fail later on listen.

Okay, I think this makes sense, thanks.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
> - if (sock->family != WS_AF_INET6 || *v6only) return 0;
> - if (!ipv4addr_from_v6( &v4addr, &addr->in6 )) return 0;
> - if ((entry = rb_get( &bound_addresses_tree, &v4addr )))
> - {
> -
>    bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, entry);
> -
>    if (bound_addr->reuse_count == -1 || !sock->reuseaddr) return 1;
> - }
> - return 0;
>
> +}
> -
>
> +static struct bound_addr *register_bound_address( struct sock *sock, const union unix_sockaddr *addr )
> +{
> - struct bound_addr *bound_addr;
> -
> - if (!(bound_addr = malloc( sizeof(*bound_addr) )))
> -
>    fatal_error( "out of memory\n" );

mem_alloc(). This also doesn't seem like the place for a fatal_error?

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about server/sock.c:
> -
>
> +#ifdef IPV6_V6ONLY
> - if (sock->family == WS_AF_INET6)
> - {
> -
>    socklen_t len = sizeof(*v6only);
> -
> -
>    getsockopt( get_unix_fd(sock->fd), IPPROTO_IPV6, IPV6_V6ONLY, v6only, &len );
> - }
>
> +#endif
> -
> - if (!is_blocking_addr( sock, addr )) return 0;
> -
> - if ((entry = rb_get( &bound_addresses_tree, addr )))
> - {
> -
>    bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, entry);
> -
>    if (bound_addr->reuse_count == -1 || !sock->reuseaddr) return 1;

This implies that if we bind a socket without REUSEADDR, and then one with REUSEADDR, then we get STATUS_ACCESS_DENIED (i.e. WSAEACCES). That doesn't match what [1] says, and I don't think we have tests for it either. (Nor tests for binding the first socket with REUSEADDR and the second without, for that matter.)

[1] https://learn.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-a...

[Paul Gofman]

On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> Zebediah Figura (@zfigura) commented about server/sock.c:
> > -
> >
> > +#ifdef IPV6_V6ONLY
> > - if (sock->family == WS_AF_INET6)
> > - {
> > -
> >    socklen_t len = sizeof(*v6only);
> > -
> > -
> >    getsockopt( get_unix_fd(sock->fd), IPPROTO_IPV6, IPV6_V6ONLY, v6only, &len );
> > - }
> >
> > +#endif
> > -
> > - if (!is_blocking_addr( sock, addr )) return 0;
> > -
> > - if ((entry = rb_get( &bound_addresses_tree, addr )))
> > - {
> > -
> >    bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, entry);
> > -
> >    if (bound_addr->reuse_count == -1 || !sock->reuseaddr) return 1;
>
> This implies that if we bind a socket without REUSEADDR, and then one with REUSEADDR, then we get STATUS_ACCESS_DENIED (i.e. WSAEACCES). That doesn't match what [1] says, and I don't think we have tests for it either. (Nor tests for binding the first socket with REUSEADDR and the second without, for that matter.)

Eh, I guess that was exactly the case at least in the 
test_so_reuseaddr() before my changes. Should I probably bring this case 
back additionally?

[Zebediah Figura]

On 10/25/22 19:19, Paul Gofman wrote:
> On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > Zebediah Figura (@zfigura) commented about server/sock.c:
> > > -
> > >
> > > +#ifdef IPV6_V6ONLY
> > > +    if (sock->family == WS_AF_INET6)
> > > +    {
> > > +        socklen_t len = sizeof(*v6only);
> > > -
> > >
> > > +        getsockopt( get_unix_fd(sock->fd), IPPROTO_IPV6, 
> > > IPV6_V6ONLY, v6only, &len );
> > > +    }
> > > +#endif
> > > -
> > >
> > > +    if (!is_blocking_addr( sock, addr )) return 0;
> > > -
> > >
> > > +    if ((entry = rb_get( &bound_addresses_tree, addr )))
> > > +    {
> > > +        bound_addr = WINE_RB_ENTRY_VALUE(entry, struct bound_addr, 
> > > entry);
> > > +        if (bound_addr->reuse_count == -1 || !sock->reuseaddr) 
> > > return 1;
> >
> > This implies that if we bind a socket without REUSEADDR, and then one 
> > with REUSEADDR, then we get STATUS_ACCESS_DENIED (i.e. WSAEACCES). 
> > That doesn't match what [1] says, and I don't think we have tests for 
> > it either. (Nor tests for binding the first socket with REUSEADDR and 
> > the second without, for that matter.)
>
> Eh, I guess that was exactly the case at least in the 
> test_so_reuseaddr() before my changes. Should I probably bring this case 
> back additionally?

It'd be nice, yes. I see you've already added in v2 :-)

[Zebediah Figura (＠zfigura)]

Of course, there's a good reason for TIME_WAIT existing, but, well, Windows :-/

Should we add tests for the TIME_WAIT part as well?

[Paul Gofman]

On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> Of course, there's a good reason for TIME_WAIT existing, but, well, Windows :-/

Do you know which is the reason it exists on Linux on the listening port 
with the same timeout value (it is clear why it is on accepted socket / 
port)? I could not deduce that from specs. The only mention of that I 
know about is BSD _REUSEADDR manual page which suggests that some 
implied timeout might be there, and that the Linux kernel keeps listen 
port busy for the same time as accept port and not going to change that. 
I am not entirely sure that Windows is violating any specific TCP rule 
by relaxing the timeout on listening port.
> Should we add tests for the TIME_WAIT part as well?

I don't know how to do that 100% reliably for a non-flaky test, the way 
I use locally is creating child processes which connect to each other 
and killing them. But that results in different TCP connection states on 
ports and sometimes (rarely) they may get lucky and not hit the longest 
timeout. Should I maybe attach a local program which reproduces that?

[Paul Gofman]

FWIW, the atatched example program shows wait time of 0ms on Windows 10 
and ~60 sec on Wine / Linux (without the patches, with the patches it is 
0 as well).

On 10/25/22 19:27, Paul Gofman wrote:
> On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > Of course, there's a good reason for TIME_WAIT existing, but, well, 
> > Windows :-/
>
> Do you know which is the reason it exists on Linux on the listening 
> port with the same timeout value (it is clear why it is on accepted 
> socket / port)? I could not deduce that from specs. The only mention 
> of that I know about is BSD _REUSEADDR manual page which suggests that 
> some implied timeout might be there, and that the Linux kernel keeps 
> listen port busy for the same time as accept port and not going to 
> change that. I am not entirely sure that Windows is violating any 
> specific TCP rule by relaxing the timeout on listening port.
> > Should we add tests for the TIME_WAIT part as well?
>
> I don't know how to do that 100% reliably for a non-flaky test, the 
> way I use locally is creating child processes which connect to each 
> other and killing them. But that results in different TCP connection 
> states on ports and sometimes (rarely) they may get lucky and not hit 
> the longest timeout. Should I maybe attach a local program which 
> reproduces that?

[Zebediah Figura]

On 10/25/22 19:27, Paul Gofman wrote:
> On 10/25/22 19:04, Zebediah Figura (@zfigura) wrote:
> > Of course, there's a good reason for TIME_WAIT existing, but, well, 
> > Windows :-/
>
> Do you know which is the reason it exists on Linux on the listening port 
> with the same timeout value (it is clear why it is on accepted socket / 
> port)? I could not deduce that from specs. The only mention of that I 
> know about is BSD _REUSEADDR manual page which suggests that some 
> implied timeout might be there, and that the Linux kernel keeps listen 
> port busy for the same time as accept port and not going to change that. 
> I am not entirely sure that Windows is violating any specific TCP rule 
> by relaxing the timeout on listening port.

I don't really know at this point, no. I tried to figure that out but 
couldn't even make sense of the regular TCP specs or state diagram, and 
at this point I think it probably makes sense just to manually implement 
SO_REUSEADDR and TIME_WAIT semantics rather than trying to change every 
POSIX environment anyway.
> > Should we add tests for the TIME_WAIT part as well?
>
> I don't know how to do that 100% reliably for a non-flaky test, the way 
> I use locally is creating child processes which connect to each other 
> and killing them. But that results in different TCP connection states on 
> ports and sometimes (rarely) they may get lucky and not hit the longest 
> timeout. Should I maybe attach a local program which reproduces that?

Is it not sufficient to close sockets in-process?

I think in general there's value in tests that don't always fail without 
a fix, provided that they always succeed with it—i.e. they still can 
help prevent regressions—but in this case maybe it's not worth it if the 
test gets too ugly (or if the relevant case is too rarely hit).