From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 14 Sep 2012 17:51:33 GMT
Subject: Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption
Message-ID: <7v392k5w7u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1347641662-3596-1-git-send-email-gitter.spiros@gmail.com>

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.

>  t/test-lib.sh |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 78c4286..f34b861 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -93,6 +93,15 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
>  export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
>  export EDITOR
>  
> +# Add libc MALLOC and MALLOC_PERTURB test 
> +# only if we are not executing the test with valgrind
> +expr "$GIT_TEST_OPTS" : ".*\(--valgrind\)" >/dev/null || {

I would write this like

	if ! expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null
	then
		...
	fi

> +	MALLOC_CHECK_=3
> +	export MALLOC_CHECK_
> +	MALLOC_PERTURB_="$( expr \( $$ % 255 \) + 1)"

How was this expression chosen?  The only effect I can think of to
use a randomly picked value here is to make it impossible to make
the test repeatable and reproducible, so you must have had some
benefit that outweighs the downside, but I cannot think of any.
Wouldn't MALLOC_PERTURB_=165 (i.e. 0xA5, half of the bits in the
byte is set, including the MSB, and is an odd number) be equally a
good choice without repeatability downside, for example?

Also, doesn't the above give 256 sometimes, which would not fit in a
byte?

> +	export MALLOC_PERTURB_
> +}
> +
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
>  unset CDPATH