Re: [Xen-devel] [PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> George Dunlap <george.dunlap@xxxxxxxxxx> 10/05/17 6:13 PM >>>
>On 10/04/2017 09:27 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> wrote:
>>> +    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
>>> +    {
>>> +        int i;
>> 
>> unsigned int (and then %u in the format strings below)
>
>Is there really an advantage to specifying 'unsigned int' for something
>like a loop variable?  It hardly seems worth the effort to consider
>signed / unsigned in such a case.

The latest when a loop variable is being used as array index it does matter on
most 64-bit architectures: Zero-extension (to machine word size) is often 
implied
by other operations, while sign-extension frequently requires an extra insn. 
This
may not matter much here, but I think it's better to follow the same common
pattern everywhere.

>>> @@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, 
>>> size_t size)
>>>          return 1;
>>>      }
>>>  
>>> -    if ( size > sizeof(input) )
>>> +    if ( size > sizeof(struct fuzz_corpus) )
>> 
>> What's the difference between the two variants?
>
>One fewer 'dereferences'.  Rather than input -> struct fuzz_corpus ->
>[structure definition], you can just do struct fuzz_corpus -> [structure
>definition].

;-)

>>> @@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, 
>>> size_t size)
>>>  
>>>      memcpy(&input, data_p, size);
>>>  
>>> -    state.corpus = &input;
>>> -    state.data_num = size;
>>> -
>>> -    setup_state(&ctxt);
>>> +    setup_fuzz_state(&state[0], data_p, size);
>>> +    
>>> +    if ( opt_rerun )
>>> +        printf("||| INITIAL RUN |||\n");
>>> +    
>>> +    runtest(&state[0]);
>>>  
>>> -    sanitize_input(&ctxt);
>>> +    if ( !opt_rerun )
>>> +        return 0;
>> 
>> Could I talk you into inverting the condition such that there'll be
>> only a single "return 0" at the end of the function?
>
>Why is that valuable?
>
>If I don't return here, then the rerun code has to be indented, which to
>me makes it slightly more difficult to see that it's identical to the
>state setup & running code above.

Then leave it this way, as being a matter of taste. I generally think that it is
helpful for functions to only have a single "main" return point (i.e. not
counting error paths), for readers to easily see the normal flow.

>> And then - has this patch actually helped pinpoint any problems? The
>> ones deaslt with by the next patch perhaps?
>
>Yes, it helped find the one dealt with in the subsequent patch.
>Surprisingly, it didn't find anything else.
>
>Since the patch represented a non-trivial amount of work, I thought it
>might be useful to include so nobody would have to re-implement it again
>in the future.  But I'd also be happy discarding this patch, as it's
>fairly invasive and I don't expect it to be used very often.

Oh, I'm certainly in favor of keeping this patch. I was rather trying to
understand whether with it in use the main (or all?) source(s) of instability
were found (and taken care of).

Jan


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