Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:45
Elia Pinto [off-list ref] 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 <redacted> --- This the third reroll of the original patch.
Well written. I have one suggestion and a question, though.
quoted hunk
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