Capability: passing fds over IPC

FileDescriptor: passing fds over IPC

This patch introduces a FileDescriptor object which can be included in IPC messages and will perform the magic needed to pass file descriptors over IPC. After some consideration, Windows will continue to do the current DuplicateHandle tricks.

[agl, 2009-02-05 01:02:55]

jeremy: ipc_channel_posix.*
darin: everything else

[darin (slow to review), 2009-02-05 21:14:06]

one thing that bothers me with the design is the asymmetry of how DupOnRead and
DupOnWrite are implemented for windows.  DupOnRead calls DuplicateHandle in
ParamTraits::Read, but DupOnWrite calls DuplicateHandle just before writing the
IPC message onto the pipe.  It seems like it would be better if either both
happened in ParamTraits or both happened near the respective ReadFile and
WriteFile calls.

a solution that didn't rely on ParamTraits so heavily (except for syntactic
sugar) might be best.  afterall, ParamTraits is really just meant as a
convenience wrapper.

http://codereview.chromium.org/20027/diff/1017/15
File base/capability.h (right):

http://codereview.chromium.org/20027/diff/1017/15#newcode8
Line 8: #if defined(OS_WIN)
oops: using OS_WIN before build_config.h has been included.

http://codereview.chromium.org/20027/diff/1017/15#newcode17
Line 17: // base::Capability<base::DupOnWrite> looks ugly.
instead of templates, how about just doing simple subclassing to generate unique
types?  this is easy since you only declare a default ctor.

class CapabilityDupOnRead : public Capability {};
class CapabilityDupOnWrite : public Capability {};

having stuff outside of the base namespace is not nice :)

http://codereview.chromium.org/20027/diff/1017/15#newcode31
Line 31: // Windows: In a our multiprocess world, the renderer process is
sandboxed and
nit: it breaks the abstraction to talk about chrome multiprocess and sandbox
stuff in base/.  can you make this generic or move it back up to chrome/common/.
 does this really need to be in base/?

http://codereview.chromium.org/20027/diff/1017/17
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1017/17#newcode414
Line 414: reinterpret_cast<char *>(cmsg) + cmsg->cmsg_len) -
nit: normally we would indent this by 4 spaces from the start of num_write_fds. 
then you would have room to put wire_fds on the second line.

http://codereview.chromium.org/20027/diff/1017/18
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/20027/diff/1017/18#newcode78
Line 78: kMaxReadFDs = (Channel::kReadBufferSize / sizeof(IPC::Message::Header))
*
MAX_READ_FDS

i know that ipc_channel.h uses some kFooBar enums and the internal google style
guide has changed to allow kFooBar, but the google style guide on
googlecode.com, which we are using, says to use MY_EXCITING_ENUM_VALUE.

http://codereview.chromium.org/20027/diff/1017/19
File chrome/common/ipc_channel_win.cc (right):

http://codereview.chromium.org/20027/diff/1017/19#newcode372
Line 372: // DuplicateHandles will have logged an error
this comment could easily become stale if DuplicateHandles changes... perhaps
best not to write it?

http://codereview.chromium.org/20027/diff/1017/21
File chrome/common/ipc_message.cc (right):

http://codereview.chromium.org/20027/diff/1017/21#newcode9
Line 9: #include "build/build_config.h"
seems like this would need to be included above where OS_POSIX is used.

http://codereview.chromium.org/20027/diff/1017/21#newcode86
Line 86: if (rels_.size()) {
nit:  if (!rels_.empty()) {

or better yet...

if (rels_.empty())
  return;

then you can eliminate indentation for the more interesting part of the
function.

http://codereview.chromium.org/20027/diff/1017/21#newcode133
Line 133: data + rels_[j].offset));
nit: odd indentation.  how about breaking the line after the first "(" and then
indent the rest by 4 spaces?  that would be a bit more consistent with other
chrome code.

http://codereview.chromium.org/20027/diff/1017/21#newcode167
Line 167: if (next_descriptor_ < descriptors_.size()) {
nit: early return when it helps make the rest of the function body non-indented
:)

http://codereview.chromium.org/20027/diff/1017/22
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/20027/diff/1017/22#newcode14
Line 14: #include "build/build_config.h"
this include is unnecessary

http://codereview.chromium.org/20027/diff/1017/22#newcode19
Line 19: #include <base/capability.h>
this include should use double-quotes and be up with the other base/ includes

http://codereview.chromium.org/20027/diff/1017/22#newcode65
Line 65: std::vector<Relocation> rels_;
nit: relocations_ ?

http://codereview.chromium.org/20027/diff/1017/22#newcode89
Line 89: kMax = 4,
MAX_DESCRIPTORS_PER_MESSAGE

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...

http://codereview.chromium.org/20027/diff/1017/22#newcode134
Line 134: std::vector<std::pair<int, bool> > descriptors_;
nit: how about a typedef for the pair to avoid the "> >" business?

http://codereview.chromium.org/20027/diff/1017/22#newcode140
Line 140: typedef DescriptorSet CapabilitySet;
i think a capability_set.h is warranted.  it would go nicely alongside
capability.h

http://codereview.chromium.org/20027/diff/1017/22#newcode286
Line 286: CHECK(source_process_);
does this really need to be a CHECK?  wouldn't you catch any errors with a
DCHECK in a debug build?  (given how common this code path will be)

http://codereview.chromium.org/20027/diff/1017/22#newcode336
Line 336: uint32 num_fds; // the number of descriptors included with this
message
nit: looks like there was an effort made to align the comments vertically. 
would be nice to preserve that.

is num_fds needed on windows?  maybe there could be a flags value that indicates
that num_fds follows the Header?

http://codereview.chromium.org/20027/diff/1017/23
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20027/diff/1017/23#newcode13
Line 13: #include "base/capability.h"
nit: please sort headers

http://codereview.chromium.org/20027/diff/1017/23#newcode656
Line 656: m->WriteIntPtr(reinterpret_cast<intptr_t>(p.handle));
you should make use of ParamTraits<HANDLE> here instead of re-implementing it
even though it is a simple implementation.

http://codereview.chromium.org/20027/diff/1017/23#newcode666
Line 666: }
i think it would be good to put a helper function on Capability (or
CapabilityDupOnRead?) that does all of this DuplicateHandle work.

http://codereview.chromium.org/20027/diff/1017/23#newcode727
Line 727: #else  // defined(OS_WIN)
#elif defined(OS_POSIX)

that way if there is another crazy port it doesn't try to build POSIX.

http://codereview.chromium.org/20027/diff/1017/24
File chrome/common/ipc_tests.cc (right):

http://codereview.chromium.org/20027/diff/1017/24#newcode213
Line 213: base::Capability<DupOnWrite> cap;
indentation is bad

http://codereview.chromium.org/20027/diff/1017/25
File chrome/common/ipc_tests.h (right):

http://codereview.chromium.org/20027/diff/1017/25#newcode15
Line 15: TEST_CAP_CLIENT,
can you use a more descriptive name than CAP_CLIENT? TEST_CAPABILITY_CLIENT
would be ok

[Scott Hess - ex-Googler, 2009-02-05 21:14:21]

Jeremy asked me to take a look, too, since I've been working on becoming
familiar with this stuff.  Not sure if he was hoping for me to take over for him
or not :-).  But I need to head out to main campus for a bit, so here is my
first feedback.

http://codereview.chromium.org/20027/diff/1017/17
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1017/17#newcode415
Line 415: wire_fds;
Why not num_wire_fds = cmsg->cmsg_len/sizeof(int)?

http://codereview.chromium.org/20027/diff/1017/17#newcode562
Line 562: bytes_written = write(pipe_, out_bytes, amt_to_write);
Why not always use the sendmsg() case, only tucking the file descriptors in on
the appropriate pass?

One reason not to do so is because you don't want to have to break the message
into blocks.  But if my other comment about failing to send with EMSGSIZE is
possible, then you anyhow need to make sure you're taking care of that with the
iov.  Either that or this should perhaps send all the fds via sendmsg() and the
rest via a following write().

http://codereview.chromium.org/20027/diff/1017/18
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/20027/diff/1017/18#newcode80
Line 80: };
I've been trying to figure out the use-case for multiple descriptors.  Is this a
just-in-case-we-need it thing?

http://codereview.chromium.org/20027/diff/1017/18#newcode88
Line 88: std::vector<int> input_overflow_fds_;
My understanding of sendmsg includes:

"If the message is too long to pass atomically  through  the  underlying
protocol, the error EMSGSIZE is returned, and the message is not transmitted."

If this is true, then do you need to handle fd overflow?

[cpu_(ooo_6.6-7.5), 2009-02-05 21:33:27]

http://codereview.chromium.org/20027/diff/1017/19
File chrome/common/ipc_channel_win.cc (right):

http://codereview.chromium.org/20027/diff/1017/19#newcode312
Line 312: 
Danger. Do not trust the peer_pid blindly. On windows at least we know the pid
of the child process and they must match the one in the message.

It is ok to assume that a child process has not been compromised at the hello
stage but I don't see how we guard against this same message being sent again
latter on.

[agl, 2009-02-05 22:40:04]

****************************************************
*** FLUSHING COMMENT QUEUE AHEAD OF MAJOR REWORK ***
****************************************************

After talking to jam and darin, I'm going to go back to something like the first
version of this patch (i.e. remove all the Windows stuff).

http://codereview.chromium.org/20027/diff/1017/15
File base/capability.h (right):

http://codereview.chromium.org/20027/diff/1017/15#newcode8
Line 8: #if defined(OS_WIN)
On 2009/02/05 21:14:06, darin wrote:
> oops: using OS_WIN before build_config.h has been included.

Done.

http://codereview.chromium.org/20027/diff/1017/15#newcode17
Line 17: // base::Capability<base::DupOnWrite> looks ugly.
On 2009/02/05 21:14:06, darin wrote:
> having stuff outside of the base namespace is not nice :)

I agree. The main reason is so that platforms other than Windows can abstract
over DupOnRead and DupOnWrite, rather than having two, identical param traits.

Since I'm moving to chrome/common (see next comment), the issue of pollution
goes away since chrome/common isn't generally namespaced.

http://codereview.chromium.org/20027/diff/1017/15#newcode31
Line 31: // Windows: In a our multiprocess world, the renderer process is
sandboxed and
On 2009/02/05 21:14:06, darin wrote:
> nit: it breaks the abstraction to talk about chrome multiprocess and sandbox
> stuff in base/.  can you make this generic or move it back up to
chrome/common/.
>  does this really need to be in base/?

Moved to chrome/common

http://codereview.chromium.org/20027/diff/1017/17
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1017/17#newcode414
Line 414: reinterpret_cast<char *>(cmsg) + cmsg->cmsg_len) -
On 2009/02/05 21:14:06, darin wrote:
> nit: normally we would indent this by 4 spaces from the start of
num_write_fds. 
> then you would have room to put wire_fds on the second line.

Done.

http://codereview.chromium.org/20027/diff/1017/17#newcode415
Line 415: wire_fds;
On 2009/02/05 21:14:21, shess wrote:
> Why not num_wire_fds = cmsg->cmsg_len/sizeof(int)?

cmsg_len includes the control-message header. This is the same logic that strace
uses.

http://codereview.chromium.org/20027/diff/1017/17#newcode562
Line 562: bytes_written = write(pipe_, out_bytes, amt_to_write);
On 2009/02/05 21:14:21, shess wrote:
> Why not always use the sendmsg() case, only tucking the file descriptors in on
> the appropriate pass?

That's possible. I just thing that write is simpler when possible. It makes it
very clear that nothing weird is going on.

http://codereview.chromium.org/20027/diff/1017/18
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/20027/diff/1017/18#newcode78
Line 78: kMaxReadFDs = (Channel::kReadBufferSize / sizeof(IPC::Message::Header))
*
On 2009/02/05 21:14:06, darin wrote:
> MAX_READ_FDS
> 
> i know that ipc_channel.h uses some kFooBar enums and the internal google
style
> guide has changed to allow kFooBar, but the google style guide on
> googlecode.com, which we are using, says to use MY_EXCITING_ENUM_VALUE.

Done.

http://codereview.chromium.org/20027/diff/1017/18#newcode80
Line 80: };
On 2009/02/05 21:14:21, shess wrote:
> I've been trying to figure out the use-case for multiple descriptors.  Is this
a
> just-in-case-we-need it thing?

Right.

http://codereview.chromium.org/20027/diff/1017/18#newcode88
Line 88: std::vector<int> input_overflow_fds_;
On 2009/02/05 21:14:21, shess wrote:
> "If the message is too long to pass atomically  through  the  underlying
> protocol, the error EMSGSIZE is returned, and the message is not transmitted."

That's for datagram orientated sockets. Our domain sockets are SOCK_STREAM.

http://codereview.chromium.org/20027/diff/1017/19
File chrome/common/ipc_channel_win.cc (right):

http://codereview.chromium.org/20027/diff/1017/19#newcode372
Line 372: // DuplicateHandles will have logged an error
On 2009/02/05 21:14:06, darin wrote:
> this comment could easily become stale if DuplicateHandles changes... perhaps
> best not to write it?

Done.

http://codereview.chromium.org/20027/diff/1017/21
File chrome/common/ipc_message.cc (right):

http://codereview.chromium.org/20027/diff/1017/21#newcode9
Line 9: #include "build/build_config.h"
On 2009/02/05 21:14:06, darin wrote:
> seems like this would need to be included above where OS_POSIX is used.

Done.

http://codereview.chromium.org/20027/diff/1017/21#newcode86
Line 86: if (rels_.size()) {
On 2009/02/05 21:14:06, darin wrote:
> nit:  if (!rels_.empty()) {
> 
> or better yet...
> 
> if (rels_.empty())
>   return;
> 
> then you can eliminate indentation for the more interesting part of the
> function.

Done.

http://codereview.chromium.org/20027/diff/1017/21#newcode133
Line 133: data + rels_[j].offset));
On 2009/02/05 21:14:06, darin wrote:
> nit: odd indentation.  how about breaking the line after the first "(" and
then
> indent the rest by 4 spaces?  that would be a bit more consistent with other
> chrome code.

Done.

http://codereview.chromium.org/20027/diff/1017/21#newcode167
Line 167: if (next_descriptor_ < descriptors_.size()) {
On 2009/02/05 21:14:06, darin wrote:
> nit: early return when it helps make the rest of the function body
non-indented
> :)

Done.

http://codereview.chromium.org/20027/diff/1017/22
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/20027/diff/1017/22#newcode14
Line 14: #include "build/build_config.h"
On 2009/02/05 21:14:06, darin wrote:
> this include is unnecessary

Technically, yes because we got build_config via base/ports.h, via
base/basictypes.h. But I'd rather have it explicit.

http://codereview.chromium.org/20027/diff/1017/22#newcode19
Line 19: #include <base/capability.h>
On 2009/02/05 21:14:06, darin wrote:
> this include should use double-quotes and be up with the other base/ includes

Changed to double quotes. It's not needed on non-Windows platforms, but moved up
anyway.

http://codereview.chromium.org/20027/diff/1017/22#newcode65
Line 65: std::vector<Relocation> rels_;
On 2009/02/05 21:14:06, darin wrote:
> nit: relocations_ ?

Done.

http://codereview.chromium.org/20027/diff/1017/22#newcode89
Line 89: kMax = 4,
On 2009/02/05 21:14:06, darin wrote:
> MAX_DESCRIPTORS_PER_MESSAGE
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...

Done.

http://codereview.chromium.org/20027/diff/1017/22#newcode134
Line 134: std::vector<std::pair<int, bool> > descriptors_;
On 2009/02/05 21:14:06, darin wrote:
> nit: how about a typedef for the pair to avoid the "> >" business?

Typedefs in that case make things worse. Have made it a struct.

http://codereview.chromium.org/20027/diff/1017/22#newcode286
Line 286: CHECK(source_process_);
On 2009/02/05 21:14:06, darin wrote:
> does this really need to be a CHECK?  wouldn't you catch any errors with a
> DCHECK in a debug build?  (given how common this code path will be)

Done.

http://codereview.chromium.org/20027/diff/1017/22#newcode336
Line 336: uint32 num_fds; // the number of descriptors included with this
message
On 2009/02/05 21:14:06, darin wrote:
> nit: looks like there was an effort made to align the comments vertically. 
> would be nice to preserve that.

Aligned.

> is num_fds needed on windows?  maybe there could be a flags value that
indicates
> that num_fds follows the Header?

#ifdef'ed out on Windows.

http://codereview.chromium.org/20027/diff/1017/23
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20027/diff/1017/23#newcode13
Line 13: #include "base/capability.h"
On 2009/02/05 21:14:06, darin wrote:
> nit: please sort headers

Done.

http://codereview.chromium.org/20027/diff/1017/23#newcode656
Line 656: m->WriteIntPtr(reinterpret_cast<intptr_t>(p.handle));
On 2009/02/05 21:14:06, darin wrote:
> you should make use of ParamTraits<HANDLE> here instead of re-implementing it
> even though it is a simple implementation.

Done.

http://codereview.chromium.org/20027/diff/1017/23#newcode727
Line 727: #else  // defined(OS_WIN)
On 2009/02/05 21:14:06, darin wrote:
> #elif defined(OS_POSIX)
> 
> that way if there is another crazy port it doesn't try to build POSIX.

Done.

http://codereview.chromium.org/20027/diff/1017/24
File chrome/common/ipc_tests.cc (right):

http://codereview.chromium.org/20027/diff/1017/24#newcode213
Line 213: base::Capability<DupOnWrite> cap;
On 2009/02/05 21:14:06, darin wrote:
> indentation is bad

Done.

http://codereview.chromium.org/20027/diff/1017/25
File chrome/common/ipc_tests.h (right):

http://codereview.chromium.org/20027/diff/1017/25#newcode15
Line 15: TEST_CAP_CLIENT,
On 2009/02/05 21:14:06, darin wrote:
> can you use a more descriptive name than CAP_CLIENT? TEST_CAPABILITY_CLIENT
> would be ok

Done.

[Scott Hess - ex-Googler, 2009-02-05 23:47:36]

http://codereview.chromium.org/20027/diff/1017/17
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1017/17#newcode415
Line 415: wire_fds;
On 2009/02/05 22:40:04, agl wrote:
> On 2009/02/05 21:14:21, shess wrote:
> > Why not num_wire_fds = cmsg->cmsg_len/sizeof(int)?
> 
> cmsg_len includes the control-message header. This is the same logic that
strace
> uses.

Aha, I see, I was seeing CMSG_DATA() which wasn't there.

My previous understanding of the DCHECK() above was "Verify that the control
message contains an integral number of ints", meaning file descriptors.

In that case, can DCHECK() rely on %sizeof(int)==0?  Or do we need to factor out
the headers?  In practice, it probably is int-aligned, just misleading.

What about (cmsg->cmsg_len - CMSG_LEN(0))/sizeof(int)?  Gah, that's hardly
better, for all the lack of reinterpret_cast.

http://codereview.chromium.org/20027/diff/1017/17#newcode562
Line 562: bytes_written = write(pipe_, out_bytes, amt_to_write);
On 2009/02/05 22:40:04, agl wrote:
> On 2009/02/05 21:14:21, shess wrote:
> > Why not always use the sendmsg() case, only tucking the file descriptors in
on
> > the appropriate pass?
> 
> That's possible. I just thing that write is simpler when possible. It makes it
> very clear that nothing weird is going on. 

My worry is getting bugs which only happen to messages that have associated fd
payloads, which trace down to a mismatch between the write() case and the
sendmsg() case.  If all inline messages go through sendmsg() and the OOB case is
just a some specialized data on the msg, that can't hopefully could not happen.

http://codereview.chromium.org/20027/diff/1017/18
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/20027/diff/1017/18#newcode88
Line 88: std::vector<int> input_overflow_fds_;
On 2009/02/05 22:40:04, agl wrote:
> On 2009/02/05 21:14:21, shess wrote:
> > "If the message is too long to pass atomically  through  the  underlying
> > protocol, the error EMSGSIZE is returned, and the message is not
transmitted."
> 
> That's for datagram orientated sockets. Our domain sockets are SOCK_STREAM.

Using SOCK_STREAM on gdapper, I get EINVAL when trying to pass 255 file
descriptors.  Using SOCK_STREAM on MacOS X, I get EMSGSIZE from recvmsg() at 70
file descriptors (I think, could be 71 or 69).

[darin (slow to review), 2009-02-06 00:16:29]

http://codereview.chromium.org/20027/diff/1017/22
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/20027/diff/1017/22#newcode14
Line 14: #include "build/build_config.h"
we aren't explicit elsewhere about things.  in fact, it is very common to not
worry about build/build_config.h when basictypes.h is included.  what is super
typical in chrome is for only the most minimal set of includes.

[agl, 2009-02-06 00:18:24]

http://codereview.chromium.org/20027/diff/1017/17
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1017/17#newcode415
Line 415: wire_fds;
On 2009/02/05 23:47:36, shess wrote:

> What about (cmsg->cmsg_len - CMSG_LEN(0))/sizeof(int)?  Gah, that's hardly
> better, for all the lack of reinterpret_cast.

Actually, that's pretty good. You're right that that DCHECK was dodgy. I've
reworked this bit of code, hopefully now it's better.

http://codereview.chromium.org/20027/diff/1017/17#newcode562
Line 562: bytes_written = write(pipe_, out_bytes, amt_to_write);
> My worry is getting bugs which only happen to messages that have associated fd
> payloads, which trace down to a mismatch between the write() case and the
> sendmsg() case.  If all inline messages go through sendmsg() and the OOB case
is
> just a some specialized data on the msg, that can't hopefully could not
happen.

Fair enough - changed.

[agl, 2009-02-06 00:21:05]

http://codereview.chromium.org/20027/diff/1017/18
File chrome/common/ipc_channel_posix.h (right):

http://codereview.chromium.org/20027/diff/1017/18#newcode88
Line 88: std::vector<int> input_overflow_fds_;
On 2009/02/05 23:47:36, shess wrote:
> Using SOCK_STREAM on gdapper, I get EINVAL when trying to pass 255 file
> descriptors.  Using SOCK_STREAM on MacOS X, I get EMSGSIZE from recvmsg() at
70
> file descriptors (I think, could be 71 or 69).

You're confusing control messages with in-band messages. The comment on the man
page is about in-band messages on datagram sockets. That doesn't apply to
SOCK_STREAM.

You can send too much control data, as you've found out. However, we already
limit ourselves to 4 descriptors per message.

[jam, 2009-02-06 00:36:33]

lgtm (on a high level), thanks for the change.  don't forget to update the issue
description.

[darin (slow to review), 2009-02-06 06:10:04]

LGreat, but I think a file is missing...

http://codereview.chromium.org/20027/diff/1049/1051
File chrome/common/file_descriptor_posix.h (right):

http://codereview.chromium.org/20027/diff/1049/1051#newcode35
Line 35: class DescriptorSet {
the corresponding .cc file looks to be missing

http://codereview.chromium.org/20027/diff/1049/1051#newcode68
Line 68: unsigned size() const;
nit: usually unix_hacker naming convention is reserved for inline functions..
since these appear to be defined elsewhere, they probably should be MixedCase.

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode29
Line 29: #include "chrome/common/file_descriptor_posix.h"
nit: sort

http://codereview.chromium.org/20027/diff/1049/1052#newcode449
Line 449: input_overflow_fds_.resize(prev_size + num_wire_fds);
do we have to worry about attacks where a compromised renderer might be able to
make num_wire_fds be a very large number?

http://codereview.chromium.org/20027/diff/1049/1056
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20027/diff/1049/1056#newcode15
Line 15: #include "build/build_config.h"
nit: this include is unnecessary

[agl, 2009-02-06 18:41:36]

http://codereview.chromium.org/20027/diff/1049/1051
File chrome/common/file_descriptor_posix.h (right):

http://codereview.chromium.org/20027/diff/1049/1051#newcode35
Line 35: class DescriptorSet {
On 2009/02/06 06:10:04, darin wrote:
> the corresponding .cc file looks to be missing

Opps, thanks. It's just the DescriptorSet code which you've seen before.

http://codereview.chromium.org/20027/diff/1049/1051#newcode68
Line 68: unsigned size() const;
On 2009/02/06 06:10:04, darin wrote:
> nit: usually unix_hacker naming convention is reserved for inline functions..
> since these appear to be defined elsewhere, they probably should be MixedCase.

Done.

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode449
Line 449: input_overflow_fds_.resize(prev_size + num_wire_fds);
On 2009/02/06 06:10:04, darin wrote:
> do we have to worry about attacks where a compromised renderer might be able
to
> make num_wire_fds be a very large number?

num_wire_fds comes from the kernel and the kernel has to write into our
control-message buffer. We've sized it to cope with the worse-case, but it's
possible that a misbehaving renderer could send more then
MAX_DESCRIPTORS_PER_MESSAGE, per message. In this case, the kernel will just
signal that the control-message was truncated and we're safe.

Just in case, I've added code to detect the case where the renderer overflowed
us and treat it as a connection-fatal error.

http://codereview.chromium.org/20027/diff/1049/1056
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20027/diff/1049/1056#newcode15
Line 15: #include "build/build_config.h"
On 2009/02/06 06:10:04, darin wrote:
> nit: this include is unnecessary

Done.

[darin (slow to review), 2009-02-06 18:51:08]

LGTM

[jeremy, 2009-02-06 18:57:03]

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode449
Line 449: input_overflow_fds_.resize(prev_size + num_wire_fds);
These APIs make me really nervous :(

I was talking to Scott, and it seems it would be simpler to only support passing
one FD along with a message.  Do we have any use cases in which we'd one to pass
more?

[agl, 2009-02-06 19:05:08]

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode449
Line 449: input_overflow_fds_.resize(prev_size + num_wire_fds);
On 2009/02/06 18:57:03, jeremy wrote:
> These APIs make me really nervous :(
> 
> I was talking to Scott, and it seems it would be simpler to only support
passing
> one FD along with a message.  Do we have any use cases in which we'd one to
pass
> more?

Right now, I don't believe we do. However, this code wouldn't be affected by
having only one fd per message because this is the receiving side. We would
still have to cope with the case where multiple, fd carrying messages are
inflight at once and a single recvmsg reads several of them. There's also the
case where the fds for a message arrive with the header, but the rest of the
message payload might not have fit.

Both those issues remain with any limit >= 1 per message.

Are you worried about locking up OSX with too many inflight? I though a
sandboxed processes could do that itself?

If you're worried about a renderer process filling up the browsers fd table, I'm
sure there are many cases where a compromised renderer could cause DOS attacks
against the renderer. However, I could add a check that, if the payload buffer
is empty, the fd buffer must be too.

[Scott Hess - ex-Googler, 2009-02-06 19:10:53]

Sorry for new complaints - I'll try to stay on top of changes so you can get
this in today.

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode451
Line 451: num_wire_fds * sizeof(int));
What if num_wire_fds is zero, here?  [Second or later packet of message which
had fds.]  I think it works because the memcpy() gets a 0 size, but that seems
uncomfortable because wire_fds is uninitialized.

http://codereview.chromium.org/20027/diff/1049/1052#newcode502
Line 502: input_overflow_fds_.resize(num_fds - fds_i);
I don't think this is right if fds==wire_fds.  Could it just .assign() like
input_overflow_buf_?

http://codereview.chromium.org/20027/diff/1049/1052#newcode545
Line 545: DCHECK_LE(num_fds, DescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE);
Is this strong enough?  If num_fds is larger, we're going to blow past the end
of buf.

http://codereview.chromium.org/20027/diff/1049/1052#newcode547
Line 547: msgh.msg_control = buf;
Buf is scoped more locally than msgh.

http://codereview.chromium.org/20027/diff/1049/1052#newcode555
Line 555: msgh.msg_controllen = cmsg->cmsg_len;
This line seems wrong.  The first assign above seems right to me.

http://codereview.chromium.org/20027/diff/1049/1052#newcode557
Line 557: msg->header()->num_fds = num_fds;
Just want to verify my understanding of this one.  Since the header has the
payload serialized after it, this change is reflected in msg->data() and thus
out_bytes.

[agl, 2009-02-06 19:37:53]

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode451
Line 451: num_wire_fds * sizeof(int));
On 2009/02/06 19:10:53, shess wrote:
> What if num_wire_fds is zero, here?  [Second or later packet of message which
> had fds.]  I think it works because the memcpy() gets a 0 size, but that seems
> uncomfortable because wire_fds is uninitialized.

Since the length is zero, it's ok to pass random data in as the source of
destination. memcpy may not touch either buffer.

http://codereview.chromium.org/20027/diff/1049/1052#newcode502
Line 502: input_overflow_fds_.resize(num_fds - fds_i);
On 2009/02/06 19:10:53, shess wrote:
> I don't think this is right if fds==wire_fds.  Could it just .assign() like
> input_overflow_buf_?

However, I think you're correct here. I added the fds == wire_fds case later and
didn't update this. Thanks.


There's no assign member on std::vector, so I've just used an assignment.
There's probably some extra copies, but it's simpler this way.

http://codereview.chromium.org/20027/diff/1049/1052#newcode545
Line 545: DCHECK_LE(num_fds, DescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE);
On 2009/02/06 19:10:53, shess wrote:
> Is this strong enough?  If num_fds is larger, we're going to blow past the end
> of buf.

I'm not sure what your concern is here. Consider cases: num_fds is unsigned,
therefore num_fds >= 0.

buf has enough space for MAX_DESCRIPTORS_PER_MESSAGE ints. GetDescriptors will
write size()*sizeof(int) bytes into CMSG_DATA(buf), thus this check is correct,
no?

http://codereview.chromium.org/20027/diff/1049/1052#newcode547
Line 547: msgh.msg_control = buf;
On 2009/02/06 19:10:53, shess wrote:
> Buf is scoped more locally than msgh.
> 

You're right, this went wrong when I moved the code. Fixed.

http://codereview.chromium.org/20027/diff/1049/1052#newcode555
Line 555: msgh.msg_controllen = cmsg->cmsg_len;
On 2009/02/06 19:10:53, shess wrote:
> This line seems wrong.  The first assign above seems right to me.

This is the recommended code from the manpage.

http://codereview.chromium.org/20027/diff/1049/1052#newcode557
Line 557: msg->header()->num_fds = num_fds;
On 2009/02/06 19:10:53, shess wrote:
> Just want to verify my understanding of this one.  Since the header has the
> payload serialized after it, this change is reflected in msg->data() and thus
> out_bytes.

Correct.

[Scott Hess - ex-Googler, 2009-02-06 22:05:16]

LGTM

You seem to have some additional files in there sneaking in from some other CL:
  base/waitable_event_watcher.h
  chrome/browser/renderer_host/test_render_view_host.h
  chrome/chrome.xcodeproj/project.pbxproj
  chrome/common/common.scons
  webkit/tools/layout_tests/test_lists/tests_fixable.txt
  
Well, I can see needing an xcodeproj change somewhere to catch the additional
.cc file, but not that particular one.

http://codereview.chromium.org/20027/diff/1049/1052
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/20027/diff/1049/1052#newcode545
Line 545: DCHECK_LE(num_fds, DescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE);
On 2009/02/06 19:37:54, agl wrote:
> On 2009/02/06 19:10:53, shess wrote:
> > Is this strong enough?  If num_fds is larger, we're going to blow past the
end
> > of buf.
> 
> I'm not sure what your concern is here. Consider cases: num_fds is unsigned,
> therefore num_fds >= 0.
> 
> buf has enough space for MAX_DESCRIPTORS_PER_MESSAGE ints. GetDescriptors will
> write size()*sizeof(int) bytes into CMSG_DATA(buf), thus this check is
correct,
> no?

The number of descriptors in Add() isn't enforced.  So in release mode you can
add more than MAX_DESCRIPTORS_PER_MESSAGE fds to the descriptor set, then
GetDescriptors() will stuff num_fds fds into buf, even if it's larger.  Am I
seeing something that is impossible to happen?

http://codereview.chromium.org/20027/diff/1049/1052#newcode555
Line 555: msgh.msg_controllen = cmsg->cmsg_len;
On 2009/02/06 19:37:54, agl wrote:
> On 2009/02/06 19:10:53, shess wrote:
> > This line seems wrong.  The first assign above seems right to me.
> 
> This is the recommended code from the manpage.

I see that the man page also sets msg_controllen twice.  Hmm.  As best I can
tell it's a difference of alignment, but in the example the difference flows the
wrong way (I would expect CMSG_SPACE(x)<=CMSG_LEN(x)).  Well, hard to fight the
man page.