[go: up one dir, main page]

mac: Allow process singleton socket paths up to 253 bytes long

When the temporary directory is too long, Chrome exited during startup.
Previously, the limit was 103 bytes, based on the size of
sockaddr_un::sun_path, leaving room for a NUL byte. Paths returned by
NSTemporaryDirectory are normally 49 bytes, and to build a singleton
socket path, 41 bytes (for com.google.Chrome, more for other bundle IDs)
are added for a total of 90, within the 103-byte limit. In a future OS
update, NSTemporaryDirectory may return a longer path, +28 bytes,
resulting in a socket path 118 bytes long, exceeding the size available
in sockaddr_un::sun_path.

By providing a version of the sockaddr_un structure sized maximally as
supported by the kernel, there's room for a 253-byte sun_path; more
careful use of the socket API as supported on macOS eliminates the need
to NUL-terminate the path, so that the full 253 bytes can be used for a
singleton socket path. While still short of the 1024-byte PATH_MAX
supported by most other parts of the system that operate on paths, it's
still enough of an increase in headroom to dramatically reduce the risk
of crashing at startup due to an inability to form a process singleton
socket path within the required constraints.

(cherry picked from commit 937fe536976a6e29ddfb29adfa80c8703d12661a)

Bug: 1266817
Change-Id: I900753e0483d3566dd4e59767b25b95daeb8e152
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3260710
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#938351}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3298434
Auto-Submit: Mark Mentovai <mark@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/branch-heads/4664@{#1178}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/base/files/file_util_mac.mm b/base/files/file_util_mac.mm
index e53732a..7dc5fff 100644
--- a/base/files/file_util_mac.mm
+++ b/base/files/file_util_mac.mm
@@ -27,13 +27,14 @@
 
 bool GetTempDir(base::FilePath* path) {
   // In order to facilitate hermetic runs on macOS, first check
-  // $MAC_CHROMIUM_TMPDIR. We check this instead of $TMPDIR because external
-  // programs currently set $TMPDIR with no effect, but when we respect it
-  // directly it can cause crashes (like crbug.com/698759).
+  // MAC_CHROMIUM_TMPDIR. This is used instead of TMPDIR for historical reasons.
+  // This was originally done for https://crbug.com/698759 (TMPDIR too long for
+  // process singleton socket path), but is hopefully obsolete as of
+  // https://crbug.com/1266817 (allows a longer process singleton socket path).
+  // Continue tracking MAC_CHROMIUM_TMPDIR as that's what build infrastructure
+  // sets on macOS.
   const char* env_tmpdir = getenv("MAC_CHROMIUM_TMPDIR");
   if (env_tmpdir) {
-    DCHECK_LT(strlen(env_tmpdir), 50u)
-        << "too-long TMPDIR causes socket name length issues.";
     *path = base::FilePath(env_tmpdir);
     return true;
   }
diff --git a/chrome/browser/process_singleton_posix.cc b/chrome/browser/process_singleton_posix.cc
index 4547eb85..f9f5a2a 100644
--- a/chrome/browser/process_singleton_posix.cc
+++ b/chrome/browser/process_singleton_posix.cc
@@ -42,6 +42,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <signal.h>
+#include <string.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -52,6 +53,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <type_traits>
 
 #include <stddef.h>
 
@@ -108,6 +110,28 @@
 
 namespace {
 
+#if defined(OS_MAC)
+// In order to allow longer paths for the singleton socket's filesystem node,
+// provide an "oversized" sockaddr_un-equivalent with a larger sun_path member.
+// sockaddr_un in the SDK has sun_path[104], which is too confined for the
+// singleton socket's path. The kernel will accept a sockaddr structure up to
+// SOCK_MAXADDRLEN (255) bytes long. This structure makes all of that space
+// available, effectively allowing sun_path[253]. Although shorter than
+// PATH_MAX (1024), this will hopefully be long enough. Many systems support an
+// extension like this, but it's not entirely portable. In this case, the OS
+// vendor has said that the behavior is stable. Learn more at SetupSockAddr.
+struct SockaddrUn {
+  decltype(sockaddr_un::sun_len) sun_len;
+  decltype(sockaddr_un::sun_family) sun_family;
+  std::remove_extent_t<decltype(sockaddr_un::sun_path)>
+      sun_path[SOCK_MAXADDRLEN - offsetof(sockaddr_un, sun_path)];
+};
+#else
+// On other platforms without a demonstrated need for paths longer than
+// sockaddr_un::sun_path, just do the portable thing.
+using SockaddrUn = sockaddr_un;
+#endif
+
 // Timeout for the current browser process to respond. 20 seconds should be
 // enough.
 const int kTimeoutInSeconds = 20;
@@ -222,11 +246,43 @@
 }
 
 // Set up a sockaddr appropriate for messaging.
-bool SetupSockAddr(const std::string& path, struct sockaddr_un* addr) {
+bool SetupSockAddr(const std::string& path,
+                   SockaddrUn* addr,
+                   socklen_t* socklen) {
   addr->sun_family = AF_UNIX;
+#if defined(OS_MAC)
+  // Allow the use of the entire length of sun_path, without reservation for a
+  // NUL terminator. The socklen parameter to bind and connect encodes the
+  // length of the sockaddr structure, and xnu does not require sun_path to be
+  // NUL-terminated. This is not portable, but it’s OK on macOS, and allows
+  // maximally-sized paths on a platform where the singleton socket path is
+  // already long. 11.5 xnu-7195.141.2/bsd/kern/uipc_usrreq.c unp_bind,
+  // unp_connect.
+  if (path.length() > base::size(addr->sun_path))
+    return false;
+
+  // On input to the kernel, sun_len is ignored and overwritten by the value of
+  // the passed-in socklen parameter. 11.5
+  // xnu-7195.141.2/bsd/kern/uipc_syscalls.c getsockaddr[_s]; note that the
+  // field is sa_len and not sun_len there because it occurs in generic code
+  // referring to sockaddr before being specialized into sockaddr_un or any
+  // other address family's sockaddr structure.
+  //
+  // Since the length needs to be computed for socklen anyway, just populate
+  // sun_len correctly.
+  addr->sun_len =
+      offsetof(std::remove_pointer_t<decltype(addr)>, sun_path) + path.length();
+
+  *socklen = addr->sun_len;
+  memcpy(addr->sun_path, path.c_str(), path.length());
+#else
+  // The portable version: NUL-terminate sun_path and don’t touch sun_len (which
+  // may not even exist).
   if (path.length() >= base::size(addr->sun_path))
     return false;
+  *socklen = sizeof(*addr);
   base::strlcpy(addr->sun_path, path.c_str(), base::size(addr->sun_path));
+#endif
   return true;
 }
 
@@ -243,9 +299,12 @@
 }
 
 // Set up a socket and sockaddr appropriate for messaging.
-void SetupSocket(const std::string& path, int* sock, struct sockaddr_un* addr) {
+void SetupSocket(const std::string& path,
+                 int* sock,
+                 SockaddrUn* addr,
+                 socklen_t* socklen) {
   *sock = SetupSocketOnly();
-  CHECK(SetupSockAddr(path, addr)) << "Socket path too long: " << path;
+  CHECK(SetupSockAddr(path, addr, socklen)) << "Socket path too long: " << path;
 }
 
 // Read a symbolic link, return empty string if given path is not a symbol link.
@@ -364,16 +423,16 @@
       return false;
     // Now we know the directory was (at that point) created by the profile
     // owner. Try to connect.
-    sockaddr_un addr;
-    if (!SetupSockAddr(socket_target.value(), &addr)) {
+    SockaddrUn addr;
+    socklen_t socklen;
+    if (!SetupSockAddr(socket_target.value(), &addr, &socklen)) {
       // If a sockaddr couldn't be initialized due to too long of a socket
       // path, we can be sure there isn't already a Chrome running with this
       // socket path, since it would have hit the CHECK() on the path length.
       return false;
     }
-    int ret = HANDLE_EINTR(connect(socket->fd(),
-                                   reinterpret_cast<sockaddr*>(&addr),
-                                   sizeof(addr)));
+    int ret = HANDLE_EINTR(
+        connect(socket->fd(), reinterpret_cast<sockaddr*>(&addr), socklen));
     if (ret != 0)
       return false;
     // Check the cookie again. We only link in /tmp, which is sticky, so, if the
@@ -388,16 +447,16 @@
   } else if (errno == EINVAL) {
     // It exists, but is not a symlink (or some other error we detect
     // later). Just connect to it directly; this is an older version of Chrome.
-    sockaddr_un addr;
-    if (!SetupSockAddr(socket_path.value(), &addr)) {
+    SockaddrUn addr;
+    socklen_t socklen;
+    if (!SetupSockAddr(socket_path.value(), &addr, &socklen)) {
       // If a sockaddr couldn't be initialized due to too long of a socket
       // path, we can be sure there isn't already a Chrome running with this
       // socket path, since it would have hit the CHECK() on the path length.
       return false;
     }
-    int ret = HANDLE_EINTR(connect(socket->fd(),
-                                   reinterpret_cast<sockaddr*>(&addr),
-                                   sizeof(addr)));
+    int ret = HANDLE_EINTR(
+        connect(socket->fd(), reinterpret_cast<sockaddr*>(&addr), socklen));
     return (ret == 0);
   } else {
     // File is missing, or other error.
@@ -576,7 +635,7 @@
     int socket) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Accepting incoming client.
-  sockaddr_un from;
+  SockaddrUn from;
   socklen_t from_len = sizeof(from);
   int connection_socket = HANDLE_EINTR(
       accept(socket, reinterpret_cast<sockaddr*>(&from), &from_len));
@@ -970,9 +1029,6 @@
 }
 
 bool ProcessSingleton::Create() {
-  int sock;
-  sockaddr_un addr;
-
   // The symlink lock is pointed to the hostname and process id, so other
   // processes can find it out.
   base::FilePath symlink_content(
@@ -1018,7 +1074,10 @@
   // leaving a dangling symlink.
   base::FilePath socket_target_path =
       socket_dir_.GetPath().Append(chrome::kSingletonSocketFilename);
-  SetupSocket(socket_target_path.value(), &sock, &addr);
+  int sock;
+  SockaddrUn addr;
+  socklen_t socklen;
+  SetupSocket(socket_target_path.value(), &sock, &addr, &socklen);
 
   // Setup the socket symlink and the two cookies.
   base::FilePath cookie(GenerateCookie());
@@ -1037,7 +1096,7 @@
     return false;
   }
 
-  if (bind(sock, reinterpret_cast<sockaddr*>(&addr), sizeof(addr)) < 0) {
+  if (bind(sock, reinterpret_cast<sockaddr*>(&addr), socklen) < 0) {
     PLOG(ERROR) << "Failed to bind() " << socket_target_path.value();
     CloseSocket(sock);
     return false;