[PATCH 0/6] MR1111: winsock: Always use Unix SO_REUSEADDR on TCP sockets.
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 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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()) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
From: Paul Gofman <pgofman(a)codeweavers.com> 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 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
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
This is probably a Testbot error. The patch diff Testbot shows has some chunk which does not belong to my patch: ``` From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- 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
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
From: Paul Gofman <pgofman(a)codeweavers.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 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))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1111
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12139
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?
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?
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?
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
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) 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"?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12138
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.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12141
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.
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) 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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12142
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12140
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) 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... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12143
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?
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 :-)
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1111#note_12144
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?
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?
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).
participants (6)
-
Marvin -
Paul Gofman -
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura -
Zebediah Figura (＠zfigura)