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

base/posix/unix_domain_socket_linux.cc

Index: base/posix/unix_domain_socket_linux.cc
diff --git a/base/posix/unix_domain_socket_linux.cc b/base/posix/unix_domain_socket_linux.cc
index 8bfc8cea7fcf32e5910225e04a62350ccfeb4b82..8f736474882fe2dce715829a88a20fae42a7e028 100644
--- a/base/posix/unix_domain_socket_linux.cc
+++ b/base/posix/unix_domain_socket_linux.cc
@@ -9,7 +9,11 @@
 #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"
@@ -63,7 +67,7 @@ bool UnixDomainSocket::SendMsg(int fd,
 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);
 }
 
@@ -71,7 +75,7 @@ ssize_t UnixDomainSocket::RecvMsg(int fd,
 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);
 }
@@ -81,7 +85,7 @@ 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();
 
@@ -131,8 +135,9 @@ ssize_t UnixDomainSocket::RecvMsgWithFlags(int fd,
   }
 
   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) {
@@ -161,44 +166,44 @@ ssize_t UnixDomainSocket::SendRecvMsgWithFlags(int fd,
                                                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;
 }