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

[Junio C Hamano <gitster@pobox.com>]

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.

 - Update the "expr" expression to make sure the --valgrind we catch
   is indeed an option, not a substring of something else.  Also
   there is no need to capture the substring.

 t/perf/perf-lib.sh |  1 +
 t/test-lib.sh      | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git i/t/perf/perf-lib.sh w/t/perf/perf-lib.sh
index a1361e5..1d0bb9d 100644
--- i/t/perf/perf-lib.sh
+++ w/t/perf/perf-lib.sh
@@ -42,6 +42,7 @@ else
 fi
 
 TEST_NO_CREATE_REPO=t
+TEST_NO_MALLOC_=t
 
 . ../test-lib.sh
 
diff --git i/t/test-lib.sh w/t/test-lib.sh
index b0c0c84..aad4606 100644
--- i/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -95,12 +95,24 @@ 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 || {
-	MALLOC_CHECK_=3
-	export MALLOC_CHECK_
-	MALLOC_PERTURB_="$( expr \( $$ % 255 \) + 1)"
-	export MALLOC_PERTURB_
-}
+if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
+   test -n "TEST_NO_MALLOC_"
+then
+	setup_malloc_check () {
+		: nothing
+	}
+	teardown_malloc_check () {
+		: nothing
+	}
+else
+	setup_malloc_check () {
+		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
+		export MALLOC_CHECK_ MALLOC_PERTURB_
+	}
+	teardown_malloc_check () {
+		unset MALLOC_CHECK_ MALLOC_PERTURB_
+	}
+fi
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
@@ -311,7 +323,9 @@ test_run_ () {
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
+		setup_malloc_check
 		test_eval_ "$test_cleanup"
+		teardown_malloc_check
 	fi
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
 	then