Issue 282870043: i#1734 Dr. Fuzz: add corpus fuzzing
DescriptionCommit log for first patchset:
---------------
i#1734 Dr. Fuzz: add corpus fuzzing
- add option fuzz_corpus for corpus directory
- add corpus_table, corpus_vec, and mutator_vec
- add fuzzer_read_corpus_list to read file list from corpus directory
- update fuzzer_init to call fuzzer_read_corpus_list
- update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus
- pre_fuzz_corpus
* corpus iteration: execute corpus input
* mutation iteration: execute mutate input
- post_fuzz_corpus
* dump input if discover new basic blocks in mutation iteration
---------------
Patch Set 1 #Patch Set 2 : PTAL #Patch Set 3 : fix a style issue #Patch Set 4 : fixed two minor WINDOWS bug #
Total comments: 72
Patch Set 5 : PTAL #
Total comments: 17
Patch Set 6 : final #Patch Set 7 : add code to limit single thread fuzzing #Patch Set 8 : Committed #
MessagesTotal messages: 14
I still do not like this CL much, too complex, not simple enough. Let me know if you have a better way to implement.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus fuzzing - add option fuzz_corpus for corpus directory - add corpus_table, corpus_vec, and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus iteration: execute corpus input * mutation iteration: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus fuzzing - add option fuzz_corpus for corpus directory - add corpus_table, corpus_vec, and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus iteration: execute corpus input * mutation iteration: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus fuzzing - add option fuzz_corpus for corpus directory - add corpus_table, corpus_vec, and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus iteration: execute corpus input * mutation iteration: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
Maybe it's worth another look as there are some bugs. Overall it looks pretty good and most of my review comments are requesting more comments in the code. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c File drmemory/fuzzer.c (left): https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode97 drmemory/fuzzer.c:97: byte *input_buffer_copy; /* threadsafe copy of the fuzz target's buffer arg */ So you removed the use of this for non-corpus-based usage models as well? Could you add that to the commit message? This is to avoid the memcpy cost? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode101 drmemory/fuzzer.c:101: uint64 num_bbs; /* number of basic blocks seen */ These fields were marked as read by another thread and now they are not: is that accurate? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode... drmemory/fuzzer.c:1092: fuzz_state->input_buffer_copy = global_alloc(fuzz_state->input_size, HEAPSTAT_MISC); See comment at top: how does removing this relate to corpus-based fuzzing? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode42 drmemory/fuzzer.c:42: # pragma comment(lib, "User32.lib") It is best to avoid user32.dll as the private loader is not able to duplicate it, so it is shared with the app. Why do we need user32? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode96 drmemory/fuzzer.c:96: uint corpus_index; /* index in corpus_vec indeicating which has been executed */ error: spelling https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode97 drmemory/fuzzer.c:97: uint mutator_index; /* index in mutator_vec indeicating which to be fuzzed */ error: spelling (again) https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode98 drmemory/fuzzer.c:98: bool mutation; /* perform mutation on on mutators from mutator_vec */ error: "on on" https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode98 drmemory/fuzzer.c:98: bool mutation; /* perform mutation on on mutators from mutator_vec */ suggest: s/mutation/should_mutate/ (b/c "mutation" sounds like data, not a flag) https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode99 drmemory/fuzzer.c:99: bool orig_input; /* run with original input from app */ suggest: s/orig_input/use_orig_input/ https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode154 drmemory/fuzzer.c:154: hashtable_t corpus_table; /* hashtable for looking up loaded corpus */ suggest: expand comment to explain what the types of the keys and values are and what the synch model is https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode155 drmemory/fuzzer.c:155: drvector_t corpus_vec; /* vector for loaded corpus */ How does this relate to corpus_table? Please explain the invariants in a comment (exact same entries in each?). https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode155 drmemory/fuzzer.c:155: drvector_t corpus_vec; /* vector for loaded corpus */ What is the synch model for this? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode156 drmemory/fuzzer.c:156: drvector_t mutator_vec; /* vector for created mutators */ What is the synch model for this? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode166 drmemory/fuzzer.c:166: /* Protects the fuzz_state_t field input_buffer and input_size for access from error: s/field/fields/ https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode230 drmemory/fuzzer.c:230: fuzzer_read_corpus_list(void) Will this be called periodically during a run? Will multiple threads call it? Seems worth adding comments to the top about its assumptions so we can understand whether the code is safe + optimal. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode236 drmemory/fuzzer.c:236: if ((dir = opendir(options.fuzz_corpus)) == NULL) { style violation: Program Structure #19 https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode242 drmemory/fuzzer.c:242: while ((ent = readdir(dir)) != NULL) { style violation: Program Structure #19 https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode250 drmemory/fuzzer.c:250: hashtable_add(&corpus_table, (void *)ent->d_name, (void *)1); Xref earlier comment: would help to know you're storing 1 https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode261 drmemory/fuzzer.c:261: HANDLE hFind = INVALID_HANDLE_VALUE; style violation: var name https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode264 drmemory/fuzzer.c:264: bool has_sep; unused var https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode269 drmemory/fuzzer.c:269: strcat(path, "*"); bug: could overflow -- how about using single dr_snprintf (see below) https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode272 drmemory/fuzzer.c:272: strcat(path, "\\*"); bug: could overflow -- how about using single dr_snprintf (see below) https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode274 drmemory/fuzzer.c:274: } suggest: double \ is fine so just have a single dr_snprintf + NULL_TERMINATE_BUFFER which seems much simpler and safer (if really want to avoid double \ use a ?: "") https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode713 drmemory/fuzzer.c:713: dr_snprintf(suffix, BUFFER_SIZE_ELEMENTS(suffix), "corpus.dat"); What is this magic name "corpus.dat"? Please explain in a comment -- is this required to match libfuzz or sthg? Maybe it should be a named constant. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode717 drmemory/fuzzer.c:717: "corpus_table lock is not held"); This caller requirement should be in a comment prior to the function https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode723 drmemory/fuzzer.c:723: LOG(2, "Dump corpus input to file %s."NL, path); s/Dump/Dumped/ https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode727 drmemory/fuzzer.c:727: static void No return of success from dump_fuzz_input? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode737 drmemory/fuzzer.c:737: "%sfuzz input for error #%d is stored in file %s\n", Won't this be very confusing if dump_fuzz_input fails? Seems like a bug. Seems best to check error code. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1401: hashtable_unlock(&corpus_table); This needs a comment as it looks really weird unless you know why https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1428: if (state->mutation && mutator_vec.entries > 0) { More comments would help: so the general algorithm is to give first priority to the input files that were sitting in the directory at init time. Once those are gone, we pick up inputs that we ourselves wrote out earlier (saved in mutator_vec) and we mutate each of those just one step each, right? How about a comment explaining that, maybe one at the top of this function, and a brief one by the decl of mutator_vec? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1428: if (state->mutation && mutator_vec.entries > 0) { So we never mutate the initial input files? We run one iter of each initial file and if no new bb is created we just move on, right? https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1442: hashtable_unlock(&corpus_table); So mutator_vec relies on corpus_table's lock? Please point that out up by decl. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1526: /* for corpus fuzzing, we stop fuzzing even see the fuzz function again */ error: even if we see https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:665: "Load a set of corpus input data and perform coverage based fuzzing.", A corpus *is* a set, so this should say: "Load a corpus of input data files and ..." https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:666: "Load a set of corpus input data from provided directory, perform coverage based fuzzing, and dump input data that causes more coverage.") Ditto https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:666: "Load a set of corpus input data from provided directory, perform coverage based fuzzing, and dump input data that causes more coverage.") error: from the suggest: s/provided/specified/
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus fuzzing - add option fuzz_corpus for corpus directory - add corpus_vec and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus iteration: execute corpus input * mutation iteration: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration - remove unused input_buffer_copy from fuzz_state_t struct Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c File drmemory/fuzzer.c (left): https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode97 drmemory/fuzzer.c:97: byte *input_buffer_copy; /* threadsafe copy of the fuzz target's buffer arg */ On 2016/01/04 19:23:41, bruening wrote: > So you removed the use of this for non-corpus-based usage models as well? Could > you add that to the commit message? This is to avoid the memcpy cost? I do not see any use of this input_buffer_copy, so remove it. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode101 drmemory/fuzzer.c:101: uint64 num_bbs; /* number of basic blocks seen */ On 2016/01/04 19:23:42, bruening wrote: > These fields were marked as read by another thread and now they are not: is that > accurate? I believe these two fields will not be read by other threads. Both mutator and num_bbs are used during mutation, not for error report (the only place other threads might touch some fields). That's why I move them up to avoid confusion. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#oldcode... drmemory/fuzzer.c:1092: fuzz_state->input_buffer_copy = global_alloc(fuzz_state->input_size, HEAPSTAT_MISC); On 2016/01/04 19:23:42, bruening wrote: > See comment at top: how does removing this relate to corpus-based fuzzing? I do not see any usage of this field, so just remove them entirely. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode42 drmemory/fuzzer.c:42: # pragma comment(lib, "User32.lib") On 2016/01/04 19:23:42, bruening wrote: > It is best to avoid user32.dll as the private loader is not able to duplicate > it, so it is shared with the app. Why do we need user32? Removed, I thought we need it for the FindFirstFile. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode96 drmemory/fuzzer.c:96: uint corpus_index; /* index in corpus_vec indeicating which has been executed */ On 2016/01/04 19:23:42, bruening wrote: > error: spelling Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode97 drmemory/fuzzer.c:97: uint mutator_index; /* index in mutator_vec indeicating which to be fuzzed */ On 2016/01/04 19:23:41, bruening wrote: > error: spelling (again) Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode98 drmemory/fuzzer.c:98: bool mutation; /* perform mutation on on mutators from mutator_vec */ On 2016/01/04 19:23:41, bruening wrote: > error: "on on" Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode98 drmemory/fuzzer.c:98: bool mutation; /* perform mutation on on mutators from mutator_vec */ On 2016/01/04 19:23:42, bruening wrote: > suggest: s/mutation/should_mutate/ (b/c "mutation" sounds like data, not a flag) Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode99 drmemory/fuzzer.c:99: bool orig_input; /* run with original input from app */ On 2016/01/04 19:23:42, bruening wrote: > suggest: s/orig_input/use_orig_input/ Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode154 drmemory/fuzzer.c:154: hashtable_t corpus_table; /* hashtable for looking up loaded corpus */ On 2016/01/04 19:23:42, bruening wrote: > suggest: expand comment to explain what the types of the keys and values are and > what the synch model is removed hashtable as we do not support load corpus multiple times. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode155 drmemory/fuzzer.c:155: drvector_t corpus_vec; /* vector for loaded corpus */ On 2016/01/04 19:23:42, bruening wrote: > What is the synch model for this? Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode155 drmemory/fuzzer.c:155: drvector_t corpus_vec; /* vector for loaded corpus */ On 2016/01/04 19:23:42, bruening wrote: > How does this relate to corpus_table? Please explain the invariants in a > comment (exact same entries in each?). removed corpus_table https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode156 drmemory/fuzzer.c:156: drvector_t mutator_vec; /* vector for created mutators */ On 2016/01/04 19:23:42, bruening wrote: > What is the synch model for this? Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode166 drmemory/fuzzer.c:166: /* Protects the fuzz_state_t field input_buffer and input_size for access from On 2016/01/04 19:23:42, bruening wrote: > error: s/field/fields/ Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode230 drmemory/fuzzer.c:230: fuzzer_read_corpus_list(void) On 2016/01/04 19:23:42, bruening wrote: > Will this be called periodically during a run? Will multiple threads call it? > Seems worth adding comments to the top about its assumptions so we can > understand whether the code is safe + optimal. Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode236 drmemory/fuzzer.c:236: if ((dir = opendir(options.fuzz_corpus)) == NULL) { On 2016/01/04 19:23:42, bruening wrote: > style violation: Program Structure #19 Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode242 drmemory/fuzzer.c:242: while ((ent = readdir(dir)) != NULL) { On 2016/01/04 19:23:42, bruening wrote: > style violation: Program Structure #19 Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode250 drmemory/fuzzer.c:250: hashtable_add(&corpus_table, (void *)ent->d_name, (void *)1); On 2016/01/04 19:23:42, bruening wrote: > Xref earlier comment: would help to know you're storing 1 removed, now do not support calling fuzzer_read_corpus_list multiple times. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode261 drmemory/fuzzer.c:261: HANDLE hFind = INVALID_HANDLE_VALUE; On 2016/01/04 19:23:41, bruening wrote: > style violation: var name Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode264 drmemory/fuzzer.c:264: bool has_sep; On 2016/01/04 19:23:42, bruening wrote: > unused var Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode269 drmemory/fuzzer.c:269: strcat(path, "*"); On 2016/01/04 19:23:41, bruening wrote: > bug: could overflow -- how about using single dr_snprintf (see below) Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode272 drmemory/fuzzer.c:272: strcat(path, "\\*"); On 2016/01/04 19:23:42, bruening wrote: > bug: could overflow -- how about using single dr_snprintf (see below) Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode274 drmemory/fuzzer.c:274: } On 2016/01/04 19:23:42, bruening wrote: > suggest: double \ is fine so just have a single dr_snprintf + > NULL_TERMINATE_BUFFER which seems much simpler and safer (if really want to > avoid double \ use a ?: "") Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode713 drmemory/fuzzer.c:713: dr_snprintf(suffix, BUFFER_SIZE_ELEMENTS(suffix), "corpus.dat"); On 2016/01/04 19:23:41, bruening wrote: > What is this magic name "corpus.dat"? Please explain in a comment -- is this > required to match libfuzz or sthg? Maybe it should be a named constant. LibFuzzer uses hash of the input data as file name, I do not think it matters. It assume the file name could be anything. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode717 drmemory/fuzzer.c:717: "corpus_table lock is not held"); On 2016/01/04 19:23:41, bruening wrote: > This caller requirement should be in a comment prior to the function remove hashtable for simplicity. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode723 drmemory/fuzzer.c:723: LOG(2, "Dump corpus input to file %s."NL, path); On 2016/01/04 19:23:41, bruening wrote: > s/Dump/Dumped/ Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode727 drmemory/fuzzer.c:727: static void On 2016/01/04 19:23:41, bruening wrote: > No return of success from dump_fuzz_input? Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode737 drmemory/fuzzer.c:737: "%sfuzz input for error #%d is stored in file %s\n", On 2016/01/04 19:23:41, bruening wrote: > Won't this be very confusing if dump_fuzz_input fails? Seems like a bug. Seems > best to check error code. Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1401: hashtable_unlock(&corpus_table); On 2016/01/04 19:23:42, bruening wrote: > This needs a comment as it looks really weird unless you know why removed https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1428: if (state->mutation && mutator_vec.entries > 0) { On 2016/01/04 19:23:42, bruening wrote: > More comments would help: so the general algorithm is to give first priority to > the input files that were sitting in the directory at init time. Once those are > gone, we pick up inputs that we ourselves wrote out earlier (saved in > mutator_vec) and we mutate each of those just one step each, right? How about a > comment explaining that, maybe one at the top of this function, and a brief one > by the decl of mutator_vec? Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1428: if (state->mutation && mutator_vec.entries > 0) { On 2016/01/04 19:23:43, bruening wrote: > So we never mutate the initial input files? We run one iter of each initial > file and if no new bb is created we just move on, right? we will, we create mutators for the init input files. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1442: hashtable_unlock(&corpus_table); On 2016/01/04 19:23:41, bruening wrote: > So mutator_vec relies on corpus_table's lock? Please point that out up by decl. removed, mutator_vec now has its own sync. https://codereview.appspot.com/282870043/diff/20002/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1526: /* for corpus fuzzing, we stop fuzzing even see the fuzz function again */ On 2016/01/04 19:23:42, bruening wrote: > error: even if we see Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:665: "Load a set of corpus input data and perform coverage based fuzzing.", On 2016/01/04 19:23:43, bruening wrote: > A corpus *is* a set, so this should say: "Load a corpus of input data files and > ..." Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:666: "Load a set of corpus input data from provided directory, perform coverage based fuzzing, and dump input data that causes more coverage.") On 2016/01/04 19:23:43, bruening wrote: > error: from the > > suggest: s/provided/specified/ Done. https://codereview.appspot.com/282870043/diff/20002/drmemory/optionsx.h#newco... drmemory/optionsx.h:666: "Load a set of corpus input data from provided directory, perform coverage based fuzzing, and dump input data that causes more coverage.") On 2016/01/04 19:23:43, bruening wrote: > Ditto Done.
Sign in to reply to this message.
There is still an overflow bug, and problems in the comments. If those are fixed then LGTM. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode156 drmemory/fuzzer.c:156: /* stores corpus input file names, all operations are synchronized. */ repeat from prior CL's: a comma cannot combine 2 separate sentences https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode158 drmemory/fuzzer.c:158: /* stores created mutators, all operations are synchronized */ repeat from prior CL's: a comma cannot combine 2 separate sentences https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode264 drmemory/fuzzer.c:264: strcpy(path, options.fuzz_corpus); bug: options.fuzz_corpus can be *much* larger than path and this can overflow! Why is this here, though? Shouldn't this line just be deleted? https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1382: * from corpus_vec for execution and create the mutator for future fuzzing. You create multiple mutators, right? Not "the mutator"? https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1382: * from corpus_vec for execution and create the mutator for future fuzzing. State here that the mutator creation is in post_fuzz, b/c I spent some time looking for where it is and whether it's somehow hidden in the this pre_fuzz code. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1383: * In the mutate phase (if state->should_mutato is true), we pick a mutator from error: invalid var name https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1401: fuzzer_mutator_init(dcontext, state); There is no longer an append to the mutator vec? This seems like a bug: your new comment says you should be doing this, but you removed the code for it. UPDATE: it is in post_fuzz -- please clarify as the pre-fuzz comment implies it should be here. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1502: /* corpus phase: simply the mutator into mutator_vec */ error: "simply the mutator"?
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus fuzzing - add option fuzz_corpus for corpus directory - add corpus_vec and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus iteration: execute corpus input * mutation iteration: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration - remove unused input_buffer_copy from fuzz_state_t struct Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode156 drmemory/fuzzer.c:156: /* stores corpus input file names, all operations are synchronized. */ On 2016/01/06 05:14:03, bruening wrote: > repeat from prior CL's: a comma cannot combine 2 separate sentences just curious, will it be ok if there is no '.' at the end, i.e., is it ok to use a comma to combine 2 phases. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode158 drmemory/fuzzer.c:158: /* stores created mutators, all operations are synchronized */ On 2016/01/06 05:14:03, bruening wrote: > repeat from prior CL's: a comma cannot combine 2 separate sentences Done. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode264 drmemory/fuzzer.c:264: strcpy(path, options.fuzz_corpus); On 2016/01/06 05:14:03, bruening wrote: > bug: options.fuzz_corpus can be *much* larger than path and this can overflow! > Why is this here, though? Shouldn't this line just be deleted? hmm, forget to delete this line. Done https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1382: * from corpus_vec for execution and create the mutator for future fuzzing. On 2016/01/06 05:14:03, bruening wrote: > You create multiple mutators, right? Not "the mutator"? Done. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1382: * from corpus_vec for execution and create the mutator for future fuzzing. On 2016/01/06 05:14:03, bruening wrote: > State here that the mutator creation is in post_fuzz, b/c I spent some time > looking for where it is and whether it's somehow hidden in the this pre_fuzz > code. The mutator creation is here, putting into the mutator_vec happens in post_fuzz. I was trying to put all mutator appending operations together at the same place (post_fuzz). https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1383: * In the mutate phase (if state->should_mutato is true), we pick a mutator from On 2016/01/06 05:14:03, bruening wrote: > error: invalid var name Done. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1401: fuzzer_mutator_init(dcontext, state); On 2016/01/06 05:14:03, bruening wrote: > There is no longer an append to the mutator vec? This seems like a bug: your > new comment says you should be doing this, but you removed the code for it. > > UPDATE: it is in post_fuzz -- please clarify as the pre-fuzz comment implies it > should be here. update comment about where to update the mutator vec. https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode... drmemory/fuzzer.c:1502: /* corpus phase: simply the mutator into mutator_vec */ On 2016/01/06 05:14:03, bruening wrote: > error: "simply the mutator"? Done.
Sign in to reply to this message.
https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/282870043/diff/70001/drmemory/fuzzer.c#newcode156 drmemory/fuzzer.c:156: /* stores corpus input file names, all operations are synchronized. */ On 2016/01/06 16:49:15, zhaoqin wrote: > On 2016/01/06 05:14:03, bruening wrote: > > repeat from prior CL's: a comma cannot combine 2 separate sentences > > just curious, will it be ok if there is no '.' at the end, i.e., is it ok to use > a comma to combine 2 phases. No. There needs to be a conjunction like "and" or "but" after the comma, and then only if they make sense being joined. (It can be ok with no conjunction if there is parallelism between the phrases ("Roses are red, violets are blue"), but that's kind of poetic license.) Here these are completely different concepts and it's hard to imagine combining them with a comma in any way.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Dr. Fuzz: add corpus based fuzzing - add option fuzz_corpus for corpus directory - add corpus_vec and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus phase: execute corpus input * mutation phase: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration * add mutator into mutator_vec - add tid in fuzz_target to limit single thread fuzzing - remove unused input_buffer_copy from fuzz_state_t struct Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/drmemory/commit/0b303f03d59b24bca565df0a23cc3707... Final commit log: --------------- i#1734 Dr. Fuzz: add corpus based fuzzing - add option fuzz_corpus for corpus directory - add corpus_vec and mutator_vec - add fuzzer_read_corpus_list to read file list from corpus directory - update fuzzer_init to call fuzzer_read_corpus_list - update pre_fuzz/post_fuzz to call pre_fuzz_corpus/post_fuzz_corpus - pre_fuzz_corpus * corpus phase: execute corpus input * mutation phase: execute mutate input - post_fuzz_corpus * dump input if discover new basic blocks in mutation iteration * add mutator into mutator_vec - add tid in fuzz_target to limit single thread fuzzing - remove unused input_buffer_copy from fuzz_state_t struct Review-URL: https://codereview.appspot.com/282870043 ---------------
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
