>>> On 25.09.17 at 16:26, <george.dun...@citrix.com> wrote: > --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c > +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c > @@ -14,6 +14,7 @@ extern unsigned int fuzz_minimal_input_size(void); > static uint8_t input[INPUT_SIZE]; > > extern bool opt_compact; > +extern bool opt_rerun;
Seeing a second such variable appear, I think it would really be better to introduce a local header, included by both producer and consumer. > @@ -886,21 +896,138 @@ int LLVMFuzzerInitialize(int *argc, char ***argv) > return 0; > } > > -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size) > +void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, > size_t size) static (also for other new helper functions)? > { > - 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 = (struct fuzz_corpus *)data_p; Please don't cast away constness here. Perhaps best to make the parameter const void *, allowing for the cast to be dropped altogether. > +int runtest(struct fuzz_state *state) { > int rc; > > - /* Reset all global state variables */ > - memset(&input, 0, sizeof(input)); > + struct x86_emulate_ctxt *ctxt = &state->ctxt; > + > + state->ops = all_fuzzer_ops; > + > + ctxt->data = state; > + ctxt->regs = &state->regs; > + ctxt->addr_size = ctxt->sp_size = 8 * sizeof(void *); Is this actually necessary? I don't see a way for set_sizes() to be bypassed. > +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; > + > + No double blank lines please. > + if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) ) > + { > + int i; unsigned int (and then %u in the format strings below) > + printf("State mismatch\n"); > + > + for ( i=0; i<5; i++) Blanks missing and please use ARRAY_SIZE() again (also further down). > + if ( state[0].cr[i] != state[1].cr[i] ) > + printf("cr[%d]: %lx != %lx\n", > + i, state[0].cr[i], state[1].cr[i]); > + > + for ( i=0; i<MSR_INDEX_MAX; i++) > + if ( state[0].msr[i] != state[1].msr[i] ) > + printf("msr[%d]: %lx != %lx\n", > + i, state[0].msr[i], state[1].msr[i]); > + > + for ( i=0; i<SEG_NUM; i++) > + if ( memcmp(&state[0].segments[i], &state[1].segments[i], > + sizeof(state[0].segments[0])) ) > + printf("segments[%d] differ!\n", i); The actual values would likely be helpful to be printed here, just like you do for all other state elements. > + 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 PINE(elem)\ > + if ( state[0].elem != state[1].elem ) \ > + printf(#elem " differ: %lx != %lx\n", \ > + (unsigned long)state[0].elem, \ > + (unsigned long)state[1].elem) > + PINE(regs.r15); > + PINE(regs.r14); > + PINE(regs.r13); > + PINE(regs.r12); > + PINE(regs.rbp); > + PINE(regs.rbx); > + PINE(regs.r10); > + PINE(regs.r11); > + PINE(regs.r9); > + PINE(regs.r8); > + PINE(regs.rax); > + PINE(regs.rcx); > + PINE(regs.rdx); > + PINE(regs.rsi); > + PINE(regs.rdi); > + > + for ( i = offsetof(struct cpu_user_regs, error_code) / > sizeof(unsigned); > + i < sizeof(state[1].regs)/sizeof(unsigned); i++ ) > + { > + printf("[%04lu] %08x %08x\n", I think this wants to be %04zu (or perhaps better %4zu or %04zx). Same for ctxt printing further down. > @@ -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? > @@ -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? And then - has this patch actually helped pinpoint any problems? The ones deaslt with by the next patch perhaps? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel