i#1734 Create Dr. Fuzz: save/restore shadow memory

Commit log for first patchset:
---------------
i#1734 Create Dr. Fuzz: save/restore shadow memory

Integrates the fuzzer with full Dr. Memory by
saving and restoring shadow memory during the
fuzzing cycle. Adds 2 tests that read an
uninitialized array during each fuzz iteration.
---------------

[zhaoqin, 2015-08-21 19:42:52]

LGTM with comment.

Some nit comment for improvement, but nothing critical, so LGTM.

https://codereview.appspot.com/261980043/diff/1/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/261980043/diff/1/drfuzz/drfuzz.h#newcode258
drfuzz/drfuzz.h:258: *                                memory must be applied to
the mcontext provided.
I am not sure I understand this comment.
If I only change the application memory instead of any register value, how could
I apply the change to the mcontex provided?

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

https://codereview.appspot.com/261980043/diff/1/drmemory/drmemory.c#newcode1891
drmemory/drmemory.c:1891: fuzzer_init(client_id, options.redzone_size,
options.check_uninitialized
in Dr.Memory, we could still use shadow memory with -no_check_uninitialized.

you may want to use option.shadowing instead, which is used to indicate if
shadow memory is set up.

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

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode96
drmemory/fuzzer.c:96: static shadow_config_t shadow_config;
nit, put this var right below the data structure shadow_config_t.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode120
drmemory/fuzzer.c:120: fuzzer_init(client_id_t client_id, uint redzone_size,
bool check_uninitialized
xref comment in drmemory.c, bool shadowing; might be better.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode244
drmemory/fuzzer.c:244: uint r9;
why must we have the reg name, do we  really use them, would an array work?
It then will make the code shadow_state_save/restore_context simpler, using a
loop. 

We may need add an array for the reg name for get_shadow_register, e.g.,
reg_id_t arg_reg_name[] = {DR_REG_XDI, DR_REG_XSI, ...}

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode268
drmemory/fuzzer.c:268: set_shadow_value(byte *shadow_dst, uint i, uint value)
This name and the one above is confusing, I was thinking it was for get/set the
application's shadow value, and later I realize it is for the saved shadow
value.
maybe get/set_saved_shadow_value, or at least add comment.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode312
drmemory/fuzzer.c:312: shadow_state_save_context(dr_mcontext_t *mc,
shadow_state_t *shadow)
_context make me think it is for the whole context, maybe shadow_state_save_args

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode378
drmemory/fuzzer.c:378: SHADOW_UNDEFINED);
SHADOW_UNDEFINED | SHADOW_UNADDRESSABLE

redzone is also not addressable.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode379
drmemory/fuzzer.c:379: shadow_set_range(tail, tail + shadow_config.redzone_size,
SHADOW_UNDEFINED);
ditto

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode383
drmemory/fuzzer.c:383: shadow_state_restore_buffer(void *fuzzcxt, shadow_state_t
*shadow)
nit, typically, I pair the save/restore functions together if possible so it is
easier to compare, check, and maybe change in the future together.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode409
drmemory/fuzzer.c:409: shadow_state_restore_context(dr_mcontext_t *mc,
shadow_state_t *shadow)
nit, the name and signature suggest it save the whole mcontext, but it actually
save/restore the arg, suggest to rename to shadow_state_restore_args.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode463
drmemory/fuzzer.c:463: slot = PC(mc->xsp + (i + 5)*sizeof(reg_t));
it is usually better to use named macro than number, at least add comment about
the number.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode469
drmemory/fuzzer.c:469: slot = PC(mc->xsp + (i + 1)*sizeof(reg_t));
it looks like the xsp is the only mc value we really used, but before we can
think of a better API, mc should be fine.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode825
drmemory/fuzzer.c:825: #endif
We use X64 to represent 64-bit for both ARM and X86, so it might be better
something like below:
#ifdef X86
# ifdef X64
#  ifdef UNIX
#  else
#  endif /* UNIX/WIN */
# else
#  define REGISTER_ARG_COUNT 0
# endif /* 64/32 */
#elif defined(ARM)
# ifdef X64
#  error NYI
# else
#  define REGISTER_ARG_COUNT 4
# endif /* 64/32 */
#endif /* X86/ARM */

https://codereview.appspot.com/261980043/diff/1/tests/fuzz_buffer.uninitializ...
File tests/fuzz_buffer.uninitialized.out (right):

https://codereview.appspot.com/261980043/diff/1/tests/fuzz_buffer.uninitializ...
tests/fuzz_buffer.uninitialized.out:33: ~~Dr.M~~       2 unique,     8 total
uninitialized access(es)
should we have .res file?

[Byron, 2015-08-23 04:51:11]

Commit log for latest patchset:
---------------
i#1734 Create Dr. Fuzz: save/restore shadow memory

Integrates the fuzzer with full Dr. Memory by
saving and restoring shadow memory during the
fuzzing cycle. Adds 12 tests that leak memory or
read or write unaddressable or uninitialized data.

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

[Byron, 2015-08-23 04:51:41]

bruening's turn to review.

https://codereview.appspot.com/261980043/diff/1/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/261980043/diff/1/drfuzz/drfuzz.h#newcode258
drfuzz/drfuzz.h:258: *                                memory must be applied to
the mcontext provided.
On 2015/08/21 19:42:52, zhaoqin wrote:
> I am not sure I understand this comment.
> If I only change the application memory instead of any register value, how
could
> I apply the change to the mcontex provided?

Right, it should be registers.

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

https://codereview.appspot.com/261980043/diff/1/drmemory/drmemory.c#newcode1891
drmemory/drmemory.c:1891: fuzzer_init(client_id, options.redzone_size,
options.check_uninitialized
On 2015/08/21 19:42:52, zhaoqin wrote:
> in Dr.Memory, we could still use shadow memory with -no_check_uninitialized.
> 
> you may want to use option.shadowing instead, which is used to indicate if
> shadow memory is set up.

Added check for options.shadowing. I didn't implement the mapping type for
-no_check_uninitialized, so I added a corresponding "NYI" comment in
fuzzer_init().

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

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode96
drmemory/fuzzer.c:96: static shadow_config_t shadow_config;
On 2015/08/21 19:42:52, zhaoqin wrote:
> nit, put this var right below the data structure shadow_config_t.

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode120
drmemory/fuzzer.c:120: fuzzer_init(client_id_t client_id, uint redzone_size,
bool check_uninitialized
On 2015/08/21 19:42:52, zhaoqin wrote:
> xref comment in drmemory.c, bool shadowing; might be better.

Done (see reply there).

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode244
drmemory/fuzzer.c:244: uint r9;
On 2015/08/21 19:42:52, zhaoqin wrote:
> why must we have the reg name, do we  really use them, would an array work?
> It then will make the code shadow_state_save/restore_context simpler, using a
> loop. 
> 
> We may need add an array for the reg name for get_shadow_register, e.g.,
> reg_id_t arg_reg_name[] = {DR_REG_XDI, DR_REG_XSI, ...}

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode268
drmemory/fuzzer.c:268: set_shadow_value(byte *shadow_dst, uint i, uint value)
On 2015/08/21 19:42:52, zhaoqin wrote:
> This name and the one above is confusing, I was thinking it was for get/set
the
> application's shadow value, and later I realize it is for the saved shadow
> value.
> maybe get/set_saved_shadow_value, or at least add comment.

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode312
drmemory/fuzzer.c:312: shadow_state_save_context(dr_mcontext_t *mc,
shadow_state_t *shadow)
On 2015/08/21 19:42:52, zhaoqin wrote:
> _context make me think it is for the whole context, maybe
shadow_state_save_args

It includes the shadow state of the stack frame itself, so I changed to
shadow_state_save_stack_frame.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode378
drmemory/fuzzer.c:378: SHADOW_UNDEFINED);
On 2015/08/21 19:42:52, zhaoqin wrote:
> SHADOW_UNDEFINED | SHADOW_UNADDRESSABLE
> 
> redzone is also not addressable.

SHADOW_UNADDRESSABLE is logically inclusive of SHADOW_UNDEFINED (the values are
1 and 3 so they can't be combined bitwise).

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode379
drmemory/fuzzer.c:379: shadow_set_range(tail, tail + shadow_config.redzone_size,
SHADOW_UNDEFINED);
On 2015/08/21 19:42:52, zhaoqin wrote:
> ditto

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode383
drmemory/fuzzer.c:383: shadow_state_restore_buffer(void *fuzzcxt, shadow_state_t
*shadow)
On 2015/08/21 19:42:52, zhaoqin wrote:
> nit, typically, I pair the save/restore functions together if possible so it
is
> easier to compare, check, and maybe change in the future together.

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode409
drmemory/fuzzer.c:409: shadow_state_restore_context(dr_mcontext_t *mc,
shadow_state_t *shadow)
On 2015/08/21 19:42:52, zhaoqin wrote:
> nit, the name and signature suggest it save the whole mcontext, but it
actually
> save/restore the arg, suggest to rename to shadow_state_restore_args.

Done (see previous reply on the save function).

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode463
drmemory/fuzzer.c:463: slot = PC(mc->xsp + (i + 5)*sizeof(reg_t));
On 2015/08/21 19:42:52, zhaoqin wrote:
> it is usually better to use named macro than number, at least add comment
about
> the number.

Done.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode469
drmemory/fuzzer.c:469: slot = PC(mc->xsp + (i + 1)*sizeof(reg_t));
On 2015/08/21 19:42:52, zhaoqin wrote:
> it looks like the xsp is the only mc value we really used, but before we can
> think of a better API, mc should be fine.

Acknowledged.

https://codereview.appspot.com/261980043/diff/1/drmemory/fuzzer.c#newcode825
drmemory/fuzzer.c:825: #endif
On 2015/08/21 19:42:52, zhaoqin wrote:
> We use X64 to represent 64-bit for both ARM and X86, so it might be better
> something like below:
> #ifdef X86
> # ifdef X64
> #  ifdef UNIX
> #  else
> #  endif /* UNIX/WIN */
> # else
> #  define REGISTER_ARG_COUNT 0
> # endif /* 64/32 */
> #elif defined(ARM)
> # ifdef X64
> #  error NYI
> # else
> #  define REGISTER_ARG_COUNT 4
> # endif /* 64/32 */
> #endif /* X86/ARM */

Done.

[bruening, 2015-08-23 17:18:26]

On 2015/08/23 04:51:41, Byron wrote:
> bruening's turn to review.

My first impression is that this is a very large CL: 1394 lines of mostly new
code.  That is far larger than the ideal.

[Byron, 2015-08-23 20:33:25]

On 2015/08/23 17:18:26, bruening wrote:
> On 2015/08/23 04:51:41, Byron wrote:
> > bruening's turn to review.
> 
> My first impression is that this is a very large CL: 1394 lines of mostly new
> code.  That is far larger than the ideal.

If I remove the tests it will be ~400 lines. But the part in fuzzer.c is all
part of one cycle, so I think it won't make any sense in smaller pieces. What do
you suggest? 

We really need to start fuzzing targets soon.

[bruening, 2015-08-24 14:12:50]

Let's discuss the shadow code: IMHO the shadow impl-specific details should all
be behind the shadow module abstraction barrier and not here.

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c
File drfuzz/drfuzz.c (right):

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c#newcode487
drfuzz/drfuzz.c:487: {   /* XXX i#1734: might prefer to return an status code,
because this may fail,
"an status"?

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c#newcode487
drfuzz/drfuzz.c:487: {   /* XXX i#1734: might prefer to return an status code,
because this may fail,
style violation: { on own line

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h#newcode258
drfuzz/drfuzz.h:258: *                                registers must be applied
to the mcontext provided.
I don't understand this sentence.  What changes?  Do you mean restoring them
back to the original state for the new iteration?  Do you mean "any desired
changes"?  Please clarify in the comment.

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

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.c#newcode43
drmemory/fuzzer.c:43: #define PC(x) ((app_pc) (x))
I'm not sure why this is a macro: if the goal is to be able to change a whole
bunch of casts to a different type in one central place, a typedef should be
used instead.  Otherwise, IMHO this makes the code harder to read, as the reader
has to go figure out what this thing is.  Maybe if it were CAST_TO_PC.  But the
actual cast seems simpler and more straightforward.

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.c#newcode118
drmemory/fuzzer.c:118: shadow_config.save_restore_enabled = false;
Why is 64-bit any different than 32-bit?  Please avoid adding hardcoded ifdefs
we'll not know we need to change when we get -check_uninitialized working for
64-bit.

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.c#newcode217
drmemory/fuzzer.c:217: * of 2 bits per byte. Code requires update when
alternatives are implemented.
It would be best to let the shadow.c module worry about the impl and just use
interfaces to talk to it.  Can we avoid hardcoding shadow impl details here?

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.h
File drmemory/fuzzer.h (right):

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.h#newcode61
drmemory/fuzzer.h:61: * memory is mapped at 2 bits per byte (i.e., option
'check_uninitialized' is on).
This should be enforced in at init time then: don't let the user pass "-light
-fuzz...", e.g.

[bruening, 2015-08-24 15:23:57]

On 2015/08/24 14:12:50, bruening wrote:
> Let's discuss the shadow code: IMHO the shadow impl-specific details should
all
> be behind the shadow module abstraction barrier and not here.

Summarizing our discussion, we want two things:

1) Add a new shadow.c routine that copies a range of shadow memory into a
separate buffer (and vice versa).
   This hides the shadow impl, and can also be optimized (in the future) beyond
the inefficient get_byte and set_byte.

2) Try to remove the assumptions on shadow granularity by saving/restoring whole
registers.

This should provide better abstraction and isolation and shouldn't involve too
much work or even too big of a change to the fuzzer code in this CL.

[Byron, 2015-08-26 21:45:10]

Commit log for latest patchset:
---------------
i#1734 Create Dr. Fuzz: save/restore shadow memory

Integrates the fuzzer with full Dr. Memory by
saving and restoring shadow memory during the
fuzzing cycle. Adds 12 tests that leak memory or
read or write unaddressable or uninitialized data.
Updates the internal DynamoRIO instance to
version 5.0.16673, and sets the minimum required
version to 5.0.16672.

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

[Byron, 2015-08-26 21:45:42]

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c
File drfuzz/drfuzz.c (right):

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c#newcode487
drfuzz/drfuzz.c:487: {   /* XXX i#1734: might prefer to return an status code,
because this may fail,
On 2015/08/24 14:12:50, bruening wrote:
> "an status"?

Done (it originally said "an error code").

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.c#newcode487
drfuzz/drfuzz.c:487: {   /* XXX i#1734: might prefer to return an status code,
because this may fail,
On 2015/08/24 14:12:50, bruening wrote:
> style violation: { on own line

Done.

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h#newcode258
drfuzz/drfuzz.h:258: *                                registers must be applied
to the mcontext provided.
On 2015/08/24 14:12:50, bruening wrote:
> I don't understand this sentence.  What changes?  Do you mean restoring them
> back to the original state for the new iteration?  Do you mean "any desired
> changes"?  Please clarify in the comment.

Done.

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

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.c#newcode43
drmemory/fuzzer.c:43: #define PC(x) ((app_pc) (x))
On 2015/08/24 14:12:50, bruening wrote:
> I'm not sure why this is a macro: if the goal is to be able to change a whole
> bunch of casts to a different type in one central place, a typedef should be
> used instead.  Otherwise, IMHO this makes the code harder to read, as the
reader
> has to go figure out what this thing is.  Maybe if it were CAST_TO_PC.  But
the
> actual cast seems simpler and more straightforward.

Done.

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.c#newcode118
drmemory/fuzzer.c:118: shadow_config.save_restore_enabled = false;
On 2015/08/24 14:12:50, bruening wrote:
> Why is 64-bit any different than 32-bit?  Please avoid adding hardcoded ifdefs
> we'll not know we need to change when we get -check_uninitialized working for
> 64-bit.

Changed to ASSERT_NOT_IMPLEMENTED for the case that the user wants shadow
save/restore on x64.

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.h
File drmemory/fuzzer.h (right):

https://codereview.appspot.com/261980043/diff/20001/drmemory/fuzzer.h#newcode61
drmemory/fuzzer.h:61: * memory is mapped at 2 bits per byte (i.e., option
'check_uninitialized' is on).
On 2015/08/24 14:12:50, bruening wrote:
> This should be enforced in at init time then: don't let the user pass "-light
> -fuzz...", e.g.

After refactoring, the only remaining issue is that if someone implements shadow
memory on x64, they'll have to make additional adjustments here in the fuzzer.
For example, the calls to register_shadow_{g,s}et_dword() won't work for x64. So
now the fuzzer exits if called with "-check_uninitialized" on x64.

Light mode should not be a problem anymore, assuming my implementation of
shadow_{save,restore}_buffer() is correct.

[Byron, 2015-08-26 22:10:20]

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c
File drmemory/shadow.c (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode550
drmemory/shadow.c:550: 
The implementation here doesn't consider the MAP_4B_TO_1B case. It works for
now--just stores more shadow information than really necessary (per byte instead
of per dword). But if the MAP_4B_TO_1B implementation changes later, that might
not be true anymore--and it may not be evident that anything needs to be done
here b/c MAP_4B_TO_1B isn't mentioned here. Should I add an XXX comment or
something? I'm not sure if this is really important.

[bruening, 2015-08-27 03:24:51]

> Updates the internal DynamoRIO instance to
> version 5.0.16673, and sets the minimum required

These pseudo-patchlevel-versions are not unique: please list the hash.  Please
describe *why* this updates DR.  I would suggest separating that part from the
rest w/ a blank line too.

[Byron, 2015-08-27 03:46:53]

On 2015/08/27 03:24:51, bruening wrote:
> > Updates the internal DynamoRIO instance to
> > version 5.0.16673, and sets the minimum required
> 
> These pseudo-patchlevel-versions are not unique: please list the hash.  Please
> describe *why* this updates DR.  I would suggest separating that part from the
> rest w/ a blank line too.

New commit message:

    i#1734 Create Dr. Fuzz: save/restore shadow memory
    
    Integrates the fuzzer with full Dr. Memory by
    saving and restoring shadow memory during the
    fuzzing cycle. Adds 12 tests that leak memory or
    read or write unaddressable or uninitialized data.
    
    Updates the internal DynamoRIO instance to version
    cbf91dff693cdf6c7c18eb86cd97e8b68b8a8425 because
    the fuzzer tests require alternate calling
    conventions on Windows. The minimum required
    version of DynamoRIO is now 5.0.16672.

[bruening, 2015-08-27 04:36:02]

LGTM w/ comments

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h
File drfuzz/drfuzz.h (right):

https://codereview.appspot.com/261980043/diff/20001/drfuzz/drfuzz.h#newcode173
drfuzz/drfuzz.h:173: typedef enum _drfuzz_flags_t {
Removing this seems notable but it it's not in the commit msg?

https://codereview.appspot.com/261980043/diff/40001/CMakeLists.txt
File CMakeLists.txt (right):

https://codereview.appspot.com/261980043/diff/40001/CMakeLists.txt#newcode729
CMakeLists.txt:729: set(DynamoRIO_VERSION_REQUIRED "5.0.16672")
I think this is already past this -- just be careful when updating/merging here
and in the submodule

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c#newcode261
drmemory/fuzzer.c:261: #  define ARG_STACK_OFFSET 1 /* retaddr */
It seems like much of this should not be hardcoded in ifdefs and should instead
come from the calling convention of the target function.  Think about
mixed-mode: a 64-bit build (thus X64 is set) will want to target a 32-bit cdecl
function.

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c#newcode323
drmemory/fuzzer.c:323: #if PLATFORM_REGISTER_ARGS
I would suggest removing all these #if's: they make the code harder to read, and
we'd prefer to act on the specified calling convention, not hardcode to a single
bitwidth.

https://codereview.appspot.com/261980043/diff/40001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:604: /* XXX i#1734: May want to consider an option to not
reset red zone shadow state.  */
I would think this comment would be in the fuzz file by the code that does the
reset, not here

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c
File drmemory/shadow.c (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode160
drmemory/shadow.c:160: #define VARSIZE_FIELD(type, name) type
name[VARSIZE_FIELD_PLACEHOLDER_SIZE]
Neither of these are used

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode169
drmemory/shadow.c:169: #define SIZEOF_SAVED_BUFFER_SHADOW(buffer_size) \
Please rename: this is not "buffer_size" but app region size

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode173
drmemory/shadow.c:173: #define SIZEOF_SAVED_BUFFER(buffer_size) \
Ditto

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode524
drmemory/shadow.c:524: bitmapx2_set(saved->shadow, i, shadow_value);
This should check MAP_4B_TO_1B and have ASSERT_NOT_IMPLEMENTED, right?

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode550
drmemory/shadow.c:550: 
On 2015/08/26 22:10:20, Byron wrote:
> The implementation here doesn't consider the MAP_4B_TO_1B case. It works for
> now--just stores more shadow information than really necessary (per byte
instead
> of per dword). But if the MAP_4B_TO_1B implementation changes later, that
might
> not be true anymore--and it may not be evident that anything needs to be done
> here b/c MAP_4B_TO_1B isn't mentioned here. Should I add an XXX comment or
> something? I'm not sure if this is really important.

There should be an ASSERT_NOT_IMPLEMENTED at line 524

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h
File drmemory/shadow.h (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h#newcode196
drmemory/shadow.h:196: /* Saves the shadow values for the supplied buffer into a
newly allocated shadow buffer.
Please rename: it is not a "supplied buffer", it is an app address range and not
a "buffer" at all.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h#newcode200
drmemory/shadow.h:200: shadow_save_buffer(app_pc buffer, size_t buffer_size);
IMHO these names are very confusing: I thought the buffer_size was the size of
the resulting buffer and was wondering how the user knew it.  Please remove the
word "buffer" from both of these param names.  This is *not* the buffer, but the
app region start and the app region size.

[Byron, 2015-08-27 17:34:59]

Committed as
https://github.com/DynamoRIO/drmemory/commit/f23c52a34aacbfb0681db73a9d5034d9...

Final commit log: 
---------------
i#1734 Create Dr. Fuzz: save/restore shadow memory

Integrates the fuzzer with full Dr. Memory by
saving and restoring shadow memory during the
fuzzing cycle. Adds 12 tests that leak memory or
read or write unaddressable or uninitialized data.

Depends on DynamoRIO 5.0.16772.

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

[Byron, 2015-08-27 19:10:15]

https://codereview.appspot.com/261980043/diff/40001/CMakeLists.txt
File CMakeLists.txt (right):

https://codereview.appspot.com/261980043/diff/40001/CMakeLists.txt#newcode729
CMakeLists.txt:729: set(DynamoRIO_VERSION_REQUIRED "5.0.16672")
On 2015/08/27 04:36:02, bruening wrote:
> I think this is already past this -- just be careful when updating/merging
here
> and in the submodule

Yes, there are more commits, but they are not required, per se. This version has
the added calling conventions.

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c
File drmemory/fuzzer.c (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c#newcode261
drmemory/fuzzer.c:261: #  define ARG_STACK_OFFSET 1 /* retaddr */
On 2015/08/27 04:36:02, bruening wrote:
> It seems like much of this should not be hardcoded in ifdefs and should
instead
> come from the calling convention of the target function.  Think about
> mixed-mode: a 64-bit build (thus X64 is set) will want to target a 32-bit
cdecl
> function.

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/fuzzer.c#newcode323
drmemory/fuzzer.c:323: #if PLATFORM_REGISTER_ARGS
On 2015/08/27 04:36:02, bruening wrote:
> I would suggest removing all these #if's: they make the code harder to read,
and
> we'd prefer to act on the specified calling convention, not hardcode to a
single
> bitwidth.

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/optionsx.h
File drmemory/optionsx.h (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/optionsx.h#newco...
drmemory/optionsx.h:604: /* XXX i#1734: May want to consider an option to not
reset red zone shadow state.  */
On 2015/08/27 04:36:02, bruening wrote:
> I would think this comment would be in the fuzz file by the code that does the
> reset, not here

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c
File drmemory/shadow.c (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode160
drmemory/shadow.c:160: #define VARSIZE_FIELD(type, name) type
name[VARSIZE_FIELD_PLACEHOLDER_SIZE]
On 2015/08/27 04:36:02, bruening wrote:
> Neither of these are used

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode169
drmemory/shadow.c:169: #define SIZEOF_SAVED_BUFFER_SHADOW(buffer_size) \
On 2015/08/27 04:36:02, bruening wrote:
> Please rename: this is not "buffer_size" but app region size

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode173
drmemory/shadow.c:173: #define SIZEOF_SAVED_BUFFER(buffer_size) \
On 2015/08/27 04:36:02, bruening wrote:
> Ditto

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode524
drmemory/shadow.c:524: bitmapx2_set(saved->shadow, i, shadow_value);
On 2015/08/27 04:36:02, bruening wrote:
> This should check MAP_4B_TO_1B and have ASSERT_NOT_IMPLEMENTED, right?

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.c#newcode550
drmemory/shadow.c:550: 
On 2015/08/27 04:36:02, bruening wrote:
> On 2015/08/26 22:10:20, Byron wrote:
> > The implementation here doesn't consider the MAP_4B_TO_1B case. It works for
> > now--just stores more shadow information than really necessary (per byte
> instead
> > of per dword). But if the MAP_4B_TO_1B implementation changes later, that
> might
> > not be true anymore--and it may not be evident that anything needs to be
done
> > here b/c MAP_4B_TO_1B isn't mentioned here. Should I add an XXX comment or
> > something? I'm not sure if this is really important.
> 
> There should be an ASSERT_NOT_IMPLEMENTED at line 524

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h
File drmemory/shadow.h (right):

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h#newcode196
drmemory/shadow.h:196: /* Saves the shadow values for the supplied buffer into a
newly allocated shadow buffer.
On 2015/08/27 04:36:02, bruening wrote:
> Please rename: it is not a "supplied buffer", it is an app address range and
not
> a "buffer" at all.

Done.

https://codereview.appspot.com/261980043/diff/40001/drmemory/shadow.h#newcode200
drmemory/shadow.h:200: shadow_save_buffer(app_pc buffer, size_t buffer_size);
On 2015/08/27 04:36:02, bruening wrote:
> IMHO these names are very confusing: I thought the buffer_size was the size of
> the resulting buffer and was wondering how the user knew it.  Please remove
the
> word "buffer" from both of these param names.  This is *not* the buffer, but
the
> app region start and the app region size.

Done.