ProcessSingletonPosix: don't CHECK if trying to connect to existing process with too long socket sy…

chrome/browser/process_singleton_posix.cc

Index: chrome/browser/process_singleton_posix.cc
diff --git a/chrome/browser/process_singleton_posix.cc b/chrome/browser/process_singleton_posix.cc
index 17c9b88606ee89b9570bb517942a9339e1a3f016..6265111fb3ce4c70be5921191c966963823bf8ab 100644
--- a/chrome/browser/process_singleton_posix.cc
+++ b/chrome/browser/process_singleton_posix.cc
@@ -218,11 +218,12 @@ ssize_t ReadFromSocket(int fd,
 }
 
 // Set up a sockaddr appropriate for messaging.
-void SetupSockAddr(const std::string& path, struct sockaddr_un* addr) {
+bool SetupSockAddr(const std::string& path, struct sockaddr_un* addr) {
   addr->sun_family = AF_UNIX;
-  CHECK(path.length() < arraysize(addr->sun_path))
-      << "Socket path too long: " << path;
+  if (path.length() >= arraysize(addr->sun_path))
+    return false;
   base::strlcpy(addr->sun_path, path.c_str(), arraysize(addr->sun_path));
+  return true;
 }
 
 // Set up a socket appropriate for messaging.
@@ -240,7 +241,7 @@ int SetupSocketOnly() {
 // Set up a socket and sockaddr appropriate for messaging.
 void SetupSocket(const std::string& path, int* sock, struct sockaddr_un* addr) {
   *sock = SetupSocketOnly();
-  SetupSockAddr(path, addr);
+  CHECK(SetupSockAddr(path, addr)) << "Socket path too long: " << path;
 }
 
 // Read a symbolic link, return empty string if given path is not a symbol link.
@@ -386,7 +387,8 @@ bool ConnectSocket(ScopedSocket* socket,
     // Now we know the directory was (at that point) created by the profile
     // owner. Try to connect.
     sockaddr_un addr;
-    SetupSockAddr(socket_target.value(), &addr);
+    if (!SetupSockAddr(socket_target.value(), &addr))
+      return false;
     int ret = HANDLE_EINTR(connect(socket->fd(),
                                    reinterpret_cast<sockaddr*>(&addr),
                                    sizeof(addr)));
@@ -405,7 +407,8 @@ bool ConnectSocket(ScopedSocket* socket,
     // 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;
-    SetupSockAddr(socket_path.value(), &addr);
+    if (!SetupSockAddr(socket_path.value(), &addr))
+      return false;
     int ret = HANDLE_EINTR(connect(socket->fd(),
                                    reinterpret_cast<sockaddr*>(&addr),
                                    sizeof(addr)));
@@ -982,9 +985,13 @@ bool ProcessSingleton::Create() {
         dir_mode == base::FILE_PERMISSION_USER_MASK)
       << "Temp directory mode is not 700: " << std::oct << dir_mode;
 
-  // Setup the socket symlink and the two cookies.
+  // Create the socket before creating the symlink, so that if the create fails
+  // a dangling link isn't left behind.
   base::FilePath socket_target_path =
       socket_dir_.GetPath().Append(chrome::kSingletonSocketFilename);
+  SetupSocket(socket_target_path.value(), &sock, &addr);

[Nico, 2017/04/13 15:54:36]

add comment why it's ok if this returns false

[mattm, 2017/04/13 20:15:27]

This one can't return false (SetupSocket does a CHECK internally). I updated the
comment to be more specific about that, and added comments on the SetupSockAddr
calls.

+
+  // Setup the socket symlink and the two cookies.
   base::FilePath cookie(GenerateCookie());
   base::FilePath remote_cookie_path =
       socket_dir_.GetPath().Append(chrome::kSingletonCookieFilename);
@@ -1001,8 +1008,6 @@ bool ProcessSingleton::Create() {
     return false;
   }
 
-  SetupSocket(socket_target_path.value(), &sock, &addr);
-
   if (bind(sock, reinterpret_cast<sockaddr*>(&addr), sizeof(addr)) < 0) {
     PLOG(ERROR) << "Failed to bind() " << socket_target_path.value();
     CloseSocket(sock);