[PATCH] memory leak reported by valgrind

[PATCH] memory leak reported by valgrind

[Ivan Kanis]

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
@@ -2341,5 +2341,6 @@
                fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),"
                        " reused %"PRIu32" (delta %"PRIu32")\n",
                        written, written_delta, reused, reused_delta);
+       free(rp_av);
        return 0;
 }

-- 
Ivan Kanis

When you're looking at life
In a strange new room
Maybe drowning soon
Is this the start of it all?
    -- Ian Curtis 

Re: [PATCH] memory leak reported by valgrind

[Junio C Hamano]

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

Re: [PATCH] memory leak reported by valgrind

[Erik Faye-Lund]

On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)
> to clean after us?

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.

Re: [PATCH] memory leak reported by valgrind

[Alex Riesen]

On Mon, Aug 9, 2010 at 22:19, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>
> On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)
> > to clean after us?
>
> 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.

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

Re: [PATCH] memory leak reported by valgrind

[Jonathan Nieder]

Junio C Hamano wrote:

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

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

[PATCH/RFC 1/3] core: Stop leaking ondisk_cache_entrys

[Jonathan Nieder]

Noticed with valgrind.

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

diff --git a/read-cache.c b/read-cache.c
index f1f789b..1f42473 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1516,6 +1516,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)
 	int size = ondisk_ce_size(ce);
 	struct ondisk_cache_entry *ondisk = xcalloc(1, size);
 	char *name;
+	int result;
 
 	ondisk->ctime.sec = htonl(ce->ce_ctime.sec);
 	ondisk->mtime.sec = htonl(ce->ce_mtime.sec);
@@ -1539,7 +1540,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)
 		name = ondisk->name;
 	memcpy(name, ce->name, ce_namelen(ce));
 
-	return ce_write(c, fd, ondisk, size);
+	result = ce_write(c, fd, ondisk, size);
+	free(ondisk);
+	return result;
 }
 
 int write_index(struct index_state *istate, int newfd)
-- 
1.7.2.1.544

[PATCH/RFC 2/3] write-tree: Avoid leak when index refers to an invalid object

[Jonathan Nieder]

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(-)

diff --git a/cache-tree.c b/cache-tree.c
index d917437..c60cf91 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -328,9 +328,11 @@ static int update_one(struct cache_tree *it,
 			mode = ce->ce_mode;
 			entlen = pathlen - baselen;
 		}
-		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
+		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+			strbuf_release(&buffer);
 			return error("invalid object %06o %s for '%.*s'",
 				mode, sha1_to_hex(sha1), entlen+baselen, path);
+		}
 
 		if (ce->ce_flags & CE_REMOVE)
 			continue; /* entry being removed */
-- 
1.7.2.1.544.ga752d.dirty

[PATCH/RFC 3/3] read-tree: stop leaking tree objects

[Jonathan Nieder]

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(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 8cf0da3..f561d88 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -329,6 +329,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long
 {
 	int i, ret, bottom;
 	struct tree_desc t[MAX_UNPACK_TREES];
+	void *buf[MAX_UNPACK_TREES];
 	struct traverse_info newinfo;
 	struct name_entry *p;
 
@@ -346,12 +347,16 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long
 		const unsigned char *sha1 = NULL;
 		if (dirmask & 1)
 			sha1 = names[i].sha1;
-		fill_tree_descriptor(t+i, sha1);
+		buf[i] = fill_tree_descriptor(t+i, sha1);
 	}
 
 	bottom = switch_cache_bottom(&newinfo);
 	ret = traverse_trees(n, t, &newinfo);
 	restore_cache_bottom(&newinfo, bottom);
+
+	for (i = 0; i < n; i++)
+		free(buf[i]);
+
 	return ret;
 }
 
-- 
1.7.2.1.544.ga752d.dirty

[PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind

[Jonathan Nieder]

Jonathan Nieder wrote:

>                       Could something like leaky() be used to insert
> an artificial stack frame at allocation time to make suppressions
> simpler to write?

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(-)

-- 
1.7.2.3

[PATCH 1/7] init: plug tiny one-time memory leak

[Jonathan Nieder]

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(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0271285..a9c54ea 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)
 	return reinit;
 }
 
+static void create_sha1_dir(void)
+{
+	const char *sha1_dir = get_object_directory();
+	int len = strlen(sha1_dir);
+	char *path = xmalloc(len + 40);
+
+	memcpy(path, sha1_dir, len);
+
+	safe_create_dir(sha1_dir, 1);
+	strcpy(path+len, "/pack");
+	safe_create_dir(path, 1);
+	strcpy(path+len, "/info");
+	safe_create_dir(path, 1);
+
+	free(path);
+}
+
 int init_db(const char *template_dir, unsigned int flags)
 {
-	const char *sha1_dir;
-	char *path;
-	int len, reinit;
+	int reinit;
 
 	safe_create_dir(get_git_dir(), 0);
 
@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)
 
 	reinit = create_default_files(template_dir);
 
-	sha1_dir = get_object_directory();
-	len = strlen(sha1_dir);
-	path = xmalloc(len + 40);
-	memcpy(path, sha1_dir, len);
-
-	safe_create_dir(sha1_dir, 1);
-	strcpy(path+len, "/pack");
-	safe_create_dir(path, 1);
-	strcpy(path+len, "/info");
-	safe_create_dir(path, 1);
+	create_sha1_dir();
 
 	if (shared_repository) {
 		char buf[10];
-- 
1.7.2.3

[PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Jonathan Nieder]

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(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index dd89404..8eac22a 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -22,6 +22,15 @@ The lockfile API serves two purposes:
 The functions
 -------------
 
+alloc_lock_file::
+
+	Allocate and clear a `struct lock_file`.  Because the
+	structure is used in an `atexit(3)` handler, its
+	storage has to stay throughout the life of the
+	program.  Using this allocation function instead of
+	`malloc` allows profiling tools like valgrind to keep
+	track of such storage and avoid reporting it as a leak.
+
 hold_lock_file_for_update::
 
 	Take a pointer to `struct lock_file`, the filename of
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..7697631 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -591,9 +591,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
+	lock_file = alloc_lock_file();
 	newfd = hold_locked_index(lock_file, 0);
 	if (newfd < 0)
 		lock_error = errno;
diff --git a/cache-tree.c b/cache-tree.c
index f755590..81cf785 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -550,12 +550,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	int entries, was_valid, newfd;
 	struct lock_file *lock_file;
 
-	/*
-	 * We can't free this memory, it becomes part of a linked list
-	 * parsed atexit()
-	 */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
+	lock_file = alloc_lock_file();
 	newfd = hold_locked_index(lock_file, 1);
 
 	entries = read_cache();
diff --git a/cache.h b/cache.h
index 2ef2fa3..9ee61a2 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ struct lock_file {
 #define LOCK_NODEREF 2
 extern int unable_to_lock_error(const char *path, int err);
 extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern struct lock_file *alloc_lock_file(void);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
diff --git a/config.c b/config.c
index 4b0a820..3f30e2b 100644
--- a/config.c
+++ b/config.c
@@ -1158,7 +1158,7 @@ int git_config_set_multivar(const char *key, const char *value,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	lock = xcalloc(sizeof(struct lock_file), 1);
+	lock = alloc_lock_file();
 	fd = hold_lock_file_for_update(lock, config_filename, 0);
 	if (fd < 0) {
 		error("could not lock config file %s: %s", config_filename, strerror(errno));
diff --git a/lockfile.c b/lockfile.c
index b0d74cd..161dea8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -184,6 +184,12 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)
 	die("%s", unable_to_lock_message(path, err));
 }
 
+__attribute__((optimize("-fno-optimize-sibling-calls")))
+struct lock_file *alloc_lock_file(void)
+{
+	return xcalloc(1, sizeof(struct lock_file));
+}
+
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 9e013fa..7789edd 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -43,3 +43,11 @@
 	fun:write_buffer
 	fun:write_loose_object
 }
+
+{
+	see api-lockfile.txt
+	Memcheck:Leak
+	fun:calloc
+	fun:xcalloc
+	fun:alloc_lock_file
+}
-- 
1.7.2.3

[PATCH 3/7] environment.c: remove unused variable

[Jonathan Nieder]

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(-)

diff --git a/environment.c b/environment.c
index 2d0c315..c44a30b 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,7 @@ char *git_work_tree_cfg;
 static char *work_tree;
 
 static const char *git_dir;
-static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
+static char *git_object_dir, *git_index_file, *git_graft_file;
 
 /*
  * Repository-local GIT_* environment variables
@@ -96,8 +96,6 @@ static void setup_git_env(void)
 		git_object_dir = xmalloc(strlen(git_dir) + 9);
 		sprintf(git_object_dir, "%s/objects", git_dir);
 	}
-	git_refs_dir = xmalloc(strlen(git_dir) + 6);
-	sprintf(git_refs_dir, "%s/refs", git_dir);
 	git_index_file = getenv(INDEX_ENVIRONMENT);
 	if (!git_index_file) {
 		git_index_file = xmalloc(strlen(git_dir) + 7);
-- 
1.7.2.3

[PATCH 4/7] setup: make sure git dir path is in a permanent buffer

[Jonathan Nieder]

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(-)

diff --git a/environment.c b/environment.c
index c44a30b..de5581f 100644
--- a/environment.c
+++ b/environment.c
@@ -87,8 +87,10 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
+	if (!git_dir) {
 		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+		git_dir = git_dir ? xstrdup(git_dir) : NULL;
+	}
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	git_object_dir = getenv(DB_ENVIRONMENT);
-- 
1.7.2.3

[PATCH 5/7] Introduce malloc/strdup/pathdup variants for permanent allocations

[Jonathan Nieder]

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(-)

diff --git a/cache.h b/cache.h
index 9ee61a2..bbf32ff 100644
--- a/cache.h
+++ b/cache.h
@@ -638,6 +638,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *git_pathdup_permanent(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
diff --git a/git-compat-util.h b/git-compat-util.h
index 81883e7..f00ee33 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -380,7 +380,9 @@ typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);
 
 extern char *xstrdup(const char *str);
+extern char *strdup_permanent(const char *str);
 extern void *xmalloc(size_t size);
+extern void *alloc_permanent(size_t size);
 extern void *xmallocz(size_t size);
 extern void *xmemdupz(const void *data, size_t len);
 extern char *xstrndup(const char *str, size_t len);
diff --git a/path.c b/path.c
index a2c9d1e..332c903 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,16 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+char *git_pathdup_permanent(const char *fmt, ...)
+{
+	char path[PATH_MAX];
+	va_list args;
+	va_start(args, fmt);
+	(void)git_vsnpath(path, sizeof(path), fmt, args);
+	va_end(args);
+	return strdup_permanent(path);
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 7789edd..d237210 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -45,6 +45,23 @@
 }
 
 {
+	leaky malloc for option parsing and configuration
+	Memcheck:Leak
+	fun:malloc
+	fun:xmalloc
+	fun:alloc_permanent
+}
+
+{
+	leaky strdup for option parsing and configuration
+	Memcheck:Leak
+	fun:malloc
+	fun:strdup
+	fun:xstrdup
+	fun:strdup_permanent
+}
+
+{
 	see api-lockfile.txt
 	Memcheck:Leak
 	fun:calloc
diff --git a/wrapper.c b/wrapper.c
index fd8ead3..88ca3dd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,6 +3,8 @@
  */
 #include "cache.h"
 
+#define KEEP_WRAPPER __attribute__((optimize("-fno-optimize-sibling-calls")))
+
 static void try_to_free_builtin(size_t size)
 {
 	release_pack_memory(size, -1);
@@ -17,6 +19,12 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)
 	return old;
 }
 
+KEEP_WRAPPER
+char *strdup_permanent(const char *str)
+{
+	return xstrdup(str);
+}
+
 char *xstrdup(const char *str)
 {
 	char *ret = strdup(str);
@@ -29,6 +37,12 @@ char *xstrdup(const char *str)
 	return ret;
 }
 
+KEEP_WRAPPER
+void *alloc_permanent(size_t size)
+{
+	return xmalloc(size);
+}
+
 void *xmalloc(size_t size)
 {
 	void *ret = malloc(size);
-- 
1.7.2.3

[PATCH 6/7] environment: use alloc_permanent() for computed git_dir and co

[Jonathan Nieder]

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(-)

diff --git a/environment.c b/environment.c
index de5581f..9bd18c7 100644
--- a/environment.c
+++ b/environment.c
@@ -89,23 +89,23 @@ static void setup_git_env(void)
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir) {
 		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
-		git_dir = git_dir ? xstrdup(git_dir) : NULL;
+		git_dir = git_dir ? strdup_permanent(git_dir) : NULL;
 	}
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {
-		git_object_dir = xmalloc(strlen(git_dir) + 9);
+		git_object_dir = alloc_permanent(strlen(git_dir) + 9);
 		sprintf(git_object_dir, "%s/objects", git_dir);
 	}
 	git_index_file = getenv(INDEX_ENVIRONMENT);
 	if (!git_index_file) {
-		git_index_file = xmalloc(strlen(git_dir) + 7);
+		git_index_file = alloc_permanent(strlen(git_dir) + 7);
 		sprintf(git_index_file, "%s/index", git_dir);
 	}
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
 	if (!git_graft_file)
-		git_graft_file = git_pathdup("info/grafts");
+		git_graft_file = git_pathdup_permanent("info/grafts");
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		read_replace_refs = 0;
 }
-- 
1.7.2.3

[PATCH 7/7] commit-tree: free commit message before exiting

[Jonathan Nieder]

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(-)

diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 87f0591..732f895 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -56,10 +56,12 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	if (strbuf_read(&buffer, 0, 0) < 0)
 		die_errno("git commit-tree: failed to read");
 
-	if (!commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
-		printf("%s\n", sha1_to_hex(commit_sha1));
-		return 0;
-	}
-	else
+	if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
+		strbuf_release(&buffer);
 		return 1;
+	}
+
+	printf("%s\n", sha1_to_hex(commit_sha1));
+	strbuf_release(&buffer);
+	return 0;
 }
-- 
1.7.2.3

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Andreas Ericsson]

On 10/02/2010 10:32 AM, Jonathan Nieder wrote:
> 
> 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().
> 

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                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Jonathan Nieder]

Andreas Ericsson wrote:

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

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.

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Sverre Rabbelier]

Heya,

On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder <jrnieder@gmail.com> wrote:
> That's the end of the series.  Thanks for reading.

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?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Jonathan Nieder]

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

>> That's the end of the series.  Thanks for reading.
>
> 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?

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
 }

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Sverre Rabbelier]

Heya,

On Sat, Oct 2, 2010 at 20:26, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Probably this series should cook outside mainline until the test suite
> runs without reporting any unsuppressed leaks.

That would be very nice :).

> 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?

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?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Jonathan Nieder]

Sverre Rabbelier wrote:

> 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?

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.

Re: [PATCH 1/7] init: plug tiny one-time memory leak

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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).

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?

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Jonathan Nieder]

Junio C Hamano wrote:

> 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.

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

> 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?

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.

[PATCH v2] init: plug tiny one-time memory leak

[Jonathan Nieder]

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:

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

Yes.

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

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0271285..9d4886c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)
 	return reinit;
 }
 
+static void create_object_directory(void)
+{
+	const char *object_directory = get_object_directory();
+	int len = strlen(object_directory);
+	char *path = xmalloc(len + 40);
+
+	memcpy(path, object_directory, len);
+
+	safe_create_dir(object_directory, 1);
+	strcpy(path+len, "/pack");
+	safe_create_dir(path, 1);
+	strcpy(path+len, "/info");
+	safe_create_dir(path, 1);
+
+	free(path);
+}
+
 int init_db(const char *template_dir, unsigned int flags)
 {
-	const char *sha1_dir;
-	char *path;
-	int len, reinit;
+	int reinit;
 
 	safe_create_dir(get_git_dir(), 0);
 
@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)
 
 	reinit = create_default_files(template_dir);
 
-	sha1_dir = get_object_directory();
-	len = strlen(sha1_dir);
-	path = xmalloc(len + 40);
-	memcpy(path, sha1_dir, len);
-
-	safe_create_dir(sha1_dir, 1);
-	strcpy(path+len, "/pack");
-	safe_create_dir(path, 1);
-	strcpy(path+len, "/info");
-	safe_create_dir(path, 1);
+	create_object_directory();
 
 	if (shared_repository) {
 		char buf[10];
-- 
1.7.2.3

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

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?

Re: [PATCH 7/7] commit-tree: free commit message before exiting

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 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.
>
> 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?

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

Re: [PATCH 4/7] setup: make sure git dir path is in a permanent buffer

[Nguyen Thai Ngoc Duy]

On Sat, Oct 2, 2010 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.

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.
-- 
Duy

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> ...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().

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?

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Jonathan Nieder]

Junio C Hamano wrote:

> 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

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

> 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?

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;
	}

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> 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
>
> Unfortunate.  With gcc 4.5 it works, but that isn't too useful.
>
>> 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?
>
> 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;
> 	}

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

Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> -	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;
>> 	}
>
> Heh, shouldn't the last one assign from p->next?

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