threads / patch / 31511

patchAdd MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption

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

## tl;dr

4 messages between Sep 12, 2012 and Sep 13, 2012. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Elia Pinto· Sep 12, 2012, 12:17 UTC · lore

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.

Setting the MALLOC_PERTURB_ environment variable causes the malloc functions in libc to return memory which has been wiped and clear memory when it is returned. Of course this does not affect calloc which always does clear the memory.

The reason for this exercise is, of course, to find code which uses memory returned by malloc without initializing it and code which uses code after it is freed. valgrind can do this but it's costly to run. The MALLOC_PERTURB_ exchanges the ability to detect problems in 100% of the cases with speed.

The byte value used to initialize values returned by malloc is the byte value of the environment value. The value used to clear memory is the bitwise inverse. Setting MALLOC_PERTURB_ to zero disables the feature.

This technique can find hard to detect bugs. It is therefore suggested to always use this flag (at least temporarily) when testing out code or a new distribution.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/test-lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)

Show changes to t/test-lib.sh +6 −0

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 78c4286..98c90b0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -93,6 +93,12 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
 
+# Add libc malloc_check and MALLOC_PERTURB test 
+export MALLOC_CHECK_=3
+export MALLOC_PERTURB_="$( expr \( $$ % 255 \) + 1)"
+#
+
+
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
 unset CDPATH

-- 
1.7.11.rc1

Junio C Hamano· Sep 12, 2012, 17:51 UTC · re: Elia Pinto · lore

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

Elia Pinto <gitter.spiros@gmail.com> writes:

Show 32 quoted lines

> 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.
>
> Setting the MALLOC_PERTURB_ environment variable causes the malloc
> functions in libc to return memory which has been wiped and clear
> memory when it is returned.
> Of course this does not affect calloc which always does clear the memory.
>
> The reason for this exercise is, of course, to find code which uses
> memory returned by malloc without initializing it and code which uses
> code after it is freed. valgrind can do this but it's costly to run.
> The MALLOC_PERTURB_ exchanges the ability to detect problems in 100%
> of the cases with speed.
>
> The byte value used to initialize values returned by malloc is the byte
> value of the environment value. The value used to clear memory is the
> bitwise inverse. Setting MALLOC_PERTURB_ to zero disables the feature.
>
> This technique can find hard to detect bugs.
> It is therefore suggested to always use this flag (at least temporarily)
> when testing out code or a new distribution.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  t/test-lib.sh | 6 ++++++
>  1 file changed, 6 insertions(+)

Interesting, but it bothers me to make it enabled unconditionally. At least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?

By the way, "export VAR=VAL" all on the same line, even though it is in POSIX.1, is reported to be unsupported by some shells people care about, and needs to be corrected to "VAR=VAL" and "export VAR" as separate commands. I think we saw a patch to fix an instance or two that snuck in recently.

Show 18 quoted lines

>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 78c4286..98c90b0 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -93,6 +93,12 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
>  export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
>  export EDITOR
>  
> +# Add libc malloc_check and MALLOC_PERTURB test 
> +export MALLOC_CHECK_=3
> +export MALLOC_PERTURB_="$( expr \( $$ % 255 \) + 1)"
> +#
> +
> +
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
>  unset CDPATH

Elia Pinto· Sep 13, 2012, 16:36 UTC · re: Junio C Hamano · lore

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

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

>
> Interesting, but it bothers me to make it enabled unconditionally.
> At least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?

Sorry for the late response and thanks.

No, setting MALLOC_CHECK don't require valgrind and it considered a best QA to have the test suite with it defined always. If the test suite fail with MALLOC_CHECK, well, there is some problem, no ? Some distro do it already in building packages (fedora for example) http://emacs.1067599.n5.nabble.com/please-set-both-MALLOC-PERTURB-and-MALLOC-CHECK-envvars-td150144.html

At @rpm5.org we do the same for popt, for example, from years

http://rpm5.org/community/rpm-devel/4156.html

Show 6 quoted lines

>
> By the way, "export VAR=VAL" all on the same line, even though it is
> in POSIX.1, is reported to be unsupported by some shells people care
> about, and needs to be corrected to "VAR=VAL" and "export VAR" as
> separate commands.  I think we saw a patch to fix an instance or two
> that snuck in recently.

Yes, right, my bad. I will reroll.

Thank you

Junio C Hamano· Sep 13, 2012, 17:46 UTC · re: Elia Pinto · lore

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

Elia Pinto <gitter.spiros@gmail.com> writes:

Show 8 quoted lines

> 2012/9/12 Junio C Hamano <gitster@pobox.com>:
>>
>> Interesting, but it bothers me to make it enabled unconditionally.
>> At least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?
> Sorry for the late response and thanks.
>
> No, setting MALLOC_CHECK don't require
> valgrind ...

You never said anything like that, and I didn't question it.

> and it considered a best QA to have the test suite with it
> defined always. If the test suite fail with MALLOC_CHECK, well, there
> is some problem, no ?

I never said using MALLOC_CHECK_ is a bad idea.

Let me ask the same question in a different way, as I seem to have failed in the previous message.

If you are using valgrind to run tests, is it sane to also enable MALLOC_CHECK? If you were testing "cat", would it make sense to do:

	$ MALLOC_CHECK_=3 valgrind cat README

Because we are not interested in testing how valgrind (not cat) uses malloc, we may be better off running

	$ valgrind cat README

without MALLOC_CHECK_; it will reduce the risk of MALLOC_CHECK_ potentially disturbing what we really want to check (i.e. cat) by triggering for something whose problems we are not trying to see (i.e. valgrind), no?

That was my question.