[PATCH 00/15] Fixing memory leaks

So here comes another bunch of mem leak fixes. While 
I consider the first 10 patches a pretty safe bet,
the last 5 hopefully spark a discussion if we want 
patches which just clean up before the program ends.

Stefan Beller (15):
  read-cache: fix memleak
  read-cache: Improve readability
  read-cache: free cache entry in add_to_index in case of early return
  update-index: fix a memleak
  builtin/apply.c: fix a memleak
  merge-blobs.c: Fix a memleak
  merge-recursive: fix memleaks
  http-push: Remove unneeded cleanup
  http: release the memory of a http pack request as well
  commit.c: fix a memory leak
  builtin/check-attr: fix a memleak
  builtin/merge-base: fix memleak
  builtin/unpack-file: fix a memleak
  builtin/cat-file: free memleak
  ls-files: fix a memleak

 builtin/apply.c        |  1 +
 builtin/cat-file.c     |  1 +
 builtin/check-attr.c   |  2 ++
 builtin/commit.c       |  6 ++++--
 builtin/ls-files.c     |  1 +
 builtin/merge-base.c   |  1 +
 builtin/unpack-file.c  |  1 +
 builtin/update-index.c |  1 +
 http-push.c            |  1 -
 http.c                 |  1 +
 merge-blobs.c          |  4 +++-
 merge-recursive.c      |  3 +++
 read-cache.c           | 13 ++++++++-----
 13 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f72ea9f..769897e 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 		    !hashcmp(alias->sha1, ce->sha1) &&
 		    ce->ce_mode == alias->ce_mode);
 
-	if (pretend)
-		;
-	else if (add_index_entry(istate, ce, add_option))
+	if (!pretend && add_index_entry(istate, ce, add_option))
 		return error("unable to add %s to index",path);
 	if (verbose && !was_same)
 		printf("add '%s'\n", path);

-- 
2.3.0.81.gc37f363

oldlines is allocated earlier in the function and also freed on the
successful code path.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/apply.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 65b97ee..e152c4d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -2776,6 +2776,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 		default:
 			if (apply_verbosely)
 				error(_("invalid start of line: '%c'"), first);
+			free(oldlines);
 			return -1;
 		}
 		if (added_blank_line) {

-- 
2.3.0.81.gc37f363

preq is NULL as the condition the line before dictates. And the cleanup
function release_http_pack_request is not null pointer safe.

Signed-off-by: Stefan Beller <redacted>
---
 http-push.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index bfb1c96..c98dad2 100644
--- a/http-push.c
+++ b/http-push.c

@@ -316,7 +316,6 @@ static void start_fetch_packed(struct transfer_request *request)
 
 	preq = new_http_pack_request(target, repo->url);
 	if (preq == NULL) {
-		release_http_pack_request(preq);
 		repo->can_update_info_refs = 0;
 		return;
 	}

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---
 builtin/commit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 961e467..da79ac4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -229,7 +229,7 @@ static int commit_index_files(void)
 static int list_paths(struct string_list *list, const char *with_tree,
 		      const char *prefix, const struct pathspec *pattern)
 {
-	int i;
+	int i, ret;
 	char *m;
 
 	if (!pattern->nr)

@@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix);
+	ret = report_path_error(m, pattern, prefix);
+	free(m);
+	return ret;
 }
 
 static void add_remove_files(struct string_list *list)

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---

Notes:
    see discussion

 builtin/ls-files.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 914054d..306defa 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -590,6 +590,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
+		free(ps_matched);
 		return bad ? 1 : 0;
 	}
 

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---

Notes:
    I do not quite recall a discussion about such fixes
    when I started doing these fixes like 2 years ago.
    
    As this is a main function of a subcommand the freed
    memory is likely to have no impact as the process
    is done soon, so then it gets freed by the OS which
    is likey to be faster as the OS frees the whole pages
    of the process. Also there is no expected memory
    shortage as the process is going to be done soon
    as opposed to fixing mem leaks early in the process.
    
    An upside of fixes like this one is however to make
    code analysis tools produce less noise, so narrowing
    down the *real* issue may be easier.
    
    I wonder if we could have a 'weak' free which does
    nothing if git is compiled regularly and actually
    frees the memory if it is build with a flag to tell
    it to do so. This would help finding the real issues
    as the noise goes down and it would still be 'fast'
    as it could be when compiled for productive use.
    
    On the other hand I don't like to have another
    'invented here again' systemcall-ish function as
    it would clutter the code and you'd have to remember
    to use that weak free.
    
    I dunno.

 builtin/check-attr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 21d2bed..fa96356 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c

@@ -182,5 +182,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 			check_attr(prefix, cnt, check, argv[i]);
 		maybe_flush_or_die(stdout, "attribute to stdout");
 	}
+
+	free(check);
 	return 0;
 }

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---

Notes:
    see discussion on previous patch.

 builtin/merge-base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 08a8217..4a8ff02 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c

@@ -22,6 +22,7 @@ static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
 		result = result->next;
 	}
 
+	free(rev);
 	return 0;
 }
 

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---

Notes:
    This is for discussion again,
    see 2nd previous patch.
    
    However why do we close the fd when we know
    the program ends soon? So let's also
    free the memory.

 builtin/unpack-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 1920029..ea5767e 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c

@@ -17,6 +17,7 @@ static char *create_temp_file(unsigned char *sha1)
 	if (write_in_full(fd, buf, size) != size)
 		die_errno("unable to write temp-file");
 	close(fd);
+	free(buf);
 	return path;
 }
 

-- 
2.3.0.81.gc37f363

Usually when using string lists it looks like:
    struct string_list a = STRING_LIST_INIT_NODUP;
    // do stuff with a such as
    string_list_insert(&a, "test string");
    print_string_list(&a, "test prefix");
    // Cleaning up works on everything inside the struct, not on the
    // struct itself:
    string_list_clear(&a);

But as we deal with the pointers to the string lists directly, we also
need to free the actual struct.

Signed-off-by: Stefan Beller <redacted>
---
 merge-recursive.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index 771f5e2..1c9c30d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -1858,6 +1858,9 @@ int merge_trees(struct merge_options *o,
 		string_list_clear(re_head, 0);
 		string_list_clear(entries, 1);
 
+		free(re_merge);
+		free(re_head);
+		free(entries);
 	}
 	else
 		clean = 1;

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---

Notes:
    see discussion

 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..8de6b0d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c

@@ -104,6 +104,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 		die("git cat-file %s: bad file", obj_name);
 
 	write_or_die(1, buf, size);
+	free(buf);
 	return 0;
 }
 

-- 
2.3.0.81.gc37f363

This frees `ce` would be leaking in the error path.

Additionally a free is moved towards the return. This helps code
readability as we often have this pattern of freeing resources just
before return/exit and not in between the code.

Signed-off-by: Stefan Beller <redacted>
---
 read-cache.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 769897e..935f91a 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -681,15 +681,18 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	alias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case);
 	if (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) {
 		/* Nothing changed, really */
-		free(ce);
 		if (!S_ISGITLINK(alias->ce_mode))
 			ce_mark_uptodate(alias);
 		alias->ce_flags |= CE_ADDED;
+
+		free(ce);
 		return 0;
 	}
 	if (!intent_only) {
-		if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))
+		if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT)) {
+			free(ce);
 			return error("unable to index file %s", path);
+		}
 	} else
 		set_object_name_for_intent_to_add_entry(ce);
 

-- 
2.3.0.81.gc37f363

`ce` is allocated in make_cache_entry and should be freed if it is not
used any more. refresh_cache_entry as a wrapper around refresh_cache_ent
will either return `ce` or a new updated cache entry which is allocated
to new memory. In that case we need to free `ce` ourselfs.

Signed-off-by: Stefan Beller <redacted>
---
 read-cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 8d71860..f72ea9f 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -747,6 +747,8 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 		free(ce);
 		return NULL;
 	} else {
+		if (ret != ce)
+			free(ce);
 		return ret;
 	}
 }

-- 
2.3.0.81.gc37f363

The cleanup function is used in 4 places now and it's always safe to
free up the memory as well.

Signed-off-by: Stefan Beller <redacted>
---
 http.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http.c b/http.c
index 9c825af..4b179f6 100644
--- a/http.c
+++ b/http.c

@@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq)
 	}
 	preq->slot = NULL;
 	free(preq->url);
+	free(preq);
 }
 
 int finish_http_pack_request(struct http_pack_request *preq)

-- 
2.3.0.81.gc37f363

`old` is not used outside the loop and would get lost
once we reach the goto.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/update-index.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5878986..6271b54 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c

@@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av,
 		path = xstrdup(ce->name);
 		update_one(path);
 		free(path);
+		free(old);
 		if (save_nr != active_nr)
 			goto redo;
 	}

-- 
2.3.0.81.gc37f363

Signed-off-by: Stefan Beller <redacted>
---
 merge-blobs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-blobs.c b/merge-blobs.c
index 57211bc..7abb894 100644
--- a/merge-blobs.c
+++ b/merge-blobs.c

@@ -14,8 +14,10 @@ static int fill_mmfile_blob(mmfile_t *f, struct blob *obj)
 	buf = read_sha1_file(obj->object.sha1, &type, &size);
 	if (!buf)
 		return -1;
-	if (type != OBJ_BLOB)
+	if (type != OBJ_BLOB) {
+		free(buf);
 		return -1;
+	}
 	f->ptr = buf;
 	f->size = size;
 	return 0;

-- 
2.3.0.81.gc37f363