Skip to content

Commit

Permalink
ipc: remove IPC_USES_READWRITE
Browse files Browse the repository at this point in the history 
The Linux sandbox performance implications that made using
recvmsg()/sendmsg() slower than read()/write() have not applied for a
long time, since we switched from seccomp (v1) to seccomp-bpf.

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

Cr-Commit-Position: refs/heads/master@{#330414}
  • Loading branch information
@mdempsky
mdempsky authored and Commit bot committed May 18, 2015
1 parent c806c71 commit ab00835
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 165 deletions.
128 changes: 5 additions & 123 deletions ipc/ipc_channel_posix.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,19 +247,6 @@ bool ChannelPosix::CreatePipe(
if (channel_handle.socket.fd != -1) {
// Case 1 from comment above.
local_pipe.reset(channel_handle.socket.fd);
#if defined(IPC_USES_READWRITE)
// Test the socket passed into us to make sure it is nonblocking.
// We don't want to call read/write on a blocking socket.
int value = fcntl(local_pipe.get(), F_GETFL);
if (value == -1) {
PLOG(ERROR) << "fcntl(F_GETFL) " << pipe_name_;
return false;
}
if (!(value & O_NONBLOCK)) {
LOG(ERROR) << "Socket " << pipe_name_ << " must be O_NONBLOCK";
return false;
}
#endif // IPC_USES_READWRITE
} else if (mode_ & MODE_NAMED_FLAG) {
#if defined(OS_NACL_NONSFI)
LOG(FATAL)
Expand Down Expand Up @@ -333,20 +320,6 @@ bool ChannelPosix::CreatePipe(
}
}

#if defined(IPC_USES_READWRITE)
// Create a dedicated socketpair() for exchanging file descriptors.
// See comments for IPC_USES_READWRITE for details.
if (mode_ & MODE_CLIENT_FLAG) {
int fd_pipe_fd = 1, remote_fd_pipe_fd = -1;
if (!SocketPair(&fd_pipe_fd, &remote_fd_pipe_fd)) {
return false;
}

fd_pipe_.reset(fd_pipe_fd);
remote_fd_pipe_.reset(remote_fd_pipe_fd);
}
#endif // IPC_USES_READWRITE

if ((mode_ & MODE_SERVER_FLAG) && (mode_ & MODE_NAMED_FLAG)) {
#if defined(OS_NACL_NONSFI)
LOG(FATAL) << "IPC channels in nacl_helper_nonsfi "
Expand Down Expand Up @@ -466,40 +439,11 @@ bool ChannelPosix::ProcessOutgoingMessages() {
// DCHECK_LE above already checks that
// num_fds < kMaxDescriptorsPerMessage so no danger of overflow.
msg->header()->num_fds = static_cast<uint16>(num_fds);

#if defined(IPC_USES_READWRITE)
if (!IsHelloMessage(*msg)) {
// Only the Hello message sends the file descriptor with the message.
// Subsequently, we can send file descriptors on the dedicated
// fd_pipe_ which makes Seccomp sandbox operation more efficient.
struct iovec fd_pipe_iov = { const_cast<char *>(""), 1 };
msgh.msg_iov = &fd_pipe_iov;
fd_written = fd_pipe_.get();
bytes_written =
HANDLE_EINTR(sendmsg(fd_pipe_.get(), &msgh, MSG_DONTWAIT));
msgh.msg_iov = &iov;
msgh.msg_controllen = 0;
if (bytes_written > 0) {
CloseFileDescriptors(msg);
}
}
#endif // IPC_USES_READWRITE
}

if (bytes_written == 1) {
fd_written = pipe_.get();
#if defined(IPC_USES_READWRITE)
if ((mode_ & MODE_CLIENT_FLAG) && IsHelloMessage(*msg)) {
DCHECK_EQ(msg->attachment_set()->size(), 1U);
}
if (!msgh.msg_controllen) {
bytes_written =
HANDLE_EINTR(write(pipe_.get(), out_bytes, amt_to_write));
} else
#endif // IPC_USES_READWRITE
{
bytes_written = HANDLE_EINTR(sendmsg(pipe_.get(), &msgh, MSG_DONTWAIT));
}
bytes_written = HANDLE_EINTR(sendmsg(pipe_.get(), &msgh, MSG_DONTWAIT));
}
if (bytes_written > 0)
CloseFileDescriptors(msg);
Expand Down Expand Up @@ -616,10 +560,6 @@ void ChannelPosix::ResetToAcceptingConnectionState() {
read_watcher_.StopWatchingFileDescriptor();
write_watcher_.StopWatchingFileDescriptor();
ResetSafely(&pipe_);
#if defined(IPC_USES_READWRITE)
fd_pipe_.reset();
remote_fd_pipe_.reset();
#endif // IPC_USES_READWRITE

while (!output_queue_.empty()) {
Message* m = output_queue_.front();
Expand Down Expand Up @@ -798,16 +738,6 @@ void ChannelPosix::QueueHelloMessage() {
if (!msg->WriteInt(GetHelloMessageProcId())) {
NOTREACHED() << "Unable to pickle hello message proc id";
}
#if defined(IPC_USES_READWRITE)
scoped_ptr<Message> hello;
if (remote_fd_pipe_.is_valid()) {
if (!msg->WriteAttachment(
new internal::PlatformFileAttachment(remote_fd_pipe_.get()))) {
NOTREACHED() << "Unable to pickle hello message file descriptors";
}
DCHECK_EQ(msg->attachment_set()->size(), 1U);
}
#endif // IPC_USES_READWRITE
output_queue_.push(msg.release());
}

Expand All @@ -828,16 +758,9 @@ ChannelPosix::ReadState ChannelPosix::ReadData(

// recvmsg() returns 0 if the connection has closed or EAGAIN if no data
// is waiting on the pipe.
#if defined(IPC_USES_READWRITE)
if (fd_pipe_.is_valid()) {
*bytes_read = HANDLE_EINTR(read(pipe_.get(), buffer, buffer_len));
msg.msg_controllen = 0;
} else
#endif // IPC_USES_READWRITE
{
msg.msg_controllen = sizeof(input_cmsg_buf_);
*bytes_read = HANDLE_EINTR(recvmsg(pipe_.get(), &msg, MSG_DONTWAIT));
}
msg.msg_controllen = sizeof(input_cmsg_buf_);
*bytes_read = HANDLE_EINTR(recvmsg(pipe_.get(), &msg, MSG_DONTWAIT));

if (*bytes_read < 0) {
if (errno == EAGAIN) {
return READ_PENDING;
Expand Down Expand Up @@ -868,28 +791,6 @@ ChannelPosix::ReadState ChannelPosix::ReadData(
return READ_SUCCEEDED;
}

#if defined(IPC_USES_READWRITE)
bool ChannelPosix::ReadFileDescriptorsFromFDPipe() {
char dummy;
struct iovec fd_pipe_iov = { &dummy, 1 };

struct msghdr msg = { 0 };
msg.msg_iov = &fd_pipe_iov;
msg.msg_iovlen = 1;
msg.msg_control = input_cmsg_buf_;
msg.msg_controllen = sizeof(input_cmsg_buf_);
ssize_t bytes_received =
HANDLE_EINTR(recvmsg(fd_pipe_.get(), &msg, MSG_DONTWAIT));

if (bytes_received != 1)
return true; // No message waiting.

if (!ExtractFileDescriptorsFromMsghdr(&msg))
return false;
return true;
}
#endif

// On Posix, we need to fix up the file descriptors before the input message
// is dispatched.
//
Expand All @@ -905,12 +806,7 @@ bool ChannelPosix::WillDispatchInputMessage(Message* msg) {
if (header_fds > input_fds_.size()) {
// The message has been completely received, but we didn't get
// enough file descriptors.
#if defined(IPC_USES_READWRITE)
if (!ReadFileDescriptorsFromFDPipe())
return false;
if (header_fds > input_fds_.size())
#endif // IPC_USES_READWRITE
error = "Message needs unreceived descriptors";
error = "Message needs unreceived descriptors";
}

if (header_fds > MessageAttachmentSet::kMaxDescriptorsPerMessage)
Expand Down Expand Up @@ -1017,19 +913,6 @@ void ChannelPosix::HandleInternalMessage(const Message& msg) {
if (!iter.ReadInt(&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.attachment_set()->size(), 1U);
scoped_refptr<MessageAttachment> attachment;
if (!msg.ReadAttachment(&iter, &attachment)) {
NOTREACHED();
}
fd_pipe_.reset(attachment->TakePlatformFile());
}
#endif // IPC_USES_READWRITE
peer_pid_ = pid;
listener()->OnChannelConnected(pid);
break;
Expand Down Expand Up @@ -1129,7 +1012,6 @@ std::string Channel::GenerateVerifiedChannelID(const std::string& prefix) {
return id.append(GenerateUniqueRandomChannelID());
}


bool Channel::IsNamedServerInitialized(
const std::string& channel_id) {
return ChannelPosix::IsNamedServerInitialized(channel_id);
Expand Down
42 changes: 0 additions & 42 deletions ipc/ipc_channel_posix.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,34 +20,6 @@
#include "ipc/ipc_channel_reader.h"
#include "ipc/ipc_message_attachment_set.h"

#if !defined(OS_MACOSX)
// On Linux, the seccomp sandbox makes it very expensive to call
// recvmsg() and sendmsg(). The restriction on calling read() and write(), which
// are cheap, is that we can't pass file descriptors over them.
//
// As we cannot anticipate when the sender will provide us with file
// descriptors, we have to make the decision about whether we call read() or
// recvmsg() before we actually make the call. The easiest option is to
// create a dedicated socketpair() for exchanging file descriptors. Any file
// descriptors are split out of a message, with the non-file-descriptor payload
// going over the normal connection, and the file descriptors being sent
// separately over the other channel. When read()ing from a channel, we'll
// notice if the message was supposed to have come with file descriptors and
// use recvmsg on the other socketpair to retrieve them and combine them
// back with the rest of the message.
//
// Mac can also run in IPC_USES_READWRITE mode if necessary, but at this time
// doesn't take a performance hit from recvmsg and sendmsg, so it doesn't
// make sense to waste resources on having the separate dedicated socketpair.
// It is however useful for debugging between Linux and Mac to be able to turn
// this switch 'on' on the Mac as well.
//
// The HELLO message from the client to the server is always sent using
// sendmsg because it will contain the file descriptor that the server
// needs to send file descriptors in later messages.
#define IPC_USES_READWRITE 1
#endif

namespace IPC {

class IPC_EXPORT ChannelPosix : public Channel,
Expand Down Expand Up @@ -107,14 +79,6 @@ class IPC_EXPORT ChannelPosix : public Channel,
bool DidEmptyInputBuffers() override;
void HandleInternalMessage(const Message& msg) override;

#if defined(IPC_USES_READWRITE)
// Reads the next message from the fd_pipe_ and appends them to the
// input_fds_ queue. Returns false if there was a message receiving error.
// True means there was a message and it was processed properly, or there was
// no messages.
bool ReadFileDescriptorsFromFDPipe();
#endif

// Finds the set of file descriptors in the given message. On success,
// appends the descriptors to the input_fds_ member and returns true
//
Expand Down Expand Up @@ -161,12 +125,6 @@ class IPC_EXPORT ChannelPosix : public Channel,
base::ScopedFD client_pipe_;
mutable base::Lock client_pipe_lock_; // Lock that protects |client_pipe_|.

#if defined(IPC_USES_READWRITE)
// Linux/BSD use a dedicated socketpair() for passing file descriptors.
base::ScopedFD fd_pipe_;
base::ScopedFD remote_fd_pipe_;
#endif

// The "name" of our pipe. On Windows this is the global identifier for
// the pipe. On POSIX it's used as a key in a local map of file descriptors.
std::string pipe_name_;
Expand Down

0 comments on commit ab00835

Please sign in to comment.