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

chrome/browser/process_singleton_posix.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // On Linux, when the user tries to launch a second copy of chrome, we check
 // for a socket in the user's profile directory.  If the socket file is open we
 // send a message to the first chrome browser process with the current
 // directory and second process command line flags.  The second process then
 // exits.
 //
@@ skipping 200 matching lines @@
       return bytes_read;
     } else {
       bytes_read += rv;
     }
   } while (bytes_read < bufsize);
 
   return bytes_read;
 }
 
 // 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.
 int SetupSocketOnly() {
   int sock = socket(PF_UNIX, SOCK_STREAM, 0);
   PCHECK(sock >= 0) << "socket() failed";
 
   DCHECK(base::SetNonBlocking(sock)) << "Failed to make non-blocking socket.";
   int rv = SetCloseOnExec(sock);
   DCHECK_EQ(0, rv) << "Failed to set CLOEXEC on socket.";
 
   return sock;
 }
 
 // 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.
 base::FilePath ReadLink(const base::FilePath& path) {
   base::FilePath target;
   if (!base::ReadSymbolicLink(path, &target)) {
     // The only errno that should occur is ENOENT.
     if (errno != 0 && errno != ENOENT)
       PLOG(ERROR) << "readlink(" << path.value() << ") failed";
   }
@@ skipping 125 matching lines @@
     if (cookie.empty())
       return false;
     base::FilePath remote_cookie = socket_target.DirName().
                              Append(chrome::kSingletonCookieFilename);
     // Verify the cookie before connecting.
     if (!CheckCookie(remote_cookie, cookie))
       return false;
     // 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)));
     if (ret != 0)
       return false;
     // Check the cookie again. We only link in /tmp, which is sticky, so, if the
     // directory is still correct, it must have been correct in-between when we
     // connected. POSIX, sadly, lacks a connectat().
     if (!CheckCookie(remote_cookie, cookie)) {
       socket->Reset();
       return false;
     }
     // Success!
     return true;
   } 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;
-    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)));
     return (ret == 0);
   } else {
     // File is missing, or other error.
     if (errno != ENOENT)
       PLOG(ERROR) << "readlink failed";
     return false;
   }
@@ skipping 556 matching lines @@
     LOG(ERROR) << "Failed to create socket directory.";
     return false;
   }
 
   // Check that the directory was created with the correct permissions.
   int dir_mode = 0;
   CHECK(base::GetPosixFilePermissions(socket_dir_.GetPath(), &dir_mode) &&
         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);
   UnlinkPath(socket_path_);
   UnlinkPath(cookie_path_);
   if (!SymlinkPath(socket_target_path, socket_path_) ||
       !SymlinkPath(cookie, cookie_path_) ||
       !SymlinkPath(cookie, remote_cookie_path)) {
     // We've already locked things, so we can't have lost the startup race,
     // but something doesn't like us.
     LOG(ERROR) << "Failed to create symlinks.";
     if (!socket_dir_.Delete())
       LOG(ERROR) << "Encountered a problem when deleting socket directory.";
     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);
     return false;
   }
 
   if (listen(sock, 5) < 0)
     NOTREACHED() << "listen failed: " << base::safe_strerror(errno);
 
   DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO));
@@ skipping 48 matching lines @@
 }
 
 void ProcessSingleton::KillProcess(int pid) {
   // TODO(james.su@gmail.com): Is SIGKILL ok?
   int rv = kill(static_cast<base::ProcessHandle>(pid), SIGKILL);
   // ESRCH = No Such Process (can happen if the other process is already in
   // progress of shutting down and finishes before we try to kill it).
   DCHECK(rv == 0 || errno == ESRCH) << "Error killing process: "
                                     << base::safe_strerror(errno);
 }