Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issue 1120343002: Make IPC::Channel buffers stack based and secure against growth (Closed)

Created:
5 years, 7 months ago by Daniel Bratell
Modified:
5 years, 6 months ago
Reviewers:
Tom Sepez, Mark Seaborn, jam
CC:
chromium-reviews, darin-cc_chromium.org, jam, Andrew Hayden (chromium.org), aurimas (slooooooooow), Mark Seaborn
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Synchronized Mac with the rest #

Patch Set 3 : Optimistic! 8 KB will work. #

Total comments: 1

Patch Set 4 : Just the cleanup left. #

Patch Set 5 : Mac fix still needed. Despite it being 6 years later. #

Patch Set 6 : Rebased to newer master #

Total comments: 1

Patch Set 7 : Moved static_assert #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -11 lines) Patch
M ipc/ipc_channel_posix.h View 1 2 3 4 5 6 1 chunk +4 lines, -8 lines 0 comments Download
M ipc/ipc_channel_posix.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 26 (6 generated)
Daniel Bratell
5 years, 7 months ago (2015-05-04 13:41:31 UTC) #1
jam
switching reviewers to tsepez
5 years, 7 months ago (2015-05-04 16:03:20 UTC) #3
Tom Sepez
Good idea, but 100K stack buffers aren't desirable either. Let's get them from the heap ...
5 years, 7 months ago (2015-05-04 16:15:48 UTC) #4
Daniel Bratell
On 2015/05/04 16:15:48, Tom Sepez wrote: > Good idea, but 100K stack buffers aren't desirable ...
5 years, 7 months ago (2015-05-04 16:21:09 UTC) #5
Tom Sepez
> There is another aspect of this and that is the size: 100 KB. Is ...
5 years, 7 months ago (2015-05-04 16:30:31 UTC) #6
Tom Sepez
+agl, who reviewed IPC on the following. The issue is that https://codereview.chromium.org/998833002/ changed kMaxDescriptorsPerMessage from ...
5 years, 7 months ago (2015-05-04 17:05:48 UTC) #7
agl
On 2015/05/04 17:05:48, Tom Sepez wrote: > The issue is that https://codereview.chromium.org/998833002/ changed > kMaxDescriptorsPerMessage ...
5 years, 7 months ago (2015-05-04 17:11:39 UTC) #8
Tom Sepez
> 128 fds * 4 bytes per fd = 512 bytes, no? Where does 128K ...
5 years, 7 months ago (2015-05-04 17:15:10 UTC) #9
Daniel Bratell
I've uploaded a new version of the patch that changes the Mac number to be ...
5 years, 7 months ago (2015-05-05 15:24:51 UTC) #10
Tom Sepez
Daniel, first let me say thanks for noticing this. This is going to kill mobile, ...
5 years, 7 months ago (2015-05-05 15:32:39 UTC) #11
Tom Sepez
Sounds reasonable to me. We should probably give a heads-up as to what a crash ...
5 years, 7 months ago (2015-05-06 16:48:40 UTC) #12
Mark Seaborn
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#newcode185 ipc/ipc_channel_posix.h:185: // of KB. Instead we note that we will ...
5 years, 7 months ago (2015-05-06 17:50:16 UTC) #14
Tom Sepez
> It seems less risky to revert back to kMaxDescriptorsPerMessage = 7, as proposed > ...
5 years, 7 months ago (2015-05-06 22:33:25 UTC) #15
Daniel Bratell
The symptoms of a too small buffer seems to be that things don't work (with ...
5 years, 7 months ago (2015-05-07 07:24:16 UTC) #16
Tom Sepez
How many KB would we be saving under the current proposal?
5 years, 7 months ago (2015-05-20 16:38:50 UTC) #19
Daniel Bratell
On 2015/05/20 16:38:50, Tom Sepez wrote: > How many KB would we be saving under ...
5 years, 6 months ago (2015-05-28 15:32:17 UTC) #20
Tom Sepez
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.cc#newcode756 ipc/ipc_channel_posix.cc:756: static_assert(kMaxReadFDBuffer <= 8192, I believe ...
5 years, 6 months ago (2015-05-28 15:57:46 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120343002/160001
5 years, 6 months ago (2015-05-29 09:51:37 UTC) #24
commit-bot: I haz the power
Committed patchset #7 (id:160001)
5 years, 6 months ago (2015-05-29 13:19:08 UTC) #25
commit-bot: I haz the power
5 years, 6 months ago (2015-05-29 13:20:06 UTC) #26
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/5937d45677732c5fe9be1ea4d442e4e1ca61c23b
Cr-Commit-Position: refs/heads/master@{#331956}

Powered by Google App Engine
This is Rietveld 408576698