Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1051243002:
Non-SFI: move socketpair() from plugin process to browser process. (Closed)
DescriptionNon-SFI: move socketpair() from plugin process to browser process.
Currently, socket pairs for the IPC channels (mainly for PPAPI IPC channels) are created in nacl_helper (and nacl_helper_nonsfi).
However, to make more secure, socketpair() will be prohibited in Non-SFI mode. So, this is its preparation.
TEST=Ran bots.
BUG=358417
Committed: https://crrev.com/7f43c10288a54b68bf4ac3a6656618d7b14c7bec
Cr-Commit-Position: refs/heads/master@{#330330}
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : Rebase & reimplement #
Total comments: 22
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 4
Patch Set 6 : Rebase #Patch Set 7 : #
Messages
Total messages: 27 (10 generated)
hidehiko@chromium.org changed reviewers: + mseaborn@chromium.org
Hi Mark, Could you take a look? Thanks, - hidehiko
On 2015/04/12 14:04:22, hidehiko wrote: > Hi Mark, > > Could you take a look? > > Thanks, > - hidehiko Friendly ping?
https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:826: if (params.enable_ipc_proxy && uses_nonsfi_mode_) { Nit: At the moment, uses_nonsfi_mode_ implies enable_ipc_proxy, I think. To avoid making this conditional look more tested/supported than it is, you could do: if (uses_nonsfi_mode_) { DCHECK(params.enable_ipc_proxy); ... } https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:829: !IPC::SocketPair(¶ms.renderer_server_ppapi_fd.fd, If the subsequent IPC::SocketPair() calls fail, won't this leak FDs, since the base::FileDescriptor type used in NaClStartParams doesn't have a destructor? Leaking FDs might be a pre-existing problem in this code path, but if we're creating 8 more FDs here it could be more likely to fail due to the FD table filling up. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:838: params.browser_client_ppapi_fd.auto_close = true; Notice how the NaCl loader process is receiving these client FDs and then immediately sending them back to the browser process via NaClProcessHostMsg_PpapiChannelsCreated. You could avoid that IPC round trip by calling OnPpapiChannelsCreated() here, in StartNaClExecution(). Then you only need to send 4 FDs instead of 8. That ought to reduce startup time a little, and reduce the time during which the renderer is blocked. It would also simplify the logic of the code. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/common/... components/nacl/common/nacl_types.h:109: FileDescriptor browser_server_ppapi_fd; base::FileDescriptor doesn't have a destructor... Does base::File work in Chrome IPC messages these days? If so, that would be more robust about freeing FDs. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:96: // we create raw FD pairs, and pass the client side FDs to the hosts, Update comment? We're not creating the raw FD pairs here any more. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:121: if (!Send(new NaClProcessHostMsg_PpapiChannelsCreated( Notice that you're receiving the client FDs from the browser process and immediately sending them back to the browser process. (See other comment...)
https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:829: !IPC::SocketPair(¶ms.renderer_server_ppapi_fd.fd, On 2015/04/15 20:52:05, Mark Seaborn wrote: > If the subsequent IPC::SocketPair() calls fail, won't this leak FDs, since the > base::FileDescriptor type used in NaClStartParams doesn't have a destructor? > > Leaking FDs might be a pre-existing problem in this code path, but if we're > creating 8 more FDs here it could be more likely to fail due to the FD table > filling up. I guess you're trying to clean up that pre-existing problem above (in https://codereview.chromium.org/1117883002/) before committing this change? Thanks for doing that. However, I'm not sure you should block this change on doing that cleanup, because the cleanup is rather difficult. It's not clear exactly how we should handle failures like these. For comparison, Chromium has a policy to abort on memory allocation failure (at least for fine-grained allocations): https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/yJUd6gKCYSE... And in a previous discussion it has been suggested that there should be a similar policy for out-of-FDs: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/lFgKOYo3iYM... So arguably it might be not too bad to crash or leak some FDs if one of these IPC::SocketPair() calls fails. This particular part is gated on uses_nonsfi_mode_, so it's not exposed to the web. Maybe you could just locally free the first socketpairs if a later IPC::SocketPair() call fails, using a scoped-something? https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:121: if (!Send(new NaClProcessHostMsg_PpapiChannelsCreated( On 2015/04/15 20:52:05, Mark Seaborn wrote: > Notice that you're receiving the client FDs from the browser process and > immediately sending them back to the browser process. (See other comment...) This was the main feedback I had. Maybe you could do this simplification and we can get this change committed?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
PTAL. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:826: if (params.enable_ipc_proxy && uses_nonsfi_mode_) { On 2015/04/15 20:52:05, Mark Seaborn wrote: > Nit: At the moment, uses_nonsfi_mode_ implies enable_ipc_proxy, I think. > > To avoid making this conditional look more tested/supported than it is, you > could do: > if (uses_nonsfi_mode_) { > DCHECK(params.enable_ipc_proxy); > ... > } Done. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:829: !IPC::SocketPair(¶ms.renderer_server_ppapi_fd.fd, On 2015/05/12 06:46:10, Mark Seaborn wrote: > On 2015/04/15 20:52:05, Mark Seaborn wrote: > > If the subsequent IPC::SocketPair() calls fail, won't this leak FDs, since the > > base::FileDescriptor type used in NaClStartParams doesn't have a destructor? > > > > Leaking FDs might be a pre-existing problem in this code path, but if we're > > creating 8 more FDs here it could be more likely to fail due to the FD table > > filling up. > > I guess you're trying to clean up that pre-existing problem above (in > https://codereview.chromium.org/1117883002/) before committing this change? > Thanks for doing that. However, I'm not sure you should block this change on > doing that cleanup, because the cleanup is rather difficult. It's not clear > exactly how we should handle failures like these. > > For comparison, Chromium has a policy to abort on memory allocation failure (at > least for fine-grained allocations): > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/yJUd6gKCYSE... > > And in a previous discussion it has been suggested that there should be a > similar policy for out-of-FDs: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/lFgKOYo3iYM... > > So arguably it might be not too bad to crash or leak some FDs if one of these > IPC::SocketPair() calls fails. > > This particular part is gated on uses_nonsfi_mode_, so it's not exposed to the > web. > > Maybe you could just locally free the first socketpairs if a later > IPC::SocketPair() call fails, using a scoped-something? Ok, done. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:838: params.browser_client_ppapi_fd.auto_close = true; On 2015/04/15 20:52:05, Mark Seaborn wrote: > Notice how the NaCl loader process is receiving these client FDs and then > immediately sending them back to the browser process via > NaClProcessHostMsg_PpapiChannelsCreated. > > You could avoid that IPC round trip by calling OnPpapiChannelsCreated() here, in > StartNaClExecution(). Then you only need to send 4 FDs instead of 8. That > ought to reduce startup time a little, and reduce the time during which the > renderer is blocked. It would also simplify the logic of the code. Done. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/common/... components/nacl/common/nacl_types.h:109: FileDescriptor browser_server_ppapi_fd; Ack. On 2015/04/15 20:52:05, Mark Seaborn wrote: > base::FileDescriptor doesn't have a destructor... > > Does base::File work in Chrome IPC messages these days? If so, that would be > more robust about freeing FDs. No. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:96: // we create raw FD pairs, and pass the client side FDs to the hosts, On 2015/04/15 20:52:05, Mark Seaborn wrote: > Update comment? We're not creating the raw FD pairs here any more. Done. https://codereview.chromium.org/1051243002/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:121: if (!Send(new NaClProcessHostMsg_PpapiChannelsCreated( On 2015/05/12 06:46:10, Mark Seaborn wrote: > On 2015/04/15 20:52:05, Mark Seaborn wrote: > > Notice that you're receiving the client FDs from the browser process and > > immediately sending them back to the browser process. (See other comment...) > > This was the main feedback I had. Maybe you could do this simplification and we > can get this change committed? Done.
LGTM, thanks https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (left): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:677: // IPC messages relating to NaCl's validation cache must not be exposed Please keep this comment, and add it after "// In Non-SFI mode, no message is expected." https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:871: if (!params.enable_ipc_proxy) { This is basically input validation of what the renderer passed to the browser process. It would be better to do this earlier, in Launch(). https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1001: // In Non-SFI mode, create socket pairs for IPC channels, here, unlike in Grammar nit: could remove comma before "here" https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1006: // Note: here, because some resources for the plugin process are already Nit: "plugin process" -> "NaCl loader process", x2 Also "resources" -> "FDs" to be more specific https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1007: // opened, they must be sent to plugin process because these cannot be "cannot be closed in this process" could be misleading for someone who doesn't understand the background. (They *could* be closed on Unix, but making the same logic work on Windows adds further complication.) A better explanation might be something like "...are transferred to NaCl loader process even if an error occurs first, because this is the simplest way to ensure that these FDs don't get leaked and that the NaCl loader process will exit properly". https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1011: // The case this condition is not satisfied should be handled in Nit: "The case where this condition..." perhaps? Or you could say: "Note that this check is redundant; we check this earlier". https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1015: ScopedChannelHandle ppapi_browser_server_channel_handle; I wonder if it would it be worth doing: typedef std::pair<ScopedChannelHandle, ScopedChannelHandle> ChannelHandlePair; bool CreateChannelHandlePair(ChannelHandlePair* pair); That would make this a bit less repetitive. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1044: // and, transfer the channel handles for the plugin process to |params|. Nit: remove comma after "and" Also "plugin process" -> "NaCl loader process" However, isn't this comment just echoing the code below? You could omit it. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.h:175: // Starts browser PPAPI proxy. Returns true on success. Otherwise false. Nit: You could drop "Otherwise false" for consistency with the comment above. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... components/nacl/common/nacl_messages.h:24: #endif // defined(OS_MACOSX) Nit: These comments are not necessary for such short #if blocks. IMHO they make the code noisier. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... components/nacl/common/nacl_types.h:87: // in plugin process. Thus, the browser process creates the ChannelHandle Nit: "plugin process" -> "NaCl loader process" again
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
hidehiko@chromium.org changed reviewers: + jln@chromium.org
R+=jln@. Julien, could you take a look at *_message.h" file as an OWNER? Thanks, - hidehiko https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (left): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:677: // IPC messages relating to NaCl's validation cache must not be exposed On 2015/05/12 23:24:06, Mark Seaborn wrote: > Please keep this comment, and add it after "// In Non-SFI mode, no message is > expected." Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:871: if (!params.enable_ipc_proxy) { On 2015/05/12 23:24:06, Mark Seaborn wrote: > This is basically input validation of what the renderer passed to the browser > process. It would be better to do this earlier, in Launch(). Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1001: // In Non-SFI mode, create socket pairs for IPC channels, here, unlike in On 2015/05/12 23:24:06, Mark Seaborn wrote: > Grammar nit: could remove comma before "here" Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1006: // Note: here, because some resources for the plugin process are already On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: "plugin process" -> "NaCl loader process", x2 > > Also "resources" -> "FDs" to be more specific Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1007: // opened, they must be sent to plugin process because these cannot be On 2015/05/12 23:24:06, Mark Seaborn wrote: > "cannot be closed in this process" could be misleading for someone who doesn't > understand the background. (They *could* be closed on Unix, but making the same > logic work on Windows adds further complication.) > > A better explanation might be something like "...are transferred to NaCl loader > process even if an error occurs first, because this is the simplest way to > ensure that these FDs don't get leaked and that the NaCl loader process will > exit properly". Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1011: // The case this condition is not satisfied should be handled in On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: "The case where this condition..." perhaps? Or you could say: "Note that > this check is redundant; we check this earlier". Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1015: ScopedChannelHandle ppapi_browser_server_channel_handle; On 2015/05/12 23:24:06, Mark Seaborn wrote: > I wonder if it would it be worth doing: > > typedef std::pair<ScopedChannelHandle, ScopedChannelHandle> ChannelHandlePair; > bool CreateChannelHandlePair(ChannelHandlePair* pair); > > That would make this a bit less repetitive. I do not want to introduce such a pair. Indeed it removes 8 lines from here, but instead we need to distribute .first and .second. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:1044: // and, transfer the channel handles for the plugin process to |params|. On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: remove comma after "and" > > Also "plugin process" -> "NaCl loader process" > > However, isn't this comment just echoing the code below? You could omit it. Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/browse... components/nacl/browser/nacl_process_host.h:175: // Starts browser PPAPI proxy. Returns true on success. Otherwise false. On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: You could drop "Otherwise false" for consistency with the comment above. Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... components/nacl/common/nacl_messages.h:24: #endif // defined(OS_MACOSX) On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: These comments are not necessary for such short #if blocks. IMHO they make > the code noisier. Done. https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/120001/components/nacl/common... components/nacl/common/nacl_types.h:87: // in plugin process. Thus, the browser process creates the ChannelHandle On 2015/05/12 23:24:06, Mark Seaborn wrote: > Nit: "plugin process" -> "NaCl loader process" again Done.
https://codereview.chromium.org/1051243002/diff/180001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:879: DCHECK(params.enable_ipc_proxy); Nit: This isn't needed now, since you check this before and after, and this function doesn't deal with anything PPAPI-proxy-related. https://codereview.chromium.org/1051243002/diff/180001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/common... components/nacl/common/nacl_messages.h:33: #endif // defined(OS_LINUX) || defined(OS_NACL_NONSFI) Nit: As before, you don't need this comment on such a small #if block. https://codereview.chromium.org/1051243002/diff/180001/components/nacl/loader... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_listener.cc:99: CHECK_NE(params.ppapi_browser_channel_handle.socket.fd, -1); One other thing occurred to me. The error handling when sending NaClProcessMsg_Start is rather subtle, so: How about putting these 4 new FD CHECK_NEs together, with a comment explaining that these can fail if the preparations for sending NaClProcessMsg_Start were incomplete.
https://codereview.chromium.org/1051243002/diff/180001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:879: DCHECK(params.enable_ipc_proxy); On 2015/05/13 06:51:11, Mark Seaborn wrote: > Nit: This isn't needed now, since you check this before and after, and this > function doesn't deal with anything PPAPI-proxy-related. Done. https://codereview.chromium.org/1051243002/diff/180001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/common... components/nacl/common/nacl_messages.h:33: #endif // defined(OS_LINUX) || defined(OS_NACL_NONSFI) On 2015/05/13 06:51:11, Mark Seaborn wrote: > Nit: As before, you don't need this comment on such a small #if block. Oh, you meant here, too. Done. https://codereview.chromium.org/1051243002/diff/180001/components/nacl/loader... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/1051243002/diff/180001/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_listener.cc:99: CHECK_NE(params.ppapi_browser_channel_handle.socket.fd, -1); On 2015/05/13 06:51:11, Mark Seaborn wrote: > One other thing occurred to me. The error handling when sending > NaClProcessMsg_Start is rather subtle, so: > > How about putting these 4 new FD CHECK_NEs together, with a comment explaining > that these can fail if the preparations for sending NaClProcessMsg_Start were > incomplete. Done.
@hidehiko: One more commenting nit... https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... components/nacl/common/nacl_messages.h:128: IPC_MESSAGE_CONTROL4(NaClProcessHostMsg_PpapiChannelsCreated, Similarly, can you add a comment: "Used for SFI mode only. Non-SFI mode passes channel handles in NaClStartParams instead." https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... components/nacl/common/nacl_types.h:85: // These are for Non-SFI mode IPC channels. Can you add: "SFI mode uses NaClProcessHostMsg_PpapiChannelsCreated instead."
@jln: I reviewed the IPC message changes for security. The message contents are fine. The error handling is tricky, though, so I'd appreciate your thoughts on this. Here's the problem: These days, Chromium has base::File for wrapping an FD or Windows handle, and its destructor auto-closes the FD/handle. However, if we have an FD/handle that's scheduled to be sent in an IPC message -- an IPC::PlatformFileForTransit -- there's no equivalent class for auto-closing this if our preparations for sending the IPC message fail. (Such an FD/handle is returned by IPC::GetFileHandleForProcess().) On Unix, close()ing a PlatformFileForTransit FD is fine. But on Windows, doing CloseHandle() on a PlatformFileForTransit handle is dangerously wrong, because this is a handle in the destination process's handle table. The strategy that Hidehiko has implemented is to send a partially-filled-out IPC message if an error occurs, and to have the receiver handle this (by exiting, in this case). This seems reasonable given the abstractions that base/ and ipc/ currently provide, but it seems brittle. Some later cleanup might be worthwhile, especially if other users of IPC in Chromium are affected by this. Maybe we should raise this on chromium-dev or security-dev?
jln@, friendly ping? On 2015/05/13 17:17:47, Mark Seaborn wrote: > @jln: I reviewed the IPC message changes for security. The message contents are > fine. > > The error handling is tricky, though, so I'd appreciate your thoughts on this. > > Here's the problem: These days, Chromium has base::File for wrapping an FD or > Windows handle, and its destructor auto-closes the FD/handle. > > However, if we have an FD/handle that's scheduled to be sent in an IPC message > -- an IPC::PlatformFileForTransit -- there's no equivalent class for > auto-closing this if our preparations for sending the IPC message fail. (Such > an FD/handle is returned by IPC::GetFileHandleForProcess().) > > On Unix, close()ing a PlatformFileForTransit FD is fine. But on Windows, doing > CloseHandle() on a PlatformFileForTransit handle is dangerously wrong, because > this is a handle in the destination process's handle table. > > The strategy that Hidehiko has implemented is to send a partially-filled-out IPC > message if an error occurs, and to have the receiver handle this (by exiting, in > this case). > > This seems reasonable given the abstractions that base/ and ipc/ currently > provide, but it seems brittle. Some later cleanup might be worthwhile, > especially if other users of IPC in Chromium are affected by this. Maybe we > should raise this on chromium-dev or security-dev?
On 2015/05/15 12:53:42, hidehiko wrote: > jln@, friendly ping? Thanks hidehiko for the well thought CL and Mark for the very careful review and explanation. This is tricky. I don't think I should hold you up any further though, so lgtm: this change seems fine and we should discuss if we could build something more robust outside of this review. I've cc-ed tsepez@ who may have some thoughts on this. I'm wondering how worried we are of closing remote file handles, given that I don't think that anything in the remote process could possibly be using these handles.
Thank you for review. Submitting. For father improvement, +1 with jln@. Let's discuss in another thread. https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... components/nacl/common/nacl_messages.h:128: IPC_MESSAGE_CONTROL4(NaClProcessHostMsg_PpapiChannelsCreated, On 2015/05/13 17:15:39, Mark Seaborn wrote: > Similarly, can you add a comment: "Used for SFI mode only. Non-SFI mode passes > channel handles in NaClStartParams instead." Done. https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1051243002/diff/200001/components/nacl/common... components/nacl/common/nacl_types.h:85: // These are for Non-SFI mode IPC channels. On 2015/05/13 17:15:39, Mark Seaborn wrote: > Can you add: "SFI mode uses NaClProcessHostMsg_PpapiChannelsCreated instead." Done.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org, jln@chromium.org Link to the patchset: https://codereview.chromium.org/1051243002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051243002/240001
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7f43c10288a54b68bf4ac3a6656618d7b14c7bec Cr-Commit-Position: refs/heads/master@{#330330} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
