Patchwork tests: set MALLOC_PERTURB_ to expose memory bugs

login
register
mail settings
Submitter Stefan Hajnoczi
Date May 17, 2013, 8:52 a.m.
Message ID <1368780720-14852-1-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/244538/
State New
Headers show

Comments

Stefan Hajnoczi - May 17, 2013, 8:52 a.m.
glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
variable is set.  The value of the environment variable determines the
bit pattern used to wipe memory.  For more information, see
http://udrepper.livejournal.com/11429.html.

Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
the environment variable to 1 so the test is deterministic.  Setting a
random variable might expose more bugs but would be harder to reproduce.

Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
it for in-tree test suites we can detect memory management errors earlier.

 tests/Makefile           | 4 +++-
 tests/qemu-iotests/check | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)
Markus Armbruster - May 17, 2013, 9:54 a.m.
Stefan Hajnoczi <stefanha@redhat.com> writes:

> glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
> variable is set.  The value of the environment variable determines the
> bit pattern used to wipe memory.  For more information, see
> http://udrepper.livejournal.com/11429.html.
>
> Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
> the environment variable to 1 so the test is deterministic.  Setting a
> random variable might expose more bugs but would be harder to reproduce.
>
> Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
> it for in-tree test suites we can detect memory management errors earlier.
>
>  tests/Makefile           | 4 +++-
>  tests/qemu-iotests/check | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/Makefile b/tests/Makefile
> index a307d5a..25f6d28 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
> +		MALLOC_PERTURB_=1 \
>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>  	  echo Gcov report for $$f:;\

If you want punishment, why not go for extra punishment?

MALLOC_PERTURB_=$(($RANDOM % 255 + 1))

[...]
Daniel P. Berrange - May 17, 2013, 10:07 a.m.
On Fri, May 17, 2013 at 11:54:12AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
> > variable is set.  The value of the environment variable determines the
> > bit pattern used to wipe memory.  For more information, see
> > http://udrepper.livejournal.com/11429.html.
> >
> > Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
> > the environment variable to 1 so the test is deterministic.  Setting a
> > random variable might expose more bugs but would be harder to reproduce.
> >
> > Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
> > it for in-tree test suites we can detect memory management errors earlier.
> >
> >  tests/Makefile           | 4 +++-
> >  tests/qemu-iotests/check | 2 +-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/Makefile b/tests/Makefile
> > index a307d5a..25f6d28 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
> >  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
> >  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
> >  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
> > +		MALLOC_PERTURB_=1 \
> >  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
> >  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
> >  	  echo Gcov report for $$f:;\
> 
> If you want punishment, why not go for extra punishment?
> 
> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))

That could lead to non-reproducable failures though. I think it is better
to use a fixed value so that you're more likely to be able to reproduce
the issue every time you run the tests.

Rather than setting MALLOC_PERTURB_=1 unconditionally in the Makefile
though, it ought to honour any existing MALLOC_PERTURB_ env variable
the user has set. That could let automated test harness run repeatedly
with random MALLOC_PERTURB_, while still giving a deterministic value
for developers by default.


Daniel
Markus Armbruster - May 17, 2013, 10:58 a.m.
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Fri, May 17, 2013 at 11:54:12AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
>> > variable is set.  The value of the environment variable determines the
>> > bit pattern used to wipe memory.  For more information, see
>> > http://udrepper.livejournal.com/11429.html.
>> >
>> > Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
>> > the environment variable to 1 so the test is deterministic.  Setting a
>> > random variable might expose more bugs but would be harder to reproduce.
>> >
>> > Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
>> >
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> > Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
>> > it for in-tree test suites we can detect memory management errors earlier.
>> >
>> >  tests/Makefile           | 4 +++-
>> >  tests/qemu-iotests/check | 2 +-
>> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tests/Makefile b/tests/Makefile
>> > index a307d5a..25f6d28 100644
>> > --- a/tests/Makefile
>> > +++ b/tests/Makefile
>> > @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>> >  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
>> >  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>> >  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>> > +		MALLOC_PERTURB_=1 \
>> >  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
>> >  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>> >  	  echo Gcov report for $$f:;\
>> 
>> If you want punishment, why not go for extra punishment?
>> 
>> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
>
> That could lead to non-reproducable failures though. I think it is better
> to use a fixed value so that you're more likely to be able to reproduce
> the issue every time you run the tests.

A fixed value misses failures.

A random value is just as reproducible as a fixed one, simply use the
same value.  Requires the value to be logged, of course.

> Rather than setting MALLOC_PERTURB_=1 unconditionally in the Makefile
> though, it ought to honour any existing MALLOC_PERTURB_ env variable
> the user has set. That could let automated test harness run repeatedly
> with random MALLOC_PERTURB_, while still giving a deterministic value
> for developers by default.

Yes, we should respect the user's MALLOC_PERTURB_.
Stefan Hajnoczi - May 17, 2013, 11:15 a.m.
On Fri, May 17, 2013 at 11:54 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
>> variable is set.  The value of the environment variable determines the
>> bit pattern used to wipe memory.  For more information, see
>> http://udrepper.livejournal.com/11429.html.
>>
>> Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
>> the environment variable to 1 so the test is deterministic.  Setting a
>> random variable might expose more bugs but would be harder to reproduce.
>>
>> Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
>> it for in-tree test suites we can detect memory management errors earlier.
>>
>>  tests/Makefile           | 4 +++-
>>  tests/qemu-iotests/check | 2 +-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index a307d5a..25f6d28 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>>  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
>>       $(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>>       $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>> +             MALLOC_PERTURB_=1 \
>>               gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
>>       $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>>         echo Gcov report for $$f:;\
>
> If you want punishment, why not go for extra punishment?
>
> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))

Covered in the commit description:
"Note we always set
the environment variable to 1 so the test is deterministic.  Setting a
random variable might expose more bugs but would be harder to reproduce."

I didn't want to clutter output with "MALLOC_PERTURB_=123" on every
run.  AFAIK we have no log where this can be silently stashed.  I
guess we could write it to a file and add that to .gitignore.

Stefan
Stefan Hajnoczi - May 17, 2013, 11:16 a.m.
On Fri, May 17, 2013 at 12:07 PM, Daniel P. Berrange
<berrange@redhat.com> wrote:
> Rather than setting MALLOC_PERTURB_=1 unconditionally in the Makefile
> though, it ought to honour any existing MALLOC_PERTURB_ env variable
> the user has set. That could let automated test harness run repeatedly
> with random MALLOC_PERTURB_, while still giving a deterministic value
> for developers by default.

Good point.  Will fix in v2.

Stefan
Lucas Meneghel Rodrigues - May 17, 2013, 12:49 p.m.
On 17/05/13 07:07 AM, Daniel P. Berrange wrote:
>> If you want punishment, why not go for extra punishment?
>>
>> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
>
> That could lead to non-reproducable failures though. I think it is better
> to use a fixed value so that you're more likely to be able to reproduce
> the issue every time you run the tests.
>
> Rather than setting MALLOC_PERTURB_=1 unconditionally in the Makefile
> though, it ought to honour any existing MALLOC_PERTURB_ env variable
> the user has set. That could let automated test harness run repeatedly
> with random MALLOC_PERTURB_, while still giving a deterministic value
> for developers by default.

Indeed. I've never thought about it, thanks for sharing the insight.
Lucas Meneghel Rodrigues - May 17, 2013, 12:50 p.m.
On 17/05/13 08:15 AM, Stefan Hajnoczi wrote:
> On Fri, May 17, 2013 at 11:54 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>
>>> glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
>>> variable is set.  The value of the environment variable determines the
>>> bit pattern used to wipe memory.  For more information, see
>>> http://udrepper.livejournal.com/11429.html.
>>>
>>> Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
>>> the environment variable to 1 so the test is deterministic.  Setting a
>>> random variable might expose more bugs but would be harder to reproduce.
>>>
>>> Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
>>> it for in-tree test suites we can detect memory management errors earlier.
>>>
>>>   tests/Makefile           | 4 +++-
>>>   tests/qemu-iotests/check | 2 +-
>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index a307d5a..25f6d28 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>>>   $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
>>>        $(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>>>        $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>>> +             MALLOC_PERTURB_=1 \
>>>                gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
>>>        $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>>>          echo Gcov report for $$f:;\
>>
>> If you want punishment, why not go for extra punishment?
>>
>> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
>
> Covered in the commit description:
> "Note we always set
> the environment variable to 1 so the test is deterministic.  Setting a
> random variable might expose more bugs but would be harder to reproduce."
>
> I didn't want to clutter output with "MALLOC_PERTURB_=123" on every
> run.  AFAIK we have no log where this can be silently stashed.  I
> guess we could write it to a file and add that to .gitignore.

Yes, you could consider to have a debug log or something similar.
Lucas Meneghel Rodrigues - May 17, 2013, 12:52 p.m.
On 17/05/13 07:58 AM, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
>
>> On Fri, May 17, 2013 at 11:54:12AM +0200, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>>
>>>> glibc wipes malloc(3) memory when the MALLOC_PERTURB_ environment
>>>> variable is set.  The value of the environment variable determines the
>>>> bit pattern used to wipe memory.  For more information, see
>>>> http://udrepper.livejournal.com/11429.html.
>>>>
>>>> Set MALLOC_PERTURB_ for gtester and qemu-iotests.  Note we always set
>>>> the environment variable to 1 so the test is deterministic.  Setting a
>>>> random variable might expose more bugs but would be harder to reproduce.
>>>>
>>>> Both make check and qemu-iotests pass with MALLOC_PERTURB_ enabled.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>> Lucas noticed KVM autotest failures when enabling MALLOC_PERTURB_.  By enabling
>>>> it for in-tree test suites we can detect memory management errors earlier.
>>>>
>>>>   tests/Makefile           | 4 +++-
>>>>   tests/qemu-iotests/check | 2 +-
>>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index a307d5a..25f6d28 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -171,6 +171,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>>>>   $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
>>>>   	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>>>>   	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>>>> +		MALLOC_PERTURB_=1 \
>>>>   		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
>>>>   	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>>>>   	  echo Gcov report for $$f:;\
>>>
>>> If you want punishment, why not go for extra punishment?
>>>
>>> MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
>>
>> That could lead to non-reproducable failures though. I think it is better
>> to use a fixed value so that you're more likely to be able to reproduce
>> the issue every time you run the tests.
>
> A fixed value misses failures.
>
> A random value is just as reproducible as a fixed one, simply use the
> same value.  Requires the value to be logged, of course.

That's a pretty great idea, Markus. I'm going to implement it in 
virt-test, thanks!
Richard W.M. Jones - May 19, 2013, 4:51 p.m.
On Fri, May 17, 2013 at 11:07:38AM +0100, Daniel P. Berrange wrote:
> On Fri, May 17, 2013 at 11:54:12AM +0200, Markus Armbruster wrote:
> > If you want punishment, why not go for extra punishment?
> > 
> > MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
> 
> That could lead to non-reproducable failures though. I think it is better
> to use a fixed value so that you're more likely to be able to reproduce
> the issue every time you run the tests.

We've tested libguestfs for years with:

random_val="$(awk 'BEGIN{srand(); print 1+int(255*rand())}' < /dev/null)"
export MALLOC_PERTURB_=$random_val

and have never seen a non-reproducable failure caused by this.

It has, however, caught some simple memory failures, although valgrind
is a lot better if you can afford to run it.

Rich.

Patch

diff --git a/tests/Makefile b/tests/Makefile
index a307d5a..25f6d28 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -171,6 +171,7 @@  GCOV_OPTIONS = -n $(if $(V),-f,)
 $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
+		MALLOC_PERTURB_=1 \
 		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
 	  echo Gcov report for $$f:;\
@@ -180,7 +181,8 @@  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 .PHONY: $(patsubst %, check-%, $(check-unit-y))
 $(patsubst %, check-%, $(check-unit-y)): check-%: %
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
-	$(call quiet-command,gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER $*")
+	$(call quiet-command,MALLOC_PERTURB_=1 \
+		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER $*")
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y); do \
 	  echo Gcov report for $$f:;\
 	  $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 432732c..1527cda 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -214,7 +214,7 @@  do
 	start=`_wallclock`
 	$timestamp && echo -n "	["`date "+%T"`"]"
 	[ ! -x $seq ] && chmod u+x $seq # ensure we can run it
-	./$seq >$tmp.out 2>&1
+	MALLOC_PERTURB_=1 ./$seq >$tmp.out 2>&1
 	sts=$?
 	$timestamp && _timestamp
 	stop=`_wallclock`