Move validator_x86_XX.rl out of unreviewed.

src/trusted/validator_ragel/validator_x86_32.rl

 /*
  * 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.
  */
 
+/*
+ * This is the core of ia32-mode validator.  Please note that this file
+ * combines ragel machine description and C language actions.  Please read
+ * validator_internals.html first to understand how the whole thing is built:
+ * it explains how the byte sequences are constructed, what constructs like
+ * “@{}” or “REX_WRX?” mean, etc.
+ */
+
 #include <assert.h>
 #include <errno.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
-#include "native_client/src/trusted/validator_ragel/unreviewed/validator_internal.h"
+#include "native_client/src/trusted/validator_ragel/validator_internal.h"
 
 /* Ignore this information: it's not used by security model in IA32 mode.  */
 #undef GET_VEX_PREFIX3
 #define GET_VEX_PREFIX3 0
 #undef SET_VEX_PREFIX3
 #define SET_VEX_PREFIX3(P)
 
 %%{
   machine x86_32_validator;
   alphtype unsigned char;
@@ skipping 19 matching lines @@
   include modrm_parsing_ia32_noactions
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include immediate_fields_actions
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include immediate_fields_parsing_ia32
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   action rel8_operand {
     rel8_operand(current_position + 1, data, jump_dests, size,
                  &instruction_info_collected);
   }
   action rel16_operand {

[Brad Chen, 2012/10/05 16:47:21]

Can you add comments to remind readers what kind of instructions
rel8/rel16/rel32 correspond to?

In general this file needs more comments...

[khim, 2012/10/15 16:38:57]

I'm not so sure. I doubt you can ever understand these file if you'll try to
just read through it without reading validator_internals.html (or,
alternatively, Ragel documentation and AMD/Intel manuals).

I agree that we may need to add some specific comments which explain minutiae
details not mentioned in validator_internals.html, but I'm not sure it's
possible to make it understandable for someone who does not want to read
documentation.  This task is similar to the task of commenting out C++ program
enough to make it understandable for someone who only knows PHP and Visual Basic
- I'm not sure if it's possible even in principle and I'm pretty sure I can not
do that.

[Brad Chen, 2012/10/16 00:16:55]

I'm not sure either but I'd like you to try. Even for somebody who has read the
documentation file, it is not reasonable to expect them to have memorized every
detail. Comments in this file will help with that.

Commenting is another style rule, not a technical question, hence not an area I
think you should be anticipating a lot of flexibility from me. If done
thoughtfully it will make the code better. That's something we should all be
able to be happy about.

On 2012/10/15 16:38:57, khim wrote:

     #error rel16_operand should never be used in nacl
   }
   action rel32_operand {
     rel32_operand(current_position + 1, data, jump_dests, size,
                   &instruction_info_collected);
   }
   include relative_fields_parsing
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
   include cpuid_actions
     "native_client/src/trusted/validator_ragel/unreviewed/parse_instruction.rl";
 
   action last_byte_is_not_immediate {
     instruction_info_collected |= LAST_BYTE_IS_NOT_IMMEDIATE;
   }
 
   include decode_x86_32 "validator_x86_32_instruction.rl";
 
   special_instruction =
     (0x83 0xe0 0xe0 0xff (0xd0|0xe0)  | # naclcall/jmp %eax
      0x83 0xe1 0xe0 0xff (0xd1|0xe1)  | # naclcall/jmp %ecx
      0x83 0xe2 0xe0 0xff (0xd2|0xe2)  | # naclcall/jmp %edx
      0x83 0xe3 0xe0 0xff (0xd3|0xe3)  | # naclcall/jmp %ebx
      0x83 0xe4 0xe0 0xff (0xd4|0xe4)  | # naclcall/jmp %esp
      0x83 0xe5 0xe0 0xff (0xd5|0xe5)  | # naclcall/jmp %ebp
      0x83 0xe6 0xe0 0xff (0xd6|0xe6)  | # naclcall/jmp %esi
      0x83 0xe7 0xe0 0xff (0xd7|0xe7))   # naclcall/jmp %edi

[Brad Chen, 2012/10/05 16:47:21]

Note: This is a case where repetition seems okay to me. Note how the pattern and
structure makes it easy to see what's different from line to line, and the
comment makes it clear why one line is different from the next.

[khim, 2012/10/15 16:38:57]

Even so: I've added one minor change to separate and and jmp/call.

     @{
-      BitmapClearBit(valid_targets, (current_position - data) - 1);
+      MakeInvalidJumpTarget((current_position - data) - 1, valid_targets);
       instruction_start -= 3;
       instruction_info_collected |= SPECIAL_INSTRUCTION;
     } |
     (0x65 0xa1 (0x00|0x04) 0x00 0x00 0x00      | # mov %gs:0x0/0x4,%eax
      0x65 0x8b (0x05|0x0d|0x015|0x1d|0x25|0x2d|0x35|0x3d)
           (0x00|0x04) 0x00 0x00 0x00);           # mov %gs:0x0/0x4,%reg
 
   # Check if call is properly aligned
   call_alignment =
     ((one_instruction &
       # Direct call
       ((data16 0xe8 rel16) |

[Brad Chen, 2012/10/05 16:47:21]

This looks like this file would benefit from the style of commenting in the
other file. Fortunately there isn't as much to do in here.

[khim, 2012/10/15 16:38:57]

All the pieces in this file have analogues in validator_x86_64.rl, but
validator_x86_64.rl contains many more corner cases.

[Brad Chen, 2012/10/16 00:16:55]

While what you state is true, it doesn't change the fact that this file needs
comments.

On 2012/10/15 16:38:57, khim wrote:

        (0xe8 rel32))) |
      (special_instruction &
       # Indirect call
       (any* data16? 0xff ((opcode_2 | opcode_3) any* &
                           (modrm_memory | modrm_registers)))))
     @{
       if (((current_position - data) & kBundleMask) != kBundleMask)
         instruction_info_collected |= BAD_CALL_ALIGNMENT;
     };
 
   main := ((call_alignment | one_instruction | special_instruction)
      >{
-       BitmapSetBit(valid_targets, current_position - data);
+       MakeJumpTargetValid(current_position - data, valid_targets);
      }
      @{
        if ((instruction_info_collected &
                                (VALIDATION_ERRORS_MASK | BAD_CALL_ALIGNMENT)) ||

[Brad Chen, 2012/10/05 16:47:21]

This doesn't look like standard indentation. Boolean expressions should be
indented to make there structure more obvious.

[khim, 2012/10/15 16:38:57]

This was the goal :-(

But this is moot point since BAD_CALL_ALIGNMENT is no longer checked here
(similarly to the production validator it's not an error, but warning now), so
this line fits in 80 characters limit.

            (options & CALL_USER_CALLBACK_ON_EACH_INSTRUCTION)) {
          result &= user_callback(instruction_start, current_position,
                                  instruction_info_collected, callback_data);
        }
        /* On successful match the instruction start must point to the next byte
         * to be able to report the new offset as the start of instruction
         * causing error.  */
        instruction_start = current_position + 1;
        instruction_info_collected = 0;
      })*
     $err{
         result &= user_callback(instruction_start, current_position,
                                 UNRECOGNIZED_INSTRUCTION, callback_data);
         continue;
     };
 
 }%%
 
 %% write data;
 
 
 Bool ValidateChunkIA32(const uint8_t *data, size_t size,
                        enum validation_options options,
                        const NaClCPUFeaturesX86 *cpu_features,
-                       validation_callback_func user_callback,
+                       ValidationCallbackFunc user_callback,
                        void *callback_data) {
   bitmap_word valid_targets_small;
   bitmap_word jump_dests_small;
   bitmap_word *valid_targets;
   bitmap_word *jump_dests;
   const uint8_t *current_position;
   const uint8_t *end_of_bundle;
   int result = TRUE;
 
   CHECK(sizeof valid_targets_small == sizeof jump_dests_small);
@@ skipping 38 matching lines @@
                                       user_callback, callback_data);
 
   /* We only use malloc for a large code sequences  */
   if (size > sizeof valid_targets_small) {
     free(jump_dests);
     free(valid_targets);
   }
   if (!result) errno = EINVAL;
   return result;
 }