i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input buffer

Commit log for first patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input buffer

- add -fuzz_replace_buffer to support allocating input buffer by Dr. Memory
- add alloc_fuzz_input_buffer/free_fuzz_input_buffer in fuzzer.c to
  alloc/free app memory
- add client_app_malloc and client_app_free in alloc_replace.c
  to allocate app memory from client
- remove mutator_span_empty in fuzz_state
- update load_fuzz_input to replace input buffer if necessary
- update find_target_buffer to replace input buffer if necessary
- add fuzz_buffer.overflow test
---------------

[bruening, 2015-12-16 23:00:43]

The alloc code needs another look -- are you sure it works on Windows?

https://codereview.appspot.com/279440043/diff/1/common/alloc.h
File common/alloc.h (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode287
common/alloc.h:287: /* called by client in a cleancall for allocating
applicaiton memory */
bug: spelling

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode291
common/alloc.h:291: /* called by client in a cleancall for freeing applicaiton
memory */
ditto

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode293
common/alloc.h:293: client_app_free(void *drcontext, void *ptr);
So neither is implemented for wrapping, right?  Seems like that should be
explained here.

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5423: /* called by client in a cleancall for allocating
applicaiton memory */
again!

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5428: arena_header_t *arena =
arena_for_libc_alloc(drcontext);
I am not sure this will work on Windows, where we rely on having saved the alloc
routine set into a TLS slot, but in this fabricated case we never took that
step.  On Windows this should just come from the default heap, right?  No reason
to try to use the same heap used by the original buffer?

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5431: INITIALIZE_MCONTEXT_FOR_REPORT(&mc);
This is not the right mcontext: if you're already on the dstack, this is not
going to see any of the app's frames.  If an error is reported while fuzzing
that includes the alloc callstack, it will just be empty except the clean callee
or sthg.

There is a bigger ques of what the callstack should look like, given that the
app didn't actually allocate this -- but normally we want an app context, not a
DR context, and wouldn't the app context on entering the target func be better
to show, maybe with a top frame of "drfuzz_reallocate_buffer" or some other
explanatory name?

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c
File drmemory/fuzzer.c (left):

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#oldcode1063
drmemory/fuzzer.c:1063: if (fuzz_state->mutator_span_empty) {
I don't even know what this is so I have no idea whether it can be removed

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode87
drmemory/fuzzer.c:87: /* While fields below are thread-local like the others,
they may be accessed
s/accessed/read/

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode431
drmemory/fuzzer.c:431: if (options.fuzz_replace_buffer && dr_file_size(data,
&file_size)) {
Shouldn't the inability to get the file size be some kind of error?  The user
will wonder why the buffer was silently *not* replaced

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode437
drmemory/fuzzer.c:437: if (options.fuzz_replace_buffer && input_buffer != NULL)
{
How about a comment explaining why the code is split around the lock acquire

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode983
drmemory/fuzzer.c:983: /* copy content to the allocated buffer, shadow state
setup will be done later */
s/,/(/;s/r /r) /
or else have to use . and thus capitalize both b/c multi-sentence

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode985
drmemory/fuzzer.c:985: input_buffer = buffer;
What about the problem of other pointers pointing at the original buffer? 
Shouldn't there be XXX comments about that kind of issue?

https://codereview.appspot.com/279440043/diff/1/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/279440043/diff/1/drmemory/optionsx.h#newcode631
drmemory/optionsx.h:631: "Replace the input data buffer with separatedly
allocated memory.")
This should talk about the danger of other pointers: this is potentially unsafe.
 This should also explain *why* anyone would want to do this.

Also, shouldn't the load-from-file option explain that you prob want to turn
this on b/c of the size issue?

[zhaoqin, 2015-12-17 23:42:42]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input
buffer

- add -fuzz_replace_buffer to support allocating input buffer by Dr. Memory
- add drfuzz_reallocate_buffer/drfuzz_free_reallocated_buffer in fuzzer.c to
  alloc/free app memory
- add client_app_malloc and client_app_free in alloc_replace.c
  to allocate app memory from client
- remove mutator_span_empty in fuzz_state
- update load_fuzz_input to replace input buffer if necessary
- update find_target_buffer to replace input buffer if necessary
- add fuzz_buffer.overflow test

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

[zhaoqin, 2015-12-17 23:43:12]

https://codereview.appspot.com/279440043/diff/1/common/alloc.h
File common/alloc.h (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode287
common/alloc.h:287: /* called by client in a cleancall for allocating
applicaiton memory */
On 2015/12/16 23:00:42, bruening wrote:
> bug: spelling

Done.

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode291
common/alloc.h:291: /* called by client in a cleancall for freeing applicaiton
memory */
On 2015/12/16 23:00:42, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode293
common/alloc.h:293: client_app_free(void *drcontext, void *ptr);
On 2015/12/16 23:00:42, bruening wrote:
> So neither is implemented for wrapping, right?  Seems like that should be
> explained here.

Done.

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5423: /* called by client in a cleancall for allocating
applicaiton memory */
On 2015/12/16 23:00:42, bruening wrote:
> again!

Done.

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5428: arena_header_t *arena =
arena_for_libc_alloc(drcontext);
On 2015/12/16 23:00:42, bruening wrote:
> I am not sure this will work on Windows, where we rely on having saved the
alloc
> routine set into a TLS slot, but in this fabricated case we never took that
> step.  On Windows this should just come from the default heap, right?  No
reason
> to try to use the same heap used by the original buffer?

Done.

https://codereview.appspot.com/279440043/diff/1/common/alloc_replace.c#newcod...
common/alloc_replace.c:5431: INITIALIZE_MCONTEXT_FOR_REPORT(&mc);
On 2015/12/16 23:00:42, bruening wrote:
> This is not the right mcontext: if you're already on the dstack, this is not
> going to see any of the app's frames.  If an error is reported while fuzzing
> that includes the alloc callstack, it will just be empty except the clean
callee
> or sthg.
> 
> There is a bigger ques of what the callstack should look like, given that the
> app didn't actually allocate this -- but normally we want an app context, not
a
> DR context, and wouldn't the app context on entering the target func be better
> to show, maybe with a top frame of "drfuzz_reallocate_buffer" or some other
> explanatory name?

Done.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c
File drmemory/fuzzer.c (left):

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#oldcode1063
drmemory/fuzzer.c:1063: if (fuzz_state->mutator_span_empty) {
On 2015/12/16 23:00:42, bruening wrote:
> I don't even know what this is so I have no idea whether it can be removed

It is used if the mutate offset is bigger than the buffer size, in which case we
can either disable the fuzz_target, or use skip_initial to skip the current
fuzzing.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode87
drmemory/fuzzer.c:87: /* While fields below are thread-local like the others,
they may be accessed
On 2015/12/16 23:00:42, bruening wrote:
> s/accessed/read/

Done.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode431
drmemory/fuzzer.c:431: if (options.fuzz_replace_buffer && dr_file_size(data,
&file_size)) {
On 2015/12/16 23:00:42, bruening wrote:
> Shouldn't the inability to get the file size be some kind of error?  The user
> will wonder why the buffer was silently *not* replaced

Done.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode437
drmemory/fuzzer.c:437: if (options.fuzz_replace_buffer && input_buffer != NULL)
{
On 2015/12/16 23:00:42, bruening wrote:
> How about a comment explaining why the code is split around the lock acquire

Done.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode983
drmemory/fuzzer.c:983: /* copy content to the allocated buffer, shadow state
setup will be done later */
On 2015/12/16 23:00:42, bruening wrote:
> s/,/(/;s/r /r) /
> or else have to use . and thus capitalize both b/c multi-sentence

removed, seems not necessary.

https://codereview.appspot.com/279440043/diff/1/drmemory/fuzzer.c#newcode985
drmemory/fuzzer.c:985: input_buffer = buffer;
On 2015/12/16 23:00:42, bruening wrote:
> What about the problem of other pointers pointing at the original buffer? 
> Shouldn't there be XXX comments about that kind of issue?

Done.

https://codereview.appspot.com/279440043/diff/1/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/279440043/diff/1/drmemory/optionsx.h#newcode631
drmemory/optionsx.h:631: "Replace the input data buffer with separatedly
allocated memory.")
On 2015/12/16 23:00:42, bruening wrote:
> This should talk about the danger of other pointers: this is potentially
unsafe.
>  This should also explain *why* anyone would want to do this.
> 
> Also, shouldn't the load-from-file option explain that you prob want to turn
> this on b/c of the size issue?

Done.

[bruening, 2015-12-18 17:48:23]

https://codereview.appspot.com/279440043/diff/1/common/alloc.h
File common/alloc.h (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode293
common/alloc.h:293: client_app_free(void *drcontext, void *ptr);
On 2015/12/17 23:43:11, zhaoqin wrote:
> On 2015/12/16 23:00:42, bruening wrote:
> > So neither is implemented for wrapping, right?  Seems like that should be
> > explained here.
> 
> Done.

I don't see it?

https://codereview.appspot.com/279440043/diff/20001/common/alloc.h
File common/alloc.h (right):

https://codereview.appspot.com/279440043/diff/20001/common/alloc.h#newcode288
common/alloc.h:288: * only work with -replace_malloc
Several issues: 2 sentences can't be connected by a comma; since 2 sentences,
there is a style violation as capitalization and end punctuation must be used;
contains a grammar error.

I see more below.  Please go through the other comments.

[zhaoqin, 2015-12-18 20:27:29]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input
buffer

- add -fuzz_replace_buffer to support allocating input buffer by Dr. Memory
- add drfuzz_reallocate_buffer/drfuzz_free_reallocated_buffer in fuzzer.c to
  alloc/free app memory
- add client_app_malloc and client_app_free in alloc_replace.c
  to allocate app memory from client
- remove mutator_span_empty in fuzz_state
- update load_fuzz_input to replace input buffer if necessary
- update find_target_buffer to replace input buffer if necessary
- add fuzz_buffer.overflow test

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

[zhaoqin, 2015-12-18 20:27:49]

https://codereview.appspot.com/279440043/diff/1/common/alloc.h
File common/alloc.h (right):

https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode293
common/alloc.h:293: client_app_free(void *drcontext, void *ptr);
On 2015/12/18 17:48:22, bruening wrote:
> On 2015/12/17 23:43:11, zhaoqin wrote:
> > On 2015/12/16 23:00:42, bruening wrote:
> > > So neither is implemented for wrapping, right?  Seems like that should be
> > > explained here.
> > 
> > Done.
> 
> I don't see it?

Doesn't "only work with -replace_malloc" suggest that?

[bruening, 2015-12-18 20:34:36]

On 2015/12/18 20:27:49, zhaoqin wrote:
> https://codereview.appspot.com/279440043/diff/1/common/alloc.h
> File common/alloc.h (right):
> 
> https://codereview.appspot.com/279440043/diff/1/common/alloc.h#newcode293
> common/alloc.h:293: client_app_free(void *drcontext, void *ptr);
> On 2015/12/18 17:48:22, bruening wrote:
> > On 2015/12/17 23:43:11, zhaoqin wrote:
> > > On 2015/12/16 23:00:42, bruening wrote:
> > > > So neither is implemented for wrapping, right?  Seems like that should
be
> > > > explained here.
> > > 
> > > Done.
> > 
> > I don't see it?
> 
> Doesn't "only work with -replace_malloc" suggest that?

Oh, ok, I guess the grammar error made me not read the semantics

[bruening, 2015-12-18 20:56:10]

Has a couple of code bugs and some grammar issues in the comments.  If all that
is fixed, and callstacks are tested, shouldn't need another look so LGTM w/
comments

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5436: mc.flags = DR_MC_CONTROL; /* only need xsp */
bug: mc.xbp is uninitialized and the callstack walk will read it

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
I don't understand why you're marking a slot beyond TOS on the app stack as
defined?  This does not seem right.  This should be removed.

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5443: drcontext, &mc, caller,
Please look at sample callstacks coming from this and ensure they're what we
want: is it transitioning properly from caller to the app stack.

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5462: mc.flags = DR_MC_CONTROL; /* only need xsp */
ditto

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5465: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
ditto

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode431
drmemory/fuzzer.c:431: /* We need hold fuzz_state_lock for updating
input_buffer/input_size fields
to

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode432
drmemory/fuzzer.c:432: * but cannot hold a lock when alloc/free application
memory, so we split
when we

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode433
drmemory/fuzzer.c:433: * code and put memory alloc/free code outside.
* the code

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode433
drmemory/fuzzer.c:433: * code and put memory alloc/free code outside.
the memory

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode438
drmemory/fuzzer.c:438: FUZZ_ERROR("Failed to get input file size."NL);
bug: it succeeded here!

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode965
drmemory/fuzzer.c:965: FUZZ_WARN("buffer offset is larger than input size: %d >=
%d, skip fuzzing"NL,
s/,/->/ or sthg

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
grammar: is stored

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
grammar: s/pointing/point/

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
the fuzzing iterations

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:663: "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 separately allocated buffer.")
a separately

[zhaoqin, 2015-12-18 23:16:32]

about callstack, do we want something like

# 0 drfuzz_reallocate_func
# 1 repeatme
# 2 main

If so, we need write over the ToS.

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5436: mc.flags = DR_MC_CONTROL; /* only need xsp */
On 2015/12/18 20:56:09, bruening wrote:
> bug: mc.xbp is uninitialized and the callstack walk will read it

Done.

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
On 2015/12/18 20:56:09, bruening wrote:
> I don't understand why you're marking a slot beyond TOS on the app stack as
> defined?  This does not seem right.  This should be removed.

I followed the example in enter_client_code

Currently, we are at the entry of the target function (e.g., repeatme), if just
walk the stack, it would be something like

# 0 drfuzz_reallocate_func
# 1 main

Ideally, we want to want the callstack to be something like:
# 0 drfuzz_reallocate_func
# 1 repeatme
# 2 main

So we should allocate the some extra space on the stack and put target_pc on the
stack for stack walk.
Is it safe to do so, or we are ok with
# 0 drfuzz_reallocate_func
# 1 main

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5462: mc.flags = DR_MC_CONTROL; /* only need xsp */
On 2015/12/18 20:56:09, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode431
drmemory/fuzzer.c:431: /* We need hold fuzz_state_lock for updating
input_buffer/input_size fields
On 2015/12/18 20:56:09, bruening wrote:
> to

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode432
drmemory/fuzzer.c:432: * but cannot hold a lock when alloc/free application
memory, so we split
On 2015/12/18 20:56:09, bruening wrote:
> when we

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode433
drmemory/fuzzer.c:433: * code and put memory alloc/free code outside.
On 2015/12/18 20:56:10, bruening wrote:
> * the code

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode433
drmemory/fuzzer.c:433: * code and put memory alloc/free code outside.
On 2015/12/18 20:56:10, bruening wrote:
> the memory

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode438
drmemory/fuzzer.c:438: FUZZ_ERROR("Failed to get input file size."NL);
On 2015/12/18 20:56:10, bruening wrote:
> bug: it succeeded here!

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/fuzzer.c#newcode965
drmemory/fuzzer.c:965: FUZZ_WARN("buffer offset is larger than input size: %d >=
%d, skip fuzzing"NL,
On 2015/12/18 20:56:09, bruening wrote:
> s/,/->/ or sthg

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
On 2015/12/18 20:56:10, bruening wrote:
> grammar: is stored

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
On 2015/12/18 20:56:10, bruening wrote:
> grammar: s/pointing/point/

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:631: "Replace the input data buffer with separately
allocated memory.  This can be used for fuzzing functions whose input data
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 pointing to the original buffer, or the replaced buffer is
used after fuzzing iterations.")
On 2015/12/18 20:56:10, bruening wrote:
> the fuzzing iterations

Done.

https://codereview.appspot.com/279440043/diff/60001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:663: "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 separately allocated buffer.")
On 2015/12/18 20:56:10, bruening wrote:
> a separately

Done.

[bruening, 2015-12-18 23:37:25]

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
On 2015/12/18 23:16:31, zhaoqin wrote:
> On 2015/12/18 20:56:09, bruening wrote:
> > I don't understand why you're marking a slot beyond TOS on the app stack as
> > defined?  This does not seem right.  This should be removed.
> 
> I followed the example in enter_client_code

??  But you're not on the app stack, and you're setting the stack pointer to
mc.xsp, so why mark this empty beyond-mc.xsp slot defined?  It serves no
purpose.

[bruening, 2015-12-18 23:42:43]

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
On 2015/12/18 23:16:31, zhaoqin wrote:
> On 2015/12/18 20:56:09, bruening wrote:
> > I don't understand why you're marking a slot beyond TOS on the app stack as
> > defined?  This does not seem right.  This should be removed.
> 
> I followed the example in enter_client_code
> 
> Currently, we are at the entry of the target function (e.g., repeatme), if
just
> walk the stack, it would be something like
> 
> # 0 drfuzz_reallocate_func
> # 1 main
> 
> Ideally, we want to want the callstack to be something like:
> # 0 drfuzz_reallocate_func
> # 1 repeatme
> # 2 main
> 
> So we should allocate the some extra space on the stack and put target_pc on
the
> stack for stack walk.
> Is it safe to do so, or we are ok with
> # 0 drfuzz_reallocate_func
> # 1 main

This is not sufficient: the repeatme function needs to be there.

This starts getting complex and it might be better to print two frames
explicitly rather than trying to fool the callstack walker, b/c of things like
Win64 unwind data.  But there's no simple interface for sthg like that.

[bruening, 2015-12-19 00:15:08]

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
File common/alloc_replace.c (right):

https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp - sizeof(void*),
final_app_xsp, true/*defined*/);
On 2015/12/18 23:42:43, bruening wrote:
> On 2015/12/18 23:16:31, zhaoqin wrote:
> > On 2015/12/18 20:56:09, bruening wrote:
> > > I don't understand why you're marking a slot beyond TOS on the app stack
as
> > > defined?  This does not seem right.  This should be removed.
> > 
> > I followed the example in enter_client_code
> > 
> > Currently, we are at the entry of the target function (e.g., repeatme), if
> just
> > walk the stack, it would be something like
> > 
> > # 0 drfuzz_reallocate_func
> > # 1 main
> > 
> > Ideally, we want to want the callstack to be something like:
> > # 0 drfuzz_reallocate_func
> > # 1 repeatme
> > # 2 main
> > 
> > So we should allocate the some extra space on the stack and put target_pc on
> the
> > stack for stack walk.
> > Is it safe to do so, or we are ok with
> > # 0 drfuzz_reallocate_func
> > # 1 main
> 
> This is not sufficient: the repeatme function needs to be there.
> 
> This starts getting complex and it might be better to print two frames
> explicitly rather than trying to fool the callstack walker, b/c of things like
> Win64 unwind data.  But there's no simple interface for sthg like that.

For a Win64 unwinder that supports being in the prologue, a fake frame that just
has the retaddr may work -- though I was considering assuming we only ever cared
about a PC that was past the prologue.

[zhaoqin, 2015-12-19 01:45:51]

So should we just have
#0 repeatme
#1 main
And do not have drfuzz_reallocate... in the frame?
On Dec 18, 2015 7:15 PM, <bruening@google.com> wrote:

>
> https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c
> File common/alloc_replace.c (right):
>
>
>
https://codereview.appspot.com/279440043/diff/60001/common/alloc_replace.c#ne...
> common/alloc_replace.c:5439: client_stack_alloc(final_app_xsp -
> sizeof(void*), final_app_xsp, true/*defined*/);
> On 2015/12/18 23:42:43, bruening wrote:
>
>> On 2015/12/18 23:16:31, zhaoqin wrote:
>> > On 2015/12/18 20:56:09, bruening wrote:
>> > > I don't understand why you're marking a slot beyond TOS on the app
>>
> stack as
>
>> > > defined?  This does not seem right.  This should be removed.
>> >
>> > I followed the example in enter_client_code
>> >
>> > Currently, we are at the entry of the target function (e.g.,
>>
> repeatme), if
>
>> just
>> > walk the stack, it would be something like
>> >
>> > # 0 drfuzz_reallocate_func
>> > # 1 main
>> >
>> > Ideally, we want to want the callstack to be something like:
>> > # 0 drfuzz_reallocate_func
>> > # 1 repeatme
>> > # 2 main
>> >
>> > So we should allocate the some extra space on the stack and put
>>
> target_pc on
>
>> the
>> > stack for stack walk.
>> > Is it safe to do so, or we are ok with
>> > # 0 drfuzz_reallocate_func
>> > # 1 main
>>
>
> This is not sufficient: the repeatme function needs to be there.
>>
>
> This starts getting complex and it might be better to print two frames
>> explicitly rather than trying to fool the callstack walker, b/c of
>>
> things like
>
>> Win64 unwind data.  But there's no simple interface for sthg like
>>
> that.
>
> For a Win64 unwinder that supports being in the prologue, a fake frame
> that just has the retaddr may work -- though I was considering assuming
> we only ever cared about a PC that was past the prologue.
>
> https://codereview.appspot.com/279440043/
>

[bruening, 2015-12-19 05:14:16]

On 2015/12/19 01:45:51, zhaoqin wrote:
> So should we just have
> #0 repeatme
> #1 main
> And do not have drfuzz_reallocate... in the frame?

Not long-term.  In order to make progress, though, that is better than
drfuzz_reallocate;main, so I would say pass the repeatme as the caller for now
with a FIXME and file an issue (xref i#639 in the issue).

[zhaoqin, 2015-12-20 02:00:21]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input
buffer

- add -fuzz_replace_buffer to support allocating input buffer by Dr. Memory
- add drfuzz_reallocate_buffer/drfuzz_free_reallocated_buffer in fuzzer.c to
  alloc/free app memory
- add client_app_malloc and client_app_free in alloc_replace.c
  to allocate app memory from client
- remove mutator_span_empty in fuzz_state
- update load_fuzz_input to replace input buffer if necessary
- update find_target_buffer to replace input buffer if necessary
- add fuzz_buffer.overflow test

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

[zhaoqin, 2015-12-20 02:32:38]

Commit log for latest patchset:
---------------
i#1734 Dr. Fuzz: add -fuzz_replace_buffer to support Dr. Fuzz replacing input
buffer

- add -fuzz_replace_buffer to support allocating input buffer by Dr. Memory
- add drfuzz_reallocate_buffer/drfuzz_free_reallocated_buffer in fuzzer.c to
  alloc/free app memory
- add client_app_malloc and client_app_free in alloc_replace.c
  to allocate app memory from client
- remove mutator_span_empty in fuzz_state
- update load_fuzz_input to replace input buffer if necessary
- update find_target_buffer to replace input buffer if necessary
- add fuzz_buffer.overflow test

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