[PATCH 00/15] Fixing memory leaks

[PATCH 00/15] Fixing memory leaks

[Stefan Beller]

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

[PATCH 01/15] read-cache: fix memleak

[Stefan Beller]

`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 <sbeller@google.com>
---
 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

[PATCH 02/15] read-cache: Improve readability

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 04/15] update-index: fix a memleak

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 05/15] builtin/apply.c: fix a memleak

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 06/15] merge-blobs.c: Fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 07/15] merge-recursive: fix memleaks

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 08/15] http-push: Remove unneeded cleanup

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 09/15] http: release the memory of a http pack request as well

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 10/15] commit.c: fix a memory leak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 11/15] builtin/check-attr: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

[PATCH 12/15] builtin/merge-base: fix memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

[PATCH 13/15] builtin/unpack-file: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

[PATCH 14/15] builtin/cat-file: free memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

[PATCH 15/15] ls-files: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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

Re: [PATCH 00/15] Fixing memory leaks

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So here comes another bunch of mem leak fixes.

Looked mostly sensible.

Re: [PATCH 01/15] read-cache: fix memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> `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 <sbeller@google.com>
> ---
>  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;
>  	}
>  }

Good, I vaguely recall that we did something similar in another
codepath that forgot the fact that refresh_cache_entry() may make
the incoming ce unnecessary.

As the rule is "if ret is different from ce, then ce must be freed"
in this codepath, I wonder if this is easier to read:

	ret = refresh_cache_entry(ce, ...);
        if (ret != ce)
        	free(ce);
	return ret;

Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

At this point ce is not yet added to the index, so it is clear it is
safe to free it---otherwise we will leak it.  Good.

> Additionally a free is moved towards the return.

I am on the fence on this one between two schools and do not have a
strong preference.  One school is to free as soon as you know you do
not need it, which is a valid stance to take.  Another is, as you
did, not to care about the minimum necessary lifetime of the storage
and free them all at the end, which is also valid.  Technically, the
former could be more performant while the latter is easier on the
eyes.

Re: [PATCH 04/15] update-index: fix a memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> `old` is not used outside the loop and would get lost
> once we reach the goto.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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);

The change looks trivially correct.

A tangent; I wonder if we want to make path a strbuf that is
declared in the function scope, reset (not released) at each
iteration of the loop and released after the loop; keep allocating
and freeing a small piece of string all the time feels somewhat
wasteful.

Also, we might want to add to the "Be careful" comment to describe
why this cannot just be a call to "update_one(ce->name)" that does
not use "path".

Re: [PATCH 05/15] builtin/apply.c: fix a memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> oldlines is allocated earlier in the function and also freed on the
> successful code path.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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;

Good.

By the way, aren't the following also leaking here?

 - the strbuf newlines that starts out as "size"
 - line[] arrays of preimage and postimage




>  		}
>  		if (added_blank_line) {

Re: [PATCH 07/15] merge-recursive: fix memleaks

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

In other words, these two were allocated for the sole use of this
fuction by get_renames(), so this function is responsible for
freeing it?

Sounds sensible.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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;

Re: [PATCH 10/15] commit.c: fix a memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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;
>  }

Looks correct.

A tangent.  We may want to move report_path_error() to somewhere
more "common"-ish, like dir.c which is where we have bulk of
pathspec matching infrastructure.  Seeing the function used from
builtin/ls-files.c makes me feel dirty.

A further tangent (Duy Cc'ed for this point).  We might want to
rethink the interface to ce_path_match() and report_path_error()
so that we do not have to do a separate allocation of "has this
pathspec been used?" array.  This was a remnant from the olden days
back when pathspec were mere "const char **" where we did not have
any room to add per-item bit---these days pathspec is repreasented
as an array of "struct pathspec" and we can afford to add a bit
to the structure---which will make this kind of leak much less
likely to happen.

Re: [PATCH 11/15] builtin/check-attr: fix a memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> 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.

You seem to understand it well without "recalling" ;-)

As you said, unless the false hits from valgrind on these overwhelm
the real issues we are looking for, it is usually not worth the
patch churn to fix these.  Another valid reason is if we plan to
libify a function that used to be a top-level.

Re: [PATCH 02/15] read-cache: Improve readability

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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);

I have a moderately strong feeling against this change, as the code
was done this way quite deliberately to keep it readable, namely, to
avoid using && to concatenate two boolean expressions that are in
totally different class inside condition part of if/while, where A
is a precondition guard for B (i.e. you cannot evaluate B unless A
holds) and B is called primarily for its side effect.  The problem
is that, once you start liberally doing

	if (A && B && C && D ...)

with booleans with mixed semantics (guards and actions), it will
quickly get harder to tell which one is which.

I could have written it as

	if (!pretend) {
        	if (add_index_entry(...))
			return error(...);
	}

and that would have been just as readable as the original; it
clearly separates the guard (i.e. only do the add-index thing when
we are not pretending) and the operation that is done for the side
effect.

But I find the original tells you "if pretend mode, do *nothing*"
and "otherwise, try add_index_entry() and act on its error" very
clearly.  Of course, I am biased as the original is my code from
38ed1d89 ("git-add -n -u" should not add but just report,
2008-05-21).

FYI, between preference and taste, I'd say this one is much closer
to the latter than the former.

By the way, aren't we leaking ce when we are merely pretending?

Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return

[Stefan Beller]

On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This frees `ce` would be leaking in the error path.
>
> At this point ce is not yet added to the index, so it is clear it is
> safe to free it---otherwise we will leak it.  Good.
>
>> Additionally a free is moved towards the return.
>
> I am on the fence on this one between two schools and do not have a
> strong preference.  One school is to free as soon as you know you do
> not need it, which is a valid stance to take.  Another is, as you
> did, not to care about the minimum necessary lifetime of the storage
> and free them all at the end, which is also valid.  Technically, the
> former could be more performant while the latter is easier on the
> eyes.

I only recall to have seen the latter school so far, which is why I
made the change in the first place assuming the school of freeing
ASAP has no strong supporters inside the git community.

I can resend the patch dropping the reordering, if you prefer.

Re: [PATCH 02/15] read-cache: Improve readability

[Stefan Beller]

On Fri, Mar 20, 2015 at 9:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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);
>
> I have a moderately strong feeling against this change, as the code
> was done this way quite deliberately to keep it readable, namely, to
> avoid using && to concatenate two boolean expressions that are in
> totally different class inside condition part of if/while, where A
> is a precondition guard for B (i.e. you cannot evaluate B unless A
> holds) and B is called primarily for its side effect.  The problem
> is that, once you start liberally doing
>
>         if (A && B && C && D ...)
>
> with booleans with mixed semantics (guards and actions), it will
> quickly get harder to tell which one is which.
>
> I could have written it as
>
>         if (!pretend) {
>                 if (add_index_entry(...))
>                         return error(...);
>         }

This makes sense to point out the different semantics to me.
Maybe I have read too much of the refs code lately as there we
have these long chains which combine precondition with error
checking. :/ That's why I thought it would be global to git to not
care much about this semantics distinction.

>
> and that would have been just as readable as the original; it
> clearly separates the guard (i.e. only do the add-index thing when
> we are not pretending) and the operation that is done for the side
> effect.
>
> But I find the original tells you "if pretend mode, do *nothing*"
> and "otherwise, try add_index_entry() and act on its error" very
> clearly.  Of course, I am biased as the original is my code from
> 38ed1d89 ("git-add -n -u" should not add but just report,
> 2008-05-21).
>
> FYI, between preference and taste, I'd say this one is much closer
> to the latter than the former.
>
> By the way, aren't we leaking ce when we are merely pretending?

Yes we are, that's how I found this spot. (coverity pointed out ce was
leaking, so I was refactoring to actually make it easier to fix it, and then
heavily reordered the patch series afterwards. That spot was forgotten
apparently.

>
>

Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> This frees `ce` would be leaking in the error path.
>>
>> At this point ce is not yet added to the index, so it is clear it is
>> safe to free it---otherwise we will leak it.  Good.
>>
>>> Additionally a free is moved towards the return.
>>
>> I am on the fence on this one between two schools and do not have a
>> strong preference.  One school is to free as soon as you know you do
>> not need it, which is a valid stance to take.  Another is, as you
>> did, not to care about the minimum necessary lifetime of the storage
>> and free them all at the end, which is also valid.  Technically, the
>> former could be more performant while the latter is easier on the
>> eyes.
>
> I only recall to have seen the latter school so far, which is why I
> made the change in the first place assuming the school of freeing
> ASAP has no strong supporters inside the git community.
>
> I can resend the patch dropping the reordering, if you prefer.

I already said that I do not have a preference ;-)

Will queue 3/15 as-is, drop 2/15 and wait on 1/15.

Re: [PATCH 02/15] read-cache: Improve readability

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Maybe I have read too much of the refs code lately as there we
> have these long chains which combine precondition with error
> checking.

Of course, things are not so black-and-white in the real world.  

You can also take an extreme stance and view

	if (!pretend && do_it_and_report_error())
        	error(...);

differently.  I would explain that what the whole thing is trying to
achieve as "'do it' part is the primary thing we want to do, but it
only can be done when we are not pretending, and we show an error
when the 'do it' part fails." and would suggest structuring it more
like this:

	if (pretend)
        	; /* nothing */
	else if (do_it_and_report_error())
        	error(...);

or

	if (!pretend) {
        	if (do_it_and_report_error())
                	error(...);
	}

But you could say "The final objective of the whole thing is to show
an error message, but if we are pretending or if our attempt to 'do
it' succeeds, we do not have to show the error", and such a view may
make sense depending on what that 'do it' is.  The original may be
justified under such an interpretation.

We may be tempted to write (note: each boolean term may be a call
with many complex arguments)

    if (A && B && C && D && E)
	...

when it is in fact logically is more like this:

    /* does not make sense to attempt C when A && B does not hold */
    if (A && B) {
    	if (C && D && E)
        	...
    }

But it becomes a judgement call if splitting that into nested two if
statements is better for overall readability when the top-level if
statement has an else clause.  You cannot turn it into

    /* does not make sense to attempt C when A && B does not hold */
    if (A && B) {
    	if (C && D && E)
        	...
    } else {
        ... /* this cannot be what the original's else clause did */
    }

blindly.  It would need further restructuring.  I think the code
inside the refs.c is a result of making such judgement calls.

>> By the way, aren't we leaking ce when we are merely pretending?
>
> Yes we are, that's how I found this spot. (coverity pointed out ce was
> leaking, so I was refactoring to actually make it easier to fix it, and then
> heavily reordered the patch series afterwards. That spot was forgotten
> apparently.

I dropped 2/15 and expect the real fix in the future; no rush,
though.

Re: [PATCH 09/15] http: release the memory of a http pack request as well

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 <sbeller@google.com>
> ---
>  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)

Freeing of preq in all the callers of this one looks sensible,
except for the one in finish_request() of http-push.c that pulls an
preq instance out of request->userData.

Can somebody help me follow the dataflow to convince me that this is
not leading to double-free in start_fetch_packed()?

Thanks.

Re: [PATCH 01/15] read-cache: fix memleak

[Stefan Beller]

On Fri, Mar 20, 2015 at 8:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> `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 <sbeller@google.com>
>> ---
>>  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;
>>       }
>>  }
>
> Good, I vaguely recall that we did something similar in another
> codepath that forgot the fact that refresh_cache_entry() may make
> the incoming ce unnecessary.
>
> As the rule is "if ret is different from ce, then ce must be freed"
> in this codepath, I wonder if this is easier to read:
>
>         ret = refresh_cache_entry(ce, ...);
>         if (ret != ce)
>                 free(ce);
>         return ret;
>

Indeed it is. Thanks for pointing out!

I'll send this as part of another series
applying on top of sb/leaks soon.

[PATCH] update-index: Don't copy memory around

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

On Fri, Mar 20, 2015 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> `old` is not used outside the loop and would get lost
>> once we reach the goto.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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);
>
> The change looks trivially correct.
>
> A tangent; I wonder if we want to make path a strbuf that is
> declared in the function scope, reset (not released) at each
> iteration of the loop and released after the loop; keep allocating
> and freeing a small piece of string all the time feels somewhat
> wasteful.

> Also, we might want to add to the "Be careful" comment to describe
> why this cannot just be a call to "update_one(ce->name)" that does
> not use "path".

Indeed I am rather wondering why we need to pass in a copy to update_one
instead of ce->name. I was reading update_one and looking for why, but
eventually noticed update_one accepts a 'const char *', so it's not changed
in there. Digging down the into update_one also doesn't make it obvious to me.

I found (5699d17ee094, 2013-11-14, read-cache.c: fix memory leaks caused by
removed cache entries), which adds this duplication, though I do not understand
why. This passes the test suite, so I wonder if this patch would be a subtle bug
now.

 builtin/update-index.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..5857405 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -564,7 +564,6 @@ static int do_reupdate(int ac, const char **av,
 		const struct cache_entry *ce = active_cache[pos];
 		struct cache_entry *old = NULL;
 		int save_nr;
-		char *path;
 
 		if (ce_stage(ce) || !ce_path_match(ce, &pathspec, NULL))
 			continue;
@@ -581,9 +580,7 @@ static int do_reupdate(int ac, const char **av,
 		 * or worse yet 'allow_replace', active_nr may decrease.
 		 */
 		save_nr = active_nr;
-		path = xstrdup(ce->name);
-		update_one(path);
-		free(path);
+		update_one(ce->name);
 		free(old);
 		if (save_nr != active_nr)
 			goto redo;
-- 
2.3.0.81.gc37f363

Re: [PATCH] update-index: Don't copy memory around

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> ... though I do not understand
> why. This passes the test suite, so I wonder if this patch would be a subtle bug
> now.

I recall the last time I traced the code I noticed that the ce can
get passed to a codepath that causes its removal from update_one(),
and then the pathname itself (which used to be ce->name but remember
that ce is invalid already at that point!) is still used after ce is
removed, which was the reason why you would introduce a bug if you
stop copying the path. You may have to follow the code again a bit
closer to make sure but I am reasonably sure that was the reason.

[PATCH] builtin/apply.c: fix a memleak (Fixup, squashable)

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

> By the way, aren't the following also leaking here?
>
>  - the strbuf newlines that starts out as "size"
>  - line[] arrays of preimage and postimage
>

yes :(

I think a viable replacement may be below.
That way the returned value of the function is not -1, but 1.
We're using the return value only as a boolean, so it doesn't
really matter.

 builtin/apply.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
 
diff --git a/builtin/apply.c b/builtin/apply.c
index e152c4d..0769b09 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2776,8 +2776,8 @@ 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;
+			applied_pos = -1;
+			goto out;
 		}
 		if (added_blank_line) {
 			if (!new_blank_lines_at_end)
@@ -2916,6 +2916,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 			      (int)(old - oldlines), oldlines);
 	}
 
+out:
 	free(oldlines);
 	strbuf_release(&newlines);
 	free(preimage.line_allocated);
-- 
2.3.0.81.gc37f363

Re: [PATCH] builtin/apply.c: fix a memleak (Fixup, squashable)

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>> By the way, aren't the following also leaking here?
>>
>>  - the strbuf newlines that starts out as "size"
>>  - line[] arrays of preimage and postimage
>>
>
> yes :(
>
> I think a viable replacement may be below.

Yeah, that looks a lot more sensible and straight-forward.

Thanks.

> That way the returned value of the function is not -1, but 1.
> We're using the return value only as a boolean, so it doesn't
> really matter.
>
>  builtin/apply.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>  
> diff --git a/builtin/apply.c b/builtin/apply.c
> index e152c4d..0769b09 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -2776,8 +2776,8 @@ 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;
> +			applied_pos = -1;
> +			goto out;
>  		}
>  		if (added_blank_line) {
>  			if (!new_blank_lines_at_end)
> @@ -2916,6 +2916,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
>  			      (int)(old - oldlines), oldlines);
>  	}
>  
> +out:
>  	free(oldlines);
>  	strbuf_release(&newlines);
>  	free(preimage.line_allocated);

[PATCH 1/2] read-cache: fix memleak

[Stefan Beller]

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 read-cache.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 60abec6..a102565 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -746,12 +746,9 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	ce->ce_mode = create_ce_mode(mode);
 
 	ret = refresh_cache_entry(ce, refresh_options);
-	if (!ret) {
+	if (ret != ce)
 		free(ce);
-		return NULL;
-	} else {
-		return ret;
-	}
+	return ret;
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
-- 
2.3.0.81.gc37f363

[PATCH 2/2] read-cache.c: fix a memleak in add_to_index

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 I have reread your remarks from the weekend, and I agree
 this looks more intuitive. Thanks for pointing out the subtle details
 to make programming an art!
 
 read-cache.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index a102565..f837212 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -706,10 +706,14 @@ 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))
-		return error("unable to add %s to index",path);
+	if (!pretend) {
+		if (add_index_entry(istate, ce, add_option)) {
+			free(ce);
+			return error("unable to add %s to index",path);
+		}
+	} else {
+		free(ce);
+	}
 	if (verbose && !was_same)
 		printf("add '%s'\n", path);
 	return 0;
-- 
2.3.0.81.gc37f363

Re: [PATCH 2/2] read-cache.c: fix a memleak in add_to_index

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  I have reread your remarks from the weekend, and I agree
>  this looks more intuitive. Thanks for pointing out the subtle details
>  to make programming an art!

Heh, Our mails crossed, I guess.  I've done this myself like this

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 23 Mar 2015 10:58:00 -0700
Subject: [PATCH] add_to_index(): free unused cache-entry

We allocate a cache-entry pretty early in the function and then
decide either not to do anything when we are pretending to add, or
add it and then get an error (another possibility is obviously to
succeed).

When pretending or failing to add, we forgot to free the
cache-entry.

Noticed during a discussion on Stefan's patch to change the coding
style without fixing the issue ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 60abec6..5b922fd5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -707,9 +707,11 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 		    ce->ce_mode == alias->ce_mode);
 
 	if (pretend)
-		;
-	else if (add_index_entry(istate, ce, add_option))
-		return error("unable to add %s to index",path);
+		free(ce);
+	else if (add_index_entry(istate, ce, add_option)) {
+		free(ce);
+		return error("unable to add %s to index", path);
+	}
 	if (verbose && !was_same)
 		printf("add '%s'\n", path);
 	return 0;
-- 
2.3.3-454-g85aa98f

Re: [PATCH 1/2] read-cache: fix memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Thanks.  I started "commit --amend" to do s/ourselfs/ourselves/, but
ended up tweaking the description a bit more.

-- .8 --
From: Stefan Beller <sbeller@google.com>
Date: Mon, 23 Mar 2015 10:57:11 -0700
Subject: [PATCH] read-cache: fix memleak

`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

 - the `ce` given as the parameter, when it was up-to-date;
 - a new updated cache entry which is allocated to new memory; or
 - a NULL when refreshing failed.

In the latter two cases, the original cache-entry `ce` is not used
and needs to be freed.  The rule can be expressed as "if the return
value from refresh is different from the original ce, ce is no
longer used."

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 5b922fd5..0052b72 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -748,12 +748,9 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	ce->ce_mode = create_ce_mode(mode);
 
 	ret = refresh_cache_entry(ce, refresh_options);
-	if (!ret) {
+	if (ret != ce)
 		free(ce);
-		return NULL;
-	} else {
-		return ret;
-	}
+	return ret;
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
-- 
2.3.3-454-g85aa98f

Re: [PATCH 10/15] commit.c: fix a memory leak

[Duy Nguyen]

On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A further tangent (Duy Cc'ed for this point).  We might want to
> rethink the interface to ce_path_match() and report_path_error()
> so that we do not have to do a separate allocation of "has this
> pathspec been used?" array.  This was a remnant from the olden days
> back when pathspec were mere "const char **" where we did not have
> any room to add per-item bit---these days pathspec is repreasented
> as an array of "struct pathspec" and we can afford to add a bit
> to the structure---which will make this kind of leak much less
> likely to happen.

I just want to say "noted" (and therefore in my backlog). But no
promise that it will happen any time soon. Low hanging fruit, perhaps
some people may be interested..
-- 
Duy

Re: [PATCH 09/15] http: release the memory of a http pack request as well

[Stefan Beller]

On Sun, Mar 22, 2015 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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 <sbeller@google.com>
>> ---
>>  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)
>
> Freeing of preq in all the callers of this one looks sensible,
> except for the one in finish_request() of http-push.c that pulls an
> preq instance out of request->userData.
>
> Can somebody help me follow the dataflow to convince me that this is
> not leading to double-free in start_fetch_packed()?

I am not sure where you suspect the double free problem.

In start_fetch_packed we have a call to release_http_pack_request
2 times but just in an error-out-and-cleanup case, so either of one cases
is called.

In the latter place (http-push.c lines 335-347), we have code like
    request->userData = preq;
    if (!start_active_slot(preq->slot)) {
        release_http_pack_request(preq);
        repo->can_update_info_refs = 0;
        release_request(request);
    }

Do you mean that the release_http_pack_request and release_request may collide
as the `release_request(request);` has a pointer to preq via its userData field?

Well there is hope, as `release_request` only touches
    free(request->url);
    free(request);

and not the userData pointer.


I am a bit puzzled what you're trying to hint at.

>
> Thanks.
>

Re: [PATCH 09/15] http: release the memory of a http pack request as well

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Well there is hope, as `release_request` only touches
>     free(request->url);
>     free(request);
>
> and not the userData pointer.

OK.

> I am a bit puzzled what you're trying to hint at.

The caller does this:

        static void start_fetch_packed(struct transfer_request *request)
        {
                ...
                preq = new_http_pack_request(target, repo->url);
                ...
                preq->slot->callback_func = process_response;
                preq->slot->callback_data = request;
                request->slot = preq->slot;
                request->userData = preq;

                /* Try to get the request started, abort the request on error */
                request->state = RUN_FETCH_PACKED;
                if (!start_active_slot(preq->slot)) {
                        fprintf(stderr, "Unable to start GET request\n");
                        release_http_pack_request(preq);
                        repo->can_update_info_refs = 0;
                        release_request(request);
                }
        }

and start_active_slot() actually not just "starts" but calls
curl_multi_perform() to do things, like calling process_response(),
which has calls to release_{,http_pack_}request().  I didn't see
those releases and the releases we see in the above (i.e. when !start)
will not run at the same time (but I see it now ;-))

In short, not hinting at anything.  I was genuinely having a hard time
following the codeflow.

Re* [PATCH 10/15] commit.c: fix a memory leak

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> A further tangent (Duy Cc'ed for this point).  We might want to
>> rethink the interface to ce_path_match() and report_path_error()
>> so that we do not have to do a separate allocation of "has this
>> pathspec been used?" array.  This was a remnant from the olden days
>> back when pathspec were mere "const char **" where we did not have
>> any room to add per-item bit---these days pathspec is repreasented
>> as an array of "struct pathspec" and we can afford to add a bit
>> to the structure---which will make this kind of leak much less
>> likely to happen.
>
> I just want to say "noted" (and therefore in my backlog). But no
> promise that it will happen any time soon. Low hanging fruit, perhaps
> some people may be interested..

OK, the other one I just did so that we won't forget.  Otherwise we
will leave too many loose ends untied.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 24 Mar 2015 14:12:10 -0700
Subject: [PATCH] report_path_error(): move to dir.c

The expected call sequence is for the caller to use match_pathspec()
repeatedly on a set of pathspecs, accumulating the "hits" in a
separate array, and then call this function to diagnose a pathspec
that never matched anything, as that can indicate a typo from the
command line, e.g. "git commit Maekfile".

Many builtin commands use this function from builtin/ls-files.c,
which is not a very healthy arrangement.  ls-files might have been
the first command to feel the need for such a helper, but the need
is shared by everybody who uses the "match and then report" pattern.

Move it to dir.c where match_pathspec() is defined.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/ls-files.c | 43 -------------------------------------------
 cache.h            |  1 -
 dir.c              | 43 +++++++++++++++++++++++++++++++++++++++++++
 dir.h              |  1 +
 4 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..47d70b2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -354,49 +354,6 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched,
-		      const struct pathspec *pathspec,
-		      const char *prefix)
-{
-	/*
-	 * Make sure all pathspec matched; otherwise it is an error.
-	 */
-	struct strbuf sb = STRBUF_INIT;
-	int num, errors = 0;
-	for (num = 0; num < pathspec->nr; num++) {
-		int other, found_dup;
-
-		if (ps_matched[num])
-			continue;
-		/*
-		 * The caller might have fed identical pathspec
-		 * twice.  Do not barf on such a mistake.
-		 * FIXME: parse_pathspec should have eliminated
-		 * duplicate pathspec.
-		 */
-		for (found_dup = other = 0;
-		     !found_dup && other < pathspec->nr;
-		     other++) {
-			if (other == num || !ps_matched[other])
-				continue;
-			if (!strcmp(pathspec->items[other].original,
-				    pathspec->items[num].original))
-				/*
-				 * Ok, we have a match already.
-				 */
-				found_dup = 1;
-		}
-		if (found_dup)
-			continue;
-
-		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec->items[num].original);
-		errors++;
-	}
-	strbuf_release(&sb);
-	return errors;
-}
-
 static const char * const ls_files_usage[] = {
 	N_("git ls-files [options] [<file>...]"),
 	NULL
diff --git a/cache.h b/cache.h
index f23fdbe..8ec0b65 100644
--- a/cache.h
+++ b/cache.h
@@ -1411,7 +1411,6 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/dir.c b/dir.c
index 797805d..5d6e102 100644
--- a/dir.c
+++ b/dir.c
@@ -377,6 +377,49 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+int report_path_error(const char *ps_matched,
+		      const struct pathspec *pathspec,
+		      const char *prefix)
+{
+	/*
+	 * Make sure all pathspec matched; otherwise it is an error.
+	 */
+	struct strbuf sb = STRBUF_INIT;
+	int num, errors = 0;
+	for (num = 0; num < pathspec->nr; num++) {
+		int other, found_dup;
+
+		if (ps_matched[num])
+			continue;
+		/*
+		 * The caller might have fed identical pathspec
+		 * twice.  Do not barf on such a mistake.
+		 * FIXME: parse_pathspec should have eliminated
+		 * duplicate pathspec.
+		 */
+		for (found_dup = other = 0;
+		     !found_dup && other < pathspec->nr;
+		     other++) {
+			if (other == num || !ps_matched[other])
+				continue;
+			if (!strcmp(pathspec->items[other].original,
+				    pathspec->items[num].original))
+				/*
+				 * Ok, we have a match already.
+				 */
+				found_dup = 1;
+		}
+		if (found_dup)
+			continue;
+
+		error("pathspec '%s' did not match any file(s) known to git.",
+		      pathspec->items[num].original);
+		errors++;
+	}
+	strbuf_release(&sb);
+	return errors;
+}
+
 /*
  * Return the length of the "simple" part of a path match limiter.
  */
diff --git a/dir.h b/dir.h
index 55e5345..ed336ad 100644
--- a/dir.h
+++ b/dir.h
@@ -135,6 +135,7 @@ extern char *common_prefix(const struct pathspec *pathspec);
 extern int match_pathspec(const struct pathspec *pathspec,
 			  const char *name, int namelen,
 			  int prefix, char *seen, int is_dir);
+extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);
 extern int within_depth(const char *name, int namelen, int depth, int max_depth);
 
 extern int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec);
-- 
2.3.4-449-gebec173

Re: Re* [PATCH 10/15] commit.c: fix a memory leak

[Stefan Beller]

On Tue, Mar 24, 2015 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Move it to dir.c where match_pathspec() is defined.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Reviewed-by: Stefan Beller <sbeller@google.com>