From a4b25027fe91e11fb9e0cef82811bc900b9c4cec Mon Sep 17 00:00:00 2001 From: Mihail Slobodyanuk Date: Wed, 7 Jun 2023 19:32:49 +0000 Subject: [PATCH] Fix Socket remoteAddress obtaining on Win32 #51778 Closes https://github.com/dart-lang/sdk/pull/51981 GitOrigin-RevId: de22efdd25a716b6c7eac086e2cb12e8793554f5 Change-Id: If1d8247f7e0188c47c19e19f33732a56493d21d8 Bug: https://github.com/dart-lang/sdk/issues/51778 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/294220 Reviewed-by: Alexander Aprelev Commit-Queue: Brian Quinlan Reviewed-by: Brian Quinlan --- runtime/bin/eventhandler_win.cc | 66 ++++++++++++++++++++++----- runtime/bin/eventhandler_win.h | 19 ++++++-- runtime/bin/socket_base_win.cc | 11 +++-- tests/standalone/standalone_vm.status | 1 - 4 files changed, 79 insertions(+), 18 deletions(-) diff --git a/runtime/bin/eventhandler_win.cc b/runtime/bin/eventhandler_win.cc index 0fe8b7ef272..07314cd9ce5 100644 --- a/runtime/bin/eventhandler_win.cc +++ b/runtime/bin/eventhandler_win.cc @@ -13,6 +13,7 @@ #include // NOLINT #include // NOLINT #include // NOLINT +#include #include "bin/builtin.h" #include "bin/dartutils.h" @@ -33,6 +34,14 @@ namespace bin { static constexpr int kBufferSize = 64 * 1024; static constexpr int kStdOverlappedBufferSize = 16 * 1024; static constexpr int kMaxUDPPackageLength = 64 * 1024; +// For AcceptEx there needs to be buffer storage for address +// information for two addresses (local and remote address). The +// AcceptEx documentation says: "This value must be at least 16 +// bytes more than the maximum address length for the transport +// protocol in use." +static constexpr int kAcceptExAddressAdditionalBytes = 16; +static constexpr int kAcceptExAddressStorageSize = + sizeof(SOCKADDR_STORAGE) + kAcceptExAddressAdditionalBytes; OverlappedBuffer* OverlappedBuffer::AllocateBuffer(int buffer_size, Operation operation) { @@ -455,17 +464,21 @@ bool ListenSocket::LoadAcceptEx() { return (status != SOCKET_ERROR); } +bool ListenSocket::LoadGetAcceptExSockaddrs() { + // Load the GetAcceptExSockaddrs function into memory using WSAIoctl. + GUID guid_get_accept_ex_sockaddrs = WSAID_GETACCEPTEXSOCKADDRS; + DWORD bytes; + int status = + WSAIoctl(socket(), SIO_GET_EXTENSION_FUNCTION_POINTER, + &guid_get_accept_ex_sockaddrs, + sizeof(guid_get_accept_ex_sockaddrs), &GetAcceptExSockaddrs_, + sizeof(GetAcceptExSockaddrs_), &bytes, nullptr, nullptr); + return (status != SOCKET_ERROR); +} + bool ListenSocket::IssueAccept() { MonitorLocker ml(&monitor_); - // For AcceptEx there needs to be buffer storage for address - // information for two addresses (local and remote address). The - // AcceptEx documentation says: "This value must be at least 16 - // bytes more than the maximum address length for the transport - // protocol in use." - const int kAcceptExAddressAdditionalBytes = 16; - const int kAcceptExAddressStorageSize = - sizeof(SOCKADDR_STORAGE) + kAcceptExAddressAdditionalBytes; OverlappedBuffer* buffer = OverlappedBuffer::AllocateAcceptBuffer(2 * kAcceptExAddressStorageSize); DWORD received; @@ -498,8 +511,25 @@ void ListenSocket::AcceptComplete(OverlappedBuffer* buffer, int rc = setsockopt(buffer->client(), SOL_SOCKET, SO_UPDATE_ACCEPT_CONTEXT, reinterpret_cast(&s), sizeof(s)); if (rc == NO_ERROR) { + // getpeername() returns incorrect results when used with a socket that + // was accepted using overlapped I/O. AcceptEx includes the remote + // address in its result so retrieve it using GetAcceptExSockaddrs and + // save it. + LPSOCKADDR local_addr; + int local_addr_length; + LPSOCKADDR remote_addr; + int remote_addr_length; + GetAcceptExSockaddrs_( + buffer->GetBufferStart(), 0, kAcceptExAddressStorageSize, + kAcceptExAddressStorageSize, &local_addr, &local_addr_length, + &remote_addr, &remote_addr_length); + RawAddr* raw_remote_addr = new RawAddr; + memmove(raw_remote_addr, remote_addr, remote_addr_length); + // Insert the accepted socket into the list. - ClientSocket* client_socket = new ClientSocket(buffer->client()); + ClientSocket* client_socket = new ClientSocket( + buffer->client(), + std::move(std::unique_ptr(raw_remote_addr))); client_socket->mark_connected(); client_socket->CreateCompletionPort(completion_port); if (accepted_head_ == nullptr) { @@ -557,8 +587,9 @@ void ListenSocket::DoClose() { } // To finish resetting the state of the ListenSocket back to what it was // before EnsureInitialized was called, we have to reset the AcceptEx_ - // function pointer. + // and GetAcceptExSockaddrs_ function pointers. AcceptEx_ = nullptr; + GetAcceptExSockaddrs_ = nullptr; } bool ListenSocket::CanAccept() { @@ -601,7 +632,12 @@ void ListenSocket::EnsureInitialized( ASSERT(event_handler_ == nullptr); event_handler_ = event_handler; CreateCompletionPort(event_handler_->completion_port()); - LoadAcceptEx(); + bool isLoaded = LoadAcceptEx(); + ASSERT(isLoaded); + } + if (GetAcceptExSockaddrs_ == nullptr) { + bool isLoaded = LoadGetAcceptExSockaddrs(); + ASSERT(isLoaded); } } @@ -977,6 +1013,14 @@ bool ClientSocket::IsClosed() { return connected_ && closed_ && !HasPendingRead() && !HasPendingWrite(); } +bool ClientSocket::PopulateRemoteAddr(RawAddr& addr) { + if (!remote_addr_) { + return false; + } + addr = *remote_addr_; + return true; +} + bool DatagramSocket::IssueSendTo(struct sockaddr* sa, socklen_t sa_len) { MonitorLocker ml(&monitor_); ASSERT(completion_port_ != INVALID_HANDLE_VALUE); diff --git a/runtime/bin/eventhandler_win.h b/runtime/bin/eventhandler_win.h index 8466fb00e6b..f7242f944f1 100644 --- a/runtime/bin/eventhandler_win.h +++ b/runtime/bin/eventhandler_win.h @@ -12,9 +12,12 @@ #include #include #include +#include +#include #include "bin/builtin.h" #include "bin/reference_counting.h" +#include "bin/socket_base.h" #include "bin/thread.h" namespace dart { @@ -387,6 +390,7 @@ class ListenSocket : public DescriptorInfoMultipleMixin { explicit ListenSocket(intptr_t s) : DescriptorInfoMultipleMixin(s, true), AcceptEx_(nullptr), + GetAcceptExSockaddrs_(nullptr), pending_accept_count_(0), accepted_head_(nullptr), accepted_tail_(nullptr), @@ -418,8 +422,10 @@ class ListenSocket : public DescriptorInfoMultipleMixin { private: bool LoadAcceptEx(); + bool LoadGetAcceptExSockaddrs(); LPFN_ACCEPTEX AcceptEx_; + LPFN_GETACCEPTEXSOCKADDRS GetAcceptExSockaddrs_; // The number of asynchronous `IssueAccept` operations which haven't completed // yet. @@ -440,12 +446,14 @@ class ListenSocket : public DescriptorInfoMultipleMixin { // Information on connected sockets. class ClientSocket : public DescriptorInfoSingleMixin { public: - explicit ClientSocket(intptr_t s) + explicit ClientSocket(intptr_t s, + std::unique_ptr remote_addr = nullptr) : DescriptorInfoSingleMixin(s, true), DisconnectEx_(nullptr), next_(nullptr), connected_(false), - closed_(false) { + closed_(false), + remote_addr_(std::move(remote_addr)) { LoadDisconnectEx(); type_ = kClientSocket; } @@ -465,13 +473,17 @@ class ClientSocket : public DescriptorInfoSingleMixin { virtual bool IssueWrite(); void IssueDisconnect(); void DisconnectComplete(OverlappedBuffer* buffer); - void ConnectComplete(OverlappedBuffer* buffer); virtual void EnsureInitialized(EventHandlerImplementation* event_handler); virtual void DoClose(); virtual bool IsClosed(); + // If `ClientSocket` was constructed with a `remote_addr`, populate `addr` + // with that value and return `true`. Otherwise leave `addr` untouched and + // return `false`. + bool PopulateRemoteAddr(RawAddr& addr); + ClientSocket* next() { return next_; } void set_next(ClientSocket* next) { next_ = next; } @@ -491,6 +503,7 @@ class ClientSocket : public DescriptorInfoSingleMixin { ClientSocket* next_; bool connected_; bool closed_; + std::unique_ptr remote_addr_; #if defined(DEBUG) static intptr_t disconnecting_; diff --git a/runtime/bin/socket_base_win.cc b/runtime/bin/socket_base_win.cc index ca91f43c4f0..567cf58b09c 100644 --- a/runtime/bin/socket_base_win.cc +++ b/runtime/bin/socket_base_win.cc @@ -188,9 +188,14 @@ SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) { ASSERT(reinterpret_cast(fd)->is_socket()); SocketHandle* socket_handle = reinterpret_cast(fd); RawAddr raw; - socklen_t size = sizeof(raw); - if (getpeername(socket_handle->socket(), &raw.addr, &size)) { - return nullptr; + if (socket_handle->is_client_socket() && + reinterpret_cast(fd)->PopulateRemoteAddr(raw)) { + // `raw` was populated by `ClientSocket::PopulateRemoteAddr`. + } else { + socklen_t size = sizeof(raw); + if (getpeername(socket_handle->socket(), &raw.addr, &size)) { + return nullptr; + } } *port = SocketAddress::GetAddrPort(raw); // Clear the port before calling WSAAddressToString as WSAAddressToString diff --git a/tests/standalone/standalone_vm.status b/tests/standalone/standalone_vm.status index 9ec5702b142..c6513a82e5f 100644 --- a/tests/standalone/standalone_vm.status +++ b/tests/standalone/standalone_vm.status @@ -42,7 +42,6 @@ io/socket_upgrade_to_secure_test: Skip # Issue 27638 [ $system == windows ] io/process_sync_test: Pass, Timeout # Issue 24596 io/sleep_test: Pass, Fail # Issue 25757 -io/socket_info_ipv6_test: Skip verbose_gc_to_bmu_test: Skip [ $arch == arm && $mode == release && $runtime == dart_precompiled && $system == android ]