Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512733003:
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC (Closed)
|
Created:
5 years ago by Mark Seaborn Modified:
4 years, 11 months ago CC:
chromium-reviews, Derek Schuff Base URL:
http://git.chromium.org/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
Committed: https://crrev.com/3bedcdc7ad86115836141f91ffd7fa331d07e2a1
Cr-Commit-Position: refs/heads/master@{#367369}
Patch Set 1 #Patch Set 2 : Working version #Patch Set 3 : Attempt to fix on Mac #Patch Set 4 : Attempt to fix Mac #2 #Patch Set 5 : Add debugging #Patch Set 6 : Fix message rewriting on Mac/Windows #Patch Set 7 : Cleanup #Patch Set 8 : Rebase + cleanup #
Total comments: 27
Patch Set 9 : Review #
Total comments: 4
Patch Set 10 : Review #
Total comments: 1
Patch Set 11 : Rebase #Messages
Total messages: 42 (18 generated)
Description was changed from ========== PNaCl: Use Chrome IPC for linking instead of SRPC ========== to ========== PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC ==========
The CQ bit was checked by mseaborn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512733003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512733003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mseaborn@chromium.org to run a CQ dry run
Description was changed from
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
==========
to
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512733003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512733003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
to
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
mseaborn@chromium.org changed reviewers: + bbudge@chromium.org
A few comments. I am OOO this afternoon. https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... File components/nacl/renderer/plugin/temporary_file.cc (right): https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... components/nacl/renderer/plugin/temporary_file.cc:67: PP_FileHandle TempFile::GetFileHandle() { DCHECK(file_handle_.IsValid()); ? https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... File components/nacl/renderer/plugin/temporary_file.h (right): https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... components/nacl/renderer/plugin/temporary_file.h:62: // This handle remain valid until the TempFile object is destroyed or nit: remains https://codereview.chromium.org/1512733003/diff/140001/ppapi/nacl_irt/irt_pna... File ppapi/nacl_irt/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/nacl_irt/irt_pna... ppapi/nacl_irt/irt_pnacl_translator_link.cc:25: CallbackFunc func): func_(func) { formatting seems strange here, before the initializer colon. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... File ppapi/proxy/nacl_message_scanner.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:199: bool ScanSyncMessage(ScanningResults* results) { David Michael has implemented this in a patch we haven't landed yet: https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag... https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:214: bool ScanSyncReply(ScanningResults* results) { ScanReply? https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:874: // needs to use the ppapi::proxy::SerializedHandle type. Should this message be moved up close to the other "implementation detail" messages, e.g. PpapiMsg_InitializeNaClDispatcher? I see the distinction you're trying to make - that the recipient is not a pepper plugin. Maybe saying that would be clearer than "not part of PPAPI proper" since that also could apply to the other implementation detail messages. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:884: IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive) This also seems like an implementation detail message. As long as you're here, could you move it near PpapiMsg_Crash and PpapiMsg_Hang? https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:902: // ppapi::proxy::SerializedHandle type. Related to my above comments, would it be clearer to say that it's a message from the translator, which is not a pepper plugin?
Thanks for reviewing! https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... File components/nacl/renderer/plugin/temporary_file.cc (right): https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... components/nacl/renderer/plugin/temporary_file.cc:67: PP_FileHandle TempFile::GetFileHandle() { On 2015/12/16 20:30:06, bbudge wrote: > DCHECK(file_handle_.IsValid()); ? OK. How about I put the same into TakeFileHandle() too for consistency? https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... File components/nacl/renderer/plugin/temporary_file.h (right): https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... components/nacl/renderer/plugin/temporary_file.h:62: // This handle remain valid until the TempFile object is destroyed or On 2015/12/16 20:30:06, bbudge wrote: > nit: remains Done. https://codereview.chromium.org/1512733003/diff/140001/ppapi/nacl_irt/irt_pna... File ppapi/nacl_irt/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/nacl_irt/irt_pna... ppapi/nacl_irt/irt_pnacl_translator_link.cc:25: CallbackFunc func): func_(func) { On 2015/12/16 20:30:06, bbudge wrote: > formatting seems strange here, before the initializer colon. OK, I've split it onto a separate line, following the style guide. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... File ppapi/proxy/nacl_message_scanner.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:199: bool ScanSyncMessage(ScanningResults* results) { On 2015/12/16 20:30:06, bbudge wrote: > David Michael has implemented this in a patch we haven't landed yet: > > https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag... Thanks for pointing that out! I was able to debug why that change doesn't work on Windows. :-) https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:214: bool ScanSyncReply(ScanningResults* results) { On 2015/12/16 20:30:06, bbudge wrote: > ScanReply? I renamed the existing ScanReply() to ScanSyncReply() just because I thought it would make it clearer that ScanSyncMessage()+ScanSyncReply() handle the two parts of a synchronous request+reply pair. This isn't entirely obvious to readers who aren't very familiar with Chrome IPC. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:874: // needs to use the ppapi::proxy::SerializedHandle type. On 2015/12/16 20:30:06, bbudge wrote: > Should this message be moved up close to the other "implementation detail" > messages, e.g. PpapiMsg_InitializeNaClDispatcher? I see the distinction you're > trying to make - that the recipient is not a pepper plugin. Maybe saying that > would be clearer than "not part of PPAPI proper" since that also could apply to > the other implementation detail messages. That's not quite the distinction I was making... By "PPAPI proper" I meant "the set of interfaces defined by ppapi/c/". NaCl has a couple of things that are outside that set of PPAPI interfaces: * Public IRT interfaces, e.g. open_resource(), defined in irt.h. The implementation of this has a slightly odd split between ppapi/nacl_irt/ (untrusted side) and components/nacl/renderer/ (trusted side). * Plumbing for running the PNaCl translator (not a public interface at all). This is also split between ppapi/nacl_irt/ (untrusted side) and components/nacl/renderer/ (trusted side). With this comment in the code, I was trying to answer the question "why isn't this message declared in one of components/nacl/common/nacl_{host_,renderer_,}messages.h instead?". Maybe that question doesn't need answering? :-) I can leave out this comment if you think it's misleading. The distinction I was making doesn't matter too much given that NaCl is not usable separately from PPAPI in Chrome. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:884: IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive) On 2015/12/16 20:30:06, bbudge wrote: > This also seems like an implementation detail message. Hmm, I would regard all the IPC messages as implementation details. By "implementation detail message", do you mean "doesn't have a 1-1 correspondence with an interface defined in ppapi/c/"? > As long as you're here, > could you move it near PpapiMsg_Crash and PpapiMsg_Hang? Not sure that is appropriate, because the file seems to be grouped by: // These are from the browser to the plugin. [PpapiMsg_* messages...] // These are from the plugin to the renderer. [PpapiHostMsg_* messages...] (Though maybe there are parts that don't follow that grouping -- it's hard to tell.) Hmm, should PnaclTranslatorLink have a PpapiPluginMsg_ prefix? The naming scheme is not entirely clear to me. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:902: // ppapi::proxy::SerializedHandle type. On 2015/12/16 20:30:06, bbudge wrote: > Related to my above comments, would it be clearer to say that it's a message > from the translator, which is not a pepper plugin? PpapiHostMsg_OpenResource is actually usable by normal nexes (those in the Web Store), in addition to the PNaCl translator. Again, the distinction I was trying to explain here maybe doesn't matter very much since NaCl is not usable separately from PPAPI in Chrome. So I could drop this extra comment if you prefer.
https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... File components/nacl/renderer/plugin/temporary_file.cc (right): https://codereview.chromium.org/1512733003/diff/140001/components/nacl/render... components/nacl/renderer/plugin/temporary_file.cc:67: PP_FileHandle TempFile::GetFileHandle() { On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > DCHECK(file_handle_.IsValid()); ? > > OK. How about I put the same into TakeFileHandle() too for consistency? sgtm https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... File ppapi/proxy/nacl_message_scanner.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:199: bool ScanSyncMessage(ScanningResults* results) { On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > David Michael has implemented this in a patch we haven't landed yet: > > > > > https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag... > > Thanks for pointing that out! I was able to debug why that change doesn't work > on Windows. :-) Not at all. Thank you for figuring out the problem! https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:214: bool ScanSyncReply(ScanningResults* results) { On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > ScanReply? > > I renamed the existing ScanReply() to ScanSyncReply() just because I thought it > would make it clearer that ScanSyncMessage()+ScanSyncReply() handle the two > parts of a synchronous request+reply pair. This isn't entirely obvious to > readers who aren't very familiar with Chrome IPC. It makes the code in CASE_FOR_REPLY confusing though, because those are async replies. Maybe a comment would help here. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:874: // needs to use the ppapi::proxy::SerializedHandle type. On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > Should this message be moved up close to the other "implementation detail" > > messages, e.g. PpapiMsg_InitializeNaClDispatcher? I see the distinction you're > > trying to make - that the recipient is not a pepper plugin. Maybe saying that > > would be clearer than "not part of PPAPI proper" since that also could apply > to > > the other implementation detail messages. > > That's not quite the distinction I was making... > > By "PPAPI proper" I meant "the set of interfaces defined by ppapi/c/". > > NaCl has a couple of things that are outside that set of PPAPI interfaces: > > * Public IRT interfaces, e.g. open_resource(), defined in irt.h. The > implementation of this has a slightly odd split between ppapi/nacl_irt/ > (untrusted side) and components/nacl/renderer/ (trusted side). > > * Plumbing for running the PNaCl translator (not a public interface at all). > This is also split between ppapi/nacl_irt/ (untrusted side) and > components/nacl/renderer/ (trusted side). > > With this comment in the code, I was trying to answer the question "why isn't > this message declared in one of > components/nacl/common/nacl_{host_,renderer_,}messages.h instead?". > > Maybe that question doesn't need answering? :-) I can leave out this comment if > you think it's misleading. The distinction I was making doesn't matter too much > given that NaCl is not usable separately from PPAPI in Chrome. Whatever seems reasonable to you. See my other comments. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:884: IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive) On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > This also seems like an implementation detail message. > > Hmm, I would regard all the IPC messages as implementation details. > > By "implementation detail message", do you mean "doesn't have a 1-1 > correspondence with an interface defined in ppapi/c/"? > > > > As long as you're here, > > could you move it near PpapiMsg_Crash and PpapiMsg_Hang? > > Not sure that is appropriate, because the file seems to be grouped by: > > // These are from the browser to the plugin. > [PpapiMsg_* messages...] > > // These are from the plugin to the renderer. > [PpapiHostMsg_* messages...] > > (Though maybe there are parts that don't follow that grouping -- it's hard to > tell.) > > Hmm, should PnaclTranslatorLink have a PpapiPluginMsg_ prefix? The naming > scheme is not entirely clear to me. Right, by "implementation detail" I meant not used for any Pepper API. It's not a very clear way to say that :) I agree, PpapiHostMsg_Keepalive should actually be moved below, after PpapiHostMsg_StartupInitializationComplete. You raise a good point about naming. I think PpapiPNaClTranslatorMsg_Link or PpapiPNaClMsg_Link would help distinguish these messages from the others. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:902: // ppapi::proxy::SerializedHandle type. On 2015/12/17 06:09:56, Mark Seaborn wrote: > On 2015/12/16 20:30:06, bbudge wrote: > > Related to my above comments, would it be clearer to say that it's a message > > from the translator, which is not a pepper plugin? > > PpapiHostMsg_OpenResource is actually usable by normal nexes (those in the Web > Store), in addition to the PNaCl translator. > > Again, the distinction I was trying to explain here maybe doesn't matter very > much since NaCl is not usable separately from PPAPI in Chrome. So I could drop > this extra comment if you prefer. Hmm, the message name is kind of misleading given the comments above. It really should be something like PpapiNaClMsg_OpenResource. I guess the comment is helpful given the naming.
A few more comments, otherwise this looks good. https://codereview.chromium.org/1512733003/diff/160001/components/nacl/render... File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right): https://codereview.chromium.org/1512733003/diff/160001/components/nacl/render... components/nacl/renderer/plugin/pnacl_translate_thread.cc:132: // the child thread when the child thread calls Shutdown(). I'm assuming this is temporary, until you remove SRPC for compiling. Maybe a TODO? https://codereview.chromium.org/1512733003/diff/160001/ppapi/nacl_irt/irt_pna... File ppapi/nacl_irt/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1512733003/diff/160001/ppapi/nacl_irt/irt_pna... ppapi/nacl_irt/irt_pnacl_translator_link.cc:66: CallbackFunc func_; DISALLOW_COPY_AND_ASSIGN ?
https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... File ppapi/proxy/nacl_message_scanner.cc (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/nacl_messa... ppapi/proxy/nacl_message_scanner.cc:214: bool ScanSyncReply(ScanningResults* results) { On 2015/12/17 19:10:21, bbudge wrote: > On 2015/12/17 06:09:56, Mark Seaborn wrote: > > On 2015/12/16 20:30:06, bbudge wrote: > > > ScanReply? > > > > I renamed the existing ScanReply() to ScanSyncReply() just because I thought > it > > would make it clearer that ScanSyncMessage()+ScanSyncReply() handle the two > > parts of a synchronous request+reply pair. This isn't entirely obvious to > > readers who aren't very familiar with Chrome IPC. > > It makes the code in CASE_FOR_REPLY confusing though, because those are async > replies. Maybe a comment would help here. OK, I can just rename this back to ScanReply(). It makes the change smaller. :-) https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:874: // needs to use the ppapi::proxy::SerializedHandle type. On 2015/12/17 19:10:24, bbudge wrote: > On 2015/12/17 06:09:56, Mark Seaborn wrote: > > On 2015/12/16 20:30:06, bbudge wrote: > > > Should this message be moved up close to the other "implementation detail" > > > messages, e.g. PpapiMsg_InitializeNaClDispatcher? I see the distinction > you're > > > trying to make - that the recipient is not a pepper plugin. Maybe saying > that > > > would be clearer than "not part of PPAPI proper" since that also could apply > > to > > > the other implementation detail messages. > > > > That's not quite the distinction I was making... > > > > By "PPAPI proper" I meant "the set of interfaces defined by ppapi/c/". > > > > NaCl has a couple of things that are outside that set of PPAPI interfaces: > > > > * Public IRT interfaces, e.g. open_resource(), defined in irt.h. The > > implementation of this has a slightly odd split between ppapi/nacl_irt/ > > (untrusted side) and components/nacl/renderer/ (trusted side). > > > > * Plumbing for running the PNaCl translator (not a public interface at all). > > This is also split between ppapi/nacl_irt/ (untrusted side) and > > components/nacl/renderer/ (trusted side). > > > > With this comment in the code, I was trying to answer the question "why isn't > > this message declared in one of > > components/nacl/common/nacl_{host_,renderer_,}messages.h instead?". > > > > Maybe that question doesn't need answering? :-) I can leave out this comment > if > > you think it's misleading. The distinction I was making doesn't matter too > much > > given that NaCl is not usable separately from PPAPI in Chrome. > > Whatever seems reasonable to you. See my other comments. OK, I have removed the part mentioning "PPAPI proper" for now, but made the comment more descriptive in other ways. https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:884: IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive) On 2015/12/17 19:10:22, bbudge wrote: > You raise a good point about naming. I think PpapiPNaClTranslatorMsg_Link or > PpapiPNaClMsg_Link would help distinguish these messages from the others. I'm a bit reluctant to start using new prefixes because I'm not sure I understand the naming conventions fully. This doc talks about the "ViewMsg_" and "ViewHostMsg_" prefixes but doesn't explain how it's supposed to be generalised: https://www.chromium.org/developers/design-documents/inter-process-communication (which I think I first read ~6 years ago!). Is it supposed to be based on the receiving C++ class or the receiving process? So I'd be inclined to leave it as PpapiMsg_PnaclTranslatorLink. But some alternatives would be: * PnaclTranslatorMsg_Link * PpapiPnaclTranslatorMsg_Link Please pick one -- I'll defer to your judgement here. :-) (The Chrome code uses the spelling "Pnacl" rather than "PNaCl", BTW.) https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:902: // ppapi::proxy::SerializedHandle type. On 2015/12/17 19:10:24, bbudge wrote: > On 2015/12/17 06:09:56, Mark Seaborn wrote: > > On 2015/12/16 20:30:06, bbudge wrote: > > > Related to my above comments, would it be clearer to say that it's a message > > > from the translator, which is not a pepper plugin? > > > > PpapiHostMsg_OpenResource is actually usable by normal nexes (those in the Web > > Store), in addition to the PNaCl translator. > > > > Again, the distinction I was trying to explain here maybe doesn't matter very > > much since NaCl is not usable separately from PPAPI in Chrome. So I could > drop > > this extra comment if you prefer. > > Hmm, the message name is kind of misleading given the comments above. It really > should be something like PpapiNaClMsg_OpenResource. I guess the comment is > helpful given the naming. OK, I'm dropping this comment change from this CL, and moving it to a separate CL (https://codereview.chromium.org/1540953002/). https://codereview.chromium.org/1512733003/diff/160001/components/nacl/render... File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right): https://codereview.chromium.org/1512733003/diff/160001/components/nacl/render... components/nacl/renderer/plugin/pnacl_translate_thread.cc:132: // the child thread when the child thread calls Shutdown(). On 2015/12/17 21:17:11, bbudge wrote: > I'm assuming this is temporary, until you remove SRPC for compiling. Maybe a > TODO? Yes. Done. https://codereview.chromium.org/1512733003/diff/160001/ppapi/nacl_irt/irt_pna... File ppapi/nacl_irt/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1512733003/diff/160001/ppapi/nacl_irt/irt_pna... ppapi/nacl_irt/irt_pnacl_translator_link.cc:66: CallbackFunc func_; On 2015/12/17 21:17:11, bbudge wrote: > DISALLOW_COPY_AND_ASSIGN ? Done.
LGTM Thanks for working on this project! https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1512733003/diff/140001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:884: IPC_MESSAGE_CONTROL0(PpapiHostMsg_Keepalive) On 2015/12/21 22:58:50, Mark Seaborn wrote: > On 2015/12/17 19:10:22, bbudge wrote: > > You raise a good point about naming. I think PpapiPNaClTranslatorMsg_Link or > > PpapiPNaClMsg_Link would help distinguish these messages from the others. > > I'm a bit reluctant to start using new prefixes because I'm not sure I > understand the naming conventions fully. > > This doc talks about the "ViewMsg_" and "ViewHostMsg_" prefixes but doesn't > explain how it's supposed to be generalised: > https://www.chromium.org/developers/design-documents/inter-process-communication > (which I think I first read ~6 years ago!). Is it supposed to be based on the > receiving C++ class or the receiving process? > > So I'd be inclined to leave it as PpapiMsg_PnaclTranslatorLink. But some > alternatives would be: > * PnaclTranslatorMsg_Link > * PpapiPnaclTranslatorMsg_Link > > Please pick one -- I'll defer to your judgement here. :-) > > (The Chrome code uses the spelling "Pnacl" rather than "PNaCl", BTW.) OK, maybe I'm overthinking this. Your first idea seems OK to me, so PpapiMsg_PnaclTranslatorLink is fine. This comment thread seems to have diverged from my original intent, which was that the existing PpapiHostMsg_KeepAlive seems misplaced. But you can leave it if it's too much trouble.
mseaborn@chromium.org changed reviewers: + wfh@chromium.org
The CQ bit was checked by mseaborn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512733003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512733003/180001
+wfh for IPC message security review of ppapi/proxy/ppapi_messages.h.
I feel like I need to stare at this code a bit more to understand what it does. Alternatively, if you can justify/explain why adding this IPC accessible from the renderer that seems to pass file handles is security-safe I'm very reasonable. :) https://codereview.chromium.org/1512733003/diff/180001/components/nacl/render... File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right): https://codereview.chromium.org/1512733003/diff/180001/components/nacl/render... components/nacl/renderer/plugin/pnacl_translate_thread.cc:214: if (!content::BrokerDuplicateHandle( Is another BrokerAddTargetPeer required here for the 32/64 case, or is the broker relationship the same here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 21 December 2015 at 21:12, <wfh@chromium.org> wrote: > I feel like I need to stare at this code a bit more to understand what it > does. > > Alternatively, if you can justify/explain why adding this IPC accessible > from > the renderer that seems to pass file handles is security-safe I'm very > reasonable. :) > OK, I'll give a bit of background. As you probably know, PNaCl allows running "pexes" (PNaCl portable executables) in the browser, and this format is based on LLVM's bitcode. In order to run a pexe, PNaCl must first translate it to produce a native-code executable for the target architecture (e.g. an x86-64 nexe). In Chrome, PNaCl launches two NaCl processes in turn to do this: a compiler process (which uses LLVM for code generation) and a linker process (which uses Gold for linking). In each case, the renderer sends across file descriptors for the subprocess's inputs/outputs. It gives the compiler process FDs for writing the object files (one object file per thread). It gives the linker process FDs for reading the object files and an FD for writing the resulting nexe. Before this change, this sending of FDs is done using SRPC, which is NaCl's old IPC system (implemented in native_client/). We've been gradually removing uses of SRPC, so that the only remaining use now is for invoking PNaCl's compiler/linker processes. Sending FDs is done by the InvokeSrpcMethod() call in pnacl_translate_thread.cc. This is a synchronous IPC call, so to minimise the amount of change in this CL, the replacement Chrome IPC message (PpapiMsg_PnaclTranslatorLink) is also synchronous. This change is safe because it doesn't change what FDs are sent between Chrome processes. We just change which IPC system is used to send them, using Chrome IPC instead of SRPC. This arrangement is safe because it follows the principle of least authority/privilege: * The renderer only sends across FDs for temporary files that each subprocess needs to do its job (nexe and object file FDs for output/input). The renderer does not open arbitrary files on behalf of the subprocesses. * We terminate the subprocesses after they've finished, thereby revoking their ability to read/write the FDs. However, this is not necessary to enforce security, because the system would be safe even if the nexe or object files got modified concurrently by a process that continued running. * We send FDs with the appropriate permissions: the linker process gets read-only FDs for the object files (although they're only made read-only at the level of NaCl's trusted runtime rather than at the kernel level, I think). However, this also isn't necessary to enforce, because it wouldn't matter if the linker could write to the object files it is given as inputs. https://codereview.chromium.org/1512733003/diff/180001/components/nacl/render... > File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right): > > > https://codereview.chromium.org/1512733003/diff/180001/components/nacl/render... > components/nacl/renderer/plugin/pnacl_translate_thread.cc:214: if > (!content::BrokerDuplicateHandle( > Is another BrokerAddTargetPeer required here for the 32/64 case, or is > the broker relationship the same here? No, the browser process already does a BrokerAddTargetPeer() call that allows the renderer process to send handles to NaCl loader processes. That call is done in components/nacl/browser/nacl_process_host.cc. The 32/64 case is covered by Chromium's Windows bots, so you would see tests fail if that BrokerAddTargetPeer() weren't happening. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the explanation. This does indeed look safe from a conceptual perspective, since it is just replacing an existing relationship. Security lgtm. I did look over nacl_message_scanner.cc which adds the sync message parsing, and it looks fine to me, but if you want to be sure, you might want to rope tsepez in for a quick review, as he knows this code very well.
mseaborn@chromium.org changed reviewers: + tsepez@chromium.org
On 2015/12/28 22:30:54, Will Harris wrote: > Thanks for the explanation. > > This does indeed look safe from a conceptual perspective, since it is just > replacing an existing relationship. > > Security lgtm. I did look over nacl_message_scanner.cc which adds the sync > message parsing, and it looks fine to me, but if you want to be sure, you might > want to rope tsepez in for a quick review, as he knows this code very well. OK, +tsepez for nacl_message_scanner.cc as you suggest.
nacl_message_scanner.cc LGTM
The CQ bit was checked by mseaborn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512733003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512733003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mseaborn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bbudge@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1512733003/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512733003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512733003/200001
Message was sent while issue was closed.
Description was changed from
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
to
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
==========
to
==========
PNaCl: Use Chrome IPC to talk to the linker process, instead of SRPC
This is the second-to-last remaining use of SRPC.
* Change pnacl_translate_thread.cc to send its request using Chrome
IPC instead of SRPC. Similarly, change
irt_pnacl_translator_link.cc to receive its request this way.
* Add plumbing so that pnacl_translate_thread.cc can acquire a Chrome
IPC channel object. Change LaunchSelLdr() (in ppb_nacl_private.h)
to return this channel, and plumb it through service_runtime.h too.
* Extend nacl_message_scanner.cc to handle a couple of things our new
IPC message needs:
* File handles inside of std::vector<>s.
* File handles inside of sync messages. (Previously, only sync
replies were handled.)
* Change a BUILD.gn to account for #includes of
content/public/common/sandbox_init.h and
ppapi/proxy/{serialized_handle.h,ppapi_messages.h}.
BUG=302078
TEST=e.g. NaClBrowserTestPnacl.PPAPICore (tests PNaCl translation)
Committed: https://crrev.com/3bedcdc7ad86115836141f91ffd7fa331d07e2a1
Cr-Commit-Position: refs/heads/master@{#367369}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3bedcdc7ad86115836141f91ffd7fa331d07e2a1 Cr-Commit-Position: refs/heads/master@{#367369} |
