Alternative workaround for mac kernel bug.

BUG=298276

Review URL: https://codereview.chromium.org/25325002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@227999 0039d316-1c4b-4281-b951-d872f2087c98

ipc/file_descriptor_set_posix.cc

@@ -126,6 +126,16 @@ void FileDescriptorSet::CommitAll() {
   consumed_descriptor_highwater_ = 0;
 }
 
+void FileDescriptorSet::ReleaseFDsToClose(std::vector<int>* fds) {
+  for (std::vector<base::FileDescriptor>::iterator
+       i = descriptors_.begin(); i != descriptors_.end(); ++i) {
+    if (i->auto_close)
+      fds->push_back(i->fd);
+  }
+  descriptors_.clear();
+  consumed_descriptor_highwater_ = 0;
+}
+
 void FileDescriptorSet::SetDescriptors(const int* buffer, unsigned count) {
   DCHECK(count <= kMaxDescriptorsPerMessage);
   DCHECK_EQ(descriptors_.size(), 0u);

ipc/file_descriptor_set_posix.h

@@ -77,6 +77,9 @@ class IPC_EXPORT FileDescriptorSet
   // Returns true if any contained file descriptors appear to be handles to a
   // directory.
   bool ContainsDirectoryDescriptor() const;
+  // Fetch all filedescriptors with the "auto close" property.
+  // Used instead of CommitAll() when closing must be handled manually.
+  void ReleaseFDsToClose(std::vector<int>* fds);
 
   // ---------------------------------------------------------------------------
 

ipc/ipc_channel.h

@@ -75,15 +75,22 @@ class IPC_EXPORT Channel : public Sender {
 #endif
   };
 
-  // The Hello message is internal to the Channel class.  It is sent
-  // by the peer when the channel is connected.  The message contains
-  // just the process id (pid).  The message has a special routing_id
-  // (MSG_ROUTING_NONE) and type (HELLO_MESSAGE_TYPE).
+  // Messages internal to the IPC implementation are defined here.
+  // Uses Maximum value of message type (uint16), to avoid conflicting
+  // with normal message types, which are enumeration constants starting from 0.
   enum {
-    HELLO_MESSAGE_TYPE = kuint16max  // Maximum value of message type (uint16),
-                                     // to avoid conflicting with normal
-                                     // message types, which are enumeration
-                                     // constants starting from 0.
+    // The Hello message is sent by the peer when the channel is connected.
+    // The message contains just the process id (pid).
+    // The message has a special routing_id (MSG_ROUTING_NONE)
+    // and type (HELLO_MESSAGE_TYPE).
+    HELLO_MESSAGE_TYPE = kuint16max,
+    // The CLOSE_FD_MESSAGE_TYPE is used in the IPC class to
+    // work around a bug in sendmsg() on Mac. When an FD is sent
+    // over the socket, a CLOSE_FD_MESSAGE is sent with hops = 2.
+    // The client will return the message with hops = 1, *after* it
+    // has received the message that contains the FD. When we
+    // receive it again on the sender side, we close the FD.
+    CLOSE_FD_MESSAGE_TYPE = HELLO_MESSAGE_TYPE - 1
   };
 
   // The maximum message size in bytes. Attempting to receive a message of this

ipc/ipc_channel_nacl.cc

@@ -344,7 +344,7 @@ bool Channel::ChannelImpl::DidEmptyInputBuffers() {
   return input_fds_.empty();
 }
 
-void Channel::ChannelImpl::HandleHelloMessage(const Message& msg) {
+void Channel::ChannelImpl::HandleInternalMessage(const Message& msg) {
   // The trusted side IPC::Channel should handle the "hello" handshake; we
   // should not receive the "Hello" message.
   NOTREACHED();

ipc/ipc_channel_nacl.h

@@ -60,7 +60,7 @@ class Channel::ChannelImpl : public internal::ChannelReader {
                              int* bytes_read) OVERRIDE;
   virtual bool WillDispatchInputMessage(Message* msg) OVERRIDE;
   virtual bool DidEmptyInputBuffers() OVERRIDE;
-  virtual void HandleHelloMessage(const Message& msg) OVERRIDE;
+  virtual void HandleInternalMessage(const Message& msg) OVERRIDE;
 
   Mode mode_;
   bool waiting_connect_;

ipc/ipc_channel_posix.cc

@@ -347,6 +347,27 @@ bool Channel::ChannelImpl::Connect() {
   return did_connect;
 }
 
+void Channel::ChannelImpl::CloseFileDescriptors(Message* msg) {
+#if defined(OS_MACOSX)
+  // There is a bug on OSX which makes it dangerous to close
+  // a file descriptor while it is in transit. So instead we
+  // store the file descriptor in a set and send a message to
+  // the recipient, which is queued AFTER the message that
+  // sent the FD. The recipient will reply to the message,
+  // letting us know that it is now safe to close the file
+  // descriptor. For more information, see:
+  // http://crbug.com/298276
+  std::vector<int> to_close;
+  msg->file_descriptor_set()->ReleaseFDsToClose(&to_close);
+  for (size_t i = 0; i < to_close.size(); i++) {
+    fds_to_close_.insert(to_close[i]);
+    QueueCloseFDMessage(to_close[i], 2);
+  }
+#else
+  msg->file_descriptor_set()->CommitAll();
+#endif
+}
+
 bool Channel::ChannelImpl::ProcessOutgoingMessages() {
   DCHECK(!waiting_connect_);  // Why are we trying to send messages if there's
                               // no connection?
@@ -419,7 +440,7 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages() {
         msgh.msg_iov = &iov;
         msgh.msg_controllen = 0;
         if (bytes_written > 0) {
-          msg->file_descriptor_set()->CommitAll();
+          CloseFileDescriptors(msg);
         }
       }
 #endif  // IPC_USES_READWRITE
@@ -440,7 +461,7 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages() {
       }
     }
     if (bytes_written > 0)
-      msg->file_descriptor_set()->CommitAll();
+      CloseFileDescriptors(msg);
 
     if (bytes_written < 0 && !SocketWriteErrorIsRecoverable()) {
 #if defined(OS_MACOSX)
@@ -575,6 +596,17 @@ void Channel::ChannelImpl::ResetToAcceptingConnectionState() {
 
   // Close any outstanding, received file descriptors.
   ClearInputFDs();
+
+#if defined(OS_MACOSX)
+  // Clear any outstanding, sent file descriptors.
+  for (std::set<int>::iterator i = fds_to_close_.begin();
+       i != fds_to_close_.end();
+       ++i) {
+    if (HANDLE_EINTR(close(*i)) < 0)
+      PLOG(ERROR) << "close";
+  }
+  fds_to_close_.clear();
+#endif
 }
 
 // static
@@ -592,7 +624,6 @@ void Channel::ChannelImpl::SetGlobalPid(int pid) {
 
 // Called by libevent when we can read from the pipe without blocking.
 void Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int fd) {
-  bool send_server_hello_msg = false;
   if (fd == server_listen_pipe_) {
     int new_pipe = 0;
     if (!ServerAcceptConnection(server_listen_pipe_, &new_pipe) ||
@@ -631,18 +662,16 @@ void Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int fd) {
     if (!AcceptConnection()) {
       NOTREACHED() << "AcceptConnection should not fail on server";
     }
-    send_server_hello_msg = true;
     waiting_connect_ = false;
   } else if (fd == pipe_) {
     if (waiting_connect_ && (mode_ & MODE_SERVER_FLAG)) {
-      send_server_hello_msg = true;
       waiting_connect_ = false;
     }
     if (!ProcessIncomingMessages()) {
       // ClosePipeOnError may delete this object, so we mustn't call
       // ProcessOutgoingMessages.
-      send_server_hello_msg = false;
       ClosePipeOnError();
+      return;
     }
   } else {
     NOTREACHED() << "Unknown pipe " << fd;
@@ -651,8 +680,8 @@ void Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int fd) {
   // If we're a server and handshaking, then we want to make sure that we
   // only send our handshake message after we've processed the client's.
   // This gives us a chance to kill the client if the incoming handshake
-  // is invalid.
-  if (send_server_hello_msg) {
+  // is invalid. This also flushes any closefd messagse.
+  if (!is_blocked_on_write_) {
     ProcessOutgoingMessages();
   }
 }
@@ -902,29 +931,80 @@ void Channel::ChannelImpl::ClearInputFDs() {
   input_fds_.clear();
 }
 
-void Channel::ChannelImpl::HandleHelloMessage(const Message& msg) {
+void Channel::ChannelImpl::QueueCloseFDMessage(int fd, int hops) {
+  switch (hops) {
+    case 1:
+    case 2: {
+      // Create the message
+      scoped_ptr<Message> msg(new Message(MSG_ROUTING_NONE,
+                                          CLOSE_FD_MESSAGE_TYPE,
+                                          IPC::Message::PRIORITY_NORMAL));
+      if (!msg->WriteInt(hops - 1) || !msg->WriteInt(fd)) {
+        NOTREACHED() << "Unable to pickle close fd.";
+      }
+      // Send(msg.release());
+      output_queue_.push(msg.release());
+      break;
+    }
+
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+void Channel::ChannelImpl::HandleInternalMessage(const Message& msg) {
   // The Hello message contains only the process id.
   PickleIterator iter(msg);
-  int pid;
-  if (!msg.ReadInt(&iter, &pid))
-    NOTREACHED();
 
-#if defined(IPC_USES_READWRITE)
-  if (mode_ & MODE_SERVER_FLAG) {
-    // With IPC_USES_READWRITE, the Hello message from the client to the
-    // server also contains the fd_pipe_, which  will be used for all
-    // subsequent file descriptor passing.
-    DCHECK_EQ(msg.file_descriptor_set()->size(), 1U);
-    base::FileDescriptor descriptor;
-    if (!msg.ReadFileDescriptor(&iter, &descriptor)) {
+  switch (msg.type()) {
+    default:
       NOTREACHED();
-    }
-    fd_pipe_ = descriptor.fd;
-    CHECK(descriptor.auto_close);
-  }
+      break;
+
+    case Channel::HELLO_MESSAGE_TYPE:
+      int pid;
+      if (!msg.ReadInt(&iter, &pid))
+        NOTREACHED();
+
+#if defined(IPC_USES_READWRITE)
+      if (mode_ & MODE_SERVER_FLAG) {
+        // With IPC_USES_READWRITE, the Hello message from the client to the
+        // server also contains the fd_pipe_, which  will be used for all
+        // subsequent file descriptor passing.
+        DCHECK_EQ(msg.file_descriptor_set()->size(), 1U);
+        base::FileDescriptor descriptor;
+        if (!msg.ReadFileDescriptor(&iter, &descriptor)) {
+          NOTREACHED();
+        }
+        fd_pipe_ = descriptor.fd;
+        CHECK(descriptor.auto_close);
+      }
 #endif  // IPC_USES_READWRITE
-  peer_pid_ = pid;
-  listener()->OnChannelConnected(pid);
+      peer_pid_ = pid;
+      listener()->OnChannelConnected(pid);
+      break;
+
+#if defined(OS_MACOSX)
+    case Channel::CLOSE_FD_MESSAGE_TYPE:
+      int fd, hops;
+      if (!msg.ReadInt(&iter, &hops))
+        NOTREACHED();
+      if (!msg.ReadInt(&iter, &fd))
+        NOTREACHED();
+      if (hops == 0) {
+        if (fds_to_close_.erase(fd) > 0) {
+          if (HANDLE_EINTR(close(fd)) < 0)
+            PLOG(ERROR) << "close";
+        } else {
+          NOTREACHED();
+        }
+      } else {
+        QueueCloseFDMessage(fd, hops);
+      }
+      break;
+#endif
+  }
 }
 
 void Channel::ChannelImpl::Close() {

ipc/ipc_channel_posix.h

@@ -10,6 +10,7 @@
 #include <sys/socket.h>  // for CMSG macros
 
 #include <queue>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -80,14 +81,16 @@ class Channel::ChannelImpl : public internal::ChannelReader,
   void ClosePipeOnError();
   int GetHelloMessageProcId();
   void QueueHelloMessage();
+  void CloseFileDescriptors(Message* msg);
+  void QueueCloseFDMessage(int fd, int hops);
 
   // ChannelReader implementation.
   virtual ReadState ReadData(char* buffer,
                              int buffer_len,
                              int* bytes_read) OVERRIDE;
   virtual bool WillDispatchInputMessage(Message* msg) OVERRIDE;
   virtual bool DidEmptyInputBuffers() OVERRIDE;
-  virtual void HandleHelloMessage(const Message& msg) OVERRIDE;
+  virtual void HandleInternalMessage(const Message& msg) OVERRIDE;
 
 #if defined(IPC_USES_READWRITE)
   // Reads the next message from the fd_pipe_ and appends them to the
@@ -184,6 +187,13 @@ class Channel::ChannelImpl : public internal::ChannelReader,
   // implementation!
   std::vector<int> input_fds_;
 
+#if defined(OS_MACOSX)
+  // On OSX, sent FDs must not be closed until we get an ack.
+  // Keep track of sent FDs here to make sure the remote is not
+  // trying to bamboozle us.
+  std::set<int> fds_to_close_;
+#endif
+
   // True if we are responsible for unlinking the unix domain socket file.
   bool must_unlink_;
 

ipc/ipc_channel_reader.cc

@@ -38,9 +38,15 @@ bool ChannelReader::AsyncReadComplete(int bytes_read) {
   return DispatchInputData(input_buf_, bytes_read);
 }
 
+bool ChannelReader::IsInternalMessage(const Message& m) const {
+  return m.routing_id() == MSG_ROUTING_NONE &&
+      m.type() >= Channel::CLOSE_FD_MESSAGE_TYPE &&
+      m.type() <= Channel::HELLO_MESSAGE_TYPE;
+}
+
 bool ChannelReader::IsHelloMessage(const Message& m) const {
   return m.routing_id() == MSG_ROUTING_NONE &&
-         m.type() == Channel::HELLO_MESSAGE_TYPE;
+      m.type() == Channel::HELLO_MESSAGE_TYPE;
 }
 
 bool ChannelReader::DispatchInputData(const char* input_data,
@@ -84,8 +90,8 @@ bool ChannelReader::DispatchInputData(const char* input_data,
                    "line", IPC_MESSAGE_ID_LINE(m.type()));
 #endif
       m.TraceMessageEnd();
-      if (IsHelloMessage(m))
-        HandleHelloMessage(m);
+      if (IsInternalMessage(m))
+        HandleInternalMessage(m);
       else
         listener_->OnMessageReceived(m);
       p = message_tail;

ipc/ipc_channel_reader.h

@@ -42,8 +42,12 @@ class ChannelReader {
   // data. See ReadData for more.
   bool AsyncReadComplete(int bytes_read);
 
-  // Returns true if the given message is the "hello" message sent on channel
-  // set-up.
+  // Returns true if the given message is internal to the IPC implementation,
+  // like the "hello" message sent on channel set-up.
+  bool IsInternalMessage(const Message& m) const;
+
+  // Returns true if the given message is an Hello message
+  // sent on channel set-up.
   bool IsHelloMessage(const Message& m) const;
 
  protected:
@@ -76,8 +80,8 @@ class ChannelReader {
   // though there could be more data ready to be read from the OS.
   virtual bool DidEmptyInputBuffers() = 0;
 
-  // Handles the first message sent over the pipe which contains setup info.
-  virtual void HandleHelloMessage(const Message& msg) = 0;
+  // Handles internal messages, like the hello message sent on channel startup.
+  virtual void HandleInternalMessage(const Message& msg) = 0;
 
  private:
   // Takes the given data received from the IPC channel and dispatches any

ipc/ipc_channel_win.cc

@@ -152,7 +152,8 @@ bool Channel::ChannelImpl::WillDispatchInputMessage(Message* msg) {
   return true;
 }
 
-void Channel::ChannelImpl::HandleHelloMessage(const Message& msg) {
+void Channel::ChannelImpl::HandleInternalMessage(const Message& msg) {
+  DCHECK_EQ(msg.type(), static_cast<unsigned>(Channel::HELLO_MESSAGE_TYPE));
   // The hello message contains one parameter containing the PID.
   PickleIterator it(msg);
   int32 claimed_pid;

ipc/ipc_channel_win.h

@@ -41,7 +41,7 @@ class Channel::ChannelImpl : public internal::ChannelReader,
                              int* bytes_read) OVERRIDE;
   virtual bool WillDispatchInputMessage(Message* msg) OVERRIDE;
   bool DidEmptyInputBuffers() OVERRIDE;
-  virtual void HandleHelloMessage(const Message& msg) OVERRIDE;
+  virtual void HandleInternalMessage(const Message& msg) OVERRIDE;
 
   static const string16 PipeName(const std::string& channel_id,
                                  int32* secret);