i#1734 drfuzz: remove redzone reset code

Commit log for first patchset:
---------------
i#1734 drfuzz: remove redzone reset code

The benefit of reset redzone is to report separate (probably similar)
errors in different fuzzing iterations on accessing the same address.
We can reset the redzone only if the input buffer is a complete allocated
region. Otherwise, we could introduce false positives.
It seems the benefit does not justify the possible false positives.
---------------

[bruening, 2015-11-18 00:22:58]

Why doesn't the reset code simply check whether the buffer start equals a malloc
start?

[zhaoqin, 2015-11-18 01:58:36]

More serious question is do we need reset redzone at all?
On Nov 17, 2015 19:22, <bruening@google.com> wrote:

> Why doesn't the reset code simply check whether the buffer start equals
> a malloc start?
>
> https://codereview.appspot.com/276060043/
>

[bruening, 2015-11-18 02:07:19]

I thought there was a general shadow memory reset action.  For pattern
mode, this is the equivalent, right?  At least a partial equivalent.  For
shadow modes, I don't remember how the reset occurs, but this seems like it
would be a subset and would happen under the general shadow reset.

On Tue, Nov 17, 2015 at 8:58 PM, Qin Zhao <zhaoqin@google.com> wrote:

> More serious question is do we need reset redzone at all?
> On Nov 17, 2015 19:22, <bruening@google.com> wrote:
>
>> Why doesn't the reset code simply check whether the buffer start equals
>> a malloc start?
>>
>> https://codereview.appspot.com/276060043/
>>
>

[zhaoqin, 2015-11-18 02:50:21]

On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening <bruening@google.com> wrote:

> I thought there was a general shadow memory reset action.  For pattern
> mode, this is the equivalent, right?  At least a partial equivalent.  For
> shadow modes, I don't remember how the reset occurs, but this seems like it
> would be a subset and would happen under the general shadow reset.
>

The common case is to clear the bit to avoid duplicated error reports. I do
not remember there is a reset action used.
I am sure we can reset redzone properly if we try, my question is should we?
If no buffer overflow happens, no need to reset.
If there is a buffer overflow, do we want to reset the redzone in the next
iteration to report likely same/similar error.


>
> On Tue, Nov 17, 2015 at 8:58 PM, Qin Zhao <zhaoqin@google.com> wrote:
>
>> More serious question is do we need reset redzone at all?
>> On Nov 17, 2015 19:22, <bruening@google.com> wrote:
>>
>>> Why doesn't the reset code simply check whether the buffer start equals
>>> a malloc start?
>>>
>>> https://codereview.appspot.com/276060043/
>>>
>>
>

[bruening, 2015-11-18 15:02:07]

On 2015/11/18 02:50:21, zhaoqin wrote:
> On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening <mailto:bruening@google.com>
wrote:
> 
> > I thought there was a general shadow memory reset action.  For pattern
> > mode, this is the equivalent, right?  At least a partial equivalent.  For
> > shadow modes, I don't remember how the reset occurs, but this seems like it
> > would be a subset and would happen under the general shadow reset.
> >
> 
> The common case is to clear the bit to avoid duplicated error reports. I do
> not remember there is a reset action used.

Please see f23c52a34aacbfb0681db73a9d5034d95f31db6c

[zhaoqin, 2015-11-18 15:39:36]

On 2015/11/18 15:02:07, bruening wrote:
> On 2015/11/18 02:50:21, zhaoqin wrote:
> > On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening <mailto:bruening@google.com>
> wrote:
> > 
> > > I thought there was a general shadow memory reset action.  For pattern
> > > mode, this is the equivalent, right?  At least a partial equivalent.  For
> > > shadow modes, I don't remember how the reset occurs, but this seems like
it
> > > would be a subset and would happen under the general shadow reset.
> > >
> > 
> > The common case is to clear the bit to avoid duplicated error reports. I do
> > not remember there is a reset action used.
> 
> Please see f23c52a34aacbfb0681db73a9d5034d95f31db6c

Yes, I know the reset is added there, for resetting the redzone between fuzzing
iteration.
And that's the code/case I am asking if we should keep or remove for simplicity.

[bruening, 2015-11-18 16:07:49]

On 2015/11/18 15:39:36, zhaoqin wrote:
> On 2015/11/18 15:02:07, bruening wrote:
> > On 2015/11/18 02:50:21, zhaoqin wrote:
> > > On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening
<mailto:bruening@google.com>
> > wrote:
> > > 
> > > > I thought there was a general shadow memory reset action.  For pattern
> > > > mode, this is the equivalent, right?  At least a partial equivalent. 
For
> > > > shadow modes, I don't remember how the reset occurs, but this seems like
> it
> > > > would be a subset and would happen under the general shadow reset.
> > > >
> > > 
> > > The common case is to clear the bit to avoid duplicated error reports. I
do
> > > not remember there is a reset action used.
> > 
> > Please see f23c52a34aacbfb0681db73a9d5034d95f31db6c
> 
> Yes, I know the reset is added there, for resetting the redzone between
fuzzing
> iteration.
> And that's the code/case I am asking if we should keep or remove for
simplicity.

Then I am confused: why does this CL only touch the pattern code, if you're
proposing removing all of the shadow/pattern reset code?  My vote would be to
just fix this pattern code to properly check the heap bounds (and move it to be
nearer the shadow code if it's not already).

[zhaoqin, 2015-11-18 16:17:45]

On 2015/11/18 16:07:49, bruening wrote:
> On 2015/11/18 15:39:36, zhaoqin wrote:
> > On 2015/11/18 15:02:07, bruening wrote:
> > > On 2015/11/18 02:50:21, zhaoqin wrote:
> > > > On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening
> <mailto:bruening@google.com>
> > > wrote:
> > > > 
> > > > > I thought there was a general shadow memory reset action.  For pattern
> > > > > mode, this is the equivalent, right?  At least a partial equivalent. 
> For
> > > > > shadow modes, I don't remember how the reset occurs, but this seems
like
> > it
> > > > > would be a subset and would happen under the general shadow reset.
> > > > >
> > > > 
> > > > The common case is to clear the bit to avoid duplicated error reports. I
> do
> > > > not remember there is a reset action used.
> > > 
> > > Please see f23c52a34aacbfb0681db73a9d5034d95f31db6c
> > 
> > Yes, I know the reset is added there, for resetting the redzone between
> fuzzing
> > iteration.
> > And that's the code/case I am asking if we should keep or remove for
> simplicity.
> 
> Then I am confused: why does this CL only touch the pattern code, if you're
> proposing removing all of the shadow/pattern reset code?  My vote would be to
> just fix this pattern code to properly check the heap bounds (and move it to
be
> nearer the shadow code if it's not already).

I am confused too.
This CL touched both shadow code and the pattern code: e.g.,
shadow_state_reset_redzone is removed.
To fix it, you need check the heap bounds for both shadow and pattern, and I do
not see much benefit of reset.
That's why my vote is to simply remove all redzone reset code.
I am ok to add heap bounds check, just want to clarify whether it worth doing.

[bruening, 2015-11-18 16:29:10]

On 2015/11/18 16:17:45, zhaoqin wrote:
> On 2015/11/18 16:07:49, bruening wrote:
> > On 2015/11/18 15:39:36, zhaoqin wrote:
> > > On 2015/11/18 15:02:07, bruening wrote:
> > > > On 2015/11/18 02:50:21, zhaoqin wrote:
> > > > > On Tue, Nov 17, 2015 at 9:07 PM, Derek Bruening
> > <mailto:bruening@google.com>
> > > > wrote:
> > > > > 
> > > > > > I thought there was a general shadow memory reset action.  For
pattern
> > > > > > mode, this is the equivalent, right?  At least a partial equivalent.

> > For
> > > > > > shadow modes, I don't remember how the reset occurs, but this seems
> like
> > > it
> > > > > > would be a subset and would happen under the general shadow reset.
> > > > > >
> > > > > 
> > > > > The common case is to clear the bit to avoid duplicated error reports.
I
> > do
> > > > > not remember there is a reset action used.
> > > > 
> > > > Please see f23c52a34aacbfb0681db73a9d5034d95f31db6c
> > > 
> > > Yes, I know the reset is added there, for resetting the redzone between
> > fuzzing
> > > iteration.
> > > And that's the code/case I am asking if we should keep or remove for
> > simplicity.
> > 
> > Then I am confused: why does this CL only touch the pattern code, if you're
> > proposing removing all of the shadow/pattern reset code?  My vote would be
to
> > just fix this pattern code to properly check the heap bounds (and move it to
> be
> > nearer the shadow code if it's not already).
> 
> I am confused too.
> This CL touched both shadow code and the pattern code: e.g.,
> shadow_state_reset_redzone is removed.
> To fix it, you need check the heap bounds for both shadow and pattern, and I
do
> not see much benefit of reset.
> That's why my vote is to simply remove all redzone reset code.
> I am ok to add heap bounds check, just want to clarify whether it worth doing.

What I see is a large CL that put in place shadow/pattern reset code.  Here
you're just removing part of it, the redzone.  Why would the redzone reset have
a different benefit from the rest of the reset code?

[zhaoqin, 2015-11-18 16:45:07]

> 
> What I see is a large CL that put in place shadow/pattern reset code.  Here
> you're just removing part of it, the redzone.  Why would the redzone reset
have
> a different benefit from the rest of the reset code?

My understand is that CL (https://codereview.appspot.com/261980043) is to add
save/restore shadow state for using Dr. Memory shadow mode.

Restoring shadow state for input buffer and other function arguments makes sense
to me as it avoids false positives, esp those argument registers, which could be
changed during the execution and need restore before we repeat the function.

While redzone is a different story. It would be changed if there is an unaddr
error or a memory free and reallocation.
For memory free and reallocation, it is a much more complex case to handle,
cannot be simply reset.
Here I want to know if we need reset for the unaddr error. For the same logic to
clear bit when reporting an unaddr error, my vote is to not reset here.

[bruening, 2015-11-18 19:36:00]

On 2015/11/18 16:45:07, zhaoqin wrote:
> While redzone is a different story. It would be changed if there is an unaddr
> error

I'm not sure what you mean: on an unaddr, Dr. Memory does not change the shadow
value, so there is nothing to "reset" in that sense.  

> or a memory free and reallocation.
> For memory free and reallocation, it is a much more complex case to handle,
> cannot be simply reset.

Isn't the shadow of the buffer itself in the same category wrt free or realloc? 
It doesn't seem like the redzone is special there.

> Here I want to know if we need reset for the unaddr error. For the same logic
to
> clear bit when reporting an unaddr error, my vote is to not reset here.

If the buffer is freed and something else allocated in the memory range of the
redzones, by not resetting you would fail to find an overflow.  Such behavior
does have other possible bad interactions if someone doesn't put back in place
the original alloc.  Though a reset of the buffer + any redzones seems like a
reasonable simple step to try and handle whatever the function is doing.

[zhaoqin, 2015-11-18 20:15:15]

On 2015/11/18 19:36:00, bruening wrote:
> On 2015/11/18 16:45:07, zhaoqin wrote:
> > While redzone is a different story. It would be changed if there is an
unaddr
> > error
> 
> I'm not sure what you mean: on an unaddr, Dr. Memory does not change the
shadow
> value, so there is nothing to "reset" in that sense.  

ok, checking the code:
in Dr. Memory, on an uninit error, the shadow memory will be set SHADOW_DEFINED,
but not for unaddr, drmemory/readwrite.c:3963.
However, in pattern mode, we clear the pattern value there,
drmemory/pattern.c:1306. I remember it is suggested by your review to do so.

> 
> > or a memory free and reallocation.
> > For memory free and reallocation, it is a much more complex case to handle,
> > cannot be simply reset.
> 
> Isn't the shadow of the buffer itself in the same category wrt free or
realloc? 
> It doesn't seem like the redzone is special there.
> 
> > Here I want to know if we need reset for the unaddr error. For the same
logic
> to
> > clear bit when reporting an unaddr error, my vote is to not reset here.
> 
> If the buffer is freed and something else allocated in the memory range of the
> redzones, by not resetting you would fail to find an overflow.  Such behavior
> does have other possible bad interactions if someone doesn't put back in place
> the original alloc.  Though a reset of the buffer + any redzones seems like a
> reasonable simple step to try and handle whatever the function is doing.

If the buffer is freed or something, it is much more complex cases to handle.
Out of the scope of this reset discussion.
I listed it above just to complete the cases if a redzone value could be
changed. So please forget about that from now on, because I do not want to
handle that in this CL.

Now let's just discuss if we need reset redzone, or a more general question,
what to reset/restore for each fuzzing iteration.
1. argument registers:
I think they should be restored.
2. argument on the stack:
I think it should be restored.
3. input buffer:
Assuming it is not freed, it most likely just been read, in which case, we do
not need restore anything.
However, if the buffer is to be written and the original buffer is not fully
initialized, would it be safe to restore the shadow state for uninit error.
I can see either way, maybe not restore it, or even mark it all defined.
For cases like padding in between data structure fields, seems restore is
better.
4. redzone:
Since we do not change the redzone value on even unaddr error, it is clear we do
not need to reset.
Even we changed it for pattern mode, I vote for not reset to avoid duplicated
error reports.

So it looks to me that redzone should not be restore/reset, the rest should be.

[zhaoqin, 2015-11-18 21:07:03]

On 2015/11/18 20:15:15, zhaoqin wrote:
> On 2015/11/18 19:36:00, bruening wrote:
> > On 2015/11/18 16:45:07, zhaoqin wrote:
> > > While redzone is a different story. It would be changed if there is an
> unaddr
> > > error
> > 
> > I'm not sure what you mean: on an unaddr, Dr. Memory does not change the
> shadow
> > value, so there is nothing to "reset" in that sense.  
> 
> ok, checking the code:
> in Dr. Memory, on an uninit error, the shadow memory will be set
SHADOW_DEFINED,
> but not for unaddr, drmemory/readwrite.c:3963.
> However, in pattern mode, we clear the pattern value there,
> drmemory/pattern.c:1306. I remember it is suggested by your review to do so.
> 
> > 
> > > or a memory free and reallocation.
> > > For memory free and reallocation, it is a much more complex case to
handle,
> > > cannot be simply reset.
> > 
> > Isn't the shadow of the buffer itself in the same category wrt free or
> realloc? 
> > It doesn't seem like the redzone is special there.
> > 
> > > Here I want to know if we need reset for the unaddr error. For the same
> logic
> > to
> > > clear bit when reporting an unaddr error, my vote is to not reset here.
> > 
> > If the buffer is freed and something else allocated in the memory range of
the
> > redzones, by not resetting you would fail to find an overflow.  Such
behavior
> > does have other possible bad interactions if someone doesn't put back in
place
> > the original alloc.  Though a reset of the buffer + any redzones seems like
a
> > reasonable simple step to try and handle whatever the function is doing.
> 
> If the buffer is freed or something, it is much more complex cases to handle.
> Out of the scope of this reset discussion.
> I listed it above just to complete the cases if a redzone value could be
> changed. So please forget about that from now on, because I do not want to
> handle that in this CL.
> 
> Now let's just discuss if we need reset redzone, or a more general question,
> what to reset/restore for each fuzzing iteration.
> 1. argument registers:
> I think they should be restored.
> 2. argument on the stack:
> I think it should be restored.
> 3. input buffer:
> Assuming it is not freed, it most likely just been read, in which case, we do
> not need restore anything.
> However, if the buffer is to be written and the original buffer is not fully
> initialized, would it be safe to restore the shadow state for uninit error.
> I can see either way, maybe not restore it, or even mark it all defined.
> For cases like padding in between data structure fields, seems restore is
> better.
> 4. redzone:
> Since we do not change the redzone value on even unaddr error, it is clear we
do
> not need to reset.
> Even we changed it for pattern mode, I vote for not reset to avoid duplicated
> error reports.
> 
> So it looks to me that redzone should not be restore/reset, the rest should
be.

Thinking more, maybe we do not need to restore input buffer either, same reason
for avoiding duplicated error reports.

[zhaoqin, 2015-11-18 21:08:04]

On 2015/11/18 21:07:03, zhaoqin wrote:
> On 2015/11/18 20:15:15, zhaoqin wrote:
> > On 2015/11/18 19:36:00, bruening wrote:
> > > On 2015/11/18 16:45:07, zhaoqin wrote:
> > > > While redzone is a different story. It would be changed if there is an
> > unaddr
> > > > error
> > > 
> > > I'm not sure what you mean: on an unaddr, Dr. Memory does not change the
> > shadow
> > > value, so there is nothing to "reset" in that sense.  
> > 
> > ok, checking the code:
> > in Dr. Memory, on an uninit error, the shadow memory will be set
> SHADOW_DEFINED,
> > but not for unaddr, drmemory/readwrite.c:3963.
> > However, in pattern mode, we clear the pattern value there,
> > drmemory/pattern.c:1306. I remember it is suggested by your review to do so.
> > 
> > > 
> > > > or a memory free and reallocation.
> > > > For memory free and reallocation, it is a much more complex case to
> handle,
> > > > cannot be simply reset.
> > > 
> > > Isn't the shadow of the buffer itself in the same category wrt free or
> > realloc? 
> > > It doesn't seem like the redzone is special there.
> > > 
> > > > Here I want to know if we need reset for the unaddr error. For the same
> > logic
> > > to
> > > > clear bit when reporting an unaddr error, my vote is to not reset here.
> > > 
> > > If the buffer is freed and something else allocated in the memory range of
> the
> > > redzones, by not resetting you would fail to find an overflow.  Such
> behavior
> > > does have other possible bad interactions if someone doesn't put back in
> place
> > > the original alloc.  Though a reset of the buffer + any redzones seems
like
> a
> > > reasonable simple step to try and handle whatever the function is doing.
> > 
> > If the buffer is freed or something, it is much more complex cases to
handle.
> > Out of the scope of this reset discussion.
> > I listed it above just to complete the cases if a redzone value could be
> > changed. So please forget about that from now on, because I do not want to
> > handle that in this CL.
> > 
> > Now let's just discuss if we need reset redzone, or a more general question,
> > what to reset/restore for each fuzzing iteration.
> > 1. argument registers:
> > I think they should be restored.
> > 2. argument on the stack:
> > I think it should be restored.
> > 3. input buffer:
> > Assuming it is not freed, it most likely just been read, in which case, we
do
> > not need restore anything.
> > However, if the buffer is to be written and the original buffer is not fully
> > initialized, would it be safe to restore the shadow state for uninit error.
> > I can see either way, maybe not restore it, or even mark it all defined.
> > For cases like padding in between data structure fields, seems restore is
> > better.
> > 4. redzone:
> > Since we do not change the redzone value on even unaddr error, it is clear
we
> do
> > not need to reset.
> > Even we changed it for pattern mode, I vote for not reset to avoid
duplicated
> > error reports.
> > 
> > So it looks to me that redzone should not be restore/reset, the rest should
> be.
> 
> Thinking more, maybe we do not need to restore input buffer either, same
reason
> for avoiding duplicated error reports.

hmm, what if some uninit value copied into the input buffer, should we restore
it to be defined?

[bruening, 2015-11-18 22:13:03]

On 2015/11/18 21:08:04, zhaoqin wrote:
> hmm, what if some uninit value copied into the input buffer, should we restore
> it to be defined?

I would think so.  Esp since the code is already there.

[zhaoqin, 2015-11-18 22:23:23]

On 2015/11/18 22:13:03, bruening wrote:
> On 2015/11/18 21:08:04, zhaoqin wrote:
> > hmm, what if some uninit value copied into the input buffer, should we
restore
> > it to be defined?
> 
> I would think so.  Esp since the code is already there.

so it is clear to restore arg in reg, arg on stack, and input buffer.
What about redzone?

[bruening, 2015-11-19 02:03:59]

On 2015/11/18 22:23:23, zhaoqin wrote:
> On 2015/11/18 22:13:03, bruening wrote:
> > On 2015/11/18 21:08:04, zhaoqin wrote:
> > > hmm, what if some uninit value copied into the input buffer, should we
> restore
> > > it to be defined?
> > 
> > I would think so.  Esp since the code is already there.
> 
> so it is clear to restore arg in reg, arg on stack, and input buffer.
> What about redzone?

I am fine either way (though I would lean toward a reset b/c the code is already
half-way there and I see little downside and possible upside).  If you keep this
removal approach, please update the commit message as it does not seem accurate.
 Please add comments in the code about why we are not resetting the redzone. 
Else LGTM.

[zhaoqin, 2015-11-19 03:17:36]

On 2015/11/19 02:03:59, bruening wrote:
> On 2015/11/18 22:23:23, zhaoqin wrote:
> > On 2015/11/18 22:13:03, bruening wrote:
> > > On 2015/11/18 21:08:04, zhaoqin wrote:
> > > > hmm, what if some uninit value copied into the input buffer, should we
> > restore
> > > > it to be defined?
> > > 
> > > I would think so.  Esp since the code is already there.
> > 
> > so it is clear to restore arg in reg, arg on stack, and input buffer.
> > What about redzone?
> 
> I am fine either way (though I would lean toward a reset b/c the code is
already
> half-way there and I see little downside and possible upside).

The redzone code is neither useful (since unaddr error won't change shadow state
in the redzone, there is no need to restore/reset),
nor correct (image a the target function something like func(string,
strlen(string)), redzone reset will set the last '\0' address as unaddr and
introduce false positives),
I see more downside than upside if any.

> If you keep this
> removal approach, please update the commit message as it does not seem
accurate.
>  Please add comments in the code about why we are not resetting the redzone. 

will update.

> Else LGTM.

[bruening, 2015-11-19 03:19:28]

On 2015/11/19 03:17:36, zhaoqin wrote:
> On 2015/11/19 02:03:59, bruening wrote:
> > On 2015/11/18 22:23:23, zhaoqin wrote:
> > > On 2015/11/18 22:13:03, bruening wrote:
> > > > On 2015/11/18 21:08:04, zhaoqin wrote:
> > > > > hmm, what if some uninit value copied into the input buffer, should we
> > > restore
> > > > > it to be defined?
> > > > 
> > > > I would think so.  Esp since the code is already there.
> > > 
> > > so it is clear to restore arg in reg, arg on stack, and input buffer.
> > > What about redzone?
> > 
> > I am fine either way (though I would lean toward a reset b/c the code is
> already
> > half-way there and I see little downside and possible upside).
> 
> The redzone code is neither useful (since unaddr error won't change shadow
state
> in the redzone, there is no need to restore/reset),
> nor correct (image a the target function something like func(string,
> strlen(string)), redzone reset will set the last '\0' address as unaddr and
> introduce false positives),
> I see more downside than upside if any.

I'm talking about downside and upside to *correct* redzone code that makes a
call to get the heap bounds, which would be a 1 or 2 line change, which is a
*smaller* change than ripping out all the code.

[zhaoqin, 2015-11-19 04:19:09]

Committed as
https://github.com/DynamoRIO/drmemory/commit/70c5d82af79b0fc53cfe77c8bf07bdbd...

Final commit log: 
---------------
i#1734 drfuzz: remove redzone reset code

- remove incorrect redzone reset implementation in fuzzer.c

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