Pepper: Stop using SRPC for irt_open_resource().

Pepper: Stop using SRPC for irt_open_resource().

This registers an IRT interface in Chromium instead of using the one provided
by NaCl. This reuses the ManifestServiceChannel used for providing
irt_open_resource() in non-SFI mode.

In this change, the Chromium implementation of NACL_IRT_RESOURCE_OPEN_v0_1
takes precedence over the one supplied by NaCl (which is SRPC-based).

The SRPC-based codepath in service_runtime.cc needs to be kept because the
PNaCl translator doesn't have the IRT available yet. I've added a check to
enforce that's the only user of that codepath.

BUG=394130
TEST=Manually tested with a file token that didn't resolve with a local patch
that forced GetFilePath to fail in nacl_process_host.cc and confirmed
that URLLoader* still passed.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32
R=dmichael@chromium.org, mseaborn@chromium.org, tsepez@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/cffdd030d9ecd5e2ef50de941fca337a94c3b476

[Mark Seaborn, 2014-07-25 22:06:36]

Have you checked that validation caching still works after this?  Do we have
test coverage for that?

https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc#...
ppapi/nacl_irt/irt_ppapi.cc:54: static int
ppapihook_not_pnacl_private_filter(void) {
Nit: remove "ppapihook" from the name.  "not_pnacl_filter"

https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc#...
ppapi/nacl_irt/irt_ppapi.cc:68: // precedence over the one supplied by NaCl
(which is SRPC-based).
You could omit this comment so that we don't have to remove it after we remove
the NaCl-side implementation. :-)  Just note this in the commit message instead?

[teravest, 2014-07-29 17:14:03]

On 2014/07/25 22:06:36, Mark Seaborn wrote:
> Have you checked that validation caching still works after this?  Do we have
> test coverage for that?

I don't think we have good test coverage for validation caching. This change
should use the same logic for providing file tokens that was previously used in
the SRPC implementation (see DownloadFile() in
components/nacl/renderer/ppb_nacl_private_impl.cc).
  

The PNaCl test failures are because the translator doesn't have the IRT
available. It looks like the linker is trying to load some libraries via
open_resource(), but it can't because this change removes the implementation of
PluginReverseInterface::OpenManifestEntry, which is used on the SRPC-based path.



> 
> https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc
> File ppapi/nacl_irt/irt_ppapi.cc (right):
> 
>
https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc#...
> ppapi/nacl_irt/irt_ppapi.cc:54: static int
> ppapihook_not_pnacl_private_filter(void) {
> Nit: remove "ppapihook" from the name.  "not_pnacl_filter"

Done.

> 
>
https://codereview.chromium.org/418423002/diff/1/ppapi/nacl_irt/irt_ppapi.cc#...
> ppapi/nacl_irt/irt_ppapi.cc:68: // precedence over the one supplied by NaCl
> (which is SRPC-based).
> You could omit this comment so that we don't have to remove it after we remove
> the NaCl-side implementation. :-)  Just note this in the commit message
instead?

Done.

[teravest, 2014-07-31 16:14:29]

Okay, I think this looks good and seems to be passing tests now.

I had to remove some existing NaCl tests that tested manifest functions via SRPC
since that's now restricted to the PNaCl translator. Let me know if you think
that's reasonable.

[teravest, 2014-07-31 20:59:07]

On 2014/07/31 16:14:29, teravest wrote:
> Okay, I think this looks good and seems to be passing tests now.
> 
> I had to remove some existing NaCl tests that tested manifest functions via
SRPC
> since that's now restricted to the PNaCl translator. Let me know if you think
> that's reasonable.

I still have to get this working on Windows, I just want to make sure this
approach seems reasonable to both of you.

[Mark Seaborn, 2014-08-01 00:58:37]

On 31 July 2014 13:59, <teravest@chromium.org> wrote:

> On 2014/07/31 16:14:29, teravest wrote:
>
>> Okay, I think this looks good and seems to be passing tests now.
>>
>
>  I had to remove some existing NaCl tests that tested manifest functions
>> via SRPC
>
>  since that's now restricted to the PNaCl translator. Let me know if you
>> think
>> that's reasonable.
>>
>
> I still have to get this working on Windows, I just want to make sure this
> approach seems reasonable to both of you.


Yep, it makes sense to remove those old "pm_*manifest" tests which test the
SRPC calls directly.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-08-04 15:22:36]

Got this working on windows now. It looks like the failed tryjobs are noise.

I think this is ready for review.

[dmichael (off chromium), 2014-08-05 16:04:04]

lgtm from my standpoint, but I'm not that knowledgeable about the NaCl and
non-SFI stuff.

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:5: #include <stdio.h>
^^^ I'm assuming this is leftover from debugging?

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:60: return !pnacl_mode;
I assume it's idiomatic C to apply ! to an int? This being C++, the result is a
bool that then gets converted again to int.

I might have expected:
return pnacl_mode ? 0 : 1;
...largely because I would have wanted the compiler to warn me if I implicitly
converted bool to int.

But I see what you wrote is perfectly valid, so feel free to ignore me if it's
idiomatic in this type of code.

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:74: void* table, size_t tablesize) {
chromium style nit: In a function declaration or definition, the parameters
should be all on one line or each on their own line.

[teravest, 2014-08-05 18:15:29]

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi.cc
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:5: #include <stdio.h>
On 2014/08/05 16:04:04, dmichael wrote:
> ^^^ I'm assuming this is leftover from debugging?

Removed.

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:60: return !pnacl_mode;
On 2014/08/05 16:04:04, dmichael wrote:
> I assume it's idiomatic C to apply ! to an int? This being C++, the result is
a
> bool that then gets converted again to int.
> 
> I might have expected:
> return pnacl_mode ? 0 : 1;
> ...largely because I would have wanted the compiler to warn me if I implicitly
> converted bool to int.
> 
> But I see what you wrote is perfectly valid, so feel free to ignore me if it's
> idiomatic in this type of code.

I think !pnacl_mode looks more natural than pnacl_mode ? 0 : 1

https://codereview.chromium.org/418423002/diff/80001/ppapi/nacl_irt/irt_ppapi...
ppapi/nacl_irt/irt_ppapi.cc:74: void* table, size_t tablesize) {
On 2014/08/05 16:04:04, dmichael wrote:
> chromium style nit: In a function declaration or definition, the parameters
> should be all on one line or each on their own line.

Done.

[Mark Seaborn, 2014-08-07 04:46:23]

Are you sure this keeps validation caching working?  IIRC, when Hidehiko
implemented the Chrome-IPC-based open_resource() (in
https://codereview.chromium.org/249183004), he didn't hook up the validation
caching metadata in the trusted code.  Your change doesn't seem to add any
plumbing for the metadata in the trusted code.

In particular:
 * The callback PP_OpenResourceCompletionCallback doesn't receive the metadata.
 * The IPC message PpapiHostMsg_OpenResource doesn't contain the metadata.

If you try breaking validation caching deliberately, do any tests catch that? 
Maybe you can try breaking validation caching with this change:

--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -1431,8 +1431,8 @@ void DownloadFile(PP_Instance instance,
   if (file_handle != PP_kInvalidFileHandle) {
     PP_NaClFileInfo file_info;
     file_info.handle = file_handle;
-    file_info.token_lo = file_token_lo;
-    file_info.token_hi = file_token_hi;
+    file_info.token_lo = 0;
+    file_info.token_hi = 0;

https://codereview.chromium.org/418423002/diff/100001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/418423002/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:84:
ppapi::proxy::SerializedHandle h;
Nit: 'h' -> 'handle' (to avoid 1-char var names)

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/irt_ppap...
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/irt_ppap...
ppapi/nacl_irt/irt_ppapi.cc:50: const nacl_irt_resource_open kIrtResourceOpen =
{
Nit: Add "static"

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (left):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:88: !ipc_fd.is_file()) {
Is this change required to fix something?  Maybe comment this in commit message?

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:94: *fd = ipc_fd.descriptor().fd;
Nit: I think multi-line ifs are meant to use {}s

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:181:
CHECK(!service_runtime_->main_service_runtime());
Doesn't this allow any nexe to crash the renderer?  If so, the function should
flag an error instead.

Maybe also add a TODO referencing
https://code.google.com/p/chromium/issues/detail?id=302078 ?

[teravest, 2014-08-07 18:47:03]

This patch currently breaks validation caching. Thanks for following up there--
I zeroed out the token and no tests broke.

Following the SRPC path, it looks like the loader calls
NaClExchangeFileTokenForMappableDesc to try to resolve the validation tokens. Is
it okay to make that method part of the public NaCl interface?

https://codereview.chromium.org/418423002/diff/100001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/418423002/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:84:
ppapi::proxy::SerializedHandle h;
On 2014/08/07 04:46:22, Mark Seaborn wrote:
> Nit: 'h' -> 'handle' (to avoid 1-char var names)

Done.

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/irt_ppap...
File ppapi/nacl_irt/irt_ppapi.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/irt_ppap...
ppapi/nacl_irt/irt_ppapi.cc:50: const nacl_irt_resource_open kIrtResourceOpen =
{
On 2014/08/07 04:46:23, Mark Seaborn wrote:
> Nit: Add "static"

Done.

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (left):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:88: !ipc_fd.is_file()) {
On 2014/08/07 04:46:23, Mark Seaborn wrote:
> Is this change required to fix something?  Maybe comment this in commit
message?

Removing this condition is required as part of this change. I've added a comment
in the code for readability at line 93.

OpenResource() has the following behavior:
Returns false if there was an IO error.
Returns true, but an fd of -1 if the file does not exist.
Returns true, with a a valid fd if the file was opened successfully.

Now that NaClIPCAdapter is being used to transmit this message, having an
"invalid" file that would resolve to -1 in ipc_fd.descriptor(). fd doesn't work.

It might be clearer to have ManifestService::OpenResource() return an enum and
only set "fd" on success-- I was a bit confused when I had to change how this
worked.

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:94: *fd = ipc_fd.descriptor().fd;
On 2014/08/07 04:46:23, Mark Seaborn wrote:
> Nit: I think multi-line ifs are meant to use {}s

The rule is that the if and else clauses must agree on whether they have braces.
I don't see any deviation from the Google style guide specified in the Chrome
style guide, and I've been encouraged to not use braces for single line if
statements.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:181:
CHECK(!service_runtime_->main_service_runtime());
On 2014/08/07 04:46:23, Mark Seaborn wrote:
> Doesn't this allow any nexe to crash the renderer?  If so, the function should
> flag an error instead.
> 
> Maybe also add a TODO referencing
> https://code.google.com/p/chromium/issues/detail?id=302078 ?

Nice catch. I've changed this to log an error here and added a TODO.

[Mark Seaborn, 2014-08-07 22:55:17]

On 7 August 2014 11:47, <teravest@chromium.org> wrote:

> This patch currently breaks validation caching. Thanks for following up
> there--
> I zeroed out the token and no tests broke.
>

Hmm, any chance of adding some test coverage here?


> Following the SRPC path, it looks like the loader calls
> NaClExchangeFileTokenForMappableDesc to try to resolve the validation
> tokens. Is
> it okay to make that method part of the public NaCl interface?


I looked at NaClExchangeFileTokenForMappableDesc().  The first thing it
does is to call ResolveFileToken(), which is defined on the Chromium side.
 If that succeeds, it creates a new NaClDesc.

So ideally, I think it doesn't make sense to
use NaClExchangeFileTokenForMappableDesc() if it immediately calls back to
the Chromium side.  (That is, it's not worth adding
NaClExchangeFileTokenForMappableDesc() to src/public/.)  Instead, it would
be better to expose just the part for creating the new NaClDesc.

On the other hand, this open_resource() refactoring is valuable enough that
if you want to call NaClExchangeFileTokenForMappableDesc() without adding
it to src/public/, that trade-off is OK with me.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-08-07 23:19:19]

On 7 August 2014 15:54, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 7 August 2014 11:47, <teravest@chromium.org> wrote:
>
>> This patch currently breaks validation caching. Thanks for following up
>> there--
>> I zeroed out the token and no tests broke.
>>
>
> Hmm, any chance of adding some test coverage here?
>

Nick pointed to chrome/test/nacl/nacl_browsertest_uma.cc as the location
for adding this.  This currently only covers validation caching for static
linking (newlib with nacl-gcc and PNaCl).  I think we'd just need to add a
nacl-glibc test to that file.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-08-11 19:41:27]

On Thu, Aug 7, 2014 at 4:54 PM, Mark Seaborn <mseaborn@chromium.org> wrote:
> On 7 August 2014 11:47, <teravest@chromium.org> wrote:
>>
>> This patch currently breaks validation caching. Thanks for following up
>> there--
>> I zeroed out the token and no tests broke.
>
>
> Hmm, any chance of adding some test coverage here?

Done. :)

>
>>
>> Following the SRPC path, it looks like the loader calls
>> NaClExchangeFileTokenForMappableDesc to try to resolve the validation
>> tokens. Is
>> it okay to make that method part of the public NaCl interface?
>
>
> I looked at NaClExchangeFileTokenForMappableDesc().  The first thing it does
> is to call ResolveFileToken(), which is defined on the Chromium side.  If
> that succeeds, it creates a new NaClDesc.
>
> So ideally, I think it doesn't make sense to use
> NaClExchangeFileTokenForMappableDesc() if it immediately calls back to the
> Chromium side.  (That is, it's not worth adding
> NaClExchangeFileTokenForMappableDesc() to src/public/.)  Instead, it would
> be better to expose just the part for creating the new NaClDesc.
>
> On the other hand, this open_resource() refactoring is valuable enough that
> if you want to call NaClExchangeFileTokenForMappableDesc() without adding it
> to src/public/, that trade-off is OK with me.

I'd like to use NaClExchangeFileTokenForMappableDesc() for now.
Eventually, the Chromium code should call ResolveFileToken() instead,
but that code is over in components/nacl, which can't be used from
ppapi/nacl_irt.

What's the right way to convert a NaClDesc* to an int that could be
provided by irt_open_resource? I searched around for a bit but didn't
find anything obvious.

>
> Cheers,
> Mark
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-08-13 17:48:22]

On 11 August 2014 12:41, Justin TerAvest <teravest@chromium.org> wrote:

> On Thu, Aug 7, 2014 at 4:54 PM, Mark Seaborn <mseaborn@chromium.org>
> wrote:
> > On 7 August 2014 11:47, <teravest@chromium.org> wrote:
> >> Following the SRPC path, it looks like the loader calls
> >> NaClExchangeFileTokenForMappableDesc to try to resolve the validation
> >> tokens. Is
> >> it okay to make that method part of the public NaCl interface?
> >
> >
> > I looked at NaClExchangeFileTokenForMappableDesc().  The first thing it
> does
> > is to call ResolveFileToken(), which is defined on the Chromium side.  If
> > that succeeds, it creates a new NaClDesc.
> >
> > So ideally, I think it doesn't make sense to use
> > NaClExchangeFileTokenForMappableDesc() if it immediately calls back to
> the
> > Chromium side.  (That is, it's not worth adding
> > NaClExchangeFileTokenForMappableDesc() to src/public/.)  Instead, it
> would
> > be better to expose just the part for creating the new NaClDesc.
> >
> > On the other hand, this open_resource() refactoring is valuable enough
> that
> > if you want to call NaClExchangeFileTokenForMappableDesc() without
> adding it
> > to src/public/, that trade-off is OK with me.
>
> I'd like to use NaClExchangeFileTokenForMappableDesc() for now.
> Eventually, the Chromium code should call ResolveFileToken() instead,
> but that code is over in components/nacl, which can't be used from
> ppapi/nacl_irt.
>
> What's the right way to convert a NaClDesc* to an int that could be
> provided by irt_open_resource? I searched around for a bit but didn't
> find anything obvious.


You're asking the wrong question. :-)  Converting between NaClDescs and
file descriptor table indexes is not something that you need to do.  It's
handled by the imc_sendmsg() and imc_recvmsg() syscalls for you.

You just need to call NaClExchangeFileTokenForMappableDesc() at the
appropriate place to convert one NaClDesc to the wrapped NaClDesc (to add
the validation caching metadata).

I think the place to do that is
either components/nacl/loader/nacl_ipc_adapter.cc
or ppapi/proxy/nacl_message_scanner.cc.  Those are the places that identify
handles/FDs in Chrome IPC messages and convert them to NaClDescs.  e.g. See
the NaClDescImcShmMake() call in nacl_ipc_adapter.cc.

You'll want to change ppapi/proxy/ppapi_messages.h to add the metadata
to PpapiHostMsg_OpenResource, and then change nacl_ipc_adapter.cc
or nacl_message_scanner.cc to process PpapiHostMsg_OpenResource specially
to process the metadata before the NaClDesc gets returned to untrusted code
via imc_recvmsg().

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-08-13 20:05:53]

On Wed, Aug 13, 2014 at 11:48 AM, Mark Seaborn <mseaborn@chromium.org> wrote:
> On 11 August 2014 12:41, Justin TerAvest <teravest@chromium.org> wrote:
>>
>> On Thu, Aug 7, 2014 at 4:54 PM, Mark Seaborn <mseaborn@chromium.org>
>> wrote:
>> > On 7 August 2014 11:47, <teravest@chromium.org> wrote:
>> >> Following the SRPC path, it looks like the loader calls
>> >> NaClExchangeFileTokenForMappableDesc to try to resolve the validation
>> >> tokens. Is
>> >> it okay to make that method part of the public NaCl interface?
>> >
>> >
>> > I looked at NaClExchangeFileTokenForMappableDesc().  The first thing it
>> > does
>> > is to call ResolveFileToken(), which is defined on the Chromium side.
>> > If
>> > that succeeds, it creates a new NaClDesc.
>> >
>> > So ideally, I think it doesn't make sense to use
>> > NaClExchangeFileTokenForMappableDesc() if it immediately calls back to
>> > the
>> > Chromium side.  (That is, it's not worth adding
>> > NaClExchangeFileTokenForMappableDesc() to src/public/.)  Instead, it
>> > would
>> > be better to expose just the part for creating the new NaClDesc.
>> >
>> > On the other hand, this open_resource() refactoring is valuable enough
>> > that
>> > if you want to call NaClExchangeFileTokenForMappableDesc() without
>> > adding it
>> > to src/public/, that trade-off is OK with me.
>>
>> I'd like to use NaClExchangeFileTokenForMappableDesc() for now.
>> Eventually, the Chromium code should call ResolveFileToken() instead,
>> but that code is over in components/nacl, which can't be used from
>> ppapi/nacl_irt.
>>
>> What's the right way to convert a NaClDesc* to an int that could be
>> provided by irt_open_resource? I searched around for a bit but didn't
>> find anything obvious.
>
>
> You're asking the wrong question. :-)  Converting between NaClDescs and file
> descriptor table indexes is not something that you need to do.  It's handled
> by the imc_sendmsg() and imc_recvmsg() syscalls for you.

For context,
I spent time looking at implementing this in the morning today and
realized that I can't easily use NaClExchangeFileTokenForMappableDesc
since I don't have a NaClValidationCache available where I'd want to
use it, so I'm exploring using the NaClProcessMsg_ResolveFileToken
message instead.


If I'm understanding you correctly, it's wrong to create a NaClDesc*
in ppapi/nacl_irt/manifest_service.cc using the fd received from
NaClProcessMsg_ResolveFileToken.

Are you suggesting that NaClProcessMsg_ResolveFileToken (or a similar
message) should be processed by NaClIPCAdapter to replace the platform
file in the message with a constructed NaClDesc? Presumably it would
have to be handled differently than other messages in NaClIPCAdapter
since that NaClDesc needs some metadata information to be populated.

>
> You just need to call NaClExchangeFileTokenForMappableDesc() at the
> appropriate place to convert one NaClDesc to the wrapped NaClDesc (to add
> the validation caching metadata).
>
> I think the place to do that is either
> components/nacl/loader/nacl_ipc_adapter.cc or
> ppapi/proxy/nacl_message_scanner.cc.  Those are the places that identify
> handles/FDs in Chrome IPC messages and convert them to NaClDescs.  e.g. See
> the NaClDescImcShmMake() call in nacl_ipc_adapter.cc.
>
> You'll want to change ppapi/proxy/ppapi_messages.h to add the metadata to
> PpapiHostMsg_OpenResource, and then change nacl_ipc_adapter.cc or
> nacl_message_scanner.cc to process PpapiHostMsg_OpenResource specially to
> process the metadata before the NaClDesc gets returned to untrusted code via
> imc_recvmsg().
>
> Cheers,
> Mark
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-08-27 15:25:56]

Patchset #9 (id:160001) has been deleted

[teravest, 2014-08-27 15:26:05]

Patchset #9 (id:180001) has been deleted

[teravest, 2014-08-27 19:51:21]

Okay, it looks like I've got validation caching working-- the tests that we have
pass, at least.

Dave, can you take a look at this before you head out for Thurs-Tues?

There are a couple of issues I'd like some feedback on as well:
  1. This change currently limits to one in-flight open_resource() call and uses
a lock. This prevents needing a look up on the reply message, using a global
instead. I think that's equivalent to the existing code. Would there be much
benefit to allowing concurrent calls?

  2. This change currently doesn't clear file tokens when validation fails with
the browser. Do we care whether invalid file tokens may be provided to untrusted
code?

[dmichael (off chromium), 2014-08-27 20:49:14]

dmichael@chromium.org changed reviewers:
+ bbudge@chromium.org

[dmichael (off chromium), 2014-08-27 20:49:14]

Leaving early, but took a quick look. +bbudge who knows this stuff as well.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1054: void
NaClProcessHost::FileResolvedAsync(
nit: the order here should match the header. Actually, looks like it's already
not in order. So... if you could try to put yours in the same order and maybe
after the same method as in the header, that would at least not make things
worse.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1073: out_file_path));
nit: indent off by one

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:483: // IPC to handle properly.
I know Mark strongly preferred knowledge of proxy messages stayed in
ppapi/proxy. (Arguably this one is more of an IRT than proxy thing, I guess.)

Also FYI...  Bill had written something to send a message while handling another
message. He had added a new "ChannelHandle" thing to SerializedHandle....  looks
like Hidehiko removed it
https://codereview.chromium.org/150713003/diff/390001/components/nacl/loader/...
a while back. I think more manual work is probably appropriate here though.

Now that you're doing Async messages, do you still care about doing this without
the lock? Maybe it would be fine to move this in to the NaClMessageScanner?

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:120: base::FilePath)>)>
nit: Maybe a typedef for the internal one might make it more self-documenting?
ResolveFileTokenResponse or something

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:125: }
Somewhere in here it might make sense to document what this class will do with
the given callback and what it requires for the nested callback. E.g., when the
callback is invoked, it should respond later by calling the
ResolveFileTokenResponse callback via PostTask on the same thread. (vs some
other thread or calling while the outer callback is on the stack).

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:137: base::FilePath)> g_resolved_cb;
I think the maybe map sounds best? I guess you're assuming you can only get 1 at
a time? If that's the case, at least a DCHECK or something that the Cb is
invalid would be good.

Also, I suspect this gives you a static initializer as-is...

[Mark Seaborn, 2014-08-28 21:33:49]

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/418423002/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:181:
CHECK(!service_runtime_->main_service_runtime());
On 2014/08/07 18:47:02, teravest wrote:
> On 2014/08/07 04:46:23, Mark Seaborn wrote:
> > Doesn't this allow any nexe to crash the renderer?  If so, the function
should
> > flag an error instead.
> > 
> > Maybe also add a TODO referencing
> > https://code.google.com/p/chromium/issues/detail?id=302078 ?
> 
> Nice catch. I've changed this to log an error here and added a TODO.

I don't see a TODO here, BTW...

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:483: // IPC to handle properly.
On 2014/08/27 20:49:14, dmichael (OOO 8.28-8.29) wrote:
> I know Mark strongly preferred knowledge of proxy messages stayed in
> ppapi/proxy.

> (Arguably this one is more of an IRT than proxy thing, I guess.)

Yeah, open_resource() isn't part of PPAPI, so it's more appropriate to have
handling of open_resource messages in a */nacl/* file than in ppapi/proxy/.  So
this part seems OK as it is.

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:108: // currently works on Linux, but not on
Windows...
Why doesn't it work on Windows?

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:112: // See
ppapi/nacl_irt/manifest_service.cc for how this function's result is
That is this file, in the function immediately below. :-)

https://codereview.chromium.org/418423002/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/418423002/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:182:
NaClLog(LOG_ERROR,
Can you make this reject the request, as a security-hardening measure, so that
we're not exposing the old SRPC-based manifest file code path to the web?

Or would it be better to reject the request earlier, in
PluginReverseInterface::OpenManifestEntry()?

[Mark Seaborn, 2014-08-28 23:40:22]

More comments after having done a full pass over the change...

https://codereview.chromium.org/418423002/diff/220001/chrome/test/data/nacl/m...
File chrome/test/data/nacl/manifest_file/pm_manifest_file_test.cc (left):

https://codereview.chromium.org/418423002/diff/220001/chrome/test/data/nacl/m...
chrome/test/data/nacl/manifest_file/pm_manifest_file_test.cc:1: /*
BTW, since this is a big change, it would be nice to commit the removal of these
old tests as a separate change first.

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:498: // renderer from running
arbitrary code in the plugin process.
"plugin process" -> "NaCl process" to clarify

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:508: return true;
Maybe comment: We don't release the message to NaCl untrusted code immediately. 
We defer it until we get an async message back from the browser process.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:598: if (ipc_fd !=
IPC::InvalidPlatformFileForTransit()) {
Maybe handle the error case first for readability:

if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
  RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
  return;
}

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:137: base::FilePath)> g_resolved_cb;
On 2014/08/27 20:49:14, dmichael (OOO 8.28-8.29) wrote:
> I think the maybe map sounds best? I guess you're assuming you can only get 1
at
> a time? If that's the case, at least a DCHECK or something that the Cb is
> invalid would be good.
> 
> Also, I suspect this gives you a static initializer as-is...

A mapping (from IDs to callbacks) would be better.  Can this be a member of the
NaClListener rather than a global?

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:278: virtual bool
OnMessageReceived(const IPC::Message& msg) OVERRIDE {
Why is this handled in a new MessageFilter?  Can't the new message be handled in
NaClListener::OnMessageReceived() below?

https://codereview.chromium.org/418423002/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.h (right):

https://codereview.chromium.org/418423002/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.h:32: OpenResourceCallback;
Nit: indent this

[teravest, 2014-09-04 21:04:51]

Patchset #12 (id:260001) has been deleted

[teravest, 2014-09-04 21:06:17]

Patchset #12 (id:280001) has been deleted

[teravest, 2014-09-04 22:13:30]

https://codereview.chromium.org/418423002/diff/220001/chrome/test/data/nacl/m...
File chrome/test/data/nacl/manifest_file/pm_manifest_file_test.cc (left):

https://codereview.chromium.org/418423002/diff/220001/chrome/test/data/nacl/m...
chrome/test/data/nacl/manifest_file/pm_manifest_file_test.cc:1: /*
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> BTW, since this is a big change, it would be nice to commit the removal of
these
> old tests as a separate change first.

Done.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1054: void
NaClProcessHost::FileResolvedAsync(
On 2014/08/27 20:49:14, dmichael wrote:
> nit: the order here should match the header. Actually, looks like it's already
> not in order. So... if you could try to put yours in the same order and maybe
> after the same method as in the header, that would at least not make things
> worse.

Done.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1073: out_file_path));
On 2014/08/27 20:49:14, dmichael wrote:
> nit: indent off by one

Done.

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:498: // renderer from running
arbitrary code in the plugin process.
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> "plugin process" -> "NaCl process" to clarify

Done.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:508: return true;
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> Maybe comment: We don't release the message to NaCl untrusted code
immediately. 
> We defer it until we get an async message back from the browser process.

Done.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:598: if (ipc_fd !=
IPC::InvalidPlatformFileForTransit()) {
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> Maybe handle the error case first for readability:
> 
> if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
>   RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
>   return;
> }

Done.

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:120: base::FilePath)>)>
On 2014/08/27 20:49:14, dmichael wrote:
> nit: Maybe a typedef for the internal one might make it more self-documenting?
> ResolveFileTokenResponse or something

Done.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:125: }
On 2014/08/27 20:49:14, dmichael wrote:
> Somewhere in here it might make sense to document what this class will do with
> the given callback and what it requires for the nested callback. E.g., when
the
> callback is invoked, it should respond later by calling the
> ResolveFileTokenResponse callback via PostTask on the same thread. (vs some
> other thread or calling while the outer callback is on the stack).

Done.

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:137: base::FilePath)> g_resolved_cb;
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> On 2014/08/27 20:49:14, dmichael (OOO 8.28-8.29) wrote:
> > I think the maybe map sounds best? I guess you're assuming you can only get
1
> at
> > a time? If that's the case, at least a DCHECK or something that the Cb is
> > invalid would be good.
> > 
> > Also, I suspect this gives you a static initializer as-is...
> 
> A mapping (from IDs to callbacks) would be better.  Can this be a member of
the
> NaClListener rather than a global?

open_resource() should only have one request in flight, so it's a bit simpler to
only have one live callback. I've moved this inside NaClListener.

I started to look at adding a map, but I realized it's possible to have two
open_resource() requests for the same file in flight, so the map would actually
have to be from a token to a queue of callbacks, which seems a little gross.

I think if we want open_resource() to support multiple requests in-flight, that
should happen in another change. Let me know if that's unreasonable.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:278: virtual bool
OnMessageReceived(const IPC::Message& msg) OVERRIDE {
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> Why is this handled in a new MessageFilter?  Can't the new message be handled
in
> NaClListener::OnMessageReceived() below?

This has to be handled in a new MessageFilter so that this work can run on the
IO thread. The main thread used by NaClListener is taken up by
NaClChromeMainAppStart(), so it can't be used for servicing messages.

https://codereview.chromium.org/418423002/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.h (right):

https://codereview.chromium.org/418423002/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.h:32: OpenResourceCallback;
On 2014/08/28 23:40:22, Mark Seaborn wrote:
> Nit: indent this

Done.

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:108: // currently works on Linux, but not on
Windows...
On 2014/08/28 21:33:49, Mark Seaborn wrote:
> Why doesn't it work on Windows?

I'm going to debug this. I wanted to get some feedback on this change and make
sure this was the right approach before I took the time to figure this out.

https://codereview.chromium.org/418423002/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:112: // See
ppapi/nacl_irt/manifest_service.cc for how this function's result is
On 2014/08/28 21:33:49, Mark Seaborn wrote:
> That is this file, in the function immediately below. :-)

Thanks, I've worded this a bit better now.

https://codereview.chromium.org/418423002/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/418423002/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:182:
NaClLog(LOG_ERROR,
On 2014/08/28 21:33:49, Mark Seaborn wrote:
> Can you make this reject the request, as a security-hardening measure, so that
> we're not exposing the old SRPC-based manifest file code path to the web?
> 
> Or would it be better to reject the request earlier, in
> PluginReverseInterface::OpenManifestEntry()?

Sure, I've changed this to reject the request (and do so earlier, in
OpenManifestEntry). Good call.

[teravest, 2014-09-04 22:22:41]

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

https://codereview.chromium.org/418423002/diff/300001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:603: RewriteMessage(orig_msg,
PpapiHostMsg_OpenResource::ID);
The failure on Windows is that we can't just rewrite the original message in
this case; the file tokens have to be cleared to zero.

After fixing this, I can add some DCHECKs() in manifest_service.cc to check that
we always return zero values for file tokens.

[Mark Seaborn, 2014-09-05 15:43:46]

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

https://codereview.chromium.org/418423002/diff/300001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:603: RewriteMessage(orig_msg,
PpapiHostMsg_OpenResource::ID);
On 2014/09/04 22:22:41, teravest wrote:
> The failure on Windows is that we can't just rewrite the original message in
> this case; the file tokens have to be cleared to zero.
> 
> After fixing this, I can add some DCHECKs() in manifest_service.cc to check
that
> we always return zero values for file tokens.

OK, I'll look again after you've done that.

BTW, a CHECK might be better than a DCHECK.  It's good to minimise differences
between release/debug builds, since the bots usually run release builds.

I think Chromium might enable DCHECKs on the _rel trybots, but I don't know if
that works for NaCl untrusted code.

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

https://codereview.chromium.org/418423002/diff/300001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:126: // renderer. When the file token
is resolvded, the
"resolved"

[teravest, 2014-09-06 02:25:15]

I was able to add the CHECKs for ensuring the file_token values are equal to
zero.

There were two bugs previously that were triggering those checks:
  1. File tokens were not cleared when a token failed to resolve.
  2. "Invalid" SerializedHandles were incorrectly serialized; extra information
was written, and a bit used to mark a file descriptor "valid" ended up getting
read in file_token_lo, causing it to have a value of 1 instead of 0.

Both of those are resolved now.

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

https://codereview.chromium.org/418423002/diff/300001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.h:126: // renderer. When the file token
is resolvded, the
On 2014/09/05 15:43:46, Mark Seaborn wrote:
> "resolved"

Done.

[Mark Seaborn, 2014-09-08 16:26:53]

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:54:
SerializedHandle::WriteHeader(handle.header(), msg);
BTW, this has a return valid that isn't checked here.  Would this only fail on
out-of-memory?

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:56: if (handle.type() !=
SerializedHandle::INVALID) {
Where is the corresponding reader for this format?  I can't find what reads the
"valid" field.

[teravest, 2014-09-08 20:25:28]

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:54:
SerializedHandle::WriteHeader(handle.header(), msg);
On 2014/09/08 16:26:53, Mark Seaborn wrote:
> BTW, this has a return valid that isn't checked here.  Would this only fail on
> out-of-memory?

I inspected SerializedHandle::WriteHeader and found the following underlying
calls to Pickle:
  WriteInt
  WriteUInt32
which, on inspection, appear to always return true.

https://codereview.chromium.org/418423002/diff/380001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:56: if (handle.type() !=
SerializedHandle::INVALID) {
On 2014/09/08 16:26:52, Mark Seaborn wrote:
> Where is the corresponding reader for this format?  I can't find what reads
the
> "valid" field.

The valid field is read out here:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ut...

I've added a comment to specify where the read side is.

[Mark Seaborn, 2014-09-09 04:42:30]

LGTM.  When you CQ this, please test against linux_rel_precise32 so that the
Non-SFI uses of this code path get tested too.

It would also be worth running the *_nacl_sdk trybots, in case they have other
corner-case uses of open_resource() that aren't covered by browser_tests.

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:278: virtual bool
OnMessageReceived(const IPC::Message& msg) OVERRIDE {
On 2014/09/04 22:13:30, teravest wrote:
> On 2014/08/28 23:40:22, Mark Seaborn wrote:
> > Why is this handled in a new MessageFilter?  Can't the new message be
handled
> in
> > NaClListener::OnMessageReceived() below?
> 
> This has to be handled in a new MessageFilter so that this work can run on the
> IO thread. The main thread used by NaClListener is taken up by
> NaClChromeMainAppStart(), so it can't be used for servicing messages.

Makes sense.  Can you add that as a comment on FileTokenMessageFilter, please?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/418423002/diff/400001/components/nacl/common/...
components/nacl/common/nacl_messages.h:105:
IPC_MESSAGE_CONTROL4(NaClProcessMsg_ResolveFileTokenAsyncReply,
Shouldn't this be "NaClProcessHostMsg_" since it's NaCl -> browser?

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

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:334: main_task_runner_ =
base::MessageLoop::current()->task_runner();
I can't see any uses of this.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:496: // We've received a valid file
token, but we have to validate it with the
This is a little misleading.  The token doesn't *have* to be validated.  The
NaCl process only has to exchange the token for a new FD in order to use the new
FD with identity-based validation caching.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:609:
new_msg->WriteInt(IPC::SyncMessage::GetMessageId(orig_msg));
Comment that this needs to be kept in sync with SyncMessage::WriteSyncHeader()? 
What ensures this is kept in sync?

Hmm, I suppose nacl_message_scanner.cc has that issue already.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:615: new_msg->WriteBool(true);
Comment that this is the "valid" field here?

What keeps this in sync with the ParamTraits implementations in
ipc_message_utils.cc?  (nacl_message_scanner.cc has this issue already too...)

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:616: new_msg->WriteInt(0);
Comment that this is the index into the message's array of FDs?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:619: new_msg->WriteUInt64(0);
Comment "// token_{lo,hi}" here?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:628: // The file token didn't resolve
successfully, so we give the
IIRC, this path only happens when the GetFilePath() call in nacl_process_host.cc
fails, when the token isn't found in the cache.

This code path isn't exercised by the tests, is it?  If not, can you comment
that here, and add a TEST= field to say how this can be manually tested?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:635: // OnMessageRecieved().
"Received"

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

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:134: // This function should be used be
provided to the ManifestChannel
Remove "be used"?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:135: // NaClIPCAdapter instead of the
one inside NaClListener; this one will always
You mean instead of the one in BrowserValidationDBProxy, right?

Can you comment that this is the "new" version of ResolveFileToken?

Oh, you do you mean the NaClListener::ResolveFileToken() you're adding.  It's
confusing that there's 3 ResolveFileToken()s.  Could you distinguish them more?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:136: // run the callback, even if the
NaClListener has been destroyed.
I'm pretty sure NaClListener never gets destroyed.  Its Listen() method runs an
event loop that shouldn't exit.  Rather than trying to handle something that
won't happen (and can't be tested), how about doing CHECK(g_listener)?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:199: virtual bool
ResolveFileToken(struct NaClFileToken* file_token,
Isn't this now only used for the initial nexe?  Can you comment that, and mark
it as the "old" code path?

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:286: if (g_listener)
Make this a CHECK() too?

[teravest, 2014-09-09 16:49:07]

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

https://codereview.chromium.org/418423002/diff/220001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:278: virtual bool
OnMessageReceived(const IPC::Message& msg) OVERRIDE {
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> On 2014/09/04 22:13:30, teravest wrote:
> > On 2014/08/28 23:40:22, Mark Seaborn wrote:
> > > Why is this handled in a new MessageFilter?  Can't the new message be
> handled
> > in
> > > NaClListener::OnMessageReceived() below?
> > 
> > This has to be handled in a new MessageFilter so that this work can run on
the
> > IO thread. The main thread used by NaClListener is taken up by
> > NaClChromeMainAppStart(), so it can't be used for servicing messages.
> 
> Makes sense.  Can you add that as a comment on FileTokenMessageFilter, please?

Done.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/418423002/diff/400001/components/nacl/common/...
components/nacl/common/nacl_messages.h:105:
IPC_MESSAGE_CONTROL4(NaClProcessMsg_ResolveFileTokenAsyncReply,
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> Shouldn't this be "NaClProcessHostMsg_" since it's NaCl -> browser?

I don't understand the difference between NaClProcessMsg and NaClProcessHostMsg.

NaClProcessMsg_QueryKnownToValidate is NaCl -> browser.
NaClProcessMsg_Start is browser -> NaCl.

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

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:334: main_task_runner_ =
base::MessageLoop::current()->task_runner();
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> I can't see any uses of this.

Removed.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:496: // We've received a valid file
token, but we have to validate it with the
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> This is a little misleading.  The token doesn't *have* to be validated.  The
> NaCl process only has to exchange the token for a new FD in order to use the
new
> FD with identity-based validation caching.

I've reworded this to be more precise, but it's a bit longer now. :(

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:609:
new_msg->WriteInt(IPC::SyncMessage::GetMessageId(orig_msg));
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> Comment that this needs to be kept in sync with
SyncMessage::WriteSyncHeader()? 
> What ensures this is kept in sync?
> 
> Hmm, I suppose nacl_message_scanner.cc has that issue already.

I've added a comment. Nothing ensures that this is kept in sync today.
I tried using SyncMessage::GenerateReply, but that caused some test failures, so
I didn't investigate further.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:615: new_msg->WriteBool(true);
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> Comment that this is the "valid" field here?
> 
> What keeps this in sync with the ParamTraits implementations in
> ipc_message_utils.cc?  (nacl_message_scanner.cc has this issue already too...)

I've added a comment. Nothing keeps this in sync, sadly. I don't like how this
is organized right now, but I can't reuse the implementation in ParamTraits with
the use of RewrittenMessage in this file.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:616: new_msg->WriteInt(0);
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> Comment that this is the index into the message's array of FDs?

Done.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:619: new_msg->WriteUInt64(0);
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> Comment "// token_{lo,hi}" here?

Done.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:628: // The file token didn't resolve
successfully, so we give the
On 2014/09/09 04:42:29, Mark Seaborn wrote:
> IIRC, this path only happens when the GetFilePath() call in
nacl_process_host.cc
> fails, when the token isn't found in the cache.
> 
> This code path isn't exercised by the tests, is it?  If not, can you comment
> that here, and add a TEST= field to say how this can be manually tested?

I've added a comment.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:635: // OnMessageRecieved().
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> "Received"

Done.

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

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:134: // This function should be used be
provided to the ManifestChannel
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> Remove "be used"?

I've removed this function. You're correct that NaClListener isn't destroyed, so
the indirection provided by this function is unnecessary.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:135: // NaClIPCAdapter instead of the
one inside NaClListener; this one will always
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> You mean instead of the one in BrowserValidationDBProxy, right?
> 
> Can you comment that this is the "new" version of ResolveFileToken?
> 
> Oh, you do you mean the NaClListener::ResolveFileToken() you're adding.  It's
> confusing that there's 3 ResolveFileToken()s.  Could you distinguish them
more?

Acknowledged.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:136: // run the callback, even if the
NaClListener has been destroyed.
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> I'm pretty sure NaClListener never gets destroyed.  Its Listen() method runs
an
> event loop that shouldn't exit.  Rather than trying to handle something that
> won't happen (and can't be tested), how about doing CHECK(g_listener)?

Acknowledged.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:199: virtual bool
ResolveFileToken(struct NaClFileToken* file_token,
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> Isn't this now only used for the initial nexe?  Can you comment that, and mark
> it as the "old" code path?

Done.

https://codereview.chromium.org/418423002/diff/400001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:286: if (g_listener)
On 2014/09/09 04:42:30, Mark Seaborn wrote:
> Make this a CHECK() too?

Done.

[dmichael (off chromium), 2014-09-09 19:19:44]

Just some nits. lgtm

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

https://codereview.chromium.org/418423002/diff/420001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:651: (NaClHandle) sh.descriptor().fd,
We don't cast at all on 570 and 572; are you sure you need these casts?

https://codereview.chromium.org/418423002/diff/420001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:675: new_fd = handle;
Why do you need to do this stuff? Could you just pass handle to
NaClDescIoDescFromHandleAllocCtor? PlatformFile and NaClHandle look like they
are defined to the same type on each platform (HANDLE for Win, int everywhere
else)

https://codereview.chromium.org/418423002/diff/420001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:686: // Mark the desc as OK for
mmapping.
I think this *really* marks it as safe for mapping as executable memory? I think
it's worth the distinction in the comment for future readers.

[teravest, 2014-09-09 19:30:09]

teravest@chromium.org changed reviewers:
+ tsepez@chromium.org

[teravest, 2014-09-09 19:30:10]

+tsepez for 
components/nacl/common/nacl_messages.h
ppapi/proxy/ppapi_messages.h

This change adds some file token fields that are communicated between the
plugin, renderer and browser processes when opening resources for a NaCl plugin.

The file tokens are used to request a file descriptor from the browser for a
nexe that's already been validated. This prevents a compromised renderer from
running arbitrary code in a plugin process.

These file token values are always cleared to 0 before being sent to the
"untrusted" side of the plugin process so they won't be exposed to any users.

[Tom Sepez, 2014-09-09 23:51:24]

Messages LGTM.

[teravest, 2014-09-09 23:54:39]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:06:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/418423002/440001

[commit-bot: I haz the power, 2014-09-10 01:08:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:08:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:08:42]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_rel_precise32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_rel_preci...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[teravest, 2014-09-10 15:24:08]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-10 15:31:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/418423002/460001

[commit-bot: I haz the power, 2014-09-10 18:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:25:09]

Try jobs failed on following builders:
  linux_rel_precise32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_rel_preci...)

[teravest, 2014-09-10 18:44:52]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:52:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/418423002/460001

[commit-bot: I haz the power, 2014-09-10 19:27:43]

Patchset 20 (id:??) landed as
https://crrev.com/cffdd030d9ecd5e2ef50de941fca337a94c3b476
Cr-Commit-Position: refs/heads/master@{#294208}

[teravest, 2014-09-10 19:29:06]

Committed patchset #20 (id:460001) manually as cffdd03 (presubmit successful).