Change UnixDomainSocket::RecvMsg to return ScopedVector<base::ScopedFD>

base/posix/unix_domain_socket_linux.cc

 // Copyright (c) 2011 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 "base/posix/unix_domain_socket_linux.h"
 
 #include <errno.h>
 #include <sys/socket.h>
 #include <sys/uio.h>
 #include <unistd.h>
 
+#include <vector>
+
+#include "base/files/scoped_file.h"
 #include "base/logging.h"
+#include "base/memory/scoped_vector.h"
 #include "base/pickle.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/stl_util.h"
 
 const size_t UnixDomainSocket::kMaxFileDescriptors = 16;
 
 // static
 bool UnixDomainSocket::EnableReceiveProcessId(int fd) {
   const int enable = 1;
   return setsockopt(fd, SOL_SOCKET, SO_PASSCRED, &enable, sizeof(enable)) == 0;
@@ skipping 33 matching lines @@
   const ssize_t r = HANDLE_EINTR(sendmsg(fd, &msg, flags));
   const bool ret = static_cast<ssize_t>(length) == r;
   delete[] control_buffer;
   return ret;
 }
 
 // static
 ssize_t UnixDomainSocket::RecvMsg(int fd,
                                   void* buf,
                                   size_t length,
-                                  std::vector<int>* fds) {
+                                  ScopedVector<base::ScopedFD>* fds) {
   return UnixDomainSocket::RecvMsgWithPid(fd, buf, length, fds, NULL);
 }
 
 // static
 ssize_t UnixDomainSocket::RecvMsgWithPid(int fd,
                                          void* buf,
                                          size_t length,
-                                         std::vector<int>* fds,
+                                         ScopedVector<base::ScopedFD>* fds,
                                          base::ProcessId* pid) {
   return UnixDomainSocket::RecvMsgWithFlags(fd, buf, length, 0, fds, pid);
 }
 
 // static
 ssize_t UnixDomainSocket::RecvMsgWithFlags(int fd,
                                            void* buf,
                                            size_t length,
                                            int flags,
-                                           std::vector<int>* fds,
+                                           ScopedVector<base::ScopedFD>* fds,
                                            base::ProcessId* out_pid) {
   fds->clear();
 
   struct msghdr msg = {};
   struct iovec iov = { buf, length };
   msg.msg_iov = &iov;
   msg.msg_iovlen = 1;
 
   char control_buffer[CMSG_SPACE(sizeof(int) * kMaxFileDescriptors) +
                       CMSG_SPACE(sizeof(struct ucred))];
@@ skipping 29 matching lines @@
   }
 
   if (msg.msg_flags & MSG_TRUNC || msg.msg_flags & MSG_CTRUNC) {
     for (unsigned i = 0; i < wire_fds_len; ++i)
       close(wire_fds[i]);
     errno = EMSGSIZE;
     return -1;
   }
 
   if (wire_fds) {
-    fds->resize(wire_fds_len);
-    memcpy(vector_as_array(fds), wire_fds, sizeof(int) * wire_fds_len);
+    fds->reserve(wire_fds_len);

[awong, 2014/04/28 18:48:16]

Is resize() or reserve() appropriate here? Is it possible for fds to be too
large initially?

[mdempsky, 2014/04/28 20:16:58]

We call fds->clear() at the very beginning, so the size should be zero.  We need
to use reserve() now instead of resize() because my CL changes the code to use
push_back() instead of just memcpy()'ing directly into the allocated memory.

[awong, 2014/04/28 20:25:18]

Can we CHECK/DCHECK that fds->empty() then? This makes me nervous about someone
in the future messing with the vector between the clear and this block of code.

For that matter...is this premature opt?

[mdempsky, 2014/04/28 21:11:54]

I added a DCHECK(fds->empty()).  The structure of the code is such that I think
it's unlikely to need more sanity checks than that.
Maybe?  I just added it to mimic the existing code structure.  I don't feel
strongly about it, and don't mind removing it if you think it's bad practice.

[awong, 2014/04/28 21:22:28]

I think the resize() was just to guarantee the memcpy could work. I call
premature opt on this, but also don't care enough now that you have the DCHECK.

+    for (unsigned i = 0; i < wire_fds_len; ++i)
+      fds->push_back(new base::ScopedFD(wire_fds[i]));
   }
 
   if (out_pid) {
     DCHECK(pid != -1);
     *out_pid = pid;
   }
 
   return r;
 }
 
 // static
 ssize_t UnixDomainSocket::SendRecvMsg(int fd,
                                       uint8_t* reply,
                                       unsigned max_reply_len,
                                       int* result_fd,
                                       const Pickle& request) {
   return UnixDomainSocket::SendRecvMsgWithFlags(fd, reply, max_reply_len,
                                                 0,  /* recvmsg_flags */
                                                 result_fd, request);
 }
 
 // static
 ssize_t UnixDomainSocket::SendRecvMsgWithFlags(int fd,
                                                uint8_t* reply,
                                                unsigned max_reply_len,
                                                int recvmsg_flags,
                                                int* result_fd,
                                                const Pickle& request) {
-  int fds[2];
+  int raw_socks[2];
 
   // This socketpair is only used for the IPC and is cleaned up before
   // returning.
-  if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) == -1)
+  if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, raw_socks) == -1)
     return -1;
 
-  std::vector<int> fd_vector;
-  fd_vector.push_back(fds[1]);
-  if (!SendMsg(fd, request.data(), request.size(), fd_vector)) {
-    close(fds[0]);
-    close(fds[1]);
-    return -1;
+  base::ScopedFD recv_sock(raw_socks[0]);
+  base::ScopedFD send_sock(raw_socks[1]);

[awong, 2014/04/28 18:48:16]

Why isn't send_sock just inside the scope below?

[brettw, 2014/04/28 19:53:44]

Actually, is there a reason for the scope below? Any why do we need to close the
send_sock then instead of when the function exits? I think the old code did this
because it had to do it explicitly and doing it later meant more work.

[mdempsky, 2014/04/28 20:16:58]

It could be, but I feel like it's less error-prone to create the base::ScopedFD
as close to the FD allocation as possible.  E.g., I don't want someone to
accidentally put in some code between here and the scope below that 'return's,
and then we leak this descriptor.

That said, I can move it below if you prefer.

[mdempsky, 2014/04/28 20:16:58]

Not a particularly good one, but it allows send_fds to free up its memory after
we don't need it anymore.  (Also, since it contains a bare FD that will no
longer be valid once send_sock is reset, it seemed like best practice to make
sure it's inaccessible sooner.)
We need to close the sending end of the socket in our process, otherwise if the
peer process dies before sending us a response, we'll hang in RecvMsgWithFlags()
instead of seeing EOF.  I've added a comment to explain this.

[awong, 2014/04/28 20:25:18]

Yeah...seems good enough. If I was being 100% strict ivory tower, I'd tell you
to pull socketpair() out into a little helper function that only returned 2
ScopedFD via output parameter such that raw_socks would never be in this scope.

Actually...given the amount of lifetime management you are doing, that's
probably worth the extra static function.

On 2014/04/28 20:16:58, mdempsky wrote:

[mdempsky, 2014/04/28 21:11:54]

Acknowledged, but I'd like to tackle this in a followup CL if you don't mind,
because I'd like this to be a public API that I can use in other files too.

+
+  {
+    std::vector<int> send_fds;
+    send_fds.push_back(send_sock.get());
+    if (!SendMsg(fd, request.data(), request.size(), send_fds)) {

[brettw, 2014/04/28 19:53:44]

No {}

[mdempsky, 2014/04/28 20:16:58]

Done.

+      return -1;
+    }
   }
-  close(fds[1]);
+  send_sock.reset();
 
-  fd_vector.clear();
+  ScopedVector<base::ScopedFD> recv_fds;
   // When porting to OSX keep in mind it doesn't support MSG_NOSIGNAL, so the
   // sender might get a SIGPIPE.
   const ssize_t reply_len = RecvMsgWithFlags(
-      fds[0], reply, max_reply_len, recvmsg_flags, &fd_vector, NULL);
-  close(fds[0]);
+      recv_sock.get(), reply, max_reply_len, recvmsg_flags, &recv_fds, NULL);
+  recv_sock.reset();
   if (reply_len == -1)
     return -1;
 
-  if ((!fd_vector.empty() && result_fd == NULL) || fd_vector.size() > 1) {
-    for (std::vector<int>::const_iterator
-         i = fd_vector.begin(); i != fd_vector.end(); ++i) {
-      close(*i);
-    }
-
+  // If we received more file descriptors than caller expected, then we treat
+  // that as an error.
+  if (recv_fds.size() > (result_fd != NULL ? 1 : 0)) {
     NOTREACHED();
-
     return -1;
   }
 
-  if (result_fd)
-    *result_fd = fd_vector.empty() ? -1 : fd_vector[0];
+  if (result_fd) {

[brettw, 2014/04/28 19:53:44]

No {}

[mdempsky, 2014/04/28 20:16:58]

Done.

+    *result_fd = recv_fds.empty() ? -1 : recv_fds[0]->release();
+  }
 
   return reply_len;
 }