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

Issue 20027: Capability: passing fds over IPC (Closed)

Created:
11 years, 10 months ago by agl
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : Changes after speaking with Darin #

Patch Set 3 : ... #

Total comments: 62

Patch Set 4 : Removing windows stuff #

Patch Set 5 : Typo fixes #

Patch Set 6 : shess's comments #

Patch Set 7 : Remove build/build_config.h #

Total comments: 25

Patch Set 8 : darin's comments #

Patch Set 9 : typo fix #

Patch Set 10 : ... #

Patch Set 11 : ... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+484 lines, -16 lines) Patch
M base/waitable_event_watcher.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_host/test_render_view_host.h View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/chrome.xcodeproj/project.pbxproj View 2 chunks +0 lines, -2 lines 0 comments Download
M chrome/common/common.scons View 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
A chrome/common/file_descriptor_posix.h View 4 5 6 7 1 chunk +115 lines, -0 lines 0 comments Download
A chrome/common/file_descriptor_posix.cc View 1 chunk +82 lines, -0 lines 0 comments Download
M chrome/common/ipc_channel_posix.h View 1 2 3 4 2 chunks +15 lines, -0 lines 0 comments Download
M chrome/common/ipc_channel_posix.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +117 lines, -4 lines 0 comments Download
M chrome/common/ipc_message.h View 1 2 3 4 5 6 4 chunks +17 lines, -3 lines 0 comments Download
M chrome/common/ipc_message.cc View 1 2 3 3 chunks +7 lines, -0 lines 0 comments Download
M chrome/common/ipc_message_utils.h View 1 2 3 4 5 6 7 2 chunks +32 lines, -0 lines 0 comments Download
M chrome/common/ipc_tests.h View 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/ipc_tests.cc View 2 3 4 chunks +92 lines, -0 lines 0 comments Download
M chrome/test/unit/unit_tests.scons View 1 chunk +1 line, -0 lines 0 comments Download
M webkit/tools/layout_tests/test_lists/tests_fixable.txt View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18 (0 generated)
agl
jeremy: ipc_channel_posix.* darin: everything else
11 years, 10 months ago (2009-02-05 01:02:55 UTC) #1
darin (slow to review)
one thing that bothers me with the design is the asymmetry of how DupOnRead and ...
11 years, 10 months ago (2009-02-05 21:14:06 UTC) #2
Scott Hess - ex-Googler
Jeremy asked me to take a look, too, since I've been working on becoming familiar ...
11 years, 10 months ago (2009-02-05 21:14:21 UTC) #3
cpu_(ooo_6.6-7.5)
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 ...
11 years, 10 months ago (2009-02-05 21:33:27 UTC) #4
agl
**************************************************** *** FLUSHING COMMENT QUEUE AHEAD OF MAJOR REWORK *** **************************************************** After talking to jam ...
11 years, 10 months ago (2009-02-05 22:40:04 UTC) #5
Scott Hess - ex-Googler
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 ...
11 years, 10 months ago (2009-02-05 23:47:36 UTC) #6
darin (slow to review)
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. ...
11 years, 10 months ago (2009-02-06 00:16:29 UTC) #7
agl
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 ...
11 years, 10 months ago (2009-02-06 00:18:24 UTC) #8
agl
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: > ...
11 years, 10 months ago (2009-02-06 00:21:05 UTC) #9
jam
lgtm (on a high level), thanks for the change. don't forget to update the issue ...
11 years, 10 months ago (2009-02-06 00:36:33 UTC) #10
darin (slow to review)
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: ...
11 years, 10 months ago (2009-02-06 06:10:04 UTC) #11
agl
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: ...
11 years, 10 months ago (2009-02-06 18:41:36 UTC) #12
darin (slow to review)
LGTM
11 years, 10 months ago (2009-02-06 18:51:08 UTC) #13
jeremy
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 ...
11 years, 10 months ago (2009-02-06 18:57:03 UTC) #14
agl
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: ...
11 years, 10 months ago (2009-02-06 19:05:08 UTC) #15
Scott Hess - ex-Googler
Sorry for new complaints - I'll try to stay on top of changes so you ...
11 years, 10 months ago (2009-02-06 19:10:53 UTC) #16
agl
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: ...
11 years, 10 months ago (2009-02-06 19:37:53 UTC) #17
Scott Hess - ex-Googler
11 years, 10 months ago (2009-02-06 22:05:16 UTC) #18
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.

Powered by Google App Engine
This is Rietveld 408576698