[PATCH V2 0/6] Memory leaks once again

[PATCH V2 0/6] Memory leaks once again

[Stefan Beller]

Here comes another bunch of memory leaks fixed.
patches 1-4 are safe bets, but 5 and 6 are not so.

In patch 5 I wonder if we need to fix more aggressively and
in patch 6 I just know there is a leak but I have no idea how to
actually fix it.

The patches are apply-able to origin/master.

Version 2 has no changes in code but in recipients. CC'ing parties who
may be qualified to review the patches.

Stefan Beller (6):
  shallow: fix a memleak
  line-log.c: fix a memleak
  line-log.c: fix a memleak
  wt-status.c: fix a memleak
  pack-bitmap: fix a memleak
  WIP/RFC/entry.c: fix a memleak

 entry.c       |  4 +++-
 line-log.c    |  4 ++++
 pack-bitmap.c | 27 ++++++++++++++++++---------
 shallow.c     |  4 ++--
 wt-status.c   |  2 ++
 5 files changed, 29 insertions(+), 12 deletions(-)

-- 
2.3.0.81.gc37f363

[PATCH V2 1/6] shallow: fix a memleak

[Stefan Beller]

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

diff --git a/shallow.c b/shallow.c
index d8bf40a..f8b0458 100644
--- a/shallow.c
+++ b/shallow.c
@@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 	uint32_t *bitmap = paint_alloc(info);
 	struct commit *c = lookup_commit_reference_gently(sha1, 1);
 	if (!c)
-		return;
+		goto out;
 	memset(bitmap, 0, bitmap_size);
 	bitmap[id / 32] |= (1 << (id % 32));
 	commit_list_insert(c, &head);
@@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 		if (o && o->type == OBJ_COMMIT)
 			o->flags &= ~SEEN;
 	}
-
+out:
 	free(tmp);
 }
 
-- 
2.3.0.81.gc37f363

Re: [PATCH V2 1/6] shallow: fix a memleak

[Eric Sunshine]

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/shallow.c b/shallow.c
> index d8bf40a..f8b0458 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
>         uint32_t *bitmap = paint_alloc(info);
>         struct commit *c = lookup_commit_reference_gently(sha1, 1);
>         if (!c)
> -               return;
> +               goto out;

Rather than slapping on a band-aid, a cleaner fix would be to move the
allocation of 'tmp' below this conditional since 'tmp' is never used
before the point of this early return.

Perhaps also move allocation of 'bitmap' below the early return (if
you find that it's safe to do so).

>         memset(bitmap, 0, bitmap_size);
>         bitmap[id / 32] |= (1 << (id % 32));
>         commit_list_insert(c, &head);
> @@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
>                 if (o && o->type == OBJ_COMMIT)
>                         o->flags &= ~SEEN;
>         }
> -
> +out:
>         free(tmp);
>  }

[PATCH V2 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

[PATCH V2 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

[PATCH V2 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/wt-status.c b/wt-status.c
index 853419f..9d2a349 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 
 	color_fprintf(s->fp, header_color, "]");
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
+
+	free((char *)base);
 }
 
 void wt_shortstatus_print(struct wt_status *s)
-- 
2.3.0.81.gc37f363

Re: [PATCH V2 4/6] wt-status.c: fix a memleak

[Eric Sunshine]

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> 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 | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/wt-status.c b/wt-status.c
> index 853419f..9d2a349 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>
>         color_fprintf(s->fp, header_color, "]");
>         fputc(s->null_termination ? '\0' : '\n', s->fp);
> +
> +       free((char *)base);

There's an early 'return' before this point (at line 1560) which will
leak 'base', so this is an unreliable fix.  A better solution would be
to free(base) immediately after it's final use (before the early
'return').

>  }
>
>  void wt_shortstatus_print(struct wt_status *s)
> --
> 2.3.0.81.gc37f363

[PATCH V2 5/6] pack-bitmap: fix a memleak

[Stefan Beller]

`recent_bitmaps` is allocated in the function load_bitmap_entries_v1
and it is not passed into any function, so it's safe to free it before
leaving that function.

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

Notes:
    I wonder however if we need to free the actual bitmaps
    stored in the recent_bitmaps as well.

 pack-bitmap.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 365f9d9..34823e9 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 {
 	static const size_t MAX_XOR_OFFSET = 160;
 
-	uint32_t i;
+	uint32_t i, ret = 0;
 	struct stored_bitmap **recent_bitmaps;
 
 	recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));
@@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 		sha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);
 
 		bitmap = read_bitmap_1(index);
-		if (!bitmap)
-			return -1;
+		if (!bitmap) {
+			ret = -1;
+			goto out;
+		}
 
-		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
-			return error("Corrupted bitmap pack index");
+		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
+			ret = error("Corrupted bitmap pack index");
+			goto out;
+		}
 
 		if (xor_offset > 0) {
 			xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];
 
-			if (xor_bitmap == NULL)
-				return error("Invalid XOR offset in bitmap pack index");
+			if (xor_bitmap == NULL) {
+				ret = error("Invalid XOR offset in bitmap pack index");
+				goto out;
+			}
 		}
 
 		recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
 			index, bitmap, sha1, xor_bitmap, flags);
 	}
-
-	return 0;
+out:
+	free(recent_bitmaps);
+	return ret;
 }
 
 static char *pack_bitmap_filename(struct packed_git *p)
@@ -986,6 +993,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: [PATCH V2 5/6] pack-bitmap: fix a memleak

[Eric Sunshine]

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> `recent_bitmaps` is allocated in the function load_bitmap_entries_v1
> and it is not passed into any function, so it's safe to free it before
> leaving that function.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 365f9d9..34823e9 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>  {
>         static const size_t MAX_XOR_OFFSET = 160;
>
> -       uint32_t i;
> +       uint32_t i, ret = 0;
>         struct stored_bitmap **recent_bitmaps;
>
>         recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));
> @@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>                 sha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);
>
>                 bitmap = read_bitmap_1(index);
> -               if (!bitmap)
> -                       return -1;
> +               if (!bitmap) {
> +                       ret = -1;
> +                       goto out;
> +               }
>
> -               if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
> -                       return error("Corrupted bitmap pack index");
> +               if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
> +                       ret = error("Corrupted bitmap pack index");
> +                       goto out;
> +               }
>
>                 if (xor_offset > 0) {
>                         xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];
>
> -                       if (xor_bitmap == NULL)
> -                               return error("Invalid XOR offset in bitmap pack index");
> +                       if (xor_bitmap == NULL) {
> +                               ret = error("Invalid XOR offset in bitmap pack index");
> +                               goto out;
> +                       }
>                 }
>
>                 recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
>                         index, bitmap, sha1, xor_bitmap, flags);
>         }
> -
> -       return 0;
> +out:
> +       free(recent_bitmaps);
> +       return ret;
>  }
>
>  static char *pack_bitmap_filename(struct packed_git *p)
> @@ -986,6 +993,8 @@ void test_bitmap_walk(struct rev_info *revs)
>                 fprintf(stderr, "OK!\n");
>         else
>                 fprintf(stderr, "Mismatch!\n");
> +
> +       free(result);

This fix is not mentioned in the commit message.

>  }
>
>  static int rebuild_bitmap(uint32_t *reposition,
> --
> 2.3.0.81.gc37f363

Re: [PATCH V2 5/6] pack-bitmap: fix a memleak

[Jeff King]

On Fri, Mar 27, 2015 at 03:32:48PM -0700, Stefan Beller wrote:

> `recent_bitmaps` is allocated in the function load_bitmap_entries_v1
> and it is not passed into any function, so it's safe to free it before
> leaving that function.

I think this is OK, though it might be easier still to just turn the
array into a stack variable. It's only 160 * sizeof(ptr), or about 1280
bytes on a 64-bit system. Note that the xcalloc there looks wrong (it is
way over-allocating by using the sizeof the struct, not a pointer to the
struct).

> Notes:
>     I wonder however if we need to free the actual bitmaps
>     stored in the recent_bitmaps as well.

No, those are just weak pointers. The memory is owned by the hash that
store_bitmap puts the bitmaps into.

>  		bitmap = read_bitmap_1(index);

This line allocates, too. So if we get down to...

> -		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
> -			return error("Corrupted bitmap pack index");
> +		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
> +			ret = error("Corrupted bitmap pack index");
> +			goto out;
> +		}

...here, for example, where we have not yet called store_bitmap, then
it's a leak, too.

-Peff

[PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

[Stefan Beller]

I  observe that filter is going out of scope, but the
implementation proposed in this patch produces just a
crash instead of any helpful fix.

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

diff --git a/entry.c b/entry.c
index 1eda8e9..5383001 100644
--- a/entry.c
+++ b/entry.c
@@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
 		if (filter &&
 		    !streaming_write_entry(ce, path, filter,
 					   state, to_tempfile,
-					   &fstat_done, &st))
+					   &fstat_done, &st)) {
+			free_stream_filter(filter);
 			goto finish;
+		}
 	}
 
 	switch (ce_mode_s_ifmt) {
-- 
2.3.0.81.gc37f363

Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

[Eric Sunshine]

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> I  observe that filter is going out of scope, but the
> implementation proposed in this patch produces just a
> crash instead of any helpful fix.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/entry.c b/entry.c
> index 1eda8e9..5383001 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
>                 if (filter &&
>                     !streaming_write_entry(ce, path, filter,
>                                            state, to_tempfile,
> -                                          &fstat_done, &st))
> +                                          &fstat_done, &st)) {
> +                       free_stream_filter(filter);

Aside from the crash you are seeing, this is a bogus fix anyway.
You're only freeing 'filter' if it was allocated _and_ if
streaming_write_entry() returned 0. I would guess your intention was
to free 'filter' regardless of the result of streaming_write_entry().

>                         goto finish;
> +               }
>         }
>
>         switch (ce_mode_s_ifmt) {
> --
> 2.3.0.81.gc37f363

Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

[Eric Sunshine]

On Friday, March 27, 2015, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> > I  observe that filter is going out of scope, but the
> > implementation proposed in this patch produces just a
> > crash instead of any helpful fix.
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> > diff --git a/entry.c b/entry.c
> > index 1eda8e9..5383001 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
> >                 if (filter &&
> >                     !streaming_write_entry(ce, path, filter,
> >                                            state, to_tempfile,
> > -                                          &fstat_done, &st))
> > +                                          &fstat_done, &st)) {
> > +                       free_stream_filter(filter);
>
> Aside from the crash you are seeing, this is a bogus fix anyway.
> You're only freeing 'filter' if it was allocated _and_ if
> streaming_write_entry() returned 0. I would guess your intention was
> to free 'filter' regardless of the result of streaming_write_entry().

Unless streaming_write_entry() is freeing the filter for you -- there
is a free_stream_filter() call in close_method_decl() in streaming.c
-- in which case your new free_stream_filter() call would attempt to
free the already-freed filter.

Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

[John Keeping]

On Fri, Mar 27, 2015 at 08:14:28PM -0400, Eric Sunshine wrote:
> On Friday, March 27, 2015, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> > > I  observe that filter is going out of scope, but the
> > > implementation proposed in this patch produces just a
> > > crash instead of any helpful fix.
> > >
> > > Signed-off-by: Stefan Beller <sbeller@google.com>
> > > ---
> > > diff --git a/entry.c b/entry.c
> > > index 1eda8e9..5383001 100644
> > > --- a/entry.c
> > > +++ b/entry.c
> > > @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
> > >                 if (filter &&
> > >                     !streaming_write_entry(ce, path, filter,
> > >                                            state, to_tempfile,
> > > -                                          &fstat_done, &st))
> > > +                                          &fstat_done, &st)) {
> > > +                       free_stream_filter(filter);
> >
> > Aside from the crash you are seeing, this is a bogus fix anyway.
> > You're only freeing 'filter' if it was allocated _and_ if
> > streaming_write_entry() returned 0. I would guess your intention was
> > to free 'filter' regardless of the result of streaming_write_entry().
> 
> Unless streaming_write_entry() is freeing the filter for you -- there
> is a free_stream_filter() call in close_method_decl() in streaming.c
> -- in which case your new free_stream_filter() call would attempt to
> free the already-freed filter.

Yes, I think the correct fix for this leak is to make
stream_blob_to_fd() always free the filter, since there's only one path
out that doesn't at the moment and there's no way for the caller to
figure out whether or not the filter has been freed:

-- >8 --
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 (;;) {