Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption

From: Elia Pinto <gitter.spiros@gmail.com>
Date: Sep 17, 2012, 12:17 UTC
Message-ID: <CA+EOSBk0hN+v+PK8jzCCZgG0Ko7SsHOSB=cM7nnaSsrUOaJ-7Q@mail.gmail.com>
In-Reply-To: <7vligc19d1.fsf@alter.siamese.dyndns.org>

2012/9/15 Junio C Hamano <gitster@pobox.com>:

Show 34 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elia Pinto <gitter.spiros@gmail.com> writes:
>>
>>> Recent versions of Linux libc (later than 5.4.23) and glibc (2.x)
>>> include a malloc() implementation which is tunable via environment
>>> variables. When MALLOC_CHECK_ is set, a special (less efficient)
>>> implementation is used which is designed to be tolerant against
>>> simple errors, such as double calls of free() with the same argument,
>>> or overruns of a single byte (off-by-one bugs). When MALLOC_CHECK_
>>> is set to 3, a diagnostic message is printed on stderr
>>> and the program is aborted.
>>> ...
>>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>>> ---
>>> This the third reroll of the original patch.
>>
>> Well written.  I have one suggestion and a question, though.
>
> After looking at it a bit more, there are a few more things I would
> suggest, in the form of an squashable patch on top.
>
> Points to note:
>
>  - When test-lib.sh is used from perf-lib, we would not want to be
>    affected with MALLOC_CHECK_; we would want to run as vanilla as
>    possible in that case.
>
>  - We are interested in checking "git" and whatever we test using
>    the test harness, i.e. what comes inside test_expect_success.
>    Setting MALLOC_CHECK_ at the beginning will cover a lot more than
>    what we want to test.
>
>  - That "165" thing I mentioned earlier.

Thank you so much for the comments, that's fine. A single consideration for MALLOC_PERTURB.

You can use any value between 1..255 for MALLOC_PERTURB_ That chooses the byte that glibc will use to memset all freed buffers. In general it is defined as

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

(as drepper pointed out http://udrepper.livejournal.com/11429.html)

I don't use RANDOM because it is not portable but $$ is sufficient for the bias. I use this in popt (http://rpm5.org/community/rpm-devel/5311.html but the site is down now)

Using a random value is slightly better than using a fixed one in case your fixed value is someday just the right/wrong value to mask a problem. At least with a random value, if you rerun the test in a different shell, the odds are good you won't use the unfortunate setting again. % is the module operation so the above expression returns a value between 1 and 255 always (for the euclidean division property :=).

So OK per the original expression ? I prefer a perfect commit for git, but no problem to follow your advice and reroll the patch.

Opinions ?

Best Regards