Refactor the process of choosing validators.

src/trusted/service_runtime/sel_ldr.h

 /*
  * Copyright (c) 2012 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 /*
  * NaCl Simple/secure ELF loader (NaCl SEL).
  *
  * This loader can only process NaCl object files as produced using
@@ skipping 36 matching lines @@
 #include "native_client/src/trusted/service_runtime/nacl_kern_services.h"
 #include "native_client/src/trusted/service_runtime/nacl_resource.h"
 
 #include "native_client/src/trusted/service_runtime/sel_mem.h"
 #include "native_client/src/trusted/service_runtime/sel_util.h"
 #include "native_client/src/trusted/service_runtime/sel_rt.h"
 
 #include "native_client/src/trusted/service_runtime/name_service/name_service.h"
 
 #include "native_client/src/trusted/validator/cpufeatures.h"
+#include "native_client/src/trusted/validator/validation_status.h"
 
 EXTERN_C_BEGIN
 
 #define NACL_SERVICE_PORT_DESCRIPTOR    3
 #define NACL_SERVICE_ADDRESS_DESCRIPTOR 4
 
 #define NACL_DEFAULT_STACK_MAX  (16 << 20)  /* main thread stack */
 
 #define NACL_SANDBOX_CHROOT_FD  "SBX_D"
 
@@ skipping 24 matching lines @@
   NACL_DEBUG_EXCEPTION_HANDLER_STARTED,
   NACL_DEBUG_EXCEPTION_HANDLER_FAILED
 };
 /*
  * Callback function used to request that an exception handler be
  * attached using the Windows debug API.  See sel_main_chrome.h.
  */
 typedef int (*NaClAttachDebugExceptionHandlerFunc)(void *info, size_t size);
 #endif
 
+/* Function type for applying a Validator.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateFunc) (

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

This is part of the validator interface, not part of sel_ldr.  My instinct is
that these function definitions (and the validation status enums) should be in
src/validator/ncvalidate.h.  The new interface will eventually replace the old,
no reason to add a new file.

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

Done.

+    uintptr_t guest_addr,
+    uint8_t *data,
+    size_t size,
+    int stubout_mode,
+    int readonly_text,
+    const NaClCPUFeatures *cpu_features,
+    struct NaClValidationCache *cache);
+
+/* Function type for applying a Validator to copy code from an existing code
+ * segment to a new code segment.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateCopyFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features);
+
+/* Function type for applying a Validator on small updates to previously
+ * validated code segments.
+ * This function is a part of NaCl Validator API: trusted/validator/ncvalidate.h
+ */
+typedef NaClValidationStatus (*NaClValidateCodeReplacementFunc) (
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeatures *cpu_features);
+
+/* Make a messy choice for validating functions and record it in NaClApp. */
+void NaClSelectValidator(struct NaClApp *nap);
+
 struct NaClApp {
   /*
    * public, user settable prior to app start.
    */
   uint8_t                   addr_bits;
   uintptr_t                 stack_size;
   /*
    * stack_size is the maximum size of the (main) stack.  The stack
    * memory is eager allocated (mapped in w/o MAP_NORESERVE) so
    * there must be enough swap space; page table entries are not
@@ skipping 237 matching lines @@
 
   int                       enable_debug_stub;
   struct NaClDebugCallbacks *debug_stub_callbacks;
   struct NaClMutex          exception_mu;
   uint32_t                  exception_handler;
   int                       enable_exception_handling;
 #if NACL_WINDOWS
   enum NaClDebugExceptionHandlerState debug_exception_handler_state;
   NaClAttachDebugExceptionHandlerFunc attach_debug_exception_handler_func;
 #endif
-  int                       enable_dfa_validator;
+
+  /* Functions for applying validation. */
+  NaClValidateFunc          validate_func;

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

I was envisioning these functions in a separate (but embedded) structure that
also contained the bundle_size parameter.  (bundle_size can be dealt with later,
but it is fundamentally a property of the validation model) This would allow
each validator to declare a InitValidatorInterface(...) function to fill out
this structure.  Less coupling.

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

well, I thought about this same possibility at first, but then on callsites:
nap->validate_func(...) is less clutter than:
nap->validator_interface->validate_func(...)
and then I'm not pleased

Less coupling is good when we have use cases for using components separately or
abstractions that are very natural (and named intuitively). struct
ValidatorInterface does not sound as a positive name for reader's intuition.

Generally I do not mind this decoupling, but for that please give advice on
naming the struct and fields. 

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

Yeah, the deref chain isn't pretty, but I figured it was just the cost of doing
business.  You could always do it in two steps:

validator = nap->validator;
validator->validate(...);

... or even 3, if you wanted to go crazy.
The type could be NaClValidatorInterface, but the field in NaClApp could just be
"validator".

If these functions were put inside a structure, you could also drop the _func
postfix.  validate_copy could also be changed to copy_code - it doesn't
"validate" beyond requiring that it can decode the instructions.

I think there's precedent for function pointers being CamelCase, too.

+  NaClValidateCopyFunc      validate_copy_func;
+  NaClValidateCodeReplacementFunc validate_code_replacement_func;
 };
 
 
 
 void  NaClAppIncrVerbosity(void);
 
 /*
  * Initializes a NaCl application with the default parameters
  * and the specified syscall table.
  *
@@ skipping 407 matching lines @@
 
 #endif
 
 #if NACL_LINUX
 void handle_r_debug(const char *switch_value, char *argv0);
 #endif
 
 EXTERN_C_END
 
 #endif  /* NATIVE_CLIENT_SRC_TRUSTED_SERVICE_RUNTIME_SEL_LDR_H_ */