[llvm][Support] Avoid silent truncation of socket paths

[jroelofs]

When the name is too long to fit in sockaddr_un::sun_path's 104 character buffer, we now surface the error condition, rather than silently truncating and failing the test with an "address in use Bind error". We also shorten the name a bit to give more headroom on CI systems that define an explicit TMP_DIR that acts as a prefix to the path created by these tests.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Jon Roelofs (jroelofs)

Changes

When the name is too long to fit in sockaddr_un::sun_path's 104 character buffer, we now surface the error condition, rather than silently truncating and failing the test with an "address in use Bind error". We also shorten the name a bit to give more headroom on CI systems that define an explicit TMP_DIR that acts as a prefix to the path created by these tests.

---

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

2 Files Affected:

• (modified) llvm/lib/Support/raw_socket_stream.cpp (+17-5)
• (modified) llvm/unittests/Support/raw_socket_stream_test.cpp (+8-12)

diff --git a/llvm/lib/Support/raw_socket_stream.cpp b/llvm/lib/Support/raw_socket_stream.cpp
index 9fb408425ddb0..cdf793c8958f9 100644
--- a/llvm/lib/Support/raw_socket_stream.cpp
+++ b/llvm/lib/Support/raw_socket_stream.cpp
@@ -61,10 +61,16 @@ static std::error_code getLastSocketErrorCode() {
 #endif
 }
 
-static sockaddr_un setSocketAddr(StringRef SocketPath) {
+static Expected<sockaddr_un> setSocketAddr(StringRef SocketPath) {
   struct sockaddr_un Addr;
   memset(&Addr, 0, sizeof(Addr));
   Addr.sun_family = AF_UNIX;
+
+  if (sizeof(sockaddr_un::sun_path) <= SocketPath.size())
+    return make_error<StringError>(
+        std::make_error_code(std::errc::filename_too_long),
+        "Socket path exceeds sockaddr_un::sun_path size limit");
+
   strncpy(Addr.sun_path, SocketPath.str().c_str(), sizeof(Addr.sun_path) - 1);
   return Addr;
 }
@@ -90,8 +96,11 @@ static Expected<int> getSocketFD(StringRef SocketPath) {
   // off the handshake (and SO_PEERCRED/getpeereid support).
   setsockopt(Socket, SOL_SOCKET, SO_PEERCRED, NULL, 0);
 #endif
-  struct sockaddr_un Addr = setSocketAddr(SocketPath);
-  if (::connect(Socket, (struct sockaddr *)&Addr, sizeof(Addr)) == -1) {
+  Expected<struct sockaddr_un> Addr = setSocketAddr(SocketPath);
+  if (!Addr)
+    return Addr.takeError();
+
+  if (::connect(Socket, (struct sockaddr *)&*Addr, sizeof(*Addr)) == -1) {
     ::close(Socket);
     return llvm::make_error<StringError>(getLastSocketErrorCode(),
                                          "Connect socket failed");
@@ -167,8 +176,11 @@ Expected<ListeningSocket> ListeningSocket::createUnix(StringRef SocketPath,
   // off the handshake (and SO_PEERCRED/getpeereid support).
   setsockopt(Socket, SOL_SOCKET, SO_PEERCRED, NULL, 0);
 #endif
-  struct sockaddr_un Addr = setSocketAddr(SocketPath);
-  if (::bind(Socket, (struct sockaddr *)&Addr, sizeof(Addr)) == -1) {
+  Expected<struct sockaddr_un> Addr = setSocketAddr(SocketPath);
+  if (!Addr)
+    return Addr.takeError();
+
+  if (::bind(Socket, (struct sockaddr *)&*Addr, sizeof(*Addr)) == -1) {
     // Grab error code from call to ::bind before calling ::close
     std::error_code EC = getLastSocketErrorCode();
     ::close(Socket);
diff --git a/llvm/unittests/Support/raw_socket_stream_test.cpp b/llvm/unittests/Support/raw_socket_stream_test.cpp
index 183a384339268..fb8e99ea15e24 100644
--- a/llvm/unittests/Support/raw_socket_stream_test.cpp
+++ b/llvm/unittests/Support/raw_socket_stream_test.cpp
@@ -33,13 +33,12 @@ TEST(raw_socket_streamTest, CLIENT_TO_SERVER_AND_SERVER_TO_CLIENT) {
     GTEST_SKIP();
 
   SmallString<100> SocketPath;
-  llvm::sys::fs::createUniquePath("client_server_comms-%%%%%%.sock", SocketPath,
-                                  true);
+  llvm::sys::fs::createUniquePath("llvm-%%%%%%%%.sock", SocketPath, true);
   auto Cleanup = llvm::scope_exit([&] { std::remove(SocketPath.c_str()); });
 
   Expected<ListeningSocket> MaybeServerListener =
       ListeningSocket::createUnix(SocketPath);
-  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
+  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded()) << SocketPath;
 
   ListeningSocket ServerListener = std::move(*MaybeServerListener);
 
@@ -73,13 +72,12 @@ TEST(raw_socket_streamTest, READ_WITH_TIMEOUT) {
     GTEST_SKIP();
 
   SmallString<100> SocketPath;
-  llvm::sys::fs::createUniquePath("read_with_timeout-%%%%%%.sock", SocketPath,
-                                  true);
+  llvm::sys::fs::createUniquePath("llvm-%%%%%%%%.sock", SocketPath, true);
   auto Cleanup = llvm::scope_exit([&] { std::remove(SocketPath.c_str()); });
 
   Expected<ListeningSocket> MaybeServerListener =
       ListeningSocket::createUnix(SocketPath);
-  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
+  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded()) << SocketPath;
   ListeningSocket ServerListener = std::move(*MaybeServerListener);
 
   Expected<std::unique_ptr<raw_socket_stream>> MaybeClient =
@@ -104,13 +102,12 @@ TEST(raw_socket_streamTest, ACCEPT_WITH_TIMEOUT) {
     GTEST_SKIP();
 
   SmallString<100> SocketPath;
-  llvm::sys::fs::createUniquePath("accept_with_timeout-%%%%%%.sock", SocketPath,
-                                  true);
+  llvm::sys::fs::createUniquePath("llvm-%%%%%%%%.sock", SocketPath, true);
   auto Cleanup = llvm::scope_exit([&] { std::remove(SocketPath.c_str()); });
 
   Expected<ListeningSocket> MaybeServerListener =
       ListeningSocket::createUnix(SocketPath);
-  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
+  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded()) << SocketPath;
   ListeningSocket ServerListener = std::move(*MaybeServerListener);
 
   Expected<std::unique_ptr<raw_socket_stream>> MaybeServer =
@@ -124,13 +121,12 @@ TEST(raw_socket_streamTest, ACCEPT_WITH_SHUTDOWN) {
     GTEST_SKIP();
 
   SmallString<100> SocketPath;
-  llvm::sys::fs::createUniquePath("accept_with_shutdown-%%%%%%.sock",
-                                  SocketPath, true);
+  llvm::sys::fs::createUniquePath("llvm-%%%%%%%%.sock", SocketPath, true);
   auto Cleanup = llvm::scope_exit([&] { std::remove(SocketPath.c_str()); });
 
   Expected<ListeningSocket> MaybeServerListener =
       ListeningSocket::createUnix(SocketPath);
-  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded());
+  ASSERT_THAT_EXPECTED(MaybeServerListener, llvm::Succeeded()) << SocketPath;
   ListeningSocket ServerListener = std::move(*MaybeServerListener);
 
   // Create a separate thread to close the socket after a delay. Simulates a

[cachemeifyoucan]

LGTM. But maybe skip the test if unittest construct a path that is too long?

[jroelofs]

Good idea!

[jroelofs]

Eh. On second thought, detecting & unwrapping that specific StringError in the test itself is pretty awkward. At least with the diagnostic, CI operators will now have some chance of catching what they need to fix.

[cachemeifyoucan]

There is std::errc::filename_too_long you can unwrap? Or worst, you can do a length check in unit test as well.

[cachemeifyoucan]

Nice. LGTM