Re: [PATCH v4 10/12] fuzz/x86_emulate: Add --rerun option to try to track down instability

[George Dunlap <george.dunlap@citrix.com>]

On 10/12/2017 04:24 PM, Jan Beulich wrote:
>>>> On 11.10.17 at 19:52, <george.dunlap@citrix.com> wrote:
>> @@ -884,20 +891,146 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
>>      return 0;
>>  }
>>  
>> -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>> +static void setup_fuzz_state(struct fuzz_state *state, const void *data_p, size_t size)
>>  {
>> -    struct fuzz_state state = {
>> -        .ops = all_fuzzer_ops,
>> -    };
>> -    struct x86_emulate_ctxt ctxt = {
>> -        .data = &state,
>> -        .regs = &state.regs,
>> -        .addr_size = 8 * sizeof(void *),
>> -        .sp_size = 8 * sizeof(void *),
>> -    };
>> +    memset(state, 0, sizeof(*state));
>> +    state->corpus = data_p;
>> +    state->data_num = size;
>> +}
>> +
>> +static int runtest(struct fuzz_state *state) {
>>      int rc;
>>  
>> -    if ( size <= fuzz_minimal_input_size() )
>> +    struct x86_emulate_ctxt *ctxt = &state->ctxt;
> 
> Please don't leave a blank line between declarations.
> 
>> +static void compare_states(struct fuzz_state state[2])
>> +{
>> +    /* First zero any "internal" pointers */
>> +    state[0].corpus = state[1].corpus = NULL;
>> +    state[0].ctxt.data = state[1].ctxt.data = NULL;
>> +    state[0].ctxt.regs = state[1].ctxt.regs = NULL;
>> +
>> +    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
>> +    {
>> +        unsigned int i;
>> +
>> +        printf("State mismatch\n");
>> +
>> +        for ( i = 0; i < ARRAY_SIZE(state[0].cr); i++ )
>> +            if ( state[0].cr[i] != state[1].cr[i] )
>> +                printf("cr[%u]: %lx != %lx\n",
>> +                       i, state[0].cr[i], state[1].cr[i]);
>> +        
>> +        for ( i = 0; i < ARRAY_SIZE(state[0].msr); i++ )
>> +            if ( state[0].msr[i] != state[1].msr[i] )
>> +                printf("msr[%u]: %lx != %lx\n",
>> +                       i, state[0].msr[i], state[1].msr[i]);
>> +        
>> +        for ( i = 0; i < ARRAY_SIZE(state[0].segments); i++ )
>> +            if ( memcmp(&state[0].segments[i], &state[1].segments[i],
>> +                        sizeof(state[0].segments[0])) )
>> +                printf("segments[%u]: [%x:%x:%x:%lx] != [%x:%x:%x:%lx]!\n", i,
>> +                       (unsigned)state[0].segments[i].sel,
>> +                       (unsigned)state[0].segments[i].attr,
>> +                       state[0].segments[i].limit,
>> +                       state[0].segments[i].base,
>> +                       (unsigned)state[1].segments[i].sel,
>> +                       (unsigned)state[1].segments[i].attr,
>> +                       state[1].segments[i].limit,
>> +                       state[1].segments[i].base);
>> +
>> +        if ( state[0].data_num != state[1].data_num )
>> +            printf("data_num: %lx !=  %lx\n", state[0].data_num,
>> +                   state[1].data_num);
>> +        if ( state[0].data_index != state[1].data_index )
>> +            printf("data_index: %lx !=  %lx\n", state[0].data_index,
>> +                   state[1].data_index);
>> +
>> +        if ( memcmp(&state[0].regs, &state[1].regs, sizeof(state[0].regs)) )
>> +        {
>> +            printf("registers differ!\n");
>> +            /* Print If Not Equal */
>> +#define PRINT_NE(elem)\
>> +            if ( state[0].elem != state[1].elem ) \
>> +                printf(#elem " differ: %lx != %lx\n", \
>> +                       (unsigned long)state[0].elem, \
>> +                       (unsigned long)state[1].elem)
>> +            PRINT_NE(regs.r15);
>> +            PRINT_NE(regs.r14);
>> +            PRINT_NE(regs.r13);
>> +            PRINT_NE(regs.r12);
>> +            PRINT_NE(regs.rbp);
>> +            PRINT_NE(regs.rbx);
>> +            PRINT_NE(regs.r10);
>> +            PRINT_NE(regs.r11);
>> +            PRINT_NE(regs.r9);
>> +            PRINT_NE(regs.r8);
>> +            PRINT_NE(regs.rax);
>> +            PRINT_NE(regs.rcx);
>> +            PRINT_NE(regs.rdx);
>> +            PRINT_NE(regs.rsi);
>> +            PRINT_NE(regs.rdi);
> 
> Aren't these register fields all of the same type? If so, why do you
> need to casts to unsigned long in the macro?

As it happens, they're all the same size; when I wrote the macro it was
designed such that the same macro could be used for all the elements
regardless of what size they were.  Since there's no time pressure,
would you rather I add the segment registers (and leave the cast), or
only add rflags (and remove the cast)?

> 
>> +            for ( i = offsetof(struct cpu_user_regs, error_code) / sizeof(unsigned);
>> +                  i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
> 
> Blanks around binary operators please (also elsewhere).

Ack

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel