Add validation caching interface.

Add validation caching interface.

This CL paves the way for Chrome-side changes that will actually inject the
validation caching interface into the sel_ldr process.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2515
TEST= ./scons run_validation_cache_test

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

[Mark Seaborn, 2012-02-29 21:33:34]

Initial feedback...

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
src/trusted/service_runtime/sel_validate_image.c:34: void * cache_context =
nap->validation_cache_context;
Spacing style

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
src/trusted/service_runtime/sel_validate_image.c:37: /* Validation caching is
currently incompatable with stubout. */
'incompatible'

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/build.scons (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/build.scons:20: if env.Bit('target_x86'):
Add a TODO for ARM.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:1: /*
Create a subdirectory for these files, e.g. 'trusted/validator/cache'

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:41: typedef struct NaClValidationCache
{
Maybe 'NaClValidationCacheFuncs', because this isn't a cache instance.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:42: void *(*create_query)(void
*context);
-> CreateQuery.  We use CamelCase for method names in vtables.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:47: } NaClValidationCache;
How about adding 'void *context' as a member of the struct?  By passing the two
around as a pair you've effectively got a fat pointer.  You might as well
combine them into a single reference.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/validation_cache_test.cc (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache_test.cc:18: const char * nop =
Spacing style.  Use an array: nops[32] = "..."

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache_test.cc:28: const char * sse4 =
Is this SSE4 or SSE4.1?

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/x86/32/ncvalidate.c:38: if (cache)
if (cache != NULL)

same elsewhere

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/x86/32/ncvalidate.c:42: cache->add_data(query, (uint8_t *)
"x86-32", (size_t) 6);
Use:
const char val_type_thing[] = "x86-32";
... sizeof(val_type_thing) ...

[Nick Bray (chromium), 2012-02-29 22:58:08]

PTAL, two disagreements.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
src/trusted/service_runtime/sel_validate_image.c:34: void * cache_context =
nap->validation_cache_context;
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Spacing style

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/service_...
src/trusted/service_runtime/sel_validate_image.c:37: /* Validation caching is
currently incompatable with stubout. */
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> 'incompatible'

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/build.scons (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/build.scons:20: if env.Bit('target_x86'):
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Add a TODO for ARM.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:1: /*
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Create a subdirectory for these files, e.g. 'trusted/validator/cache'

Why?  A directory for two files, and adding a build.scons files seems like extra
clutter.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:41: typedef struct NaClValidationCache
{
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Maybe 'NaClValidationCacheFuncs', because this isn't a cache instance.

I wanted to avoid embedding to many implementation details in the type name. 
This seemed simplest.  We can talk in person.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:42: void *(*create_query)(void
*context);
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> -> CreateQuery.  We use CamelCase for method names in vtables.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache.h:47: } NaClValidationCache;
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> How about adding 'void *context' as a member of the struct?  By passing the
two
> around as a pair you've effectively got a fat pointer.  You might as well
> combine them into a single reference.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/validation_cache_test.cc (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache_test.cc:18: const char * nop =
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Spacing style.  Use an array: nops[32] = "..."

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/validation_cache_test.cc:28: const char * sse4 =
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Is this SSE4 or SSE4.1?

Done.

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/x86/32/ncvalidate.c:38: if (cache)
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> if (cache != NULL)
> 
> same elsewhere

Done, but why?

https://chromiumcodereview.appspot.com/9535001/diff/7006/src/trusted/validato...
src/trusted/validator/x86/32/ncvalidate.c:42: cache->add_data(query, (uint8_t *)
"x86-32", (size_t) 6);
On 2012/02/29 21:33:34, Mark Seaborn wrote:
> Use:
> const char val_type_thing[] = "x86-32";
> ... sizeof(val_type_thing) ...

Done.

[Mark Seaborn, 2012-03-01 00:26:20]

http://codereview.chromium.org/9535001/diff/13001/src/tools/validator_tools/n...
File src/tools/validator_tools/ncstubout.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/tools/validator_tools/n...
src/tools/validator_tools/ncstubout.c:43: code_size,  bundle_size,
&cpu_features, NULL);
You could fix the double space there

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.c:77: static NaClValidationCache
*g_validation_cache = NULL;
Move this to the global vars block at the top

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.c:79: /* TODO(ncbray) consolidate
Chrome's sel_ldr setup to a single function. */
NaClSetIrtFileDesc() is in sel_ldr.h so this should be, too.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:36: /* Validation caching is
currently incompatible with stubout. */
Technically, the second pass is not incompatible.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
File src/trusted/validator/validation_cache.h (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:18: * CreateQuery: create an opaque
query object, given an opaque context object.
If you could do examples, that would aid readability, e.g.

query = cache->CreateQuery(cache->context);
-- Returns a fresh query object.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:19: * The context object contains
persistant variables that will be used for all
'persistent'

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:23: * AddData: add a blob of binary
data to the query.  Conceptually, the query
query->AddData(query, data, data_size);
-- Adds a blob...

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:24: * will concatinate all the binary
data it is given, in the order it is given,
'concatenate'

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:32: * DoQuery: the key is complete,
query the validation status.  AddData must
validation_passed = query->DoQuery(query)

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:35: * set_status: set the database
entry for the given key.  DoQuery must be
set_status doesn't match.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:42: void *context;
Maybe 'handle'

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:44: void (*AddData)(void *query, const
unsigned char *data, size_t length);
uint8_t instead of unsigned char?

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:45: int (*DoQuery)(void *query);
Maybe: GetValidationStatus(), QueryValidationStatus(), QueryCodeValidating()

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:46: void (*SetValidates)(void *query);
Maybe: SetValidationStatus(), MarkCodeValidating(), SetValidationStatusToTrue()

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
File src/trusted/validator/validation_cache_test.cc (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:14: # define
UNREFERENCED_PARAMETER(P) (P)
Why is this here?  Use src/include/nacl_compiler_annotations.h

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:122: bundle_size = 32;
Rename the global code_size to bundle_size?

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:161:
TEST_F(ValidationCachingInterfaceTests, CacheHit) {
You can check cache hit more thoroughly by passing the validator code that
*doesn't* validate but having the cache say it does.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:199: memcpy(code_buffer, ret,
32);
32 -> code_size?

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:201: status = Validate();
This is C++ so you can write
NaClValidationStatus status = Validate();

or
EXPECT_EQ(Validate(), NaClValidationFailed);

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:203: EXPECT_EQ(1,
context.query_destroyed);
More conventional is:
EXPECT_EQ(context.query_destroyed, 1);

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:209: // This causes an SEH
exception to be thrown on Windows if the mutex is not
Unnecessary detail?  Just say: "Initialise the log module because the validator
uses it.  It'd be nice if we didn't have to..."

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
File src/trusted/validator/x86/32/ncvalidate.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:12: /* TODO(ncbray) is there a better
way to access validation stats? */
You could add a getter function

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:34: 
remove empty line, keep variables block together?

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:43: cache->AddData(query, (uint8_t *)
cpu_features, sizeof(NaClCPUFeaturesX86));
sizeof(*cpu_features) reduces the risk of skew

[Nick Bray (chromium), 2012-03-01 01:25:16]

Last call.

http://codereview.chromium.org/9535001/diff/13001/src/tools/validator_tools/n...
File src/tools/validator_tools/ncstubout.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/tools/validator_tools/n...
src/tools/validator_tools/ncstubout.c:43: code_size,  bundle_size,
&cpu_features, NULL);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> You could fix the double space there

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main_chrome.c:77: static NaClValidationCache
*g_validation_cache = NULL;
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Move this to the global vars block at the top

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:36: /* Validation caching is
currently incompatible with stubout. */
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Technically, the second pass is not incompatible.

Based on future plans, validating code twice without knowing if the first pass
modified it is a potential security hole.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
File src/trusted/validator/validation_cache.h (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:18: * CreateQuery: create an opaque
query object, given an opaque context object.
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> If you could do examples, that would aid readability, e.g.
> 
> query = cache->CreateQuery(cache->context);
> -- Returns a fresh query object.

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:19: * The context object contains
persistant variables that will be used for all
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> 'persistent'

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:24: * will concatinate all the binary
data it is given, in the order it is given,
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> 'concatenate'

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:32: * DoQuery: the key is complete,
query the validation status.  AddData must
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> validation_passed = query->DoQuery(query)

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:35: * set_status: set the database
entry for the given key.  DoQuery must be
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> set_status doesn't match.

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:42: void *context;
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Maybe 'handle'

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:44: void (*AddData)(void *query, const
unsigned char *data, size_t length);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> uint8_t instead of unsigned char?

Avoids dependencies.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:45: int (*DoQuery)(void *query);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Maybe: GetValidationStatus(), QueryValidationStatus(), QueryCodeValidating()

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache.h:46: void (*SetValidates)(void *query);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Maybe: SetValidationStatus(), MarkCodeValidating(),
SetValidationStatusToTrue()

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
File src/trusted/validator/validation_cache_test.cc (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:14: # define
UNREFERENCED_PARAMETER(P) (P)
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Why is this here?  Use src/include/nacl_compiler_annotations.h

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:122: bundle_size = 32;
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Rename the global code_size to bundle_size?

bundle_size and code_size only happen to be the same.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:161:
TEST_F(ValidationCachingInterfaceTests, CacheHit) {
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> You can check cache hit more thoroughly by passing the validator code that
> *doesn't* validate but having the cache say it does.

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:199: memcpy(code_buffer, ret,
32);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> 32 -> code_size?

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:201: status = Validate();
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> This is C++ so you can write
> NaClValidationStatus status = Validate();
> 
> or
> EXPECT_EQ(Validate(), NaClValidationFailed);

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:203: EXPECT_EQ(1,
context.query_destroyed);
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> More conventional is:
> EXPECT_EQ(context.query_destroyed, 1);

No.  EXPECT_EQ(EXPECTED, ACTUAL);

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/valid...
src/trusted/validator/validation_cache_test.cc:209: // This causes an SEH
exception to be thrown on Windows if the mutex is not
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> Unnecessary detail?  Just say: "Initialise the log module because the
validator
> uses it.  It'd be nice if we didn't have to..."

Detail is good in this case, it may provide breadcrumbs in the future.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
File src/trusted/validator/x86/32/ncvalidate.c (right):

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:12: /* TODO(ncbray) is there a better
way to access validation stats? */
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> You could add a getter function

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:34: 
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> remove empty line, keep variables block together?

Done.

http://codereview.chromium.org/9535001/diff/13001/src/trusted/validator/x86/3...
src/trusted/validator/x86/32/ncvalidate.c:43: cache->AddData(query, (uint8_t *)
cpu_features, sizeof(NaClCPUFeaturesX86));
On 2012/03/01 00:26:20, Mark Seaborn wrote:
> sizeof(*cpu_features) reduces the risk of skew

Done.

[bsy, 2012-03-01 19:22:49]

interface nits.

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:36: * AddData must not be called after
calling this function.
can this be invoked more than once?

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:40: * QueryCodeValidates must be called
first.
monotonic cache:  no way to clear the bit, except by hoping for the cache to
fill and evict an entry?  so the cache exhibits one-sided error, so that if
cache->QueryCodeValides(query) returns "false", that really means "unknown"? 
maybe rename it to something that suggests "KnownToValidate"?

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:43: * DestroyQuery: cleanup and
deallocate the query object.
if query could be NULL here, it would make some cleanup code simpler.  if it
took the address of the query, then it could also automatically set the variable
to be NULL, so that unless the caller copied the pointer after invoking
CreateQuery, we're unlikely to have use-after-free issues.

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/x86/32/ncvalidate.c:45: cache->DestroyQuery(query);
the cache->DestroyQuery invocations throughout here makes me want this to be
single return w/ goto statements to a single cleanup section at the end, before
the return, so that future edits won't introduce a leak (since we don't have
scoped ptrs in C).  the other alternative is to do the nested if, which isn't
much more readable.  but i'm ok with this code as is.

[Mark Seaborn, 2012-03-01 19:23:11]

LGTM with fixes below

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache.h:46: typedef struct NaClValidationCache
{
Suggestion: I'd be inclined to refer to it as 'struct NaClValidationCache' and
not use the typedef, for consistency with service_runtime and NaClDesc.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/validation_cache_test.cc (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:16: const char nop[CODE_SIZE+1] =
space around '+'

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:31: /* Sanity check that we're
getting the right object. */
Putting the comment on the same line as 'int marker' would make it clearer what
it's referring to.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:34: int set_validates_expected;
"expect_set_code_validates_call"?  This is C++, so you can use the "bool" type.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:35: int query_destroyed;
"bool"

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:40: QUERY_GET,
QUERY_GET_CALLED, QUERY_SET_CALLED?

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:56: EXPECT_EQ(31,
mcontext->marker);
Magic number, move to enum/const

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:57: mquery->marker = 37;
Ditto - magic number

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:67: UNREFERENCED_PARAMETER(data);
These should go as close to the function start as possible.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:69: /* Small data is supicious.
*/
'suspicious'

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:112: unsigned char
code_buffer[32];
32 -> CODE_SIZE

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:131: memset(code_buffer, 0x90,
32);
32 -> sizeof(code_buffer) or CODE_SIZE

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:148: cache.AddData(query, 0, 6);
0 -> NULL

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/x86/32/ncvalidate.c:12: /* HACK to get access to
didstubout */
I thought you were going to add a getter function for 'didstubout'?

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/x86/64/ncvalidate.c:13: /* HACK to get access to
did_stub_out */
Ditto.  Add an accessor function?  (At least you should write "TODO" not
"HACK".)

[Nick Bray (chromium), 2012-03-01 21:16:57]

Committed revision 7920.

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:36: * AddData must not be called after
calling this function.
On 2012/03/01 19:22:50, bsy wrote:
> can this be invoked more than once?

Done.

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:40: * QueryCodeValidates must be called
first.
On 2012/03/01 19:22:50, bsy wrote:
> monotonic cache:  no way to clear the bit, except by hoping for the cache to
> fill and evict an entry?  so the cache exhibits one-sided error, so that if
> cache->QueryCodeValides(query) returns "false", that really means "unknown"? 
> maybe rename it to something that suggests "KnownToValidate"?

Done.

https://chromiumcodereview.appspot.com/9535001/diff/11009/src/trusted/validat...
src/trusted/validator/validation_cache.h:43: * DestroyQuery: cleanup and
deallocate the query object.
On 2012/03/01 19:22:50, bsy wrote:
> if query could be NULL here, it would make some cleanup code simpler.  if it
> took the address of the query, then it could also automatically set the
variable
> to be NULL, so that unless the caller copied the pointer after invoking
> CreateQuery, we're unlikely to have use-after-free issues.

Skipping.  It's a marginal safety improvement for bad code, and it makes an
assumption there is a single, mutable query pointer - not a bad assumption, but
still an assumption.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/validation_cache.h (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache.h:46: typedef struct NaClValidationCache
{
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> Suggestion: I'd be inclined to refer to it as 'struct NaClValidationCache' and
> not use the typedef, for consistency with service_runtime and NaClDesc.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/validation_cache_test.cc (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:16: const char nop[CODE_SIZE+1] =
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> space around '+'

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:31: /* Sanity check that we're
getting the right object. */
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> Putting the comment on the same line as 'int marker' would make it clearer
what
> it's referring to.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:34: int set_validates_expected;
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> "expect_set_code_validates_call"?  This is C++, so you can use the "bool"
type.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:35: int query_destroyed;
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> "bool"

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:40: QUERY_GET,
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> QUERY_GET_CALLED, QUERY_SET_CALLED?

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:56: EXPECT_EQ(31,
mcontext->marker);
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> Magic number, move to enum/const

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:57: mquery->marker = 37;
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> Ditto - magic number

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:67: UNREFERENCED_PARAMETER(data);
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> These should go as close to the function start as possible.

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:69: /* Small data is supicious.
*/
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> 'suspicious'

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:112: unsigned char
code_buffer[32];
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> 32 -> CODE_SIZE

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:131: memset(code_buffer, 0x90,
32);
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> 32 -> sizeof(code_buffer) or CODE_SIZE

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/validation_cache_test.cc:148: cache.AddData(query, 0, 6);
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> 0 -> NULL

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/x86/32/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/x86/32/ncvalidate.c:12: /* HACK to get access to
didstubout */
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> I thought you were going to add a getter function for 'didstubout'?

Done.

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
File src/trusted/validator/x86/64/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/9535001/diff/19002/src/trusted/validat...
src/trusted/validator/x86/64/ncvalidate.c:13: /* HACK to get access to
did_stub_out */
On 2012/03/01 19:23:12, Mark Seaborn wrote:
> Ditto.  Add an accessor function?  (At least you should write "TODO" not
> "HACK".)

Done.