Enable meta-based validation for shared libraries.

Enable meta-based validation for shared libraries.

BUG= https://code.google.com/p/chromium/issues/detail?id=224434
R=bsy@google.com

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=11340

[Nick Bray (chromium), 2013-05-10 22:54:56]

This is a respin of a CL I sent to you as a preview.  I had to change
approaches, but I think this is the final approach.

https://codereview.chromium.org/15039022/diff/1/src/trusted/manifest_name_ser...
File src/trusted/manifest_name_service_proxy/manifest_proxy.c (right):

https://codereview.chromium.org/15039022/diff/1/src/trusted/manifest_name_ser...
src/trusted/manifest_name_service_proxy/manifest_proxy.c:169: desc =
NaClDescIoDescFromHandleAllocCtor((NaClHandle) new_fd,
Question: is there a better cross-platform way to deal with file-descriptors? 
NaClHandle is part of the IMC headers, so I didn't want to use that type as part
of the validation cache interface -  it is implemented in Chrome.

[bsy, 2013-05-10 23:52:56]

i think this is going to have problems with sequencing a consistent set of
changes in to both NaCl and Cr.  You probably should introduce a new virtual
function OpenManifestEntryEx in parallel to the existing OpenManifestEntry
function, so that the DEPS can roll.

o/w it looks good

https://codereview.chromium.org/15039022/diff/1/src/trusted/manifest_name_ser...
File src/trusted/manifest_name_service_proxy/manifest_proxy.c (right):

https://codereview.chromium.org/15039022/diff/1/src/trusted/manifest_name_ser...
src/trusted/manifest_name_service_proxy/manifest_proxy.c:169: desc =
NaClDescIoDescFromHandleAllocCtor((NaClHandle) new_fd,
On 2013/05/10 22:54:56, Nick Bray (chromium) wrote:
> Question: is there a better cross-platform way to deal with file-descriptors? 
> NaClHandle is part of the IMC headers, so I didn't want to use that type as
part
> of the validation cache interface -  it is implemented in Chrome.

We can add a better interface to wrap up the composition of NaClDescIoDescMake
NaClHostDescPosix[MT]ake, which would get rid of the dependency/use of
NaClHandle.  Or we can open code that composition here.  I think I prefer the
former.

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
File src/trusted/reverse_service/reverse_service.cc (right):

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
src/trusted/reverse_service/reverse_service.cc:110: return
wrapper->iface->OpenManifestEntry(nacl::string(url_key), info);
isn't this implemented in chrome?

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
File src/trusted/reverse_service/reverse_service.h (right):

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
src/trusted/reverse_service/reverse_service.h:81: virtual bool
OpenManifestEntry(nacl::string url_key,
isn't this going to cause the chrome-side code to deregister, since the impl no
longer overrides this?

[Nick Bray (chromium), 2013-05-13 20:27:44]

PTAL

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
File src/trusted/reverse_service/reverse_service.h (right):

https://codereview.chromium.org/15039022/diff/1/src/trusted/reverse_service/r...
src/trusted/reverse_service/reverse_service.h:81: virtual bool
OpenManifestEntry(nacl::string url_key,
On 2013/05/10 23:52:56, bsy wrote:
> isn't this going to cause the chrome-side code to deregister, since the impl
no
> longer overrides this?

Done.

[bsy, 2013-05-13 21:28:52]

a couple of style nits.  okay to ignore but please consider.

lgtm.

https://codereview.chromium.org/15039022/diff/18001/src/trusted/validator/val...
File src/trusted/validator/validation_cache.c (right):

https://codereview.chromium.org/15039022/diff/18001/src/trusted/validator/val...
src/trusted/validator/validation_cache.c:184: if (Deserialize(buffer,
buffer_length, file_path, info->file_path_length,
just fyi.  consider assigning to info->file_path after Deserialize to express
intent that the data becomes owned by the info object, which isn't going to
modify its contents.  this complicates the cleanup code in on_error code path,
but i'd just set file_path to NULL after passing ownership (as well as
initialize to NULL at declaration, and have the error code path free that as
well as invoking the dtor.

https://codereview.chromium.org/15039022/diff/18001/src/trusted/validator/val...
src/trusted/validator/validation_cache.c:210: RichFileInfoCtor(info);
reads funny.  maybe just memset directly here?  invoking ctor makes readers
think that the object is initialized and contain live data.

[commit-bot: I haz the power, 2013-05-13 22:09:31]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/ncbray@chromium.org/15039022/38004

[commit-bot: I haz the power, 2013-05-13 22:09:39]

Presubmit check for 15039022-38004 failed and returned exit status 1.

INFO:root:Found 16 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/nacl/PRESUBMIT.py
Error processing
/b/commit-queue/workdir/nacl/site_scons/site_tools/library_deps.py:
  Cannot execute command ['pychecker', '-stdlib']. failed reason: [Errno 2] No
such file or directory 

** Presubmit Warnings **
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:
cpplint
 
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:48:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:52:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:55:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:58:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:85:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:90:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:117:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:127:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:136:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:143:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:150:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:162:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:173:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:189:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:197:
 Extra space before ( in function call  [whitespace/parens] [4]
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h:205:
 Extra space before ( in function call  [whitespace/parens] [4]
Done processing
/b/commit-queue/workdir/nacl/src/trusted/reverse_service/reverse_service_c.h
Total errors found: 16


Presubmit checks took 2.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Nick Bray (chromium), 2013-05-13 22:12:33]

Committed patchset #3 manually as r11340 (presubmit successful).