Make ConnectionFileDescription work with all sockets

[aadsm]

Summary

My main goal here is to make lldb-server work with Android Studio.

This is currently not the case because lldb-server is started in platform mode listening on a domain socket. When Android Studio connects to it lldb-server crashes because even though it's listening on a domain socket as soon as it gets a connection it asserts that it's a TCP connection, which will obviously fails for any non-tcp connection.

To do this I came up with a new method called GetConnectURI() in Socket that returns the URI needed to connect to the connected portion of the socket.

[aadsm]

I'm not really sure which source version lldb-server is being shipped with Android Studio today (since their version works just fine obviously)...

[aadsm]

Avoid parsing the URL when starting the GDB server if it's not a TCP connection.

[labath]

The patch looks fine, modulo some inline nits. I am somewhat doubtful about the name of the new function though. When I first saw GetConnectURI, i thought this would be the URI used to connect to this socket, but in reality it is the URI of the other end (right?). In that case, I think GetRemoteURI would be better. Or maybe, borrowing from the BSD socket vocabulary, GetPeerURI ? WDYT?

In D62089#1507283, @aadsm wrote:

I'm not really sure which source version lldb-server is being shipped with Android Studio today (since their version works just fine obviously)...

I'm not sure either, but I'd expect it's a pretty old one (>1yr) at this point. I guess this behavior regressed somehow since then..

lldb/source/Host/posix/DomainSocket.cpp

147–151 ↗

(On Diff #200126)

You could just write something like llvm::formatv("{0}://{1}", GetNameOffset() == 0 ? ... , GetSocketName())

lldb/unittests/Host/SocketTest.cpp

165–166 ↗

(On Diff #200126)

The same issue as in the previous patch. You'll need to store the GetConnectURI result in a local variable.

167 ↗

(On Diff #200126)

EXPECT_EQ("connect", scheme); is enough.

[aadsm]

Glad you brought the naming up, I was also not happy with it. I guess I kind of convinced myself in the end with "it's the url the socket used to create its connection". The other name I tinkered with was GetConnectRemoteURI, I wanted to have Connect in the name because of the naming scheme that it's being using, it seems something specifically to be used with ConnectionFileDescriptor::Connect (it's not clear to me why though). Actually now that I re read that function maybe GetRemoteConnectionURI would be better?

lldb/unittests/Host/SocketTest.cpp

165–166 ↗

(On Diff #200126)

Ah! thanks!

[labath]

GetRemoteConnectionURI sounds fine.

lldb/source/Host/posix/DomainSocket.cpp

130 ↗

(On Diff #200126)

This should also be GetRemoteSocketName or something similar (though the word remote looks a bit weird when it comes to domain sockets).

[aadsm]

lldb/source/Host/posix/DomainSocket.cpp

130 ↗

(On Diff #200126)

uhm.. I'm a bit skeptical on this one 😄
As far as I understand domain sockets don't really have endpoints, there's just a single point of consumption (the inode). Here's what I see in my head when comparing TCP to Domain sockets:

TCP:

consumer o---------o provider

Domain:

consumer -----o----- provider

While on TCP we have 2 things that we can name "remote" and "local". On Domain there's only one thing to name, so I think it would be odd calling it "remote" given there's no parallel with the tcp model.

I don't have strong feelings on this though, and I'm happy to change to land the patch.

[aadsm]

• Change GetConnectURI to GetRemoteConnectionURI
• Fixes tests when it comes to using dangling pointers on ConstRefs
• Better usage of the EXPECT_* macros.

[labath]

It looks like diff now contains the changes from D61833 as well. It would be better to upload just the changes relative to the previous patch in the series, otherwise things get confusing about what is being changed where. It's kind of obvious here, but you can use the "parent revision" field in phabricator to indicate that one diff depends on another.

Other than that, I *think* this is fine.

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp.rej

1–23 ↗

(On Diff #200403)

Delete this.

lldb/source/Host/posix/DomainSocket.cpp

130 ↗

(On Diff #200126)

Oops, sorry, you're totally right. I just noticed this name in drive-by and didn't stop to think about it. Please ignore this comment.

lldb/unittests/Host/SocketTest.cpp.rej

1–16 ↗

(On Diff #200403)

Delete this.

[aadsm]

Fix the mess I created by including parts of the previous diff

[aadsm]

Add missing bits from previous revision

[labath]

lldb/unittests/Host/SocketTest.cpp

172–178 ↗

(On Diff #200597)

You have a bunch of unused variables here.

[aadsm]

Remove unused variables

lldb/trunk/include/lldb/Host/Socket.h

public:
 
   bool IsValid() const override { return m_socket != kInvalidSocketValue; }
   WaitableHandle GetWaitableHandle() override;
 
   static bool DecodeHostAndPort(llvm::StringRef host_and_port,
                                 std::string &host_str, std::string &port_str,
                                 int32_t &port, Status *error_ptr);
 
+  // If this Socket is connected then return the URI used to connect.
+  virtual std::string GetRemoteConnectionURI() const { return ""; };
+
 protected:
   Socket(SocketProtocol protocol, bool should_close,
          bool m_child_process_inherit);
 
   virtual size_t Send(const void *buf, const size_t num_bytes);
 
   static void SetLastError(Status &error);
   static NativeSocket CreateSocket(const int domain, const int type,

lldb/trunk/include/lldb/Host/common/TCPSocket.h

public:
   Status Connect(llvm::StringRef name) override;
   Status Listen(llvm::StringRef name, int backlog) override;
   Status Accept(Socket *&conn_socket) override;
 
   Status CreateSocket(int domain);
 
   bool IsValid() const override;
 
+  std::string GetRemoteConnectionURI() const override;
+
 private:
   TCPSocket(NativeSocket socket, const TCPSocket &listen_socket);
 
   void CloseListenSockets();
 
   std::map<int, SocketAddress> m_listen_sockets;
 };
 }
 
 #endif // ifndef liblldb_TCPSocket_h_

lldb/trunk/include/lldb/Host/common/UDPSocket.h

 namespace lldb_private {
 class UDPSocket : public Socket {
 public:
   UDPSocket(bool should_close, bool child_processes_inherit);
 
   static Status Connect(llvm::StringRef name, bool child_processes_inherit,
                         Socket *&socket);
 
+  std::string GetRemoteConnectionURI() const override;
+
 private:
   UDPSocket(NativeSocket socket);
 
   size_t Send(const void *buf, const size_t num_bytes) override;
   Status Connect(llvm::StringRef name) override;
   Status Listen(llvm::StringRef name, int backlog) override;
   Status Accept(Socket *&socket) override;
 
   SocketAddress m_sockaddr;
 };
 }
 
 #endif // ifndef liblldb_UDPSocket_h_

lldb/trunk/include/lldb/Host/posix/DomainSocket.h

 class DomainSocket : public Socket {
 public:
   DomainSocket(bool should_close, bool child_processes_inherit);
 
   Status Connect(llvm::StringRef name) override;
   Status Listen(llvm::StringRef name, int backlog) override;
   Status Accept(Socket *&socket) override;
 
+  std::string GetRemoteConnectionURI() const override;
+
 protected:
   DomainSocket(SocketProtocol protocol, bool child_processes_inherit);
 
   virtual size_t GetNameOffset() const;
   virtual void DeleteSocketFile(llvm::StringRef name);
+  std::string GetSocketName() const;
 
 private:
   DomainSocket(NativeSocket socket, const DomainSocket &listen_socket);
 };
 }
 
 #endif // ifndef liblldb_DomainSocket_h_

lldb/trunk/source/Host/common/TCPSocket.cpp

if (m_socket != kInvalidSocketValue) {
     SocketAddress sock_addr;
     socklen_t sock_addr_len = sock_addr.GetMaxLength();
     if (::getpeername(m_socket, sock_addr, &sock_addr_len) == 0)
       return sock_addr.GetIPAddress();
   }
   return "";
 }
 
+std::string TCPSocket::GetRemoteConnectionURI() const {
+  if (m_socket != kInvalidSocketValue) {
+    return llvm::formatv("connect://[{0}]:{1}", GetRemoteIPAddress(),
+                         GetRemotePortNumber());
+  }
+  return "";
+};
+
 Status TCPSocket::CreateSocket(int domain) {
   Status error;
   if (IsValid())
     error = Close();
   if (error.Fail())
     return error;
   m_socket = Socket::CreateSocket(domain, kType, IPPROTO_TCP,
                                   m_child_processes_inherit, error);

lldb/trunk/source/Host/common/UDPSocket.cpp

#endif
   struct sockaddr_in source_info;
   socklen_t address_len = sizeof (struct sockaddr_in);
   err = ::getsockname(final_socket->GetNativeSocket(), (struct sockaddr *) &source_info, &address_len);
 
   socket = final_socket.release();
   error.Clear();
   return error;
 }
+
+std::string UDPSocket::GetRemoteConnectionURI() const {
+  if (m_socket != kInvalidSocketValue) {
+    return llvm::formatv("udp://[{0}]:{1}", m_sockaddr.GetIPAddress(),
+                         m_sockaddr.GetPort());
+  }
+  return "";
+}

lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp

 }
 
 void ConnectionFileDescriptor::SetChildProcessesInherit(
     bool child_processes_inherit) {
   m_child_processes_inherit = child_processes_inherit;
 }
 
 void ConnectionFileDescriptor::InitializeSocket(Socket *socket) {
-  assert(socket->GetSocketProtocol() == Socket::ProtocolTcp);
-  TCPSocket *tcp_socket = static_cast<TCPSocket *>(socket);
-
   m_write_sp.reset(socket);
   m_read_sp = m_write_sp;
-  StreamString strm;
-  strm.Printf("connect://[%s]:%u", tcp_socket->GetRemoteIPAddress().c_str(),
-              tcp_socket->GetRemotePortNumber());
-  m_uri = strm.GetString();
+  m_uri = socket->GetRemoteConnectionURI();
 }

lldb/trunk/source/Host/posix/DomainSocket.cpp

Status DomainSocket::Accept(Socket *&socket) {
   return error;
 }
 
 size_t DomainSocket::GetNameOffset() const { return 0; }
 
 void DomainSocket::DeleteSocketFile(llvm::StringRef name) {
   llvm::sys::fs::remove(name);
 }
+
+std::string DomainSocket::GetSocketName() const {
+  if (m_socket != kInvalidSocketValue) {
+    struct sockaddr_un saddr_un;
+    saddr_un.sun_family = AF_UNIX;
+    socklen_t sock_addr_len = sizeof(struct sockaddr_un);
+    if (::getpeername(m_socket, (struct sockaddr *)&saddr_un, &sock_addr_len) ==
+        0)
+      return std::string(saddr_un.sun_path + GetNameOffset(),
+                         sock_addr_len -
+                             offsetof(struct sockaddr_un, sun_path) -
+                             GetNameOffset());
+  }
+  return "";
+}
+
+std::string DomainSocket::GetRemoteConnectionURI() const {
+  if (m_socket != kInvalidSocketValue) {
+    return llvm::formatv("{0}://{1}",
+                         GetNameOffset() == 0 ? "unix-connect"
+                                              : "unix-abstract-connect",
+                         GetSocketName());
+  }
+  return "";
+}

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer(
   // Do not run in a new session so that it can not linger after the platform
   // closes.
   debugserver_launch_info.SetLaunchInSeparateProcessGroup(false);
   debugserver_launch_info.SetMonitorProcessCallback(
       std::bind(&GDBRemoteCommunicationServerPlatform::DebugserverProcessReaped,
                 this, std::placeholders::_1),
       false);
 
+  std::ostringstream url;
+// debugserver does not accept the URL scheme prefix.
+#if !defined(__APPLE__)
+  url << m_socket_scheme << "://";
+#endif
+  uint16_t *port_ptr = &port;
+  if (m_socket_protocol == Socket::ProtocolTcp) {
     llvm::StringRef platform_scheme;
     llvm::StringRef platform_ip;
     int platform_port;
     llvm::StringRef platform_path;
     std::string platform_uri = GetConnection()->GetURI();
     bool ok = UriParser::Parse(platform_uri, platform_scheme, platform_ip,
                                platform_port, platform_path);
     UNUSED_IF_ASSERT_DISABLED(ok);
     assert(ok);
-
-  std::ostringstream url;
-// debugserver does not accept the URL scheme prefix.
-#if !defined(__APPLE__)
-  url << m_socket_scheme << "://";
-#endif
-  uint16_t *port_ptr = &port;
-  if (m_socket_protocol == Socket::ProtocolTcp)
     url << platform_ip.str() << ":" << port;
-  else {
+  } else {
     socket_name = GetDomainSocketPath("gdbserver").GetPath();
     url << socket_name;
     port_ptr = nullptr;
   }
 
   Status error = StartDebugserverProcess(
       url.str().c_str(), nullptr, debugserver_launch_info, port_ptr, &args, -1);
 

lldb/trunk/unittests/Host/SocketTest.cpp

 //===-- SocketTest.cpp ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SocketTestUtilities.h"
+#include "lldb/Utility/UriParser.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 
 class SocketTest : public testing::Test {
 public:
   void SetUp() override {
     ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
TEST_F(SocketTest, TCPListen0GetPort) {
   Predicate<uint16_t> port_predicate;
   port_predicate.SetValue(0, eBroadcastNever);
   Status err =
       Socket::TcpListen("10.10.12.3:0", false, server_socket, &port_predicate);
   std::unique_ptr<TCPSocket> socket_up((TCPSocket*)server_socket);
   EXPECT_TRUE(socket_up->IsValid());
   EXPECT_NE(socket_up->GetLocalPortNumber(), 0);
 }
+
+TEST_F(SocketTest, TCPGetConnectURI) {
+  std::unique_ptr<TCPSocket> socket_a_up;
+  std::unique_ptr<TCPSocket> socket_b_up;
+  if (!IsAddressFamilySupported("127.0.0.1")) {
+    GTEST_LOG_(WARNING) << "Skipping test due to missing IPv4 support.";
+    return;
+  }
+  CreateTCPConnectedSockets("127.0.0.1", &socket_a_up, &socket_b_up);
+
+  llvm::StringRef scheme;
+  llvm::StringRef hostname;
+  int port;
+  llvm::StringRef path;
+  std::string uri(socket_a_up->GetRemoteConnectionURI());
+  EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
+  EXPECT_EQ(scheme, "connect");
+  EXPECT_EQ(port, socket_a_up->GetRemotePortNumber());
+}
+
+TEST_F(SocketTest, UDPGetConnectURI) {
+  if (!IsAddressFamilySupported("127.0.0.1")) {
+    GTEST_LOG_(WARNING) << "Skipping test due to missing IPv4 support.";
+    return;
+  }
+  Socket *socket;
+  bool child_processes_inherit = false;
+  auto error =
+      UDPSocket::Connect("127.0.0.1:0", child_processes_inherit, socket);
+
+  llvm::StringRef scheme;
+  llvm::StringRef hostname;
+  int port;
+  llvm::StringRef path;
+  std::string uri(socket->GetRemoteConnectionURI());
+  EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
+  EXPECT_EQ(scheme, "udp");
+}
+
+#ifndef LLDB_DISABLE_POSIX
+TEST_F(SocketTest, DomainGetConnectURI) {
+  llvm::SmallString<64> domain_path;
+  std::error_code EC =
+      llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", domain_path);
+  ASSERT_FALSE(EC);
+  llvm::sys::path::append(domain_path, "test");
+
+  std::unique_ptr<DomainSocket> socket_a_up;
+  std::unique_ptr<DomainSocket> socket_b_up;
+  CreateDomainConnectedSockets(domain_path, &socket_a_up, &socket_b_up);
+
+  llvm::StringRef scheme;
+  llvm::StringRef hostname;
+  int port;
+  llvm::StringRef path;
+  std::string uri(socket_a_up->GetRemoteConnectionURI());
+  EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
+  EXPECT_EQ(scheme, "unix-connect");
+  EXPECT_EQ(path, domain_path);
+}
+#endif
 No newline at end of file