Refactor the process of choosing validators.

src/trusted/validator/x86/32/ncvalidate.c

Index: src/trusted/validator/x86/32/ncvalidate.c
diff --git a/src/trusted/validator/x86/32/ncvalidate.c b/src/trusted/validator/x86/32/ncvalidate.c
index d28b56fd9f64c7be14da424da5d470ac8b1a936a..350281b4e138338abf07cadae57780c0d935329b 100644
--- a/src/trusted/validator/x86/32/ncvalidate.c
+++ b/src/trusted/validator/x86/32/ncvalidate.c
@@ -23,7 +23,7 @@
 # endif
 #endif
 
-NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidator, NACL_TARGET_ARCH, 32) (
+static NaClValidationStatus ApplyValidator_x86_32(
     uintptr_t guest_addr,
     uint8_t *data,
     size_t size,
@@ -90,12 +90,12 @@ NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidator, NACL_TARGET_ARCH, 32) (
       ? NaClValidationSucceeded : NaClValidationFailed;
 }
 
-NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, x86, 32)
-    (uintptr_t guest_addr,
-     uint8_t *data_old,
-     uint8_t *data_new,
-     size_t size,
-     const NaClCPUFeaturesX86 *cpu_features) {
+static NaClValidationStatus ApplyValidatorCodeReplacement_x86_32(
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeaturesX86 *cpu_features) {
   /* Check that the given parameter values are supported. */
   if (!NaClArchSupported(cpu_features))
     return NaClValidationFailedCpuNotSupported;
@@ -104,3 +104,64 @@ NaClValidationStatus NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, x86, 32)
                                size, cpu_features)
       ? NaClValidationSucceeded : NaClValidationFailed;
 }
+
+/* Copy a single instruction, avoiding the possibility of other threads
+ * executing a partially changed instruction.
+ */
+static Bool CopyInstruction(NCDecoderStatePair* tthis,

[Mark Seaborn, 2012/06/14 20:11:56]

"tthis" is a strange name.  Use "self"?

[pasko-google - do not use, 2012/06/17 14:46:45]

I moved this code as-is from /src/trusted/validator_x86/nccopycode.c as-is,
without any cleanup changes to make it trivial to compare during review. I can
make suggested cleanup changes in this code in later changes if you are willing
to approve them quickly.

+                            NCDecoderInst *dinst_old,

[Mark Seaborn, 2012/06/14 20:11:56]

You're mixing "*" spacing style in adjacent lines.  Please check the whole patch
for mixed-up "*" spacing.

[pasko-google - do not use, 2012/06/17 14:46:45]

same here, not my code

+                            NCDecoderInst *dinst_new) {
+  NCRemainingMemory* mem_old = &dinst_old->dstate->memory;
+  NCRemainingMemory* mem_new = &dinst_new->dstate->memory;
+
+  return tthis->copy_func(mem_old->mpc, mem_new->mpc, mem_old->read_length);
+}
+
+/* Copies code from src to dest in a thread safe way, returns 1 on success,
+ * returns 0 on error. This will likely assert on error to avoid partially
+ * copied code or undefined state.
+ */
+static int NCCopyCode(uint8_t *dst, uint8_t *src, NaClPcAddress vbase,
+                      size_t sz, const NaClCopyInstructionFunc copy_func) {

[Mark Seaborn, 2012/06/14 20:11:56]

Nit: this "const" isn't needed.

[pasko-google - do not use, 2012/06/17 14:46:45]

Done.

+  NCDecoderState dst_dstate;
+  NCDecoderInst  dst_inst;
+  NCDecoderState src_dstate;
+  NCDecoderInst  src_inst;
+  NCDecoderStatePair pair;
+  int result = 0;
+
+  NCDecoderStateConstruct(&dst_dstate, dst, vbase, sz, &dst_inst, 1);
+  NCDecoderStateConstruct(&src_dstate, src, vbase, sz, &src_inst, 1);
+  NCDecoderStatePairConstruct(&pair, &dst_dstate, &src_dstate, copy_func);
+  pair.action_fn = CopyInstruction;
+  if (NCDecoderStatePairDecode(&pair)) result = 1;
+  NCDecoderStatePairDestruct(&pair);
+  NCDecoderStateDestruct(&src_dstate);
+  NCDecoderStateDestruct(&dst_dstate);
+
+  return result;
+}
+
+static NaClValidationStatus ApplyValidatorCopy_x86_32(
+    uintptr_t guest_addr,
+    uint8_t *data_old,
+    uint8_t *data_new,
+    size_t size,
+    const NaClCPUFeaturesX86 *cpu_features,
+    const NaClCopyInstructionFunc copy_func) {

[Mark Seaborn, 2012/06/14 20:11:56]

Ditto about the const

[pasko-google - do not use, 2012/06/17 14:46:45]

Done.

+  if (!NaClArchSupported(cpu_features))
+    return NaClValidationFailedCpuNotSupported;
+
+  return ((0 == NCCopyCode(data_old, data_new, guest_addr, size, copy_func))
+            ? NaClValidationFailed : NaClValidationSucceeded);
+}
+
+static const struct NaClValidatorInterface validator = {
+  ApplyValidator_x86_32,
+  ApplyValidatorCopy_x86_32,
+  ApplyValidatorCodeReplacement_x86_32,
+};
+
+const struct NaClValidatorInterface* NaClValidatorCreate_x86_32() {
+  return &validator;
+}