Fix IPv6 support on lldb-server platform

[aadsm]

Summary

This is a general fix for the ConnectionFileDescriptor class but my main motivation was to make lldb-server working with IPv6.
The connect URI can use square brackets ([]) to wrap the interface part of the URI (e.g.: <scheme>://[<interface>]:<port>). For IPv6 addresses this is a must since its ip can include colons and it will overlap with the port colon otherwise. The URIParser class parses the square brackets correctly but the ConnectionFileDescriptor doesn't generate them for IPv6 addresses making it impossible to connect to the gdb server when using this protocol.

How to reproduce the issue:

$ lldb-server p --server --listen [::1]:8080
...
$ lldb
(lldb) platform select remote-macosx
(lldb) platform connect connect://[::1]:8080
(lldb) platform process -p <pid>
error: unable to launch a GDB server on 'computer'

The server was actually launched we were just not able to connect to it. With this fix lldb will correctly connect. I fixed this by wrapping the ip portion with [].

[labath]

The actual change looks obviously correct. Below are my comments on the test situation.

I wanted to reuse the CreateConnectedSockets part of the SocketTests. Initially I thought about creating a few functions in a test socket utility file, but then I realized the tests need to initialize the Socket plugin for these functions to work. In the end I created a new class (SocketBasedTest) so I can ensure this plugin is correctly initialized and teared down. However, I'm not a fan of subclassing as a means of code shared (as I prefer composition or delegation). What are your thoughts on this?

I don't think that the necessity of the Socket::Initialize call should prevent you from creating free functions. Everything else in lldb dealing with sockets already assumes that Socket::Initialize has been called, so I don't see why this couldn't be an implicit prerequisite of these functions (or you can even document it explicitly if you want). Though I generally also prefer free functions, I'm fairly ambivalent in this case, so I'll leave it up to you whether you want to leave this as a separate class, or convert it into a bunch of free functions. (If you do leave it as-is, I think you should at least remove the `setUp/tearDown overrides in the derived classes as they're pretty useless now - you're just overriding the methods with identical implementations.).

Another question I have (and this is more a n00b question as I don't have prior c++ experience), the CreateConnectedSockets functions that exist use unique pointers, and after reading the documentation about this I decided to use .release() so I could safely pass them to the ConnectionFileDescriptor in the test I created for it. Should I have changed the unique pointers to shared pointers instead?

Using release is the correct thing to do here as ConnectionFileDescriptor takes ownership of the passed-in object. What would be a nice improvement here would be to change the interface of ConnectionFileDescriptor constructor so that it takes a unique_ptr instead of a raw pointer. That way, you would just say ConnectionFileDescriptor(std::move(whatever_up)). That way the ownership part would be explicitly documented (now, I've had to look at the source to check the ownership semantics). If you want to do that please make a separate patch for that.

lldb/unittests/Host/ConnectionFileDescriptorTest.cpp

40 ↗

(On Diff #199173)

You can just do EXPECT_EQ(ip, hostname)

lldb/unittests/Host/SocketBasedTest.cpp

63–68 ↗

(On Diff #199173)

The fact that you felt the need to place // this does foo comments everywhere is a signal that this behavior is unintuitive. I think it would be better to split this part into a separate function, and name it something like IsAddressFamilySupported. Then each test can do something like:

if (!IsAddressFamilySupported(...)) {
  GTEST_LOG_(WARNING) << "Skipping test due to missing ??? support.";
  return;
}

[aadsm]

Use functions instead of subclassing

[labath]

Looks fine to me. Thank you for fixing this.

lldb/unittests/Host/SocketTestUtilities.cpp

16 ↗

(On Diff #199331)

LLVM prefers static functions over ones in anonymous namespaces.

[aadsm]

Move function from anonymous namespace to const

[xiaobai]

Landed -- r361079

[xiaobai]

This broke windows buildbots. I tried to fix with svn r361083 but there were other things that got broken. @aadsm: We should revisit this patch next week and see what we need to fix up before it goes in.

[labath]

unittests/Host/ConnectionFileDescriptorTest.cpp

42

Judging by Davide's email, this looks like the cause of your problem. GetURI() returns a temporary string, which means that the StringRefs UriParser parses it into become dangling after this statement.

[aadsm]

• Fixed function from const to static (mea culpa)
• Fixed inet_pton usage on windows
• Fixed issue with GetURL return creating a temp object

[labath]

lldb/unittests/Host/CMakeLists.txt

12–13 ↗

(On Diff #200353)

Please sort this list before committing.

[aadsm]

A-Z order the CMakeLists files

source/Host/posix/ConnectionFileDescriptorPosix.cpp

 
 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(),
+  strm.Printf("connect://[%s]:%u", tcp_socket->GetRemoteIPAddress().c_str(),
               tcp_socket->GetRemotePortNumber());
   m_uri = strm.GetString();
 }

unittests/Host/CMakeLists.txt

 set (FILES
+  ConnectionFileDescriptorTest.cpp
   FileActionTest.cpp
   FileSystemTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
   NativeProcessProtocolTest.cpp
   ProcessLaunchInfoTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
+  SocketTestUtilities.cpp
   TaskPoolTest.cpp
 )
 
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND FILES
     linux/HostTest.cpp
     linux/SupportTest.cpp
   )

unittests/Host/ConnectionFileDescriptorTest.cpp

+//===-- ConnectionFileDescriptorTest.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 "gtest/gtest.h"
+
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+#include "lldb/Utility/UriParser.h"
+
+using namespace lldb_private;
+
+class ConnectionFileDescriptorTest : public testing::Test {
+public:
+  void SetUp() override {
+    ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
+  }
+
+  void TearDown() override { Socket::Terminate(); }
+
+  void TestGetURI(std::string ip) {
+    std::unique_ptr<TCPSocket> socket_a_up;
+    std::unique_ptr<TCPSocket> socket_b_up;
+    if (!IsAddressFamilySupported(ip)) {
+      GTEST_LOG_(WARNING) << "Skipping test due to missing IPv"
+                          << (IsIPv4(ip) ? "4" : "6") << " support.";
+      return;
+    }
+    CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
+    auto socket = socket_a_up.release();
+    ConnectionFileDescriptor connection_file_descriptor(socket);
+
+    llvm::StringRef scheme;
+    llvm::StringRef hostname;
+    int port;
+    llvm::StringRef path;
+    std::string uri(connection_file_descriptor.GetURI());
+    EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
+    EXPECT_EQ(ip, hostname);
+    EXPECT_EQ(socket->GetRemotePortNumber(), port);
+  }
+};
+
+TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) { TestGetURI("127.0.0.1"); }
+
+TEST_F(ConnectionFileDescriptorTest, TCPGetURIv6) { TestGetURI("::1"); }
 No newline at end of file

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 <cstdio>
-#include <functional>
-#include <thread>
-
+#include "SocketTestUtilities.h"
 #include "gtest/gtest.h"
 
-#include "lldb/Host/Config.h"
-#include "lldb/Host/Socket.h"
-#include "lldb/Host/common/TCPSocket.h"
-#include "lldb/Host/common/UDPSocket.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Testing/Support/Error.h"
-
-#ifndef LLDB_DISABLE_POSIX
-#include "lldb/Host/posix/DomainSocket.h"
-#endif
-
 using namespace lldb_private;
 
 class SocketTest : public testing::Test {
 public:
   void SetUp() override {
     ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
   }
 
   void TearDown() override { Socket::Terminate(); }
-
-protected:
-  static void AcceptThread(Socket *listen_socket,
-                           bool child_processes_inherit, Socket **accept_socket,
-                           Status *error) {
-    *error = listen_socket->Accept(*accept_socket);
-  }
-
-  template <typename SocketType>
-  void CreateConnectedSockets(
-      llvm::StringRef listen_remote_address,
-      const std::function<std::string(const SocketType &)> &get_connect_addr,
-      std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up) {
-    bool child_processes_inherit = false;
-    Status error;
-    std::unique_ptr<SocketType> listen_socket_up(
-        new SocketType(true, child_processes_inherit));
-    EXPECT_FALSE(error.Fail());
-    error = listen_socket_up->Listen(listen_remote_address, 5);
-    EXPECT_FALSE(error.Fail());
-    EXPECT_TRUE(listen_socket_up->IsValid());
-
-    Status accept_error;
-    Socket *accept_socket;
-    std::thread accept_thread(AcceptThread, listen_socket_up.get(),
-                              child_processes_inherit, &accept_socket,
-                              &accept_error);
-
-    std::string connect_remote_address = get_connect_addr(*listen_socket_up);
-    std::unique_ptr<SocketType> connect_socket_up(
-        new SocketType(true, child_processes_inherit));
-    EXPECT_FALSE(error.Fail());
-    error = connect_socket_up->Connect(connect_remote_address);
-    EXPECT_FALSE(error.Fail());
-    EXPECT_TRUE(connect_socket_up->IsValid());
-
-    a_up->swap(connect_socket_up);
-    EXPECT_TRUE(error.Success());
-    EXPECT_NE(nullptr, a_up->get());
-    EXPECT_TRUE((*a_up)->IsValid());
-
-    accept_thread.join();
-    b_up->reset(static_cast<SocketType *>(accept_socket));
-    EXPECT_TRUE(accept_error.Success());
-    EXPECT_NE(nullptr, b_up->get());
-    EXPECT_TRUE((*b_up)->IsValid());
-
-    listen_socket_up.reset();
-  }
 };
 
 TEST_F(SocketTest, DecodeHostAndPort) {
   std::string host_str;
   std::string port_str;
   int32_t port;
   Status error;
   EXPECT_TRUE(Socket::DecodeHostAndPort("localhost:1138", host_str, port_str,

 TEST_F(SocketTest, DomainListenConnectAccept) {
   llvm::SmallString<64> Path;
   std::error_code EC = llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(Path, "test");
 
   std::unique_ptr<DomainSocket> socket_a_up;
   std::unique_ptr<DomainSocket> socket_b_up;
-  CreateConnectedSockets<DomainSocket>(
-      Path, [=](const DomainSocket &) { return Path.str().str(); },
-      &socket_a_up, &socket_b_up);
+  CreateDomainConnectedSockets(Path, &socket_a_up, &socket_b_up);
 }
 #endif
 
 TEST_F(SocketTest, TCPListen0ConnectAccept) {
   std::unique_ptr<TCPSocket> socket_a_up;
   std::unique_ptr<TCPSocket> socket_b_up;
-  CreateConnectedSockets<TCPSocket>(
-      "127.0.0.1:0",
-      [=](const TCPSocket &s) {
-        char connect_remote_address[64];
-        snprintf(connect_remote_address, sizeof(connect_remote_address),
-                 "127.0.0.1:%u", s.GetLocalPortNumber());
-        return std::string(connect_remote_address);
-      },
-      &socket_a_up, &socket_b_up);
+  CreateTCPConnectedSockets("127.0.0.1", &socket_a_up, &socket_b_up);
 }
 
 TEST_F(SocketTest, TCPGetAddress) {
   std::unique_ptr<TCPSocket> socket_a_up;
   std::unique_ptr<TCPSocket> socket_b_up;
-  CreateConnectedSockets<TCPSocket>(
-      "127.0.0.1:0",
-      [=](const TCPSocket &s) {
-        char connect_remote_address[64];
-        snprintf(connect_remote_address, sizeof(connect_remote_address),
-                 "127.0.0.1:%u", s.GetLocalPortNumber());
-        return std::string(connect_remote_address);
-      },
-      &socket_a_up, &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);
 
   EXPECT_EQ(socket_a_up->GetLocalPortNumber(),
             socket_b_up->GetRemotePortNumber());
   EXPECT_EQ(socket_b_up->GetLocalPortNumber(),
             socket_a_up->GetRemotePortNumber());
   EXPECT_NE(socket_a_up->GetLocalPortNumber(),
             socket_b_up->GetLocalPortNumber());
   EXPECT_STREQ("127.0.0.1", socket_a_up->GetRemoteIPAddress().c_str());

unittests/Host/SocketTestUtilities.h

+//===--------------------- SocketTestUtilities.h ----------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UNITTESTS_HOST_SOCKETTESTUTILITIES_H
+#define LLDB_UNITTESTS_HOST_SOCKETTESTUTILITIES_H
+
+#include <cstdio>
+#include <functional>
+#include <thread>
+
+#include "lldb/Host/Config.h"
+#include "lldb/Host/Socket.h"
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/common/UDPSocket.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
+
+#ifndef LLDB_DISABLE_POSIX
+#include "lldb/Host/posix/DomainSocket.h"
+#endif
+
+namespace lldb_private {
+template <typename SocketType>
+void CreateConnectedSockets(
+    llvm::StringRef listen_remote_address,
+    const std::function<std::string(const SocketType &)> &get_connect_addr,
+    std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up);
+bool CreateTCPConnectedSockets(std::string listen_remote_ip,
+                               std::unique_ptr<TCPSocket> *a_up,
+                               std::unique_ptr<TCPSocket> *b_up);
+#ifndef LLDB_DISABLE_POSIX
+void CreateDomainConnectedSockets(llvm::StringRef path,
+                                  std::unique_ptr<DomainSocket> *a_up,
+                                  std::unique_ptr<DomainSocket> *b_up);
+#endif
+
+bool IsAddressFamilySupported(std::string ip);
+bool IsIPv4(std::string ip);
+} // namespace lldb_private
+
+#endif
 No newline at end of file

unittests/Host/SocketTestUtilities.cpp

+//===----------------- SocketTestUtilities.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/StreamString.h"
+
+#ifdef _WIN32
+#include <winsock2.h>
+#include <ws2tcpip.h>
+#else
+#include <arpa/inet.h>
+#endif
+
+using namespace lldb_private;
+
+static void AcceptThread(Socket *listen_socket, bool child_processes_inherit,
+                         Socket **accept_socket, Status *error) {
+  *error = listen_socket->Accept(*accept_socket);
+}
+
+template <typename SocketType>
+void lldb_private::CreateConnectedSockets(
+    llvm::StringRef listen_remote_address,
+    const std::function<std::string(const SocketType &)> &get_connect_addr,
+    std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up) {
+  bool child_processes_inherit = false;
+  Status error;
+  std::unique_ptr<SocketType> listen_socket_up(
+      new SocketType(true, child_processes_inherit));
+  EXPECT_FALSE(error.Fail());
+  error = listen_socket_up->Listen(listen_remote_address, 5);
+  EXPECT_FALSE(error.Fail());
+  EXPECT_TRUE(listen_socket_up->IsValid());
+
+  Status accept_error;
+  Socket *accept_socket;
+  std::thread accept_thread(AcceptThread, listen_socket_up.get(),
+                            child_processes_inherit, &accept_socket,
+                            &accept_error);
+
+  std::string connect_remote_address = get_connect_addr(*listen_socket_up);
+  std::unique_ptr<SocketType> connect_socket_up(
+      new SocketType(true, child_processes_inherit));
+  EXPECT_FALSE(error.Fail());
+  error = connect_socket_up->Connect(connect_remote_address);
+  EXPECT_FALSE(error.Fail());
+  EXPECT_TRUE(connect_socket_up->IsValid());
+
+  a_up->swap(connect_socket_up);
+  EXPECT_TRUE(error.Success());
+  EXPECT_NE(nullptr, a_up->get());
+  EXPECT_TRUE((*a_up)->IsValid());
+
+  accept_thread.join();
+  b_up->reset(static_cast<SocketType *>(accept_socket));
+  EXPECT_TRUE(accept_error.Success());
+  EXPECT_NE(nullptr, b_up->get());
+  EXPECT_TRUE((*b_up)->IsValid());
+
+  listen_socket_up.reset();
+}
+
+bool lldb_private::CreateTCPConnectedSockets(
+    std::string listen_remote_ip, std::unique_ptr<TCPSocket> *socket_a_up,
+    std::unique_ptr<TCPSocket> *socket_b_up) {
+  StreamString strm;
+  strm.Printf("[%s]:0", listen_remote_ip.c_str());
+  CreateConnectedSockets<TCPSocket>(
+      strm.GetString(),
+      [=](const TCPSocket &s) {
+        char connect_remote_address[64];
+        snprintf(connect_remote_address, sizeof(connect_remote_address),
+                 "[%s]:%u", listen_remote_ip.c_str(), s.GetLocalPortNumber());
+        return std::string(connect_remote_address);
+      },
+      socket_a_up, socket_b_up);
+  return true;
+}
+
+#ifndef LLDB_DISABLE_POSIX
+void lldb_private::CreateDomainConnectedSockets(
+    llvm::StringRef path, std::unique_ptr<DomainSocket> *socket_a_up,
+    std::unique_ptr<DomainSocket> *socket_b_up) {
+  return CreateConnectedSockets<DomainSocket>(
+      path, [=](const DomainSocket &) { return path.str(); }, socket_a_up,
+      socket_b_up);
+}
+#endif
+
+bool lldb_private::IsAddressFamilySupported(std::string ip) {
+  auto addresses = lldb_private::SocketAddress::GetAddressInfo(
+      ip.c_str(), NULL, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
+  return addresses.size() > 0;
+}
+
+bool lldb_private::IsIPv4(std::string ip) {
+  struct sockaddr_in sock_addr;
+  return inet_pton(AF_INET, ip.c_str(), &(sock_addr.sin_addr)) != 0;
+}