Implement IPC::ChannelFactory, a class that accept()s on a UNIX socket.

ipc/unix_domain_socket_util.cc

Index: ipc/unix_domain_socket_util.cc
diff --git a/ipc/unix_domain_socket_util.cc b/ipc/unix_domain_socket_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..aacfc158059e00ddbaf86e12cea2c79045b161d5
--- /dev/null
+++ b/ipc/unix_domain_socket_util.cc
@@ -0,0 +1,174 @@
+// Copyright 2013 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.
+
+#include "ipc/unix_domain_socket_util.h"
+
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/un.h>
+#include <unistd.h>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+
+namespace IPC {
+
+// Verify that kMaxPipeNameLength is a decent size.
+COMPILE_ASSERT(sizeof(((sockaddr_un*)0)->sun_path) >= kMaxPipeNameLength,
+               BAD_SUN_PATH_LENGTH);
+
+namespace {
+
+// returns fd (>= 0) on success, -1 on failure.
+int MakeUnixAddrForPath(const std::string& pipe_name,

[Mark Mentovai, 2013/03/05 20:08:03]

The comment should explain these parameters, because it’s not clear just from
reading this prototype whether unix_addr and unix_addr_len are in/out-parameters
or just out-parameters.

[Mark Mentovai, 2013/03/05 20:08:03]

Similar comments about the use of the name “pipe” in this file apply as well.

[jeremya, 2013/03/06 05:03:18]

Done.

[jeremya, 2013/03/06 05:03:18]

Done.

+                        struct sockaddr_un* unix_addr,
+                        size_t* unix_addr_len) {
+  DCHECK_GT(pipe_name.length(), 0u);
+  DCHECK_LT(pipe_name.length(), kMaxPipeNameLength);
+
+  if (pipe_name.length() == 0 || pipe_name.length() >= kMaxPipeNameLength) {

[Mark Mentovai, 2013/03/05 20:08:03]

I know you said that you didn’t think you needed to worry about pipe_name.length
== kMaxPipeNameLength because the sockaddr_un structure contains the sun_len
field which encodes the length distinctly from the sun_path field, thus
rendering NUL-termination unnecessary. I don’t think that this is correct. Some
code expects sockaddr_un::sun_path to be NUL-terminated. See, for example, the
definition of the SUN_LEN macro in <sys/un.h> on both Mac and Linux. See “man 7
unix” on Linux as well.

Anyway, this looks correct now. I just wanted to make sure that we all agree
that it’s correct for the right reasons, and not by accident.

[jeremya, 2013/03/06 05:03:18]

I see. How confusing. I added a comment here to clarify lest someone later see
the same logic that I did.

+    LOG(ERROR) << "Pipe name too long: " << pipe_name;

[Mark Mentovai, 2013/03/05 20:08:03]

Or short.

[jeremya, 2013/03/06 05:03:18]

Done.

+    return -1;
+  }
+
+  // Create socket.
+  int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+  if (fd < 0) {
+    PLOG(ERROR) << "socket";
+    return -1;
+  }

[Mark Mentovai, 2013/03/05 20:08:03]

You should make a scoped_fd here (and return scoper.release()) so you can do
away with the manual close() on line 48 and make this code more friendly to
future modifications that might want to add early returns.

[jeremya, 2013/03/06 05:03:18]

Done.

+
+  // Make socket non-blocking
+  if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
+    PLOG(ERROR) << "fcntl(O_NONBLOCK) " << pipe_name;

[Mark Mentovai, 2013/03/05 20:08:03]

pipe_name is meaningless for error reporting for any system call on a socket FD
before the socket has been bound. Same on line 49.

[jeremya, 2013/03/06 05:03:18]

Done.

+    if (HANDLE_EINTR(close(fd)) < 0)
+      PLOG(ERROR) << "close " << pipe_name;
+    return -1;
+  }
+
+  // Create unix_addr structure.
+  DCHECK(unix_addr);
+  memset(unix_addr, 0, sizeof(struct sockaddr_un));
+  unix_addr->sun_family = AF_UNIX;
+  unix_addr->sun_len = pipe_name.length();
+  strncpy(unix_addr->sun_path, pipe_name.c_str(), kMaxPipeNameLength);
+  *unix_addr_len =
+      offsetof(struct sockaddr_un, sun_path) + pipe_name.length() + 1;

[Mark Mentovai, 2013/03/05 20:08:03]

I think that the +1 isn’t necessary.

[jeremya, 2013/03/06 05:03:18]

Done.

+  return fd;
+}
+
+}  // namespace
+
+bool CreateServerUnixDomainSocket(const base::FilePath& pipe_path,
+                                  int* server_listen_fd) {
+  DCHECK(server_listen_fd);
+
+  std::string pipe_name = pipe_path.value();
+  base::FilePath pipe_dir = pipe_path.DirName();
+
+  struct sockaddr_un unix_addr;
+  size_t unix_addr_len;
+  int fd = MakeUnixAddrForPath(pipe_name, &unix_addr, &unix_addr_len);
+  if (fd < 0)
+    return false;
+  file_util::ScopedFD scoped_fd(&fd);
+
+  // Make sure the path we need exists.
+  if (!file_util::CreateDirectory(pipe_dir)) {

[jeremya, 2013/03/05 03:52:44]

Hm, problem: IPC channels normally want to be used on the IO thread, but the IO
thread disallows IO (ha), so this has to be called on the FILE thread. Should
this use a base::ThreadRestricitons::ScopedAllowIO?

[Mark Mentovai, 2013/03/05 20:08:03]

jeremya wrote:
Oh, this sucks!

The problem is actually more than just this call: the unlink and bind also go
out to the filesystem.

No great answer for you here. Allowing IO sucks. Just mandating that this
directory exist when you’re called could work, but the bind() will still do
filesystem IO, so it would get you past Chrome’s checks (since we don’t wrap
bind() in something that checks whether IO is allowed or whether IO will be
done) but wouldn’t really get you past the problem of doing IO on this thread.

[jeremya, 2013/03/06 05:03:18]

Allowing IO might not be so bad as this is a rare operation (~1 per Chrome
session). It's certainly easier than posting a task over to the file thread and
forcing channel creation to be asynchronous (it looks like certain things
assume/require that channel creation happen synchronously).

I wouldn't be super worried about allowing IO here.

+    LOG(ERROR) << "Couldn't create directory: " << pipe_dir.value();
+    return false;
+  }
+
+  // Delete any old FS instances.
+  if (unlink(pipe_name.c_str()) < 0 && errno != ENOENT)

[Mark Mentovai, 2013/03/05 20:08:03]

This is where you want to #include <errno.h>.

[jeremya, 2013/03/06 05:03:18]

Done.

+    PLOG(ERROR) << "unlink " << pipe_name;
+
+  // Bind the socket.
+  if (bind(fd, reinterpret_cast<const sockaddr*>(&unix_addr),
+           unix_addr_len) < 0) {
+    PLOG(ERROR) << "bind " << pipe_path.value();
+    return false;
+  }
+
+  // Start listening on the socket.
+  if (listen(fd, 1) < 0) {

[Mark Mentovai, 2013/03/05 20:08:03]

1 is small. Use SOMAXCONN?

[jeremya, 2013/03/06 05:03:18]

Done.

+    PLOG(ERROR) << "listen " << pipe_path.value();
+    unlink(pipe_name.c_str());
+    return false;
+  }
+
+  *server_listen_fd = *scoped_fd.release();
+  return true;
+}
+
+bool CreateClientUnixDomainSocket(const base::FilePath& pipe_path,
+                                  int* client_socket) {
+  DCHECK(client_socket);
+
+  std::string pipe_name = pipe_path.value();
+  base::FilePath pipe_dir = pipe_path.DirName();
+
+  struct sockaddr_un unix_addr;
+  size_t unix_addr_len;
+  int fd = MakeUnixAddrForPath(pipe_name, &unix_addr, &unix_addr_len);
+  if (fd < 0)
+    return false;
+  file_util::ScopedFD scoped_fd(&fd);
+
+  if (HANDLE_EINTR(connect(fd, reinterpret_cast<sockaddr*>(&unix_addr),
+                           unix_addr_len)) < 0) {
+    PLOG(ERROR) << "connect " << pipe_path.value();
+    return false;
+  }
+
+  *client_socket = *scoped_fd.release();
+  return true;
+}
+
+bool GetPeerEuid(int fd, uid_t* client_euid) {
+#if defined(OS_MACOSX) || defined(OS_OPENBSD) || defined(OS_FREEBSD)
+  uid_t peer_euid;
+  gid_t peer_gid;
+  if (getpeereid(fd, &peer_euid, &peer_gid) < 0) {
+    PLOG(ERROR) << "getpeereid " << fd;
+    return false;
+  }
+  *client_euid = peer_euid;
+  return true;
+#else
+  struct ucred cred;
+  socklen_t cred_len = sizeof(cred);
+  if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &cred, &cred_len) < 0) {
+    PLOG(ERROR) << "getsockopt " << fd;
+    return false;
+  }
+  if (static_cast<unsigned>(cred_len) < sizeof(cred)) {
+    NOTREACHED() << "Truncated ucred from SO_PEERCRED?";
+    return false;
+  }
+  *client_euid = cred.uid;
+  return true;
+#endif
+}
+
+bool ServerAcceptConnection(int server_listen_fd, int* server_socket) {
+  DCHECK(server_socket);
+
+  int accept_fd = HANDLE_EINTR(accept(server_listen_fd, NULL, 0));
+  if (accept_fd < 0)
+    return false;

[Mark Mentovai, 2013/03/05 20:08:03]

“false” may be a little simplistic as a return value. Consider my previous
comment about ECONNABORTED.

[jeremya, 2013/03/06 05:03:18]

Addressed in a previous reply.

+  file_util::ScopedFD scoped_fd(&accept_fd);
+  if (fcntl(accept_fd, F_SETFL, O_NONBLOCK) < 0) {
+    PLOG(ERROR) << "fcntl(O_NONBLOCK) " << accept_fd;
+    return false;
+  }
+
+  *server_socket = *scoped_fd.release();
+  return true;
+}
+
+}  // namespace IPC