[RFC PATCH 0/5] valgrind in test scripts

[RFC PATCH 0/5] valgrind in test scripts

[Jeff King]

I spent some time last week running git through the paces with
valgrind's memcheck tool. The good news is that I didn't find any
serious issues, so we are doing a pretty good job overall. The bad news
is that running the whole test suite with valgrind takes a few hours on
a quad-core (but thank goodness for "make -j4 test").

I did uncover a few potential problems, and patches are in the latter
part of the series. I suppose an argument could be made that these fixes
are code churn, since these are not problems in practice, but I think
they are worth fixing. The fixes are few in number and small, and we are
very close to a valgrind-error-free code-base, which would make it easy
to spot any new problems when they arise. We could always suppress these
errors, but there is the possibility of an overzealous suppression
masking a real problem (especially if one of these issues changes from
theoretical to practical).

There are a few things I don't like:

 1. The "fake git PATH" is set up by the Makefile, since it needs to
    know which dashed commands to override (based on $(PROGRAMS)).

    I think it would be nicer if the test script itself set this up when
    --memcheck was requested, so that we always know it is fresh. But:

      - if the fake PATH isn't inside the trash directory, then we have
        a problem with multiple tests trying to set it up at the same
        time

      - if the fake PATH is inside the trash directory, I'm not sure of
        the best place to put it, as I don't want to influence the
        outcome of the tests. It could go into .git/valgrind, without
        hurting anything.

 2. I wanted to have a completely clean valgrind run before posting.
    With these patches, the only errors I get are for an uninitialized
    "struct stat" in t4121 and t4127. After much looking (including
    carefully reading the code, and using a a debugger, which shows the
    data in question looking very much initialized) I can't figure out
    what the problem might be. So it might simply be a false positive in
    valgrind, but I would like another set of eyes.

-Peff

[RFC PATCH 1/5] add valgrind support in test scripts

[Jeff King]

This patch adds the ability to use valgrind's memcheck tool
to diagnose memory problems in git while running the test
scripts. It works by placing a "fake" git in the front of
the test script's PATH; this fake git runs the real git
under valgrind. It also points the exec-path such that any
stand-alone dashed git programs are run using the same
script. In this way we avoid having to modify the actual git
code in any way.

The memcheck tool can be used by specifying
"GIT_TEST_OPTS=--memcheck" in the make invocation. Any
invocation of git that finds any errors under valgrind will
exit with failure code 126. Any valgrind output will go to
the usual stderr channel for tests (i.e., /dev/null, unless
-v has been specified).

A few default suppressions are included, since libz seems to
trigger quite a few false positives. We'll assume that libz
works and that we can ignore any errors which are reported
there.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore              |    1 +
 Makefile                |   12 ++++++++++--
 t/test-lib.sh           |   11 +++++++++++
 t/valgrind/.gitignore   |    1 +
 t/valgrind/default.supp |   21 +++++++++++++++++++++
 test-valgrind.sh        |   24 ++++++++++++++++++++++++
 6 files changed, 68 insertions(+), 2 deletions(-)
 create mode 100644 t/valgrind/.gitignore
 create mode 100644 t/valgrind/default.supp
 create mode 100755 test-valgrind.sh

diff --git a/.gitignore b/.gitignore
index bbaf9de..45045cb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -151,6 +151,7 @@ test-match-trees
 test-parse-options
 test-path-utils
 test-sha1
+test-valgrind
 common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index d6f3695..68f0172 100644
--- a/Makefile
+++ b/Makefile
@@ -279,6 +279,8 @@ SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	  git-instaweb
 
+VALGRIND_SH += test-valgrind.sh
+
 # Empty...
 EXTRA_PROGRAMS =
 
@@ -1139,7 +1141,7 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
+$(patsubst %.sh,%,$(SCRIPT_SH) $(VALGRIND_SH)) : % : %.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
@@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)
 
 export NO_SVN_TESTS
 
-test: all
+valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))
+	rm -rf t/valgrind/bin
+	mkdir t/valgrind/bin
+	for i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done
+
+test: all valgrind-setup
 	$(MAKE) -C t/ all
 
 test-date$X: date.o ctype.o
@@ -1501,6 +1508,7 @@ endif
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
 .PHONY: .FORCE-GIT-BUILD-OPTIONS
+.PHONY: valgrind-setup
 
 ### Check documentation
 #
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8936173..e753654 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -94,6 +94,8 @@ do
 	--no-python)
 		# noop now...
 		shift ;;
+	-m|--m|--me|--mem|--memc|--memch|--memche|--memchec|--memcheck)
+		memcheck=t shift ;;
 	*)
 		break ;;
 	esac
@@ -468,6 +470,15 @@ test_done () {
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 TEST_DIRECTORY=$(pwd)
 PATH=$TEST_DIRECTORY/..:$PATH
+if test -n "$memcheck"; then
+	PATH=$TEST_DIRECTORY/valgrind/bin:$PATH
+	GIT_VALGRIND=$TEST_DIRECTORY/valgrind; export GIT_VALGRIND
+	if ! test -f "$GIT_VALGRIND/bin/git"; then
+		echo >&2 'You need to setup the valgrind bin:'
+		echo >&2 'Run "make valgrind-setup" in the (toplevel) directory'
+		exit 1
+	fi
+fi
 GIT_EXEC_PATH=$(pwd)/..
 GIT_TEMPLATE_DIR=$(pwd)/../templates/blt
 unset GIT_CONFIG
diff --git a/t/valgrind/.gitignore b/t/valgrind/.gitignore
new file mode 100644
index 0000000..ba077a4
--- /dev/null
+++ b/t/valgrind/.gitignore
@@ -0,0 +1 @@
+bin
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
new file mode 100644
index 0000000..2482b3b
--- /dev/null
+++ b/t/valgrind/default.supp
@@ -0,0 +1,21 @@
+{
+	ignore-zlib-errors-cond
+	Memcheck:Cond
+	obj:*libz.so*
+}
+
+{
+	ignore-zlib-errors-value4
+	Memcheck:Value4
+	obj:*libz.so*
+}
+
+{
+	writing-data-from-zlib-triggers-errors
+	Memcheck:Param
+	write(buf)
+	obj:/lib/ld-*.so
+	fun:write_in_full
+	fun:write_buffer
+	fun:write_loose_object
+}
diff --git a/test-valgrind.sh b/test-valgrind.sh
new file mode 100755
index 0000000..61c1428
--- /dev/null
+++ b/test-valgrind.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+base=`echo $0 | sed 's,.*/,,'`
+case "$base" in
+git-*)
+	set -- "$GIT_EXEC_PATH/$base" "$@"
+	;;
+git)
+	set -- "$GIT_EXEC_PATH/git" --exec-path="$GIT_VALGRIND/bin" "$@"
+	;;
+*)
+	echo >&2 "test-valgrind invoked on non-git command: $0"
+	exit 1
+esac
+
+exec valgrind -q --error-exitcode=126 \
+	--leak-check=no \
+	--suppressions="$GIT_VALGRIND/default.supp" \
+	--gen-suppressions=all \
+	--log-fd=4 \
+	--input-fd=4 \
+	${GIT_VALGRIND_DEBUG:+--db-attach=yes} \
+	${GIT_VALGRIND_DEBUGCMD:+--db-command="$GIT_VALGRIND_DEBUGCMD"} \
+	"$@"
-- 
1.6.0.2.825.g6d19d

[RFC PATCH 2/5] valgrind: ignore ldso errors

[Jeff King]

On some Linux systems, we get a host of Cond and Addr errors
from calls to dlopen that are caused by nss modules. We
should be able to safely ignore anything happening in
ld-*.so as "not our problem."

Signed-off-by: Jeff King <peff@peff.net>
---
This was from a Debian etch system, while my lenny systems seemed to
have no trouble. I'm sure we will pick up a few commits like these as
people run it on various platforms, but hopefully they should shake out
pretty quickly.

 t/valgrind/default.supp |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 2482b3b..1013847 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -11,6 +11,18 @@
 }
 
 {
+	ignore-ldso-cond
+	Memcheck:Cond
+	obj:*ld-*.so
+}
+
+{
+	ignore-ldso-addr8
+	Memcheck:Addr8
+	obj:*ld-*.so
+}
+
+{
 	writing-data-from-zlib-triggers-errors
 	Memcheck:Param
 	write(buf)
-- 
1.6.0.2.825.g6d19d

[RFC PATCH 3/5] correct cache_entry allocation

[Jeff King]

Most cache_entry structs are allocated by using the
cache_entry_size macro, which rounds the size of the struct
up to the nearest multiple of 8 bytes (presumably to avoid
memory fragmentation).

There is one exception: the special "conflict entry" is
allocated with an empty name, and so is explicitly given
just one extra byte to hold the NUL.

However, later code doesn't realize that this particular
struct has been allocated differently, and happily tries
reading and copying it based on the ce_size macro, which
assumes the 8-byte alignment.

This can lead to reading uninitalized data, though since
that data is simply padding, there shouldn't be any problem
as a result. Still, it makes sense to hold the padding
assumption so as not to surprise later maintainers.

This fixes valgrind errors in t1005, t3030, t4002, and
t4114.

Signed-off-by: Jeff King <peff@peff.net>
---
 unpack-trees.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index e59d144..e5749ef 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -382,7 +382,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->merge_size = len;
 
 	if (!dfc)
-		dfc = xcalloc(1, sizeof(struct cache_entry) + 1);
+		dfc = xcalloc(1, cache_entry_size(0));
 	o->df_conflict_entry = dfc;
 
 	if (len) {
-- 
1.6.0.2.825.g6d19d

[RFC PATCH 4/5] pack-objects: avoid reading uninitalized data

[Jeff King]

In the main loop of find_deltas, we do:

  struct object_entry *entry = *list++;
  ...
  if (!*list_size)
	  ...
	  break

Because we look at and increment *list _before_ the check of
list_size, in the very last iteration of the loop we will
look at uninitialized data, and increment the pointer beyond
one past the end of the allocated space. Since we don't
actually do anything with the data until after the check,
this is not a problem in practice.

But since it technically violates the C standard, and
because it provokes a spurious valgrind warning, let's just
move the initialization of entry to a safe place.

This fixes valgrind errors in t5300, t5301, t5302, t303, and
t9400.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-pack-objects.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 59c30d1..15b80db 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1375,7 +1375,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 	array = xcalloc(window, sizeof(struct unpacked));
 
 	for (;;) {
-		struct object_entry *entry = *list++;
+		struct object_entry *entry;
 		struct unpacked *n = array + idx;
 		int j, max_depth, best_base = -1;
 
@@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 			progress_unlock();
 			break;
 		}
+		entry = *list++;
 		(*list_size)--;
 		if (!entry->preferred_base) {
 			(*processed)++;
-- 
1.6.0.2.825.g6d19d

[RFC PATCH 5/5] fix overlapping memcpy in normalize_absolute_path

[Jeff King]

The comments for normalize_absolute_path explicitly claim
that the source and destination buffers may be the same
(though they may not otherwise overlap). Thus the call to
memcpy may involve copying overlapping data, and memmove
should be used instead.

This fixes a valgrind error in t1504.

Signed-off-by: Jeff King <peff@peff.net>
---
An alternative fix, since we declare that only true equality is OK,
would be to keep the memcpy and explicitly check for "comp_start ==
dst". That might have a performance benefit if memcpy is faster than
memmove, but I don't know that normalize_absolute_path is in any
critical paths.

 path.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/path.c b/path.c
index 76e8872..c1cb54b 100644
--- a/path.c
+++ b/path.c
@@ -348,7 +348,7 @@ int normalize_absolute_path(char *buf, const char *path)
 			goto next;
 		}
 
-		memcpy(dst, comp_start, comp_len);
+		memmove(dst, comp_start, comp_len);
 		dst += comp_len;
 	next:
 		comp_start = comp_end;
-- 
1.6.0.2.825.g6d19d

Re: [RFC PATCH 1/5] add valgrind support in test scripts

[Johannes Schindelin]

Hi,

On Wed, 22 Oct 2008, Jeff King wrote:

> diff --git a/Makefile b/Makefile
> index d6f3695..68f0172 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)
>  
>  export NO_SVN_TESTS
>  
> -test: all
> +valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))
> +	rm -rf t/valgrind/bin
> +	mkdir t/valgrind/bin
> +	for i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done

I wonder if it would not be better to scrap the t/valgrind/ directory and 
regenerate it everytime you run a test manually; I'd use "ln" instead of 
"cp", and also parse command-list.txt to catch really all of them (even if 
a dashed form is used for a builtin by mistake).

Otherwise: good work, I like it!

Ciao,
Dscho

Re: [RFC PATCH 1/5] add valgrind support in test scripts

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Wed, 22 Oct 2008, Jeff King wrote:
>
>> diff --git a/Makefile b/Makefile
>> index d6f3695..68f0172 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)
>>  
>>  export NO_SVN_TESTS
>>  
>> -test: all
>> +valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))
>> +	rm -rf t/valgrind/bin
>> +	mkdir t/valgrind/bin
>> +	for i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done
>
> I wonder if it would not be better to scrap the t/valgrind/ directory and 
> regenerate it everytime you run a test manually; I'd use "ln" instead of 
> "cp", and also parse command-list.txt to catch really all of them (even if 
> a dashed form is used for a builtin by mistake).

Going one step further, I wonder if this approach can also be used to
catch such a mistake.  Install a dashed form that records the fact that it
was called when it shouldn't, and by whom.

Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data

[Nicolas Pitre]

On Wed, 22 Oct 2008, Jeff King wrote:

> In the main loop of find_deltas, we do:
> 
>   struct object_entry *entry = *list++;
>   ...
>   if (!*list_size)
> 	  ...
> 	  break
> 
> Because we look at and increment *list _before_ the check of
> list_size, in the very last iteration of the loop we will
> look at uninitialized data, and increment the pointer beyond
> one past the end of the allocated space. Since we don't
> actually do anything with the data until after the check,
> this is not a problem in practice.
> 
> But since it technically violates the C standard, and
> because it provokes a spurious valgrind warning, let's just
> move the initialization of entry to a safe place.
> 
> This fixes valgrind errors in t5300, t5301, t5302, t303, and
> t9400.
> 
> Signed-off-by: Jeff King <peff@peff.net>

OK.  Minor nit...

> ---
>  builtin-pack-objects.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 59c30d1..15b80db 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1375,7 +1375,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
>  	array = xcalloc(window, sizeof(struct unpacked));
>  
>  	for (;;) {
> -		struct object_entry *entry = *list++;
> +		struct object_entry *entry;
>  		struct unpacked *n = array + idx;
>  		int j, max_depth, best_base = -1;
>  
> @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
>  			progress_unlock();
>  			break;
>  		}

---> Please preserve the empty line here so the previous code
     chunk still appears logically separate.

> +		entry = *list++;
>  		(*list_size)--;
>  		if (!entry->preferred_base) {
>  			(*processed)++;


Nicolas

Re: [RFC PATCH 1/5] add valgrind support in test scripts

[Jeff King]

On Thu, Oct 23, 2008 at 12:13:47AM +0200, Johannes Schindelin wrote:

> I wonder if it would not be better to scrap the t/valgrind/ directory and 
> regenerate it everytime you run a test manually;

Yeah, I mentioned that in my 0/5 cover letter. The problem is where to
put it that won't impact test results, but also allow running multiple
tests simultaneously. I'm going to try sticking it in .git/valgrind in
the trash directory, which presumably won't affect any tests.

> I'd use "ln" instead of "cp"

I specifically stayed away from 'ln' for Windows portability. It looks
like for builtins, we do "ln || ln -s || cp". We can probably do the
same here.

I also failed to use git$X in the fake path, which would probably be
necessary for Windows.

> and also parse command-list.txt to catch really all of them (even if 
> a dashed form is used for a builtin by mistake).

That is a little bit trickier. I don't actually want to intercept
git-am, for example, since I have no interest in running valgrind on the
shell. But it is do-able; I will give details in my response to Junio's
suggestion.

> Otherwise: good work, I like it!

Thanks.

-Peff

Re: [RFC PATCH 1/5] add valgrind support in test scripts

[Jeff King]

On Wed, Oct 22, 2008 at 05:14:52PM -0700, Junio C Hamano wrote:

> > I wonder if it would not be better to scrap the t/valgrind/ directory and 
> > regenerate it everytime you run a test manually; I'd use "ln" instead of 
> > "cp", and also parse command-list.txt to catch really all of them (even if 
> > a dashed form is used for a builtin by mistake).
> 
> Going one step further, I wonder if this approach can also be used to
> catch such a mistake.  Install a dashed form that records the fact that it
> was called when it shouldn't, and by whom.

I think that makes sense, though it is somewhat orthogonal to valgrind.
Do we want to always set up such a fake path? It could actually be even
simpler than a wrapper; just stop adding the build directory to the
PATH, and instead have a pseudo-installation directory with the bin and
exec-path directories set up appropriately. This would more closely
model the actual installation.

I think there are actually several classes of dashed commands we need to
differentiate:

 1. dashed commands which get installed in bin; these should be allowed

 2. dashed commands which don't get installed in bin; these should be
    flagged as an error

 3. dashed commands in exec-path which are stand-alone C programs. These
    should be run under valgrind.

 4. dashed commands in exec-path which are scripts. These do get run,
    but should not be run under valgrind.

 5. dashed commands in exec-path which are builtins. These should never
    get run by our test scripts, since they are "legacy" links for people
    who want to put the exec-path in their PATH

Right now we always point PATH at the build directory, which has
everything. So we can't easily differentiate between '1' and '2'. I used
the $(PROGRAMS) variable in the Makefile to find '3' (as opposed to '4'
and '5').

-Peff

Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data

[Jeff King]

On Wed, Oct 22, 2008 at 09:11:16PM -0400, Nicolas Pitre wrote:

> >  	for (;;) {
> > -		struct object_entry *entry = *list++;
> > +		struct object_entry *entry;
> >  		struct unpacked *n = array + idx;
> >  		int j, max_depth, best_base = -1;
> >  
> > @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
> >  			progress_unlock();
> >  			break;
> >  		}
> 
> ---> Please preserve the empty line here so the previous code
>      chunk still appears logically separate.
> 
> > +		entry = *list++;
> >  		(*list_size)--;
> >  		if (!entry->preferred_base) {
> >  			(*processed)++;

Er, there was no empty line there (or else there would have been a '-'
line in the diff). I am happy to add it, like:

  if (!*list_size) {
    progress_unlock();
    break;
  }

  entry = *list++;
  (*list_size)--;
  if (!entry->preferred_base)
    ...

if you like, but the current version seems to format it as one stanza
inside of the progress_lock/progress_unlock. Look at the version in
'master' to see what I mean.

-Peff

Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data

[Nicolas Pitre]

On Thu, 23 Oct 2008, Jeff King wrote:

> On Wed, Oct 22, 2008 at 09:11:16PM -0400, Nicolas Pitre wrote:
> 
> > >  	for (;;) {
> > > -		struct object_entry *entry = *list++;
> > > +		struct object_entry *entry;
> > >  		struct unpacked *n = array + idx;
> > >  		int j, max_depth, best_base = -1;
> > >  
> > > @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
> > >  			progress_unlock();
> > >  			break;
> > >  		}
> > 
> > ---> Please preserve the empty line here so the previous code
> >      chunk still appears logically separate.
> > 
> > > +		entry = *list++;
> > >  		(*list_size)--;
> > >  		if (!entry->preferred_base) {
> > >  			(*processed)++;
> 
> Er, there was no empty line there (or else there would have been a '-'
> line in the diff).

Oh, right.  I'm confused.
Nevermind...


Nicolas