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 197c98ddcbfcf1c8b73ed6c2a26096cd549be40d..0b9e9974b5d0995e02f3b747ae49be3b0a97912f 100644
--- a/src/trusted/service_runtime/sel_validate_image.c
+++ b/src/trusted/service_runtime/sel_validate_image.c
@@ -27,30 +27,67 @@ 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);
-  /* Avoid linking two validators into Chromium to keep download size small. */
-#if defined(__arm__) || !defined(NACL_STANDALONE)
-  UNREFERENCED_PARAMETER(nap);
+Bool UseDfaValidator() {
+  if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
+    return TRUE;
+  }
+  return FALSE;
+}
+
+void NaClSelectValidator(struct NaClValidatorInterface **val) {
+  /* TODO: make it more nested. */

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

I started with it being nested, but unless you indent the preprocessor
directives, it looks horrible.  Hence the linear structure. :/

[pasko-google - do not use, 2012/05/12 12:18:40]

to me it looks almost equally horrible, so I'd better keep your preference

+#if defined(__i386__) && defined(NACL_STANDALONE)
+  if (UseDfaValidator()) {
+    fprintf(stderr, "DANGER! USING THE UNSTABLE DFA VALIDATOR!\n");

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

Nit: Experimental?  Untested?

[pasko-google - do not use, 2012/05/12 12:18:40]

Done.

+    NaClDfaValidatorInit_x86_32(val);
+  } else {
+    NaClValidatorInit_x86_32(val);
+  }
+#elif defined(__i386__)
+  NaClValidatorInit_x86_32(val);
+#elif defined(__x86_64__) && defined(NACL_STANDALONE)
+  if (UseDfaValidator()) {
+    fprintf(stderr, "DANGER! USING THE UNSTABLE DFA VALIDATOR!\n");
+    NaClDfaValidatorInit_x86_64(val);
+  } else {
+    NaClValidatorInit_x86_64(val);
+  }
+#elif defined(__x86_64__)
+  NaClValidatorInit_x86_64(val);
+#elif defined(__arm__)
+  NaClValidatorInitArm(val);
 #else
-  if (nap->enable_dfa_validator) {
-    ret = NACL_SUBARCH_NAME(ApplyDfaValidator,
-                            NACL_TARGET_ARCH, NACL_TARGET_SUBARCH);
+#error "No validator available for this architecture!"
+#endif
+}
+
+#if 0
+void NaClSelectValidator(struct NaClApp *nap) {
+  nap->validate_func = NACL_SUBARCH_NAME(ApplyValidator,
+                                         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
 }
+#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);
+  struct NaClValidatorInterface *validator = nap->validator;
 
   if (size < kMinimumCachedCodeSize) {
     /*
@@ -79,26 +116,27 @@ int NaClValidateCode(struct NaClApp *nap, uintptr_t guest_addr,
             "stub_out_mode and fixed_feature_cpu_mode are incompatible\n");
     return LOAD_VALIDATION_FAILED;
   }
+
   if (nap->validator_stub_out_mode) {
     /* Validation caching is currently incompatible with stubout. */
     cache = NULL;
     /* 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 = validator->Validate(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 = validator->Validate(guest_addr, data, size,
+                                 FALSE, /* do not stub out */
+                                 readonly_text,
+                                 &nap->cpu_features,
+                                 cache);
   }
   return NaClValidateStatus(status);
 }
@@ -114,11 +152,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->validator->ValidateCodeReplacement(
+      guest_addr, data_old, data_new, size, &nap->cpu_features));
 }
 
 int NaClCopyCode(struct NaClApp *nap, uintptr_t guest_addr,
@@ -130,11 +165,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->validator->ValidateCopy(

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

Bad name.  Validating a copy of what?  Hence why I suggested "CopyCode".  I
understand why you want to have all the function have the same prefix, but I
think it confuses what the verb is supposed to be.  (Copy, not Validate.)

+      guest_addr, data_old, data_new, size, &nap->cpu_features));
 }
 
 NaClErrorCode NaClValidateImage(struct NaClApp  *nap) {