[PATCHv3 0/6] Memory leaks once again

[PATCHv3 0/6] Memory leaks once again

[Stefan Beller]

I changed the patches based on Erics feedback, all of them look sane to me now.

I still retain 2 patches which I need to convince myself are reasonable to send
to the list though.

John Keeping (1):
  entry.c: fix a memleak

Stefan Beller (5):
  shallow: fix a memleak
  line-log.c: fix a memleak
  line-log.c: fix a memleak
  wt-status.c: fix a memleak
  pack-bitmap.c: fix a memleak

 line-log.c    | 4 ++++
 pack-bitmap.c | 2 ++
 shallow.c     | 5 +++--
 streaming.c   | 5 ++++-
 wt-status.c   | 1 +
 5 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.3.0.81.gc37f363

[PATCHv3 1/6] shallow: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 shallow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index d8bf40a..11d5c4e 100644
--- a/shallow.c
+++ b/shallow.c
@@ -412,11 +412,12 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 	struct commit_list *head = NULL;
 	int bitmap_nr = (info->nr_bits + 31) / 32;
 	int bitmap_size = bitmap_nr * sizeof(uint32_t);
-	uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
-	uint32_t *bitmap = paint_alloc(info);
+	uint32_t *tmp, *bitmap;
 	struct commit *c = lookup_commit_reference_gently(sha1, 1);
 	if (!c)
 		return;
+	tmp = xmalloc(bitmap_size); /* to be freed before return */
+	bitmap = paint_alloc(info);
 	memset(bitmap, 0, bitmap_size);
 	bitmap[id / 32] |= (1 << (id % 32));
 	commit_list_insert(c, &head);
-- 
2.3.0.81.gc37f363

Re: [PATCHv3 1/6] shallow: fix a memleak

[Eric Sunshine]

On Mon, Mar 30, 2015 at 9:22 PM, Stefan Beller <sbeller@google.com> wrote:
> shallow: fix a memleak
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/shallow.c b/shallow.c
> index d8bf40a..11d5c4e 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -412,11 +412,12 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
>         struct commit_list *head = NULL;
>         int bitmap_nr = (info->nr_bits + 31) / 32;
>         int bitmap_size = bitmap_nr * sizeof(uint32_t);
> -       uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
> -       uint32_t *bitmap = paint_alloc(info);
> +       uint32_t *tmp, *bitmap;
>         struct commit *c = lookup_commit_reference_gently(sha1, 1);
>         if (!c)
>                 return;
> +       tmp = xmalloc(bitmap_size); /* to be freed before return */
> +       bitmap = paint_alloc(info);

You've made two unrelated changes in this patch, only one of which is
a fix for the memory leak mentioned by the commit message.

The 'tmp' change fixes the memory leak. The 'bitmap' change, however,
merely avoids doing work unnecessarily in the "early return" case.
Since 'bitmap' gets released by the caller of paint_down() when the
paint_info structure is freed, it is misleading to claim that that
particular change is a "memleak" fix.

Either split the two changes into separate patches or augment the
commit message to say something along the lines of:

    While here, also avoid unnecessarily allocating 'bitmap' within
    paint_info in the early-return case.

>         memset(bitmap, 0, bitmap_size);
>         bitmap[id / 32] |= (1 << (id % 32));
>         commit_list_insert(c, &head);
> --
> 2.3.0.81.gc37f363

[PATCHv3 2/6] line-log.c: fix a memleak

[Stefan Beller]

The `filepair` is assigned new memory with any iteration via
process_diff_filepair, so free it before the current iteration ends.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/line-log.c b/line-log.c
index a490efe..b43ac58 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1099,6 +1099,7 @@ static int process_all_files(struct line_log_data **range_out,
 			rg->pair = diff_filepair_dup(queue->queue[i]);
 			memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));
 		}
+		free(pairdiff);
 	}
 
 	return changed;
-- 
2.3.0.81.gc37f363

[PATCHv3 3/6] line-log.c: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 line-log.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/line-log.c b/line-log.c
index b43ac58..db6e58d 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
 	changed = process_all_files(&parent_range, rev, &queue, range);
 	if (parent)
 		add_line_range(rev, parent, parent_range);
+	else
+		free_line_log_data(parent_range);
+
 	return changed;
 }
 
-- 
2.3.0.81.gc37f363

Re: [PATCHv3 3/6] line-log.c: fix a memleak

[Torsten Bögershausen]

On 03/31/2015 03:22 AM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   line-log.c | 3 +++
>   1 file changed, 3 insertions(+)
>
Would it make sense to join 2/6 and 3/6 into a single patch ?

Both fix the memleaks in the same file, and the
header can be "fix memleaks".

Re: [PATCHv3 3/6] line-log.c: fix a memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  line-log.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/line-log.c b/line-log.c
> index b43ac58..db6e58d 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
>  	changed = process_all_files(&parent_range, rev, &queue, range);
>  	if (parent)
>  		add_line_range(rev, parent, parent_range);
> +	else
> +		free_line_log_data(parent_range);
> +

Should this be in the else clause?  From a cursory scan
add-line-range feeds it to either line-log-data-merge or
line-log-data-copy to create "new" that is a separate thing from the
parent_range we are feeding it, and that copy is used by
add-decoration.  If add-line-range does not take ownership of
parent_range from us, shouldn't we be freeing it whether we called
it or not???

>  	return changed;
>  }

Re: [PATCHv3 3/6] line-log.c: fix a memleak

[Stefan Beller]

On 30.03.2015 22:06, Junio C Hamano wrote:
> If add-line-range does not take ownership of
> parent_range from us, shouldn't we be freeing it whether we called
> it or not???

In my understanding it does take ownership of it. (According to my code
review on friday)

Re: [PATCHv3 3/6] line-log.c: fix a memleak

[Junio C Hamano]

Stefan Beller <stefanbeller@gmail.com> writes:

> On 30.03.2015 22:06, Junio C Hamano wrote:
>> If add-line-range does not take ownership of
>> parent_range from us, shouldn't we be freeing it whether we called
>> it or not???
>
> In my understanding it does take ownership of it. (According to my code
> review on friday)

Hmmm.

add_line_range() is given the "parent_range" as its third parameter,
i.e. "range".

If there is nothing for the commit yet, it calls line_log_data_copy()
on it to make a deep copy and stores that in new, which is then used
as the decoration for the commit---the copy is what is retained.

I am not quite sure what the lifetime rule is for the original of
this deep copy, i.e. "range", aka "parent_range" of the caller.
Who holds pointers into it after this function is done?

[PATCHv3 4/6] wt-status.c: fix a memleak

[Stefan Beller]

In any code path of shorten_unambiguous_ref the return value is a
xstrdup(some string), so it is safe to free the variable `base`
in any codepath.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index 853419f..38cb165 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1550,6 +1550,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	base = shorten_unambiguous_ref(base, 0);
 	color_fprintf(s->fp, header_color, "...");
 	color_fprintf(s->fp, branch_color_remote, "%s", base);
+	free((char *)base);
 
 	if (!upstream_is_gone && !num_ours && !num_theirs) {
 		fputc(s->null_termination ? '\0' : '\n', s->fp);
-- 
2.3.0.81.gc37f363

[PATCHv3 5/6] pack-bitmap.c: fix a memleak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pack-bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 365f9d9..62a98cc 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -986,6 +986,8 @@ void test_bitmap_walk(struct rev_info *revs)
 		fprintf(stderr, "OK!\n");
 	else
 		fprintf(stderr, "Mismatch!\n");
+
+	free(result);
 }
 
 static int rebuild_bitmap(uint32_t *reposition,
-- 
2.3.0.81.gc37f363

Re: [PATCHv3 5/6] pack-bitmap.c: fix a memleak

[Jeff King]

On Mon, Mar 30, 2015 at 06:22:10PM -0700, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  pack-bitmap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 365f9d9..62a98cc 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -986,6 +986,8 @@ void test_bitmap_walk(struct rev_info *revs)
>  		fprintf(stderr, "OK!\n");
>  	else
>  		fprintf(stderr, "Mismatch!\n");
> +
> +	free(result);

I think this one is fine. I see you dropped the other bits from
load_bitmap_entries_v1. There are definitely some leaks there in the
error case, and I left some notes in the other thread.

I don't think those leaks are all that critical (we only load the
bitmaps one per process, and the memory persists if we succeed, so the
worst case is that we spend the memory but do not get to use the
bitmaps). But if your goal is making us valgrind-clean, they would
certainly count.

-Peff

[PATCHv3 6/6] entry.c: fix a memleak

[Stefan Beller]

From: John Keeping <john@keeping.me.uk>

stream_blob_to_fd() always frees the filter now, so there is no memory
leak in entry.c:152 just before the `goto finish`.

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
 I added Johns signoff here tentatively, John can you confirm the sign off?
 
 streaming.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/streaming.c b/streaming.c
index 2ff036a..811fcc2 100644
--- a/streaming.c
+++ b/streaming.c
@@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
 	int result = -1;
 
 	st = open_istream(sha1, &type, &sz, filter);
-	if (!st)
+	if (!st) {
+		if (filter)
+			free_stream_filter(filter);
 		return result;
+	}
 	if (type != OBJ_BLOB)
 		goto close_and_exit;
 	for (;;) {
-- 
2.3.0.81.gc37f363

Re: [PATCHv3 6/6] entry.c: fix a memleak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> From: John Keeping <john@keeping.me.uk>
>
> stream_blob_to_fd() always frees the filter now, so there is no memory
> leak in entry.c:152 just before the `goto finish`.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  
>  I added Johns signoff here tentatively, John can you confirm the sign off?
>  
>  streaming.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/streaming.c b/streaming.c
> index 2ff036a..811fcc2 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
>  	int result = -1;
>  
>  	st = open_istream(sha1, &type, &sz, filter);
> -	if (!st)
> +	if (!st) {
> +		if (filter)
> +			free_stream_filter(filter);
>  		return result;
> +	}

I think this one is fine. I see you moved the callsite to free this
thing from entry.c down to streaming (stream_blob_to_fd()), which
means you would need to adjust the title of the change, too.

It somewhat feels dirty that we free what the caller gave us, but
given that closing a filtered stream frees the filter at the end,
I think this is the right place to discard the filter.  In essence,
the overall use of the API in this code is equivalent to

	filter = ... prepare a filter ...
        if (open_istream(... filter) == OK) {
		... use filter ...
                close_istream(); /* which frees the filter */
	} else {
           	/* we failed to free filter here without the patch */
		free_stream_filter(filter);
	}

so the patch makes sense.

We may want to teach free_stream_filter() that a request to free a
NULL filter is OK, though. That would be a separate clean-up topic.


>  	if (type != OBJ_BLOB)
>  		goto close_and_exit;
>  	for (;;) {

Re: [PATCHv3 6/6] entry.c: fix a memleak

[John Keeping]

On Mon, Mar 30, 2015 at 06:22:11PM -0700, Stefan Beller wrote:
> From: John Keeping <john@keeping.me.uk>
> 
> stream_blob_to_fd() always frees the filter now, so there is no memory
> leak in entry.c:152 just before the `goto finish`.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  
>  I added Johns signoff here tentatively, John can you confirm the sign off?

Signed-off-by: John Keeping <john@keeping.me.uk>

>  streaming.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/streaming.c b/streaming.c
> index 2ff036a..811fcc2 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
>  	int result = -1;
>  
>  	st = open_istream(sha1, &type, &sz, filter);
> -	if (!st)
> +	if (!st) {
> +		if (filter)
> +			free_stream_filter(filter);
>  		return result;
> +	}
>  	if (type != OBJ_BLOB)
>  		goto close_and_exit;
>  	for (;;) {
> -- 
> 2.3.0.81.gc37f363
> 

Re: [PATCHv3 6/6] entry.c: fix a memleak

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Mon, Mar 30, 2015 at 06:22:11PM -0700, Stefan Beller wrote:
>> From: John Keeping <john@keeping.me.uk>
>> 
>> stream_blob_to_fd() always frees the filter now, so there is no memory
>> leak in entry.c:152 just before the `goto finish`.
>> 
>> Signed-off-by: John Keeping <john@keeping.me.uk>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  
>>  I added Johns signoff here tentatively, John can you confirm the sign off?
>
> Signed-off-by: John Keeping <john@keeping.me.uk>

Thanks. I'll queue with a retitle, as this no longer is about
entry.c at all.

>
>>  streaming.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/streaming.c b/streaming.c
>> index 2ff036a..811fcc2 100644
>> --- a/streaming.c
>> +++ b/streaming.c
>> @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
>>  	int result = -1;
>>  
>>  	st = open_istream(sha1, &type, &sz, filter);
>> -	if (!st)
>> +	if (!st) {
>> +		if (filter)
>> +			free_stream_filter(filter);
>>  		return result;
>> +	}
>>  	if (type != OBJ_BLOB)
>>  		goto close_and_exit;
>>  	for (;;) {
>> -- 
>> 2.3.0.81.gc37f363
>>