Make IPC::Channel buffers stack based and secure against growth

Make IPC::Channel buffers stack based and secure against growth

Auxiliary IPC::Channel buffers have been permanently allocated
even though their use is very temporary. This moves those to
the stack instead to reflect their temporary nature and also
adds an assert to catch accidental out-of-control growth of
the buffer as happened recently.

BUG=484154
R=tsepez@chromium.org

Committed: https://crrev.com/5937d45677732c5fe9be1ea4d442e4e1ca61c23b
Cr-Commit-Position: refs/heads/master@{#331956}

[jam, 2015-05-04 16:03:20]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org

[jam, 2015-05-04 16:03:20]

switching reviewers to tsepez

[Tom Sepez, 2015-05-04 16:15:48]

Good idea, but 100K stack buffers aren't desirable either.  Let's get them from
the heap as needed.

[Daniel Bratell, 2015-05-04 16:21:09]

On 2015/05/04 16:15:48, Tom Sepez wrote:
> Good idea, but 100K stack buffers aren't desirable either.  Let's get them
from
> the heap as needed.

There is another aspect of this and that is the size: 100 KB. Is that really
necessary or is there a miscalculation? In Mac the buffers are 1024 bytes (see
the lines in ipc_channel_posix.h right above the removed code) which is much
less and small enough not to worry too much. I do not really know anything about
this code so I cannot instinctively 
tell what is reasonable or not.

[Tom Sepez, 2015-05-04 16:30:31]

> There is another aspect of this and that is the size: 100 KB. Is that really
> necessary or is there a miscalculation? In Mac the buffers are 1024 bytes (see
> the lines in ipc_channel_posix.h right above the removed code) which is much
> less and small enough not to worry too much. I do not really know anything
about
> this code so I cannot instinctively 
> tell what is reasonable or not.
Let me take a look.  One other thing to note is that this is used on every read,
so getting them from the heap won't fly either.

[Tom Sepez, 2015-05-04 17:05:48]

+agl, who reviewed IPC on the following.

The issue is that https://codereview.chromium.org/998833002/ changed
kMaxDescriptorsPerMessage from 7 to 128, thereby raising the amount of memory
buffered per channel on posix from around 7K to 128K. Interestingly, it didn't
adjust the MAC OS number (which might prevent the new code from working there).

Can we get a compromise on this number?

[agl, 2015-05-04 17:11:39]

On 2015/05/04 17:05:48, Tom Sepez wrote:
> The issue is that https://codereview.chromium.org/998833002/ changed
> kMaxDescriptorsPerMessage from 7 to 128, thereby raising the amount of memory
> buffered per channel on posix from around 7K to 128K. Interestingly, it didn't
> adjust the MAC OS number (which might prevent the new code from working
there).

128 fds * 4 bytes per fd = 512 bytes, no? Where does 128K come from?

[Tom Sepez, 2015-05-04 17:15:10]

> 128 fds * 4 bytes per fd = 512 bytes, no? Where does 128K come from?

You might get more than one message per kReadBufferSize, no?
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po...

  // We assume a worst case: kReadBufferSize bytes of messages, where each
  // message has no payload and a full complement of descriptors.
  static const size_t kMaxReadFDs =
      (Channel::kReadBufferSize / sizeof(IPC::Message::Header)) *
      MessageAttachmentSet::kMaxDescriptorsPerMessage;

so 4096 / 16 * 128 == 32768

  // Buffer size for file descriptors used for recvmsg. On Mac the CMSG macros
  // don't seem to be constant so we have to pick a "large enough" value.
#if defined(OS_MACOSX)
  static const size_t kMaxReadFDBuffer = 1024;
#else
  static const size_t kMaxReadFDBuffer = CMSG_SPACE(sizeof(int) * kMaxReadFDs);
#endif

so 32768 * 4 is passed to 

 char input_cmsg_buf_[kMaxReadFDBuffer];

[Daniel Bratell, 2015-05-05 15:24:51]

I've uploaded a new version of the patch that changes the Mac number to be
similar to the rest, and no other changes.

In practice it seems 1024 has been "good enough" since Mac products keep
working. The buffer size calculated here is extremely pessimistic but worst case
really seems to be 102 KB of file descriptors and I cannot see any way to
determine a smaller buffer as long as the constants are what they are. If
anything the constants are likely to grow over time making this even worse.

There seems to be roughly 8 Channel objects for a single renderer (four
server/client pairs for renderer and gpu processes) so total memory lost seems
to be around 1 MB for a single renderer scenario (content_shell).

Personally I think I could accept a 100 KB stack object, assuming that few other
things will do anything similar at the same time. A common stack limit is 1 MB
so it would be 10% of the stack which is a lot, but still far from all of it and
very temporary. If it could be made smaller (how?) that would be better though.

[Tom Sepez, 2015-05-05 15:32:39]

Daniel, first let me say thanks for noticing this.  This is going to kill
mobile, where swapping is disabled, and the memory stays resident forever,
particularly given the memset() in the ctor.  If we can't reduce the max
descriptors, we probably want to go an optimistic solution and give it something
like 8k and hope for the best.

[Tom Sepez, 2015-05-06 16:48:40]

Sounds reasonable to me.  We should probably give a heads-up as to what a crash
caused by this would look like.  Just for grins, what does the stack look like
if we run this with an absurdly small value to induce a crash?

LGTM otherwise.

[Mark Seaborn, 2015-05-06 17:50:16]

mseaborn@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2015-05-06 17:50:16]

https://codereview.chromium.org/1120343002/diff/40001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/1120343002/diff/40001/ipc/ipc_channel_posix.h...
ipc/ipc_channel_posix.h:185: // of KB. Instead we note that we will only send
file descriptors
This assumption seems rather risky to me, because it breaks composability: 
Sending many FDs in one IPC message will work, and sending many IPC messages
will work, but doing both together (with many FDs in each message) can fail.

What's worse is that whether this works depends on process scheduling.  If the
receiving process's recvmsg() call gets scheduled when a few messages have been
queued up in the socket's buffer, it will work, but if the recvmsg() call gets
scheduled later when more messages have been queued up, it will fail.

It seems less risky to revert back to kMaxDescriptorsPerMessage = 7, as proposed
in https://codereview.chromium.org/1127913002/.

[Tom Sepez, 2015-05-06 22:33:25]

> It seems less risky to revert back to kMaxDescriptorsPerMessage = 7, as
proposed
> in https://codereview.chromium.org/1127913002/.
Right. NOT LGTM. This is not a comment on the fine work by the author, but that
the change has already been reverted, so this patch shouldn't land.

[Daniel Bratell, 2015-05-07 07:24:16]

The symptoms of a too small buffer seems to be that things don't work (with a 0
size byte buffer I couldn't load a single page) and that you get these warnings:

[5480:5492:0507/091149:7829271164858:WARNING:ipc_channel_posix.cc(921)] Message
needs unreceived descriptors channel:0x166b71352600 message-type:65535
header()->num_fds:1
[5480:5492:0507/091149:7829271210426:WARNING:ipc_channel_posix.cc(921)] Message
needs unreceived descriptors channel:0x166b7134f000 message-type:65535
header()->num_fds:1
[5480:5492:0507/091149:7829271220130:WARNING:ipc_message_attachment_set.cc(37)]
MessageAttachmentSet destroyed with unconsumed descriptors: 0/1

Note that an 8 bytes buffer seems enough to load pages in content_shell.

So with the size increase reverted, what do you think about making the buffers
stack allocated in general? They are temporary buffers, size limited and size
checked, so it seems unnecessary to have any kind of such buffer permanently
allocated regardless of size. The only reason I can think of is that it
separates the buffer from the stack which makes it harder to exploit bugs in
buffer management.

[Daniel Bratell, 2015-05-07 07:48:43]

Patchset #4 (id:60001) has been deleted

[Daniel Bratell, 2015-05-07 08:00:22]

Patchset #5 (id:100001) has been deleted

[Tom Sepez, 2015-05-20 16:38:50]

How many KB would we be saving under the current proposal?

[Daniel Bratell, 2015-05-28 15:32:17]

On 2015/05/20 16:38:50, Tom Sepez wrote:
> How many KB would we be saving under the current proposal?

It's no longer primarily a patch to save memory (I've updated the description),
but the savings would be 5728 bytes per active channel with the current
constants. Was there 8 active channels at startup? In that case about 40 KB.

That 5728 makes the previous mac channel of 1024 bytes a bit suspicious too.

But please take a look and see if you like that is still there.

[Tom Sepez, 2015-05-28 15:57:46]

See comment, otherwise LGTM.

https://codereview.chromium.org/1120343002/diff/140001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/1120343002/diff/140001/ipc/ipc_channel_posix....
ipc/ipc_channel_posix.cc:756: static_assert(kMaxReadFDBuffer <= 8192,
I believe that a static_assert can appear inside a class body, so this can move
to the .h, right after the declaration of kMaxReadFDBuffer Member.

[Daniel Bratell, 2015-05-29 09:51:14]

The CQ bit was checked by bratell@opera.com

[Daniel Bratell, 2015-05-29 09:51:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1120343002/#ps160001
(title: "Moved static_assert")

[commit-bot: I haz the power, 2015-05-29 09:51:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120343002/160001

[commit-bot: I haz the power, 2015-05-29 13:19:08]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-05-29 13:20:06]

Patchset 7 (id:??) landed as
https://crrev.com/5937d45677732c5fe9be1ea4d442e4e1ca61c23b
Cr-Commit-Position: refs/heads/master@{#331956}