i#1734 Dr. Fuzz: add -fuzz_corpus_min option

Commit log for first patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_min option

- add -fuzz_corpus_min option to produce minimized the corpus inputs
- update fuzzer_read_corpus_list() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump corpus for minimizing corpus inputs
---------------

[zhaoqin, 2016-01-11 04:11:42]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_min option

- add -fuzz_corpus_min option to produce minimized the corpus inputs
- update fuzzer_read_corpus_list() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump corpus for minimizing corpus inputs

Review-URL: https://codereview.appspot.com/281320043
---------------

[bruening, 2016-01-11 20:37:44]

> - add -fuzz_corpus_min option to produce minimized the corpus inputs

Please rewrite to correct the grammar errors

[bruening, 2016-01-11 21:27:19]

Can we add some tests?

https://codereview.appspot.com/281320043/diff/20001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281320043/diff/20001/drmemory/fuzzer.c#newcode296
drmemory/fuzzer.c:296: }
Why not just use dr_directory_exists() and avoid any platform-specific code?

https://codereview.appspot.com/281320043/diff/20001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281320043/diff/20001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:671: "Minimize the corpus of input data files from
'-fuzz_corpus'",
The user has no idea what this string value means.  This is a description for a
bool, not a string.

[zhaoqin, 2016-01-12 19:32:30]

https://codereview.appspot.com/281320043/diff/20001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281320043/diff/20001/drmemory/fuzzer.c#newcode296
drmemory/fuzzer.c:296: }
On 2016/01/11 21:27:19, bruening wrote:
> Why not just use dr_directory_exists() and avoid any platform-specific code?

Done.

https://codereview.appspot.com/281320043/diff/20001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281320043/diff/20001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:671: "Minimize the corpus of input data files from
'-fuzz_corpus'",
On 2016/01/11 21:27:19, bruening wrote:
> The user has no idea what this string value means.  This is a description for
a
> bool, not a string.

Done.

[zhaoqin, 2016-01-12 19:32:48]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_min option

- add -fuzz_corpus_min option to produce the minimized corpus inputs
- update fuzzer_init() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump minimized corpus inputs
- add fuzz_corpus test

Review-URL: https://codereview.appspot.com/281320043
---------------

[zhaoqin, 2016-01-12 19:43:58]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_min option

- add -fuzz_corpus_min option to produce the minimized corpus inputs
- update fuzzer_init() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump minimized corpus inputs
- add fuzz_corpus test

Review-URL: https://codereview.appspot.com/281320043
---------------

[zhaoqin, 2016-01-12 20:03:36]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_min option

- add -fuzz_corpus_min option to produce the minimized corpus inputs
- update fuzzer_init() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump minimized corpus inputs
- adjust fuzz repeat count check in post_fuzz_corpus()
- add fuzz_corpus test

Review-URL: https://codereview.appspot.com/281320043
---------------

[bruening, 2016-01-12 22:20:13]

I do not understand the repeat count check change, which seems to have been
added at the last minute -- please explain further.

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c#newcode...
drmemory/fuzzer.c:1546: ++state->repeat_index < fuzz_target.repeat_count) {
This seems like a big change: 2-iter difference, right?  The commit msg just
says "adjust".  Why is this change here?  It seems odd.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:634: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data is
stored in read-only memory, or for fuzzing functions with different input data
sizes, e.g., loading data via '-fuzz_input_file'.  Note: this may cause problems
if other pointers point to the original buffer, or the replaced buffer is used
after the fuzzing iterations.")
Please remove the quotes (see below)

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:666: "Load data from specified file as fuzz input.  It can
be used with '-fuzz_num_iters 0' to reproduce an error from the input generated
by '-fuzz_dump_on_error'.  The data might be truncated if the data size is
larger than the input buffer size.  Use '-fuzz_replace_buffer' to replace the
input buffer with a separately allocated buffer.")
Please remove the quotes (see below)

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:670: OPTION_CLIENT_STRING(drmemscope, fuzz_corpus_min, "",
I am not a fan of this name.  It sounds like an integer.  How about
fuzz_corpus_dest or fuzz_corpus_out?

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:671: "Create and store the minimized corpus inputs from
'-fuzz_corpus' to 'fuzz_corpus_min'",
The existing options are referred to in many places in these options docs, and
this single-quote style is never used except in 2 places that you recently
added.  Let's keep this consistent: go with the existing style that does not use
single quotes.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:672: "Create the minimized corpus inputs from '-fuzz_corpus'
and dump them to directory specified by '-fuzz_corpus_min'.")
the directory

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:672: "Create the minimized corpus inputs from '-fuzz_corpus'
and dump them to directory specified by '-fuzz_corpus_min'.")
Ditto re: the quotes

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/CMakeLists.txt
File tests/fuzz/CMakeLists.txt (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/CMakeLists.txt...
tests/fuzz/CMakeLists.txt:52: message(FATAL "Did not find corpus path for
fuzz_corpus test")
Isn't this checked in to the repo?  Why do we need the complexity of searching
for it?  I would think a single simple path would be the way to go.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.c
File tests/fuzz/fuzz_corpus.c (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.c#...
tests/fuzz/fuzz_corpus.c:72: printf("Error: some value is not seen\n");
was

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.out
File tests/fuzz/fuzz_corpus.out (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.ou...
tests/fuzz/fuzz_corpus.out:21: all done
Are there existing files like this, w/ an empty .res too?  If so let's share the
existing instead of checking in a redundant file.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.res
File tests/fuzz/fuzz_corpus.res (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.re...
tests/fuzz/fuzz_corpus.res:21: # empty
See prior comment

[zhaoqin, 2016-01-13 04:39:33]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_corpus_out option

- add -fuzz_corpus_out option to store the minimized corpus inputs
- update fuzzer_init() to check corpus directory
- update dump_fuzz_corpus_input() to pick output directory
- update post_fuzz_corpus() to dump minimized corpus inputs
- fix fuzz repeat count check bug in post_fuzz_corpus()
- add fuzz_corpus test

Review-URL: https://codereview.appspot.com/281320043
---------------

[zhaoqin, 2016-01-13 04:40:43]

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c#newcode...
drmemory/fuzzer.c:1546: ++state->repeat_index < fuzz_target.repeat_count) {
On 2016/01/12 22:20:12, bruening wrote:
> This seems like a big change: 2-iter difference, right?  The commit msg just
> says "adjust".  Why is this change here?  It seems odd.

the original one is not correct.
e.g, if the -fuzz_num_iters is 1, the original code:
  state->repeat_index++ <= fuzz_target.repeat_count
would cause it execute 3 iterations, while
  ++state->repeat_index < fuzz_target.repeat_count
would execute only once.
So it basically fixed a bug, will update the commit msg.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:634: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data is
stored in read-only memory, or for fuzzing functions with different input data
sizes, e.g., loading data via '-fuzz_input_file'.  Note: this may cause problems
if other pointers point to the original buffer, or the replaced buffer is used
after the fuzzing iterations.")
On 2016/01/12 22:20:12, bruening wrote:
> Please remove the quotes (see below)

Done.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:666: "Load data from specified file as fuzz input.  It can
be used with '-fuzz_num_iters 0' to reproduce an error from the input generated
by '-fuzz_dump_on_error'.  The data might be truncated if the data size is
larger than the input buffer size.  Use '-fuzz_replace_buffer' to replace the
input buffer with a separately allocated buffer.")
On 2016/01/12 22:20:12, bruening wrote:
> Please remove the quotes (see below)

Done.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:670: OPTION_CLIENT_STRING(drmemscope, fuzz_corpus_min, "",
On 2016/01/12 22:20:12, bruening wrote:
> I am not a fan of this name.  It sounds like an integer.  How about
> fuzz_corpus_dest or fuzz_corpus_out?

Done.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:671: "Create and store the minimized corpus inputs from
'-fuzz_corpus' to 'fuzz_corpus_min'",
On 2016/01/12 22:20:12, bruening wrote:
> The existing options are referred to in many places in these options docs, and
> this single-quote style is never used except in 2 places that you recently
> added.  Let's keep this consistent: go with the existing style that does not
use
> single quotes.

Done.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:672: "Create the minimized corpus inputs from '-fuzz_corpus'
and dump them to directory specified by '-fuzz_corpus_min'.")
On 2016/01/12 22:20:12, bruening wrote:
> Ditto re: the quotes

Done.

https://codereview.appspot.com/281320043/diff/80001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:672: "Create the minimized corpus inputs from '-fuzz_corpus'
and dump them to directory specified by '-fuzz_corpus_min'.")
On 2016/01/12 22:20:13, bruening wrote:
> the directory

Done.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/CMakeLists.txt
File tests/fuzz/CMakeLists.txt (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/CMakeLists.txt...
tests/fuzz/CMakeLists.txt:52: message(FATAL "Did not find corpus path for
fuzz_corpus test")
On 2016/01/12 22:20:13, bruening wrote:
> Isn't this checked in to the repo?  Why do we need the complexity of searching
> for it?  I would think a single simple path would be the way to go.

Done.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.c
File tests/fuzz/fuzz_corpus.c (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.c#...
tests/fuzz/fuzz_corpus.c:72: printf("Error: some value is not seen\n");
On 2016/01/12 22:20:13, bruening wrote:
> was

Done.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.out
File tests/fuzz/fuzz_corpus.out (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.ou...
tests/fuzz/fuzz_corpus.out:21: all done
On 2016/01/12 22:20:13, bruening wrote:
> Are there existing files like this, w/ an empty .res too?  If so let's share
the
> existing instead of checking in a redundant file.

not in current directory, find asmtest in the parent directory.

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.res
File tests/fuzz/fuzz_corpus.res (right):

https://codereview.appspot.com/281320043/diff/80001/tests/fuzz/fuzz_corpus.re...
tests/fuzz/fuzz_corpus.res:21: # empty
On 2016/01/12 22:20:13, bruening wrote:
> See prior comment

ditto

[bruening, 2016-01-13 05:00:26]

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c#newcode...
drmemory/fuzzer.c:1546: ++state->repeat_index < fuzz_target.repeat_count) {
On 2016/01/13 04:40:42, zhaoqin wrote:
> On 2016/01/12 22:20:12, bruening wrote:
> > This seems like a big change: 2-iter difference, right?  The commit msg just
> > says "adjust".  Why is this change here?  It seems odd.
> 
> the original one is not correct.
> e.g, if the -fuzz_num_iters is 1, the original code:
>   state->repeat_index++ <= fuzz_target.repeat_count
> would cause it execute 3 iterations, while
>   ++state->repeat_index < fuzz_target.repeat_count
> would execute only once.
> So it basically fixed a bug, will update the commit msg.
>  

Could you elaborate?  Isn't repeat_count set to -fuzz_num_iters?  So won't:
   state->repeat_index++ <= fuzz_target.repeat_count
execute 2 iters, not 3?  Isn't the bug fix just s/<=/</?

[bruening, 2016-01-13 05:05:43]

On 2016/01/13 05:00:26, bruening wrote:
> https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c
> File drmemory/fuzzer.c (right):
> 
>
https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c#newcode...
> drmemory/fuzzer.c:1546: ++state->repeat_index < fuzz_target.repeat_count) {
> On 2016/01/13 04:40:42, zhaoqin wrote:
> > On 2016/01/12 22:20:12, bruening wrote:
> > > This seems like a big change: 2-iter difference, right?  The commit msg
just
> > > says "adjust".  Why is this change here?  It seems odd.
> > 
> > the original one is not correct.
> > e.g, if the -fuzz_num_iters is 1, the original code:
> >   state->repeat_index++ <= fuzz_target.repeat_count
> > would cause it execute 3 iterations, while
> >   ++state->repeat_index < fuzz_target.repeat_count
> > would execute only once.
> > So it basically fixed a bug, will update the commit msg.
> >  
> 
> Could you elaborate?  Isn't repeat_count set to -fuzz_num_iters?  So won't:
>    state->repeat_index++ <= fuzz_target.repeat_count
> execute 2 iters, not 3?  Isn't the bug fix just s/<=/</?

OK I guess it's b/c this check is in the post, not the pre.

[zhaoqin, 2016-01-14 04:50:19]

On 2016/01/13 05:05:43, bruening wrote:
> On 2016/01/13 05:00:26, bruening wrote:
> > https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c
> > File drmemory/fuzzer.c (right):
> > 
> >
>
https://codereview.appspot.com/281320043/diff/80001/drmemory/fuzzer.c#newcode...
> > drmemory/fuzzer.c:1546: ++state->repeat_index < fuzz_target.repeat_count) {
> > On 2016/01/13 04:40:42, zhaoqin wrote:
> > > On 2016/01/12 22:20:12, bruening wrote:
> > > > This seems like a big change: 2-iter difference, right?  The commit msg
> just
> > > > says "adjust".  Why is this change here?  It seems odd.
> > > 
> > > the original one is not correct.
> > > e.g, if the -fuzz_num_iters is 1, the original code:
> > >   state->repeat_index++ <= fuzz_target.repeat_count
> > > would cause it execute 3 iterations, while
> > >   ++state->repeat_index < fuzz_target.repeat_count
> > > would execute only once.
> > > So it basically fixed a bug, will update the commit msg.
> > >  
> > 
> > Could you elaborate?  Isn't repeat_count set to -fuzz_num_iters?  So won't:
> >    state->repeat_index++ <= fuzz_target.repeat_count
> > execute 2 iters, not 3?  Isn't the bug fix just s/<=/</?
> 
> OK I guess it's b/c this check is in the post, not the pre.

yes

[zhaoqin, 2016-01-15 15:56:49]

ping, I want to make sure that you are not waiting for me to do something,
right?

[bruening, 2016-01-15 16:20:23]

On 2016/01/15 15:56:49, zhaoqin wrote:
> ping, I want to make sure that you are not waiting for me to do something,
> right?

I did not know there was a new patchset.  There was no email about a new
patchset after the comment discussion.  Looking at it, it seems that the
confusion stems from the replies to my comments coming *after* your uploaded the
new patchset -- I guess I expect a new patchset to come after comment
discussion, not before, esp when the comments say "will update the commit msg"
when in fact you already updated it in the past at the time you wrote that.

[zhaoqin, 2016-01-15 16:24:30]

Sorry about that.
because the reply also include "Done", that's why I upload the patch set
first.
I will always update the comment before upload patch set in the future.

On Fri, Jan 15, 2016 at 11:20 AM, <bruening@google.com> wrote:

> On 2016/01/15 15:56:49, zhaoqin wrote:
>
>> ping, I want to make sure that you are not waiting for me to do
>>
> something,
>
>> right?
>>
>
> I did not know there was a new patchset.  There was no email about a new
> patchset after the comment discussion.  Looking at it, it seems that the
> confusion stems from the replies to my comments coming *after* your
> uploaded the new patchset -- I guess I expect a new patchset to come
> after comment discussion, not before, esp when the comments say "will
> update the commit msg" when in fact you already updated it in the past
> at the time you wrote that.
>
> https://codereview.appspot.com/281320043/
>

[bruening, 2016-01-15 16:25:06]

LGTM