SFI NaCl: Batch-open resource files

SFI NaCl: Batch-open resource files

Add a hook to the IPC adapter to intercept PpapiHostMsg_OpenResource
messages from the untrusted side so that the trusted side can immediately
return prefetched descriptors without issuing Chrome IPC to the renderer
process.

This CL is an SFI NaCl version of https://codereview.chromium.org/649603004/.

BUG=nativeclient:3802
BUG=348232
TEST=browser_tests (*PackagedAppTest.SuccessfulLoad)
TEST=manually tested SFI ARC apps and PNaCl demo apps

Committed: https://crrev.com/7bd00a274893ee7334dfbc78ca5046f1af876382
Cr-Commit-Position: refs/heads/master@{#328382}

[Yusuke Sato, 2015-03-17 21:57:46]

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

[Yusuke Sato, 2015-03-17 21:57:47]

Mark, please take a look.

[Mark Seaborn, 2015-03-20 00:40:53]

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:635: void
NaClIPCAdapter::OnFileTokenResolvedLocked(
Since this is reused for the new open_resource case when there's no file token
involved, it seems like you should rename this.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:769: (msg->type() ==
PpapiHostMsg_OpenResource::ID) &&
Nit: ()s not needed around this

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.h (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:144: // send a pre-opened resrouce
descriptor to the untrusted side.
"resource"

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:283: std::map<std::string, std::pair<
Nit: Define a typedef for this std::map?

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:287: if (key.find(files_prefix) == 0)
You're stripping off the "files/" prefix which is added by untrusted code (in
ppapi/nacl_irt/manifest_service.cc).

It looks like you don't need a fallback because the "files/" prefix should
always be there.

How about changing manifest_service.cc not to add the prefix?  AFAICT,
json_manifest.cc requires it, and then strips it off, so that check can be
removed too.  This can be a standalone cleanup change.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:288: it =
prefetched_resource_files_.find(key.substr(files_prefix.length()));
Can you comment this as a fast path, like how it's commented in the Non-SFI
version?

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:295:
prefetched_resource_files_.erase(it);
Can you add a comment saying why this is thread-safe: which threads it runs on,
and which lock it holds.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:302: // Return false to let the IPC
adapter issue an IPC to the renderer.
...and fall back to the slow path.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:426: // TODO(yusukes): Support
OS_WIN.
What would be required to support Windows?

Since there's no OS_WIN check in the browser process, what would happen if a
compromised renderer skipped this check and requested prefetched files?  Would
it get invalid Windows file handles back?

[Yusuke Sato, 2015-04-15 23:02:07]

Patchset #3 (id:40001) has been deleted

[Yusuke Sato, 2015-04-16 02:38:45]

Please take another look.

Please review the diff between PS2 and PS3. PS5 is a small fix just to revert
some unneeded changes in PS2. Please also review the diff between PS4 and PS5.
PS5 says "wip" but it's actually review ready. Thanks!

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:635: void
NaClIPCAdapter::OnFileTokenResolvedLocked(
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> Since this is reused for the new open_resource case when there's no file token
> involved, it seems like you should rename this.

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:665: SaveMessage(*new_msg,
rewritten_msg.get());
Reverted this change.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:683: SaveMessage(*new_msg,
rewritten_msg.get());
same. reverted.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:765: // Handle
PpapiHostMsg_OpenResource locally without sending an IPC to the
Moved this to SendMessageOnIOThread so that
NaClIPCAdapter::OnFileTokenResolvedLocked (which is now renamed to
SaveOpenResourceMessage) is called on the IO thread too.

The comment at line 506
 // resolve_file_token_cb_ must be invoked from the main thread
was incorrect, and the slow path actually calls OnFileTokenResolved on the IO
thread. The fast path should do the same.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:769: (msg->type() ==
PpapiHostMsg_OpenResource::ID) &&
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> Nit: ()s not needed around this

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.h (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:144: // send a pre-opened resrouce
descriptor to the untrusted side.
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> "resource"

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:283: std::map<std::string, std::pair<
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> Nit: Define a typedef for this std::map?

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:287: if (key.find(files_prefix) == 0)
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> You're stripping off the "files/" prefix which is added by untrusted code (in
> ppapi/nacl_irt/manifest_service.cc).
> 
> It looks like you don't need a fallback because the "files/" prefix should
> always be there.
> 
> How about changing manifest_service.cc not to add the prefix?  AFAICT,
> json_manifest.cc requires it, and then strips it off, so that check can be
> removed too.  This can be a standalone cleanup change.

Sure, let me add a TODO for now.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:288: it =
prefetched_resource_files_.find(key.substr(files_prefix.length()));
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> Can you comment this as a fast path, like how it's commented in the Non-SFI
> version?

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:295:
prefetched_resource_files_.erase(it);
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> Can you add a comment saying why this is thread-safe: which threads it runs
on,
> and which lock it holds.

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:302: // Return false to let the IPC
adapter issue an IPC to the renderer.
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> ...and fall back to the slow path.

Done.

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:426: // TODO(yusukes): Support
OS_WIN.
On 2015/03/20 00:40:52, Mark Seaborn wrote:
> What would be required to support Windows?

Probably nothing, but I haven't confirmed it yet since I don't have Windows dev
environment (yet). Unless there's an urgent need for OS_WIN support, I would
like to enable it later.
 
> Since there's no OS_WIN check in the browser process, what would happen if a
> compromised renderer skipped this check and requested prefetched files?  Would
> it get invalid Windows file handles back?

Good point. Checking this in the renderer side is probably a bad idea. I moved
this #ifdef to browser side so that browser ignores the URL list on OS_WIN for
now.

[Yusuke Sato, 2015-04-16 03:51:14]

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:287: if (key.find(files_prefix) == 0)
On 2015/04/16 02:38:45, Yusuke Sato wrote:
> On 2015/03/20 00:40:52, Mark Seaborn wrote:
> > You're stripping off the "files/" prefix which is added by untrusted code
(in
> > ppapi/nacl_irt/manifest_service.cc).
> > 
> > It looks like you don't need a fallback because the "files/" prefix should
> > always be there.
> > 
> > How about changing manifest_service.cc not to add the prefix?  AFAICT,
> > json_manifest.cc requires it, and then strips it off, so that check can be
> > removed too.  This can be a standalone cleanup change.
> 
> Sure, let me add a TODO for now.

https://codereview.chromium.org/1070233007/

[Mark Seaborn, 2015-04-16 19:00:31]

https://codereview.chromium.org/1010183002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:367:
manifest_ipc_adapter->set_resolve_file_token_callback(
Yesterday we discussed doing a small cleanup refactoring so that this setter is
called after ConnectChannel(), to avoid a potential race if messages start
arriving on this channel.  Were you going to do that first?

[Yusuke Sato, 2015-04-16 20:22:02]

https://codereview.chromium.org/1010183002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:367:
manifest_ipc_adapter->set_resolve_file_token_callback(
On 2015/04/16 19:00:31, Mark Seaborn wrote:
> Yesterday we discussed doing a small cleanup refactoring so that this setter
is
> called after ConnectChannel(), to avoid a potential race if messages start
> arriving on this channel.  Were you going to do that first?

Done in https://codereview.chromium.org/1090043002/

[Yusuke Sato, 2015-04-16 23:50:26]

Following your request, I've submitted two clean-up CLs and rebased this on top
of them. Please take another look at PS6.

[Yusuke Sato, 2015-04-29 23:17:36]

ping?

[Mark Seaborn, 2015-05-04 17:08:39]

LGTM

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:426: // TODO(yusukes): Support
OS_WIN.
On 2015/04/16 02:38:45, Yusuke Sato wrote:
> On 2015/03/20 00:40:52, Mark Seaborn wrote:
> > What would be required to support Windows?
> 
> Probably nothing, but I haven't confirmed it yet since I don't have Windows
dev
> environment (yet). Unless there's an urgent need for OS_WIN support, I would
> like to enable it later.
>  
> > Since there's no OS_WIN check in the browser process, what would happen if a
> > compromised renderer skipped this check and requested prefetched files? 
Would
> > it get invalid Windows file handles back?
> 
> Good point. Checking this in the renderer side is probably a bad idea. I moved
> this #ifdef to browser side so that browser ignores the URL list on OS_WIN for
> now.

What happens if you remove the "#if !defined(OS_WIN)" conditional that's
currently in components/nacl/browser/nacl_host_message_filter.cc?  Does this
pass on the trybots, and if not, what's the failure?  Can you try that, even if
you haven't got a local Windows machine?

I'd rather not have a conditional in here unless the specific reason why this
doesn't work on Windows is commented.

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:303: // Return false to fall back to the
slow path. Let the IPC adapter issue an
Nit: "the IPC adapter" -> "NaClIPCAdapter" to be more specific?

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:336: bool result =
prefetched_resource_files_.insert(std::make_pair(
I think you'll need to rebase this use of std::pair on Hidehiko's refactoring.

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:342: DLOG(ERROR) << "Duplicated
open_resource key: "
How about just making this fatal?  The renderer should never send duplicated
keys anyway.

[Yusuke Sato, 2015-05-05 17:40:57]

Patchset #7 (id:140001) has been deleted

[Yusuke Sato, 2015-05-05 18:27:16]

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1010183002/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:426: // TODO(yusukes): Support
OS_WIN.
On 2015/05/04 17:08:39, Mark Seaborn wrote:
> On 2015/04/16 02:38:45, Yusuke Sato wrote:
> > On 2015/03/20 00:40:52, Mark Seaborn wrote:
> > > What would be required to support Windows?
> > 
> > Probably nothing, but I haven't confirmed it yet since I don't have Windows
> dev
> > environment (yet). Unless there's an urgent need for OS_WIN support, I would
> > like to enable it later.
> >  
> > > Since there's no OS_WIN check in the browser process, what would happen if
a
> > > compromised renderer skipped this check and requested prefetched files? 
> Would
> > > it get invalid Windows file handles back?
> > 
> > Good point. Checking this in the renderer side is probably a bad idea. I
moved
> > this #ifdef to browser side so that browser ignores the URL list on OS_WIN
for
> > now.
> 
> What happens if you remove the "#if !defined(OS_WIN)" conditional that's
> currently in components/nacl/browser/nacl_host_message_filter.cc?  Does this
> pass on the trybots, and if not, what's the failure?  Can you try that, even
if
> you haven't got a local Windows machine?
> 
> I'd rather not have a conditional in here unless the specific reason why this
> doesn't work on Windows is commented.

Thanks to Hidehiko's recent refactoring, it turned out that the code for
releasing prefetched descriptors in NaClProcessHost::~NaClProcessHost() was not
fully compatible with OS_WIN. Updated comments in
components/nacl/browser/nacl_host_message_filter.cc and
components/nacl/browser/nacl_process_host.cc. Let me continue to exclude OS_WIN
for now.

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:303: // Return false to fall back to the
slow path. Let the IPC adapter issue an
On 2015/05/04 17:08:39, Mark Seaborn wrote:
> Nit: "the IPC adapter" -> "NaClIPCAdapter" to be more specific?

Done.

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:336: bool result =
prefetched_resource_files_.insert(std::make_pair(
On 2015/05/04 17:08:39, Mark Seaborn wrote:
> I think you'll need to rebase this use of std::pair on Hidehiko's refactoring.

Done.

https://codereview.chromium.org/1010183002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:342: DLOG(ERROR) << "Duplicated
open_resource key: "
On 2015/05/04 17:08:39, Mark Seaborn wrote:
> How about just making this fatal?  The renderer should never send duplicated
> keys anyway.

Done.

[Yusuke Sato, 2015-05-05 18:28:03]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2015-05-05 18:28:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1010183002/#ps180001
(title: "address all comments")

[commit-bot: I haz the power, 2015-05-05 18:28:54]

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

[commit-bot: I haz the power, 2015-05-05 19:41:28]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-05-05 19:43:09]

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