Fix connecting via abstract socket

[emrekultursay]

Commit 82ee31f and Commit 2e89312 added socket sharing, but only for unix domain sockets. That broke Android, which uses unix-abstract sockets.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Emre Kultursay (emrekultursay)

Changes

Commits 82ee31f and 2e89312 added socket sharing, but only for unix domain sockets. That broke Android, which uses unix-abstract sockets.

---

Full diff: https://github.com/llvm/llvm-project/pull/136466.diff

5 Files Affected:

• (modified) lldb/include/lldb/Host/linux/AbstractSocket.h (+1)
• (modified) lldb/include/lldb/Host/posix/DomainSocket.h (+1)
• (modified) lldb/source/Host/linux/AbstractSocket.cpp (+3)
• (modified) lldb/source/Host/posix/DomainSocket.cpp (+6)
• (modified) lldb/tools/lldb-server/lldb-platform.cpp (+36-4)

diff --git a/lldb/include/lldb/Host/linux/AbstractSocket.h b/lldb/include/lldb/Host/linux/AbstractSocket.h
index accfd01457a5e..c6a0e2f8af63b 100644
--- a/lldb/include/lldb/Host/linux/AbstractSocket.h
+++ b/lldb/include/lldb/Host/linux/AbstractSocket.h
@@ -15,6 +15,7 @@ namespace lldb_private {
 class AbstractSocket : public DomainSocket {
 public:
   AbstractSocket();
+  AbstractSocket(NativeSocket socket, bool should_close);
 
 protected:
   size_t GetNameOffset() const override;
diff --git a/lldb/include/lldb/Host/posix/DomainSocket.h b/lldb/include/lldb/Host/posix/DomainSocket.h
index 3dbe6206da2c5..562c011ee73e6 100644
--- a/lldb/include/lldb/Host/posix/DomainSocket.h
+++ b/lldb/include/lldb/Host/posix/DomainSocket.h
@@ -33,6 +33,7 @@ class DomainSocket : public Socket {
 
 protected:
   DomainSocket(SocketProtocol protocol);
+  DomainSocket(SocketProtocol protocol, NativeSocket socket, bool should_close);
 
   virtual size_t GetNameOffset() const;
   virtual void DeleteSocketFile(llvm::StringRef name);
diff --git a/lldb/source/Host/linux/AbstractSocket.cpp b/lldb/source/Host/linux/AbstractSocket.cpp
index 8393a80e86e72..681aa2d1ebc72 100644
--- a/lldb/source/Host/linux/AbstractSocket.cpp
+++ b/lldb/source/Host/linux/AbstractSocket.cpp
@@ -15,6 +15,9 @@ using namespace lldb_private;
 
 AbstractSocket::AbstractSocket() : DomainSocket(ProtocolUnixAbstract) {}
 
+AbstractSocket::AbstractSocket(NativeSocket socket, bool should_close)
+    : DomainSocket(ProtocolUnixAbstract, socket, should_close) {}
+
 size_t AbstractSocket::GetNameOffset() const { return 1; }
 
 void AbstractSocket::DeleteSocketFile(llvm::StringRef name) {}
diff --git a/lldb/source/Host/posix/DomainSocket.cpp b/lldb/source/Host/posix/DomainSocket.cpp
index 6c490cdda47ed..9b1e4f71bba2a 100644
--- a/lldb/source/Host/posix/DomainSocket.cpp
+++ b/lldb/source/Host/posix/DomainSocket.cpp
@@ -67,6 +67,12 @@ DomainSocket::DomainSocket(NativeSocket socket,
   m_socket = socket;
 }
 
+DomainSocket::DomainSocket(SocketProtocol protocol, NativeSocket socket,
+                           bool should_close)
+    : Socket(protocol, should_close) {
+  m_socket = socket;
+}
+
 Status DomainSocket::Connect(llvm::StringRef name) {
   sockaddr_un saddr_un;
   socklen_t saddr_un_len;
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 23d36ffb4cb66..1f12f0902b954 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -39,11 +39,19 @@
 #if LLDB_ENABLE_POSIX
 #include "lldb/Host/posix/DomainSocket.h"
 #endif
+#ifdef __linux__
+#include "lldb/Host/linux/AbstractSocket.h"
+#endif
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/UriParser.h"
 
+#if LLDB_ENABLE_POSIX
+#include <sys/socket.h>
+#include <sys/un.h>
+#endif
+
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::lldb_server;
@@ -455,14 +463,28 @@ int main_platform(int argc, char *argv[]) {
   lldb_private::Args inferior_arguments;
   inferior_arguments.SetArguments(argc, const_cast<const char **>(argv));
 
+  Log *log = GetLog(LLDBLog::Platform);
   Socket::SocketProtocol protocol = Socket::ProtocolUnixDomain;
 
   if (fd != SharedSocket::kInvalidFD) {
     // Child process will handle the connection and exit.
-    if (gdbserver_port)
+    if (gdbserver_port) {
       protocol = Socket::ProtocolTcp;
+    } else {
+#ifdef LLDB_ENABLE_POSIX
+      // Check if fd represents domain socket or abstract socket.
+      struct sockaddr_un addr;
+      socklen_t addr_len = sizeof(addr);
+      if (getsockname(fd, (struct sockaddr*)&addr, &addr_len) == -1) {
+        LLDB_LOGF(log, "lldb-platform child: not a socket or error occurred");
+        return socket_error;
+      }
 
-    Log *log = GetLog(LLDBLog::Platform);
+      if (addr.sun_family == AF_UNIX && addr.sun_path[0] == '\0') {
+          protocol = Socket::ProtocolUnixAbstract;
+      }
+#endif
+    }
 
     NativeSocket sockfd;
     error = SharedSocket::GetNativeSocket(fd, sockfd);
@@ -473,17 +495,27 @@ int main_platform(int argc, char *argv[]) {
 
     GDBRemoteCommunicationServerPlatform platform(protocol, gdbserver_port);
     Socket *socket;
-    if (protocol == Socket::ProtocolTcp)
+    if (protocol == Socket::ProtocolTcp) {
       socket = new TCPSocket(sockfd, /*should_close=*/true);
-    else {
+    } else if (protocol == Socket::ProtocolUnixDomain) {
 #if LLDB_ENABLE_POSIX
       socket = new DomainSocket(sockfd, /*should_close=*/true);
 #else
       WithColor::error() << "lldb-platform child: Unix domain sockets are not "
                             "supported on this platform.";
       return socket_error;
+#endif
+    } else {
+#ifdef __linux__
+      socket = new AbstractSocket(sockfd, /*should_close=*/true);
+#else
+      WithColor::error()
+          << "lldb-platform child: Abstract domain sockets are not "
+             "supported on this platform.";
+      return socket_error;
 #endif
     }
+
     platform.SetConnection(
         std::unique_ptr<Connection>(new ConnectionFileDescriptor(socket)));
     client_handle(platform, inferior_arguments);

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[emrekultursay]

@slydiman Could you also take a look at this PR, as you are the author of the 2 commits mentioned in the description? Thanks.

[slydiman]

I like it.
I would suggest to move domain/abstract socket checking to a static helper, something like

Socket* DomainSocket::Create(NativeSocket sockfd, bool should_close) {
#ifdef __linux__
  // Check if fd represents domain socket or abstract socket.
  if (/*abstract*/) 
    return new AbstractSocket(sockfd, should_close);
#endif
  return new DomainSocket(sockfd, should_close);
}

But please wait for @labath comments first.

[labath]

Agreed

[slydiman]

Don't forget to update the protocol passed to the GDBRemoteCommunicationServerPlatform constructor.

[labath]

We should also have a test for this. A unit test for the new function should be easy enough. I'm not going to insist on a test for the whole "lldb-server platform" flow, but I would recommend it, as that's the only way to ensure this does not break again.

[emrekultursay]

PTAL. I added unit tests for the method, but I'm open to suggestions on how to improve that test.

[emrekultursay]

Thanks. PTAL.

[slydiman]

Thanks

[emrekultursay]

Thanks folks:

1. Can you please squash-and-merge this PR? (I don't have write permissions)
2. Would you like me to create a cherry-pick into release/20.x branch, or is there a separate process for cherry-picks (e.g., wait some time to validate, submit a cherry-pick request somewhere, etc.)?

[JDevlieghere]

2. Would you like me to create a cherry-pick into release/20.x branch, or is there a separate process for cherry-picks (e.g., wait some time to validate, submit a cherry-pick request somewhere, etc.)?

Please file and issue (here's an example: #135922) and add the LLVM 20.x Release milestone. You can use the /cherry-pick command to have the automation create a backporting PR. The reviewers can then express whether they support back-porting it. It's fair to wait a little bit make sure this makes this through the post-commit CI.

[llvmbot]

2. Would you like me to create a cherry-pick into release/20.x branch, or is there a separate process for cherry-picks (e.g., wait some time to validate, submit a cherry-pick request somewhere, etc.)?

Please file and issue (here's an example: #135922) and add the LLVM 20.x Release milestone. You can use the /cherry-pick command to have the automation create a backporting PR.

Error: Command failed due to missing milestone.

[llvmbot]

2. Would you like me to create a cherry-pick into release/20.x branch, or is there a separate process for cherry-picks (e.g., wait some time to validate, submit a cherry-pick request somewhere, etc.)?

Please file and issue (here's an example: #135922) and add the LLVM 20.x Release milestone. You can use the /cherry-pick command to have the automation create a backporting PR. The reviewers can then express whether they support back-porting it. It's fair to wait a little bit make sure this makes this through the post-commit CI.

Error: Command failed due to missing milestone.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder lldb-remote-linux-ubuntu running on as-builder-9 while building lldb at step 15 "test-check-lldb-unit".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/8141

Here is the relevant piece of the build log for the reference

Step 15 (test-check-lldb-unit) failure: Test just built components: check-lldb-unit completed (failure)
******************** TEST 'lldb-unit :: Host/./HostTests/62/98' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/tools/lldb/unittests/Host/./HostTests-lldb-unit-299106-62-98.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=98 GTEST_SHARD_INDEX=62 /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/tools/lldb/unittests/Host/./HostTests
--

Script:
--
/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/tools/lldb/unittests/Host/./HostTests --gtest_filter=SocketTest.DomainSocketFromBoundNativeSocket
--
/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/unittests/Host/SocketTest.cpp:354: Failure
Value of: llvm::detail::TakeError(error.takeError())
Expected: succeeded
  Actual: failed  (Failed to set socket address)


/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/unittests/Host/SocketTest.cpp:354
Value of: llvm::detail::TakeError(error.takeError())
Expected: succeeded
  Actual: failed  (Failed to set socket address)



********************

[emrekultursay]

We're getting SetSockAddr error which can only fail if the path is too long. The path generated by createUniqueDirectory is an absolute path to a temporary directory. I guess it can be longer than 107 bytes on the CI machines.

All the other DomainSocket related tests skip the test if the generated path is longer than 107 bytes:

  // Skip the test if the $TMPDIR is too long to hold a domain socket.
  if (Path.size() > 107u)
    return;

I'll add that to see if it fixes the issue.

[slydiman]

Note DomainSocket is not supported on Windows, so removing the following code broke the Windows build!

#if LLDB_ENABLE_POSIX
      socket = new DomainSocket(sockfd, /*should_close=*/true);
#else
      WithColor::error() << "lldb-platform child: Unix domain sockets are not "
                            "supported on this platform.";
      return socket_error;
#endif

[emrekultursay]

Thanks. Are the following patches good enough, or do we prefer a revert?

• Test fix: Skip tests if socket name is longer than 107 bytes #137405
• Windows fix: Fix Windows build #137414