Enable mmap and identity-based validation caching on pnacl-{llc,ld}.nexe

Enable mmap and identity-based validation caching on pnacl-{llc,ld}.nexe

Register the executables w/ the validation cache on open and hook up the
file info just like the other NaCl module loads.

The non-executable files are still opened without the windows-specific
FLAG_EXEC.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3614
TEST= ValidationCacheOfTranslatorNexes

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280605

[teravest, 2014-06-26 21:19:25]

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_file_host.cc:42: // so that there is a function
pointer.
I'm confused. Why can't you make a function pointer to
NaClHostMsg_GetReadonlyPnaclFD::WriteReplyParams?

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_file_host.cc:67: base::File file,
something is funky in the indentation for the arguments here.

https://codereview.chromium.org/356923004/diff/40001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/356923004/diff/40001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:548: uint64_t *nonce_lo,
uint64_t *nonce_hi) {
Chromium style is "uint64_t* nonce_lo".
It trips me up all the time that Chromium and NaCl differ there.

Arguments should probably be one per line here.

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:251: PP_NaClFileInfo
file_info_for_ipc = kInvalidNaClFileInfo;
I don't think you need to introduce file_info_for_ipc here.
The current code always uses kInvalidNaClFileInfo since this code path isn't
used for Non-SFI.

I have a change to move all this nexe loading to happen over IPC instead of
SRPC; just have to wrap up a failure that's happening on windows.

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:55: // File handles /
info for llc and ld executables, after they've been opened.
you can drop the "handles /" here.

[jvoung (off chromium), 2014-06-26 22:48:39]

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_file_host.cc:42: // so that there is a function
pointer.
On 2014/06/26 21:19:24, teravest wrote:
> I'm confused. Why can't you make a function pointer to
> NaClHostMsg_GetReadonlyPnaclFD::WriteReplyParams?

Hmm, Bind was failing to infer the template parameter. I added a typedef
instead.

https://codereview.chromium.org/356923004/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_file_host.cc:67: base::File file,
On 2014/06/26 21:19:24, teravest wrote:
> something is funky in the indentation for the arguments here.

Done.

https://codereview.chromium.org/356923004/diff/40001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/356923004/diff/40001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:548: uint64_t *nonce_lo,
uint64_t *nonce_hi) {
On 2014/06/26 21:19:24, teravest wrote:
> Chromium style is "uint64_t* nonce_lo".
> It trips me up all the time that Chromium and NaCl differ there.
> 
> Arguments should probably be one per line here.

oops -- done.

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:251: PP_NaClFileInfo
file_info_for_ipc = kInvalidNaClFileInfo;
On 2014/06/26 21:19:24, teravest wrote:
> I don't think you need to introduce file_info_for_ipc here.
> The current code always uses kInvalidNaClFileInfo since this code path isn't
> used for Non-SFI.
> 
> I have a change to move all this nexe loading to happen over IPC instead of
> SRPC; just have to wrap up a failure that's happening on windows.

Okay, will leave this as it was.

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

https://codereview.chromium.org/356923004/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:55: // File handles /
info for llc and ld executables, after they've been opened.
On 2014/06/26 21:19:24, teravest wrote:
> you can drop the "handles /" here.

Done.

[Nick Bray (chromium), 2014-06-27 21:03:16]

LGTM

https://codereview.chromium.org/356923004/diff/80001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/356923004/diff/80001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:123: is_executable ?
Nit: build the flags before the call to avoid redundant code and improve
clarity.

https://codereview.chromium.org/356923004/diff/80001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/356923004/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1463: false /* executable*/,
nit: space before */

[teravest, 2014-06-27 21:29:55]

lgtm

[jvoung (off chromium), 2014-06-27 22:30:08]

Thanks!

+tsepez for nacl_host_messages OWNERS

https://codereview.chromium.org/356923004/diff/80001/components/nacl/browser/...
File components/nacl/browser/nacl_browser.cc (right):

https://codereview.chromium.org/356923004/diff/80001/components/nacl/browser/...
components/nacl/browser/nacl_browser.cc:123: is_executable ?
On 2014/06/27 21:03:16, Nick Bray (chromium) wrote:
> Nit: build the flags before the call to avoid redundant code and improve
> clarity.

Done.

https://codereview.chromium.org/356923004/diff/80001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/356923004/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1463: false /* executable*/,
On 2014/06/27 21:03:16, Nick Bray (chromium) wrote:
> nit: space before */

Done.

[Tom Sepez, 2014-06-27 22:41:56]

The messages themselves LG, but I don't have the background to understand
whether this allows a (compromised) renderer to access a file that it wouldn't
have been able to before.

[jvoung (off chromium), 2014-06-27 22:46:28]

+jln who looked at this before -- I just wasn't sure if he was OOO already or
not.

Also a pointer to where the IPC goes -- it attempts to check the filename.

https://codereview.chromium.org/356923004/diff/120001/components/nacl/browser...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/356923004/diff/120001/components/nacl/browser...
components/nacl/browser/nacl_file_host.cc:180: bool PnaclCanOpenFile(const
std::string& filename,
This attempts to only allow the file to have a-z, 0-9, and underscore, along
with a known prefix "pnacl_public_" and only if it's based within the PNaCl
component updater subdirectory, under user-data-dir.

[Tom Sepez, 2014-06-27 22:48:02]

Ok, I'll give you a LGTM based on the above explanation. jln, please object if
I've missed something.

[jvoung (off chromium), 2014-06-30 15:19:49]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-06-30 15:20:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jvoung@chromium.org/356923004/120001

[commit-bot: I haz the power, 2014-06-30 16:08:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-06-30 16:18:56]

Change committed as 280605