threads / patch / 38930

v2, 6 partsMemory leaks once again

Subject: [PATCH V2 0/6] Memory leaks once again

## tl;dr

14 messages between Mar 27, 2015 and Mar 31, 2015. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Stefan Beller· Mar 27, 2015, 22:32 UTC · lore

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
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 shallow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to shallow.c +2 −2
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
Eric Sunshine· Mar 27, 2015, 23:01 UTC · re: Stefan Beller · lore

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

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
Show 12 quoted lines
> 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).

Show 11 quoted lines
>         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);
>  }
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to line-log.c +1 −0
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
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 line-log.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to line-log.c +3 −0
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
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to wt-status.c +2 −0
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
Eric Sunshine· Mar 27, 2015, 23:16 UTC · re: Stefan Beller · lore

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

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
Show 19 quoted lines
> 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').

Show 5 quoted lines
>  }
>
>  void wt_shortstatus_print(struct wt_status *s)
> --
> 2.3.0.81.gc37f363
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

`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(-)
Show changes to pack-bitmap.c +18 −9
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
Eric Sunshine· Mar 27, 2015, 23:27 UTC · re: Stefan Beller · lore

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

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
Show 65 quoted lines
> `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.
Show 5 quoted lines
>  }
>
>  static int rebuild_bitmap(uint32_t *reposition,
> --
> 2.3.0.81.gc37f363
Jeff King· Mar 31, 2015, 03:29 UTC · re: Stefan Beller · lore

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

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...
Show 6 quoted lines
> -		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
Stefan Beller· Mar 27, 2015, 22:32 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to entry.c +3 −1
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
Eric Sunshine· Mar 27, 2015, 23:32 UTC · re: Stefan Beller · lore

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

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
Show 17 quoted lines
> 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().

Show 7 quoted lines
>                         goto finish;
> +               }
>         }
>
>         switch (ce_mode_s_ifmt) {
> --
> 2.3.0.81.gc37f363
Eric Sunshine· Mar 28, 2015, 00:14 UTC · re: Eric Sunshine · lore

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

On Friday, March 27, 2015, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 23 quoted lines
> 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.

John Keeping· Mar 28, 2015, 11:09 UTC · re: Eric Sunshine · lore

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

On Fri, Mar 27, 2015 at 08:14:28PM -0400, Eric Sunshine wrote:
Show 29 quoted lines
> 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 --
Show changes to streaming.c +4 −1
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 (;;) {

← back to recent threads