Check that the PNaCl cache hash is truly derived from the bitcode content.

Check that the PNaCl cache hash is truly derived from the bitcode content.

Do this before putting anything in the translation cache.
This catches errors where the user forgets to update the hash
and only updated the bitcode content.  If we move the hash
into the bitcode header, such an error is *much* less likely,
but could still happen if we have a bug in our drivers.

We don't check when there is a cache hit.  In the case of a
cache hit, we do not even attempt to download the bitcode.

Add a check for this in the pnacl_error_handling test.

Add a UMA error code ERROR_PNACL_CACHE_HASH_MISMATCH

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3414

[jvoung (off chromium), 2013-04-30 22:46:30]

dmichael is this separation of ppapi/native_client/src/trusted/plugin from
chrome's crypto useful (via a private "ppb" interface)?

Otherwise, can you review the renderer and ppapi code?

dschuff, can you review the PNaCl code?

Thanks!

[Derek Schuff, 2013-04-30 23:52:38]

https://codereview.chromium.org/14683004/diff/11001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:956:
nacl_hash_private_interface_->Update(bitcode_hash_verifier_,
The hash we put in the bitcode header is not the hash of the whole file, but
only the hash of the contents that are actually the bitcode. So this hash will
not match that one. 
I'm not sure how you are generating the hashes in the manifest now (presumably
just commandline sha256sum if this is actually working). If we do go with the
hash in the bitcode header, then will just read it directly and can also get the
offset of the actual bitcode and adjust this accordingly.

[jvoung (off chromium), 2013-05-01 00:01:30]

https://codereview.chromium.org/14683004/diff/11001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:956:
nacl_hash_private_interface_->Update(bitcode_hash_verifier_,
On 2013/04/30 23:52:38, Derek Schuff wrote:
> The hash we put in the bitcode header is not the hash of the whole file, but
> only the hash of the contents that are actually the bitcode. So this hash will
> not match that one. 
> I'm not sure how you are generating the hashes in the manifest now (presumably
> just commandline sha256sum if this is actually working). If we do go with the
> hash in the bitcode header, then will just read it directly and can also get
the
> offset of the actual bitcode and adjust this accordingly.

The current hash generator is a function in the SDK's generate_nmf.py.  It just
takes the whole file and hashes it to make this work.

Yes if we use the bitcode header hash, we would need to skip ahead N bytes
before hashing.

[dmichael (off chromium), 2013-05-01 17:22:15]

ppapi bits lgtm, thanks.

I do like this approach. Any time you can put logic in the renderer behind a
thin interface, I definitely prefer it over adding more code to the plugin.

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_hash_private.idl (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:15: mem_t CreateSHA256Hash();
Should that be "CreateSHA256Hasher" or "...HashObject" or something? Just to
make it clearer it's not returning an actual hash.

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:24: void Delete([inout] mem_t
hasher);
I think it's appropriate to just use [in] for the hasher param in all of these.

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:27: PP_Bool SecureMemEqual([in]
str_t hash1, [in] str_t hash2,
Any reason you're using str_t and mem_t elsewhere? It seems like mem_t is
equally appropriate here as for Update and Finish.

[Derek Schuff, 2013-05-01 17:32:10]

plugin/coordinator stuff LGTM for now.
I think might make more sense to use the hash in the bitcode header rather than
the manifest (one less thing for the developer to worry about), but we can sit
on this for now until we converge on the wire format a bit more.

[jvoung (off chromium), 2013-05-01 18:21:58]

On 2013/05/01 17:32:10, Derek Schuff wrote:
> plugin/coordinator stuff LGTM for now.
> I think might make more sense to use the hash in the bitcode header rather
than
> the manifest (one less thing for the developer to worry about), but we can sit
> on this for now until we converge on the wire format a bit more.

Yeah I agree, one less thing to go wrong while developing.

[jvoung (off chromium), 2013-05-01 18:22:59]

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_hash_private.idl (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:15: mem_t CreateSHA256Hash();
On 2013/05/01 17:22:15, dmichael wrote:
> Should that be "CreateSHA256Hasher" or "...HashObject" or something? Just to
> make it clearer it's not returning an actual hash.

Done.

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:24: void Delete([inout] mem_t
hasher);
On 2013/05/01 17:22:15, dmichael wrote:
> I think it's appropriate to just use [in] for the hasher param in all of
these.

Hmm, but Update() and Finish() can modify the hasher (the methods aren't const)?

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:27: PP_Bool SecureMemEqual([in]
str_t hash1, [in] str_t hash2,
On 2013/05/01 17:22:15, dmichael wrote:
> Any reason you're using str_t and mem_t elsewhere? It seems like mem_t is
> equally appropriate here as for Update and Finish.

Ah yes, will convert to mem_t.  Originally I had str_t, but changed it since the
original crypto/ headers use void* instead of char*. The original MemEqual also
takes a void*.

[jvoung (off chromium), 2013-05-01 18:24:00]

+bradnelson for chrome/test/data/nacl OWNERS

[bradn, 2013-05-01 18:26:40]

LGTM

[dmichael (off chromium), 2013-05-01 18:34:15]

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_hash_private.idl (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:24: void Delete([inout] mem_t
hasher);
On 2013/05/01 18:23:00, jvoung (cr) wrote:
> On 2013/05/01 17:22:15, dmichael wrote:
> > I think it's appropriate to just use [in] for the hasher param in all of
> these.
> 
> Hmm, but Update() and Finish() can modify the hasher (the methods aren't
const)?
But you don't modify the actual |hasher| pointer. It's basically an opaque
handle, much like PP_Resource. In those cases, where the "handle" itself isn't
modified, we just use [in].

[jvoung (off chromium), 2013-05-01 19:18:43]

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_hash_private.idl (right):

https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_hash_private.idl:24: void Delete([inout] mem_t
hasher);
On 2013/05/01 18:34:15, dmichael wrote:
> On 2013/05/01 18:23:00, jvoung (cr) wrote:
> > On 2013/05/01 17:22:15, dmichael wrote:
> > > I think it's appropriate to just use [in] for the hasher param in all of
> > these.
> > 
> > Hmm, but Update() and Finish() can modify the hasher (the methods aren't
> const)?
> But you don't modify the actual |hasher| pointer. It's basically an opaque
> handle, much like PP_Resource. In those cases, where the "handle" itself isn't
> modified, we just use [in].
> 

Do you mean that the pointer won't end up pointing to something else?

I think the issue is that these API files are normally for a C interface where
"const T*" means that the pointer is const, but the implementation and user of
this API is C++ where that syntax means that the pointed-to object is const?

[dmichael (off chromium), 2013-05-01 20:55:03]

On 2013/05/01 19:18:43, jvoung (cr) wrote:
>
https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
> File ppapi/api/private/ppb_nacl_hash_private.idl (right):
> 
>
https://codereview.chromium.org/14683004/diff/11001/ppapi/api/private/ppb_nac...
> ppapi/api/private/ppb_nacl_hash_private.idl:24: void Delete([inout] mem_t
> hasher);
> On 2013/05/01 18:34:15, dmichael wrote:
> > On 2013/05/01 18:23:00, jvoung (cr) wrote:
> > > On 2013/05/01 17:22:15, dmichael wrote:
> > > > I think it's appropriate to just use [in] for the hasher param in all of
> > > these.
> > > 
> > > Hmm, but Update() and Finish() can modify the hasher (the methods aren't
> > const)?
> > But you don't modify the actual |hasher| pointer. It's basically an opaque
> > handle, much like PP_Resource. In those cases, where the "handle" itself
isn't
> > modified, we just use [in].
> > 
> 
> Do you mean that the pointer won't end up pointing to something else?
> 
> I think the issue is that these API files are normally for a C interface where
> "const T*" means that the pointer is const, but the implementation and user of
> this API is C++ where that syntax means that the pointed-to object is const?
Okay, I see what you mean. Sorry for the confusion. I wasn't thinking [inout]
even made any semantic difference, but apparently it does for pointers. Not
clear to me that its behavior makes sense completely (I was wondering why
[inout] doesn't make it void**; for non-pointer types it should be adding * or
it's not really an "out" param at all).


Anyway, your stuff lgtm.

[commit-bot: I haz the power, 2013-05-01 21:59:37]

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

[jvoung (off chromium), 2013-05-01 22:22:10]

On 2013/05/01 21:59:37, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jvoung%40chromium.org/14683004/26001

Brad, I think only your @chromium.org account is an OWNER =)

[bradnelson, 2013-05-01 22:42:02]

lgtm

[commit-bot: I haz the power, 2013-05-01 22:52:00]

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

[commit-bot: I haz the power, 2013-05-02 00:23:07]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...