patchmemory leak reported by valgrind

Hello,

This patches the following memory leak reported by valgrind:

512 bytes in 1 blocks are definitely lost in loss record 5 of 8
  at 0x4C203E4: calloc (vg_replace_malloc.c:397)
  by 0x4C5F9D: xcalloc (wrapper.c:96)
  by 0x445741: cmd_pack_objects (pack-objects.c:2117)
  by 0x4048EE: handle_internal_command (git.c:270)
  by 0x404B03: main (git.c:470)

diff -u git-1.7.2.1/builtin/pack-objects.c git.ivan/builtin/pack-objects.c --- git-1.7.2.1/builtin/pack-objects.c 2010-07-28 19:03:43.000000000 +0200 +++ git.ivan/builtin/pack-objects.c 2010-08-09 11:38:40.000000000 +0200

Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2) to clean after us?

On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

I don't know if you consider this relevant, but in Windows 9x (and ME) allocated memory isn't returned to the global heap on process termination unless explicitly free'd.

On Mon, Aug 9, 2010 at 22:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:

...and nothing but DOS, Windows 9x and ME does that.

Junio C Hamano wrote:

Right. This is the case for many leaks reported by valgrind for git; it is hard to separate the ones the matter.

Example: the lockfile records.  There are so few per process as not to
matter except for the sake of valgrind, and they cannot be removed
before exiting because the list they are inserted in is singly-linked.
I am cooking a patch to free them at exit optionally, somewhat like
Pierre’s leaky()[1].  Could something like leaky() be used to insert
an artificial stack frame at allocation time to make suppressions
simpler to write?

Well, that’s a separate story.  Here are a few run-of-the-mill leaks.

Jonathan Nieder (3):
  core: Stop leaking ondisk_cache_entrys
  write-tree: Avoid leak when index refers to an invalid object
  read-tree: stop leaking tree objects

 cache-tree.c   |    4 +++-
 read-cache.c   |    5 ++++-
 unpack-trees.c |    7 ++++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

[1] http://thread.gmane.org/gmane.comp.version-control.git/86138

Noticed with valgrind.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 read-cache.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Noticed by valgrind during test t0000.35 “writing this tree without --missing-ok”.

Even in the cherry-pick foo..bar code path, such an error is the end of the line. But maybe some day an interactive porcelain will want to link to libgit, making this matter.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 cache-tree.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

The underlying problem is that the fill_tree_descriptor() API is easy to misuse, and this patch does not fix that.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 unpack-trees.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Jonathan Nieder wrote:

Yes, I think it could.

Thanks to Pierre Habouzit for the initial idea and David Barr for advice on the execution.

Thoughts?  Bugs?  Improvements?

Jonathan Nieder (7):
  init: plug tiny memory leak
  lockfile: introduce alloc_lock_file() to avoid valgrind noise
  environment.c: remove unused variable
  setup: make sure git dir path is in a permanent buffer
  Introduce malloc/strdup/pathdup variants for permanent allocations
  environment: use alloc_permanent() for computed git_dir and co
  commit-tree: free commit message before exiting

 Documentation/technical/api-lockfile.txt |    9 ++++++++
 builtin/commit-tree.c                    |   12 ++++++----
 builtin/init-db.c                        |   32 +++++++++++++++++------------
 builtin/update-index.c                   |    4 +--
 cache-tree.c                             |    7 +-----
 cache.h                                  |    3 ++
 config.c                                 |    2 +-
 environment.c                            |   14 ++++++------
 git-compat-util.h                        |    2 +
 lockfile.c                               |    6 +++++
 path.c                                   |   10 +++++++++
 t/valgrind/default.supp                  |   25 +++++++++++++++++++++++
 wrapper.c                                |   14 +++++++++++++
 13 files changed, 105 insertions(+), 35 deletions(-)

The buffer used to construct paths like ".git/objects/info" and ".git/objects/pack" is allocated on the heap and never freed.

So free it. While at it, factor out the relevant code into its own function to make it more obvious that this data is not used elsewhere.

Noticed by valgrind while setting up tests (in test-lib).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/init-db.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

While at it it might be a good idea to rename its outdated name to something saner, e.g. object_dir, perhaps?

In v0.99 days, it make some sense to call that sha1_dir because we prepopulated 256 subdirectories 00-ff in it, and as an afterthought we reused the buffer to create an extra subdirectory "/pack" there. But we do not create the fan-out in advance since 9106c09 (Create object subdirectories on demand (phase II), 2005-10-09); sha1_dir is not a very good name for the buffer to hold the path to ".git/objects" anymore.

The buffer used to construct paths like ".git/objects/info" and ".git/objects/pack" is allocated on the heap and never freed.

So free it. While at it, factor out the relevant code into its own function and rename the sha1_dir variable to object_directory (to match the change in everyday usage after the renaming of SHA1_FILE_DIRECTORY in v0.99~603^2~7, 2005).

Noticed by valgrind while setting up tests (in test-lib).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

Yes.

 builtin/init-db.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

The current lockfile API often requires leaking memory:

 - the list of lockfiles to remove at exit is maintained in a
   singly linked list, so one cannot easily remove a lockfile
   record when it is no longer needed

 - some lockfile records are statically allocated and others are on
   the heap, so the remove_lock_file() atexit handler cannot easily
   free them at exit (not to mention that it would be a waste of time
   since _exit() takes care of freeing them anyway)

As the git libification effort proceeds, maybe this will become worth fixing. For now, since there are so few lock files used by a given process, it is simpler to tolerate the leak.

Add a alloc_lock_file() helper to make such tolerance easier. Allocations made through this helper function will not be reported as leaks by valgrind even if there is no corresponding free(). The ((optimize("-fno-optimize-sibling-calls"))) attribute is needed on platforms with GCC to ensure that the stack frame for alloc_lock_file() is not replaced by the stack frame for xcalloc().

For illustration, this patch updates a few example xcalloc(1, sizeof(struct lockfile)) call sites to use the new wrapper.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-lockfile.txt |    9 +++++++++
 builtin/update-index.c                   |    4 +---
 cache-tree.c                             |    7 +------
 cache.h                                  |    1 +
 config.c                                 |    2 +-
 lockfile.c                               |    6 ++++++
 t/valgrind/default.supp                  |    8 ++++++++
 7 files changed, 27 insertions(+), 10 deletions(-)

On 10/02/2010 10:32 AM, Jonathan Nieder wrote:

So we're basically increasing runtime to shut up a leakchecking tool and also making that leakchecking tool falsely not report positives. Hmm...

Andreas Ericsson wrote:

Yep, nice summary.  Probably I should have also mentioned:

 - An extra stack frame with no locals is not a lot of overhead, but
   in any case these are by design not in performance-critical places.

 - By using a special function like this, we make instances nicely
   grep-able and give the leak prominence in t/valgrind/default.supp.
   So a person can discover, for example, that writing a lot of trees
   in a single process (like cherry-pick -n foo..bar currently does)
   is going to be leaky.

 - valgrind would be most useful if it can be used to identify
   _regressions_ by running the test suite with --valgrind.  git is
   deliberately leaky in a lot of places; it is not useful to record
   that.

 - valgrind does have the ability to turn suppressions off if you want.

Jonathan Nieder <jrnieder@gmail.com> writes:

Hmm, I am getting

    cc1: warnings being treated as errors
    lockfile.c:189: error: 'optimize' attribute directive ignored
    make: *** [lockfile.o] Error 1

from this patch with gcc (Debian 4.3.2-1.1) 4.3.2

Aren't "struct lock_file" instances supposed to be reachable from the linked list, i.e. lock_file_list? Why does valgrind consider that elements on that list are leaked in the first place?

We keep loaded objects in *obj_hash[] and they sure are "leaked" by the same definition of leakage, no? How are we squelching valgrind on them?

Junio C Hamano wrote:

Unfortunate.  With gcc 4.5 it works, but that isn't too useful.

At exit, we walk the lock file list and clear it in the process. Which suggests a cleaner workaround (thanks!):

 static void remove_lock_file(void)
 {
	pid_t me = getpid();
+	struct lock_file *p = lock_file_list;

-	while (lock_file_list) {
-		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0]) {
-			if (lock_file_list->fd >= 0)
-				close(lock_file_list->fd);
-			unlink_or_warn(lock_file_list->filename);
-		}
-		lock_file_list = lock_file_list->next;
+	while (p) {
+		if (p->owner == me &&
+		    p->filename[0]) {
+			if (p->fd >= 0)
+				close(p->fd);
+			unlink_or_warn(p->filename);
+		}
+		p = lock_file_list->next;
	}

Jonathan Nieder <jrnieder@gmail.com> writes:

Heh, shouldn't the last one assign from p->next?

Junio C Hamano wrote:

Yes.  And I also should have mentioned: this is untested.

After v1.6.0-rc0~230^2^ (environment.c: remove unused function, 2008-06-19), git_refs_dir is not used any more.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 environment.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

If setup_git_env() is run before the usual repository discovery sequence and .git is a file with the text

	gitdir: <path>

(with <path> any string) then the in-core git_dir variable is set to the result of converting <path> to an absolute path using make_absolute_path().

Unfortunately make_absolute_path() returns its result in a static buffer that is overwritten by later calls. Such a call could cause later accesses to git_dir (from git_pathdup(), for example) to read the wrong path, leaving git very confused.

It is not obvious whether any existing code in git will trigger the problem, but in any case, it is worth a few dozen bytes to copy the return value from make_absolute_path() for some added peace of mind.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 environment.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

On Sat, Oct 2, 2010 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

It looks like make_absolute_path() is only used when git_dir has not been set in stone yet. Anyway, yes yes yes! I don't want to pull my hair when I find out this bug hits me.

In git (option parsing and some other places), we sometimes leak tiny bits of memory on purpose. Introduce a way to declare such allocations, to make the lives of people searching for real memory leaks easier.

Currently these are thin wrappers around xmalloc and xstrdup. The KEEP_WRAPPER attribute avoids a tail call to ensure their stack frame does not get replaced with the true xmalloc/xstrdup, so tools like valgrind can easily distinguish them and know that the memory they return is not meant to be freed.

In the long run it might be nice to tighten the semantics of these functions and use memory from a large pool that is never freed (to avoid fragmentation and save some allocation overhead).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 cache.h                 |    2 ++
 git-compat-util.h       |    2 ++
 path.c                  |   10 ++++++++++
 t/valgrind/default.supp |   17 +++++++++++++++++
 wrapper.c               |   14 ++++++++++++++
 5 files changed, 45 insertions(+), 0 deletions(-)

The in-core variables to match $GIT_DIR, $GIT_OBJECT_DIRECTORY, etc come from three sources:

 a) environment variables [getenv("GIT_INDEX_FILE")]
 b) computation [git_pathdup("objects")]
 c) static strings [".git"]

Only in case (b) are they malloc()'d; even in that case, it would not make sense to free the values until program termination, when _exit() takes care of freeing all memory already.

So mark the allocations in case (b) as permanent so no one wastes time trying to fix the leak.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 environment.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

This buffer is freed by the C runtime when commit-tree exits moments later, but freeing it explicitly should hopefully make this code easier to reuse (in addition to making valgrind quieter).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  Thanks for reading.

 builtin/commit-tree.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

Heya,

On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder <jrnieder@gmail.com> wrote:

Thanks, I've used git under valgrind a couple of times and always had to resort to running 'regular' git, looking at what valgrind reports for it, and then running my modified git, and try to make sure that I didn't introduce any new leaks. How does the current valgrind output look like, are we close to being leak free, or is this just a drop in (or perhaps out of :P) the bucket?

Sverre Rabbelier wrote:

It is a drop in the bucket. With some extra suppressions[1], I am only able to run t0000 through valgrind.

Probably this series should cook outside mainline until the test suite runs without reporting any unsuppressed leaks. I still would be interested in more comments on the approach, though. If KEEP_WRAPPER were conditionally defined to be empty (so by default people _would_ find the tail calls in those sentinel functions eliminated --- i.e., no performance impact unless you ask for it), would that be more acceptable?

[1]
 {
	cache entry leak
	Memcheck:Leak
	fun:calloc
	fun:xcalloc
	fun:add_one_path
 }
 {
	cache entry leak 2
	Memcheck:Leak
	fun:calloc
	fun:xcalloc
	fun:unpack_callback
 }
 {
	nobody seems to free tree buffers
	Memcheck:Leak
	fun:malloc
	fun:xmalloc
	fun:xmallocz
	fun:unpack_sha1_file
	fun:read_object
	fun:read_sha1_file_repl
	fun:parse_object
	fun:parse_tree_indirect
 }
 {
	really, nobody seems to free tree buffers
	Memcheck:Leak
	fun:malloc
	fun:xmalloc
	fun:xmallocz
	fun:unpack_sha1_file
	fun:read_object
	fun:read_sha1_file_repl
	fun:parse_tree
 }

Heya,

On Sat, Oct 2, 2010 at 20:26, Jonathan Nieder <jrnieder@gmail.com> wrote:

That would be very nice :).

I suppose developers that are interested can easily turn it on in their config.mak, and I think it's definitely useful to have this available to make sure we don't introduce any new leaks.

The question of course is, is there anyone interested in cleaning up this mess? Do you mean to keep it up and get the test suite to the point where there are no leak warnings?

Sverre Rabbelier wrote:

I was thinking more along the lines of maintaining a branch with --valgrind set up to do leak checking, and collecting patches to deal with leak warnings as people write them.

My current itch is sussing out cherry-pick leaks. So at the moment I only need to be able to run a small subset of the test suite.

Jonathan Nieder <jrnieder@gmail.com> writes:

I'd agree this is a change in a good direction because it will reduce false positives from valgrind. I do not believe it would make it any easier to reuse, though. Read what the code does (e.g. calling git-config, showing usage or dying on sick arguments, etc.). So I have to say that the proposed commit log message advertises a benefit that it does not bring.

By the way, is there an easy and concise way to tell valgrind to

 (1) treat allocations made in main() that are not freed as non-issues;
     and

 (2) treat functions whose name begin with cmd_ as if they are all their
     own main(), and apply the above special casing to them?

Junio C Hamano wrote:

Yep, that was sloppy of me.  commit-tree has already been libified.

As long as the term "allocation" is defined clearly. For example (written in the modern syntax for conciseness):

	{
		xmalloc from main()
		Memcheck:Leak
		...
		fun:xmalloc
		fun:main
	}
	{
		xmalloc from built-in main
		Memcheck:Leak
		...
		fun:xmalloc
		fun:cmd_*
	}

Even better would be to exit() instead of returning from cmd_ functions. Valgrind will not consider any memory that still has a pointer to it at exit time as a leak.

Jonathan Nieder <jrnieder@gmail.com> writes:

Hmm, is there a way to tell valgrind that in these functions pretend as if they left the scope by calling exit() when they return?

Junio C Hamano wrote:

You mean a way to tell it to trap the return from these functions? No, I don't think so.