[PATCH v27 0/1] MR3074: Move getpeername() implementation to wineserver
`getpeername()` is currently handled in ntdll. This merge request changes ntdll to forward `getpeername()` to wineserver. The implementation utilises new `peer_addr` and `peer_addr_len` fields in wineserver's `struct sock`. This fixes multiple `todo_wine`s in `ws2_32/tests` and allows for more accurate peer names to be provided in case the address the socket is bound to on the Unix side does not match what `bind()` was called with on the Windows side. *This merge request was originally intended to be included in !2786 (Add support for AF_UNIX sockets) but was split into its own merge request because this is not an insignificant change.* -- v27: server: Move getpeername() implementation from ntdll/unix. https://gitlab.winehq.org/wine/wine/-/merge_requests/3074
From: Ally Sommers <dropbear.sh(a)gmail.com> This brings getpeername() in line with getsockname(), which is also implemented in wineserver. It also allows getpeername() to return a possibly-more-accurate peer name, as in the case of AF_UNIX sockets. This commit also removes todo_wine from multiple now-successful tests. --- dlls/ntdll/unix/socket.c | 24 ++--------------- dlls/ws2_32/tests/sock.c | 18 ++++++------- server/sock.c | 58 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index e09bf34b07d..32eba1af088 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -1939,30 +1939,10 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc } case IOCTL_AFD_WINE_GETPEERNAME: - { - union unix_sockaddr unix_addr; - socklen_t unix_len = sizeof(unix_addr); - int len; - - if ((status = server_get_unix_fd( handle, 0, &fd, &needs_close, NULL, NULL ))) - return status; - - if (getpeername( fd, &unix_addr.addr, &unix_len ) < 0) - { - status = sock_errno_to_status( errno ); - break; - } + if (in_size) FIXME( "unexpected input size %u\n", in_size ); - len = sockaddr_from_unix( &unix_addr, out_buffer, out_size ); - if (out_size < len) - { - status = STATUS_BUFFER_TOO_SMALL; - break; - } - io->Information = len; - status = STATUS_SUCCESS; + status = STATUS_BAD_DEVICE_TYPE; break; - } case IOCTL_AFD_WINE_GET_SO_BROADCAST: return do_getsockopt( handle, io, SOL_SOCKET, SO_BROADCAST, out_buffer, out_size ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 31dff1a11c7..d5fcbc61b25 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -9217,16 +9217,16 @@ static void test_shutdown(void) addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); - todo_wine ok(!memcmp(&addr, &server_addr, sizeof(server_addr)), "address didn't match\n"); + ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!memcmp(&addr, &server_addr, sizeof(server_addr)), "address didn't match\n"); addrlen = sizeof(client_addr); ret = getsockname(client, (struct sockaddr *)&client_addr, &addrlen); ok(!ret, "got error %u\n", WSAGetLastError()); addrlen = sizeof(addr); ret = getpeername(server, (struct sockaddr *)&addr, &addrlen); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); - todo_wine ok(!memcmp(&addr, &client_addr, sizeof(addr)), "address didn't match\n"); + ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!memcmp(&addr, &client_addr, sizeof(addr)), "address didn't match\n"); WSASetLastError(0xdeadbeef); ret = connect(client, (struct sockaddr *)&server_addr, sizeof(server_addr)); @@ -9285,16 +9285,16 @@ static void test_shutdown(void) addrlen = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &addrlen); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); - todo_wine ok(!memcmp(&addr, &server_addr, sizeof(server_addr)), "address didn't match\n"); + ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!memcmp(&addr, &server_addr, sizeof(server_addr)), "address didn't match\n"); addrlen = sizeof(client_addr); ret = getsockname(client, (struct sockaddr *)&client_addr, &addrlen); ok(!ret, "got error %u\n", WSAGetLastError()); addrlen = sizeof(addr); ret = getpeername(server, (struct sockaddr *)&addr, &addrlen); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); - todo_wine ok(!memcmp(&addr, &client_addr, sizeof(addr)), "address didn't match\n"); + ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!memcmp(&addr, &client_addr, sizeof(addr)), "address didn't match\n"); closesocket(client); closesocket(server); @@ -12517,7 +12517,7 @@ static void test_connecting_socket(void) len = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &len); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!ret, "got error %u\n", WSAGetLastError()); if (!ret) { ok(addr.sin_family == AF_INET, "got family %u\n", addr.sin_family); diff --git a/server/sock.c b/server/sock.c index ac55a5448f7..defb1d0be1e 100644 --- a/server/sock.c +++ b/server/sock.c @@ -242,6 +242,8 @@ struct sock struct poll_req *main_poll; /* main poll */ union win_sockaddr addr; /* socket name */ int addr_len; /* socket name length */ + union win_sockaddr peer_addr; /* peer name */ + int peer_addr_len; /* peer name length */ unsigned int rcvbuf; /* advisory recv buffer size */ unsigned int sndbuf; /* advisory send buffer size */ unsigned int rcvtimeo; /* receive timeout in ms */ @@ -1725,6 +1727,8 @@ static struct sock *create_socket(void) sock->main_poll = NULL; memset( &sock->addr, 0, sizeof(sock->addr) ); sock->addr_len = 0; + memset( &sock->peer_addr, 0, sizeof(sock->peer_addr) ); + sock->peer_addr_len = 0; sock->rd_shutdown = 0; sock->wr_shutdown = 0; sock->wr_shutdown_pending = 0; @@ -2034,8 +2038,15 @@ static struct sock *accept_socket( struct sock *sock ) } unix_len = sizeof(unix_addr); if (!getsockname( acceptfd, &unix_addr.addr, &unix_len )) + { acceptsock->addr_len = sockaddr_from_unix( &unix_addr, &acceptsock->addr.addr, sizeof(acceptsock->addr) ); + if (!getpeername( acceptfd, &unix_addr.addr, &unix_len )) + acceptsock->peer_addr_len = sockaddr_from_unix( &unix_addr, + &acceptsock->peer_addr.addr, + sizeof(acceptsock->peer_addr) ); + } } + clear_error(); sock->pending_events &= ~AFD_POLL_ACCEPT; sock->reported_events &= ~AFD_POLL_ACCEPT; @@ -2090,7 +2101,13 @@ static int accept_into_socket( struct sock *sock, struct sock *acceptsock ) unix_len = sizeof(unix_addr); if (!getsockname( get_unix_fd( newfd ), &unix_addr.addr, &unix_len )) + { acceptsock->addr_len = sockaddr_from_unix( &unix_addr, &acceptsock->addr.addr, sizeof(acceptsock->addr) ); + if (!getpeername( get_unix_fd( newfd ), &unix_addr.addr, &unix_len )) + acceptsock->peer_addr_len = sockaddr_from_unix( &unix_addr, + &acceptsock->peer_addr.addr, + sizeof(acceptsock->peer_addr) ); + } clear_error(); sock->pending_events &= ~AFD_POLL_ACCEPT; @@ -2559,7 +2576,7 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) { const struct afd_connect_params *params = get_req_data(); const struct WS_sockaddr *addr; - union unix_sockaddr unix_addr; + union unix_sockaddr unix_addr, peer_addr; struct connect_req *req; socklen_t unix_len; int send_len, ret; @@ -2621,6 +2638,7 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) if (unix_addr.addr.sa_family == AF_INET && !memcmp( &unix_addr.in.sin_addr, magic_loopback_addr, 4 )) unix_addr.in.sin_addr.s_addr = htonl( INADDR_LOOPBACK ); + memcpy( &peer_addr, &unix_addr, sizeof(unix_addr) ); ret = connect( unix_fd, &unix_addr.addr, unix_len ); if (ret < 0 && errno == ECONNABORTED) { @@ -2644,7 +2662,10 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) unix_len = sizeof(unix_addr); if (!getsockname( unix_fd, &unix_addr.addr, &unix_len )) + { sock->addr_len = sockaddr_from_unix( &unix_addr, &sock->addr.addr, sizeof(sock->addr) ); + sock->peer_addr_len = sockaddr_from_unix( &peer_addr, &sock->peer_addr.addr, sizeof(sock->peer_addr)); + } sock->bound = 1; if (!ret) @@ -2979,6 +3000,41 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) set_reply_data( &sock->addr, sock->addr_len ); return; + case IOCTL_AFD_WINE_GETPEERNAME: + if (sock->state != SOCK_CONNECTED && + sock->state != SOCK_CONNECTING && + sock->state != SOCK_CONNECTIONLESS) + { + set_error( STATUS_INVALID_CONNECTION ); + return; + } + + /* If ConnectEx() hasn't finished connecting (or failing to connect) the provided + * socket, getpeername() can't be called on it. This seems to be undocumented + * and is *not* the case for connect(), but we do test for it in ws2_32. + * connect_req is non-NULL iff ConnectEx() was used and has not finished, + * so we can use it as a check for ConnectEx() usage here. */ + if (sock->connect_req) + { + set_error( STATUS_INVALID_CONNECTION ); + return; + } + + if (!sock->peer_addr_len && sock->type == WS_SOCK_DGRAM) + { + set_error( STATUS_INVALID_CONNECTION ); + return; + } + + if (get_reply_max_size() < sock->peer_addr_len) + { + set_error( STATUS_BUFFER_TOO_SMALL ); + return; + } + + set_reply_data( &sock->peer_addr, sock->peer_addr_len); + return; + case IOCTL_AFD_WINE_DEFER: { const obj_handle_t *handle = get_req_data(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3074
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=137571 Your paranoid android. === w1064_2qxl (64 bit report) === ws2_32: sock.c:12909: Test failed: wait timed out sock.c:12773: Test failed: got size 5 sock.c:12774: Test failed: got "datad" sock.c:12773: Test failed: got size 3 sock.c:12774: Test failed: got "ata" === w1064_adm (64 bit report) === ws2_32: sock.c:12909: Test failed: wait timed out sock.c:12773: Test failed: got size 5 sock.c:12774: Test failed: got "datad" sock.c:12773: Test failed: got size 3 sock.c:12774: Test failed: got "ata"
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
len = sizeof(addr); ret = getpeername(client, (struct sockaddr *)&addr, &len); - todo_wine ok(!ret, "got error %u\n", WSAGetLastError()); + ok(!ret, "got error %u\n", WSAGetLastError()); if (!ret) {
We can get rid of the "if (!ret)" block now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3074#note_45780
Zebediah Figura (@zfigura) commented about server/sock.c:
unix_len = sizeof(unix_addr); if (!getsockname( unix_fd, &unix_addr.addr, &unix_len )) + { sock->addr_len = sockaddr_from_unix( &unix_addr, &sock->addr.addr, sizeof(sock->addr) ); + sock->peer_addr_len = sockaddr_from_unix( &peer_addr, &sock->peer_addr.addr, sizeof(sock->peer_addr)); + }
This being guarded out by getsockname() succeeding looks wrong. This is broadly true of the other calls to getpeername(), but this one especially looks wrong. It's a bit of a moot point since we don't expect getsockname() or getpeername() to fail, but it does look confusing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3074#note_45782
Zebediah Figura (@zfigura) commented about server/sock.c:
unix_len = sizeof(unix_addr); if (!getsockname( get_unix_fd( newfd ), &unix_addr.addr, &unix_len )) + { acceptsock->addr_len = sockaddr_from_unix( &unix_addr, &acceptsock->addr.addr, sizeof(acceptsock->addr) ); + if (!getpeername( get_unix_fd( newfd ), &unix_addr.addr, &unix_len )) + acceptsock->peer_addr_len = sockaddr_from_unix( &unix_addr, + &acceptsock->peer_addr.addr, + sizeof(acceptsock->peer_addr) );
The indentation here seems wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3074#note_45781
Sorry for the long delay. As I mentioned I was hoping to see the AF_UNIX stuff upstream first, but that's clearly stalled, so I've gone ahead and reviewed this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3074#note_45783
participants (4)
-
Ally Sommers -
Ally Sommers (＠dropbear)
-
Marvin -
Zebediah Figura (＠zfigura)