Re: [PATCH] memory leak reported by valgrind

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 [off-list ref] 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.

On Mon, Aug 9, 2010 at 22:19, Erik Faye-Lund [off-list ref] wrote:

On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano [off-list ref] wrote:

quoted

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.

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

Noticed with valgrind.

Signed-off-by: Jonathan Nieder <redacted>
---
 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

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

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

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

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

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

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

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

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

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

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

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.

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.

Heya,

On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder [off-list ref] 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

Sverre Rabbelier wrote:

On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder [off-list ref] wrote:

quoted

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
 }

Heya,

On Sat, Oct 2, 2010 at 20:26, Jonathan Nieder [off-list ref] 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

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.

On Sat, Oct 2, 2010 at 3:36 PM, Jonathan Nieder [off-list ref] 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