Refactor the process of choosing validators.

src/trusted/service_runtime/sel_validate_image.c

Index: src/trusted/service_runtime/sel_validate_image.c
diff --git a/src/trusted/service_runtime/sel_validate_image.c b/src/trusted/service_runtime/sel_validate_image.c
index 5b30195b388942ba310e2fbe74fd8e8d3d3155d2..fd26125982a613136d89894f66fe7efca9a818bc 100644
--- a/src/trusted/service_runtime/sel_validate_image.c
+++ b/src/trusted/service_runtime/sel_validate_image.c
@@ -27,29 +27,58 @@ static int NaClValidateStatus(NaClValidationStatus status) {
   }
 }
 
-typedef NaClValidationStatus (*ValidateFunc) (
-    uintptr_t, uint8_t*, size_t, int, int,
-    const NaClCPUFeatures*, struct NaClValidationCache*);
-
-static ValidateFunc NaClSelectValidator(struct NaClApp *nap) {
-  ValidateFunc ret = NACL_SUBARCH_NAME(ApplyValidator,
-                                       NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
-#ifdef __arm__
-  UNREFERENCED_PARAMETER(nap);
-#else
-  if (nap->enable_dfa_validator) {
-    ret = NACL_SUBARCH_NAME(ApplyDfaValidator,
-                            NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
+static NaClValidationStatus ValidatorCopyNotImplemented(

[Nick Bray, 2012/04/25 20:57:42]

I think that each validator that doesn't implement a function should explicitly
provide these stubs itself.  sel_ldr selects a validator, and then the validator
fills out the function table - sel_ldr doesn't need to know the implemented /
unimplemented details.

You could have a .c file in src/trusted/validator containing these stubs, but
linking would be easier if each validator declared their own stub.  (It would
also be better documentation and provide a skeleton function to fill out?  I
realize this may seem to contradict my prior "no stub for ARM" advice, but that
was about adding a new entry point, not about stubbing out the existing entry
points.)

[pasko-google - do not use, 2012/04/26 15:17:01]

hm, we seem to agreed that validator selection is a tricky process involving a
lot of parameters: environment vars, platform (maybe OS?), NaClApp, etc. And it
is hard to regularize this process. So, if we leave this choice partially to
each of the "validators", then we risk separating this process into parts
non-trivially which would only increase complication.

Imagine the need to check for the same env var in many files identically. Or
having to pass many args to InitializeValidator(...) which would take more lines
than a simple choice in a single selection function.

If the function grows beyond 50 lines of code, I am more open to reconsidering
it's structure and separation. Decoupling too early is like premature
optimization.
Moving stubs to a separate utility file sounds good. I'll postpone it for a
later iteration of this review. Filename suggestions wanted.

[Nick Bray, 2012/04/27 00:41:30]

As I see it, each "validator" will always behave the same, and will always have
the same interface functions.  The only choice is which validator.  Can you
describe a specific case where the decision logic would start to leak into the
validator or the interface initialization?  I'll take a shot at figuring out how
it could be cleaned up.

[pasko-google - do not use, 2012/04/27 17:32:11]

Purely hypothetical: if we want to choose validator strategy via nacl manifest.
Less hypothetical: we'd want to patch TLS access sequences on some OS/Machine
configurations, but not on others (for speedup), where platform qualification
may be involved.

Okay, I made a draft change with full control over the choice of validation to
the platform-dependent files. I could not separate not-implemented stubs into a
separate lib because the build system won the battle. As a result there is much
more boilerplate code now: stubs, function declarations, trivial implementations
repeated all over. Though anyway, looks better than it was.

I uploaded a draft change. Please, look at the general idea, do not pay
attention to various extra TODOs left there temporarily. Enjoy code duplication
:)

Also, I could not make it to run the validation_cache_test, getting undefined
reference to 'NaCl_mprotect', no idea why and how I could affect that.

Also, one funny/annoying thing, this stuff gets built even if I ask to only
build platform=x86-32:
scons-out/dbg-linux-x86-32/obj/src/trusted/validator_arm/libncvalidate_arm_v2.a

Tired.

[Nick Bray, 2012/04/27 22:21:36]

That would affect the choice of validator and would not leak into any particular
validator.  What we're doing would support this cleanly.
Hmmm.
1) Add a "patch_TLS" argument to the validator in question.
2) Create two new entry points that wrap this validator, one passing "1", the
other passing "0".
3) Either treat the two new entry points as "different validators", or
parameterize the interface construction function.

Alternatively, this was suggested by bsy, we could move from a table of pointers
to a "object oriented" approach where there was data associated with each
validator.  I rather not make that change until we need to, however.
I take no pleasure that the stubs got duplicated, I just know not to bet against
the build system.
I suspect that we're relying on accidental dependencies, somewhere, somehow?
Yeah, I've thought about fixing that, but I figured as long as I was refactoring
things it was a "feature" - more compile errors per compile!
... of working on this CL?  (I'd happily take over.)  Or are you warning me your
thoughts are not as clear as you'd like?

+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features) {
+  UNREFERENCED_PARAMETER(guest_addr);
+  UNREFERENCED_PARAMETER(data_old);
+  UNREFERENCED_PARAMETER(data_new);
+  UNREFERENCED_PARAMETER(size);
+  UNREFERENCED_PARAMETER(cpu_features);
+  return NaClValidationFailedNotImplemented;
+}
+
+static NaClValidationStatus ValidatorCodeReplacementNotImplemented(
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features) {
+  UNREFERENCED_PARAMETER(guest_addr);
+  UNREFERENCED_PARAMETER(data_old);
+  UNREFERENCED_PARAMETER(data_new);
+  UNREFERENCED_PARAMETER(size);
+  UNREFERENCED_PARAMETER(cpu_features);
+  return NaClValidationFailedNotImplemented;
+}
+
+void NaClSelectValidator(struct NaClApp *nap) {
+  nap->validate_func = NACL_SUBARCH_NAME(ApplyValidator,

[Nick Bray, 2012/04/25 20:57:42]

I'd reorganize this into mutually exclusive ifdefs that call validator-specific
functions to fill out the function table.  Figure that this is going to be
expanded to deal with x86-32 and x86-64 seperately?  In some ways it seems like
a step back, but if done right it should make what validator is being used
explicit.

I think (maybe not in this CL) that this function should be moved into seperate
file (in src/trusted/validator?) so that it can include header files for
different validators without poluting sel_ldr's namespace, etc.

#if !defined(__arm__)
#if defined(NACL_STANDALONE)
  if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
    NACL_SUBARCH_NAME(InitDfaValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
  } else {
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
  }
#else
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
#endif
#else
    NACL_SUBARCH_NAME(InitStandardValidator,
          NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
#endif

[pasko-google - do not use, 2012/04/26 15:17:01]

That was my first thought as well. Mutually exclusive cases are easy to read.
However, then we get compiler complaints that the stubs are left unused for some
platforms. Then we may decide to make the stubs non-static, but that pollutes
the namespace.

So I can make it non-static mutually exclusive if you express this preference.
Ouch. So many functions, each of them making a couple of assignments. Are you
sure so much decoupling benefits anyone?

[Nick Bray, 2012/04/27 00:41:30]

But if you require each validator declare its own stubs, no compiler problem. :)
Each of those functions will actually be a single assignment:

*validator = {
  ValidatorFunc,
  ReplaceFunc,
  CopyFunc,
  VALIDATOR_BUNDLE_SIZE
}

Or something like that.  Declarative is good.  (I am not sure if I like this
idea, but each validator could just declare a const global interface that
NaClApp could point to.)  If this method is used, modifications to a given
validator will not require changes to the service runtime, except if the name of
the init function changes.  Keeping the initialization in a function keeps the
function table consistent if it's specified multiple times: see standalone vs.
non standalone.  This scheme would also allow us to write generic tests that
iterate over different validators, but that's just a dream right now.

I think, ultimately, I am pushing this design because it allows the validator to
declare its interface.  Declarative is the idea that motivates me.

BTW, that code snippet was ugly, let me take it to its logical conclusion:

#if defined(__i386__) && defined(NACL_STANDALONE)

  if (UseDFAValidator()) {
    NaCl_DFAValidatorInit_x86_32(&nap->validator);
  } else {
    NaCl_ValidatorInit_x86_32(&nap->validator);
  }

#elif defined(__i386__)

  NaCl_ValidatorInit_x86_32(&nap->validator);

#elif defined(__x86_64__) && defined(NACL_STANDALONE)

  if (UseDFAValidator()) {
    NaCl_DFAValidatorInit_x86_64(&nap->validator);
  } else {
    NaCl_ValidatorInit_x86_64(&nap->validator);
  }

#elif defined(__x86_64__)

  NaCl_ValidatorInit_x86_64(&nap->validator);

#elif defined(__arm__)

  NaCl_ValidatorInit_ARM(&nap->validator);

#else
#error "No validator available for this architecture!"
#endif

+                                         NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
+#if !defined(__arm__) && defined(NACL_STANDALONE)
+  if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
+    fprintf(stderr, "DANGER! USING THE UNSTABLE DFA VALIDATOR!\n");
+    nap->validate_func = NACL_SUBARCH_NAME(ApplyDfaValidator,
+        NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
   }
 #endif
-  return ret;
+  nap->validate_copy_func = ValidatorCopyNotImplemented;
+  nap->validate_code_replacement_func = ValidatorCodeReplacementNotImplemented;
+#ifndef __arm__
+  nap->validate_copy_func = NACL_SUBARCH_NAME(ApplyValidatorCopy,
+      NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
+  nap->validate_code_replacement_func = NACL_SUBARCH_NAME(
+      ApplyValidatorCodeReplacement, NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
+#endif
 }
 
 int NaClValidateCode(struct NaClApp *nap, uintptr_t guest_addr,
                      uint8_t *data, size_t size) {
   NaClValidationStatus status = NaClValidationSucceeded;
   struct NaClValidationCache *cache = nap->validation_cache;
-  ValidateFunc validate_func = NaClSelectValidator(nap);
 
   if (size < kMinimumCachedCodeSize) {
     /*
@@ -84,20 +113,20 @@ int NaClValidateCode(struct NaClApp *nap, uintptr_t guest_addr,
     /* In stub out mode, we do two passes.  The second pass acts as a
        sanity check that bad instructions were indeed overwritten with
        allowable HLTs. */
-    status = validate_func(guest_addr, data, size,
-                           TRUE, /* stub out */
-                           FALSE, /* text is not read-only */
-                           &nap->cpu_features,
-                           cache);
+    status = nap->validate_func(guest_addr, data, size,
+                                TRUE, /* stub out */
+                                FALSE, /* text is not read-only */
+                                &nap->cpu_features,
+                                cache);
   }
   if (status == NaClValidationSucceeded) {
     /* Fixed feature CPU mode implies read-only. */
     int readonly_text = nap->fixed_feature_cpu_mode;
-    status = validate_func(guest_addr, data, size,
-                           FALSE, /* do not stub out */
-                           readonly_text,
-                           &nap->cpu_features,
-                           cache);
+    status = nap->validate_func(guest_addr, data, size,
+                                FALSE, /* do not stub out */
+                                readonly_text,
+                                &nap->cpu_features,
+                                cache);
   }
   return NaClValidateStatus(status);
 }
@@ -113,11 +142,8 @@ int NaClValidateCodeReplacement(struct NaClApp *nap, uintptr_t guest_addr,
     return LOAD_BAD_FILE;
   }
 
-  return NaClValidateStatus(
-      NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement,
-                        NACL_TARGET_ARCH,
-                        NACL_TARGET_SUBARCH)
-      (guest_addr, data_old, data_new, size, &nap->cpu_features));
+  return NaClValidateStatus(nap->validate_code_replacement_func(
+      guest_addr, data_old, data_new, size, &nap->cpu_features));
 }
 
 int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
@@ -129,11 +155,8 @@ int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
    * before reaching this.
    */
   if (nap->fixed_feature_cpu_mode) return LOAD_BAD_FILE;
-  return NaClValidateStatus(
-      NACL_SUBARCH_NAME(ApplyValidatorCopy,
-                        NACL_TARGET_ARCH,
-                        NACL_TARGET_SUBARCH)
-      (guest_addr, data_old, data_new, size, &nap->cpu_features));
+  return NaClValidateStatus(nap->validate_copy_func(
+      guest_addr, data_old, data_new, size, &nap->cpu_features));
 }
 
 NaClErrorCode NaClValidateImage(struct NaClApp  *nap) {